Re: [Qemu-devel] [PATCH] linux-user: Allow getdents to be provided by getdents64

[Laurent Vivier]

I agree with all your comments.

 

Tested-by: Laurent Vivier <address@hidden>

Reviewed-by: Laurent Vivier <address@hidden>

> Le 3 juin 2013 à 13:28, Peter Maydell <address@hidden> a écrit :
>
>
> On 3 June 2013 12:11, Laurent Vivier <address@hidden> wrote:
> > Tested on m68k on x86_64 as described in the patch comment, in a a
> > debian-etch linux container.
> >
> > Works fine, except the drec_len differs between getdents() and getdents64().
>
> >> Le 2 juin 2013 à 19:10, Peter Maydell <address@hidden> a écrit :
> >> + namelen = strlen(de->d_name);
> >> + treclen = offsetof(struct target_dirent, d_name) + namelen + 2;
> >> + treclen = QEMU_ALIGN_UP(treclen, sizeof(abi_long));
> >
> > You should use ABI_LONG_ALIGNMENT instead of sizeof(abi_long), as some
> > targets (at least m68k) don't align on long size.
>
> I'm following what the kernel source does:
> http://lxr.linux.no/#linux+v3.5/fs/readdir.c#L154
>
> which if I'm not misreading it also aligns based on
> the size of the type, not its alignment requirements.
> (the C spec guarantees that the former is never less strict
> than the latter, so it's just unnecessary padding, not
> misaligning the next record).
>
> Looking at our code for 32-bit-guest-on-64-bit-host
> getdents-in-terms-of-getdents, I think it is actually
> the existing code which is not getting the record
> length correct, which is why you're seeing a difference
> between that and the code added in this patch. If you
> have access to the real hardware (or even just a system
> emulated QEMU I guess) I would expect to see that the
> record lengths it uses match this patch's results.
>
> getdents64() calculates the record length by aligning to
> a multiple of sizeof(u64), and getdents() does it by
> aligning to a multiple of sizeof(long). So if you work
> out the record length for target_dirent by back-calculating
> it from that of linux_dirent when the sizeof(long)
> is different between host and target, you'll end up
> with possibly a word of extra padding you didn't need.
> This won't hurt any guest code, since it shouldn't
> care, but it is why I chose in my code to determine the
> length of the filename via strlen(), so I could get
> the record length exactly correct.
>
> (The returned value, ie total number of bytes read
> into the buffer, will unavoidably differ, because
> the conversion process packs the results down into
> a smaller space. This shouldn't break guests either,
> fortunately.)
>
> thanks
> -- PMM