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

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

[Peter Maydell]

Newer architectures may only implement the getdents64 syscall, not
getdents. Provide an implementation of getdents in terms of getdents64
so that we can run getdents-using targets on a getdents64-only host.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Guess which exciting new architecture doesn't have getdents :-)
Tested on i386 by temporarily nobbling the #ifdefs so we use the
via-getdents64 path rather than via-getdents, and with the test program
from the getdents(2) manpage as the guest binary.

 linux-user/syscall.c |   60 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 053fa14..15c771a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -208,8 +208,11 @@ static int gettid(void) {
     return -ENOSYS;
 }
 #endif
+#ifdef __NR_getdents
 _syscall3(int, sys_getdents, uint, fd, struct linux_dirent *, dirp, uint, count);
-#if defined(TARGET_NR_getdents64) && defined(__NR_getdents64)
+#endif
+#if !defined(__NR_getdents) || \
+    (defined(TARGET_NR_getdents64) && defined(__NR_getdents64))
 _syscall3(int, sys_getdents64, uint, fd, struct linux_dirent64 *, dirp, uint, count);
 #endif
 #if defined(TARGET_NR__llseek) && defined(__NR_llseek)
@@ -6963,6 +6966,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         break;
 #endif
     case TARGET_NR_getdents:
+#ifdef __NR_getdents
 #if TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64
         {
             struct target_dirent *target_dirp;
@@ -7035,6 +7039,60 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             unlock_user(dirp, arg2, ret);
         }
 #endif
+#else
+        /* Implement getdents in terms of getdents64 */
+        {
+            struct linux_dirent64 *dirp;
+            abi_long count = arg3;
+
+            dirp = lock_user(VERIFY_WRITE, arg2, count, 0);
+            if (!dirp) {
+                goto efault;
+            }
+            ret = get_errno(sys_getdents64(arg1, dirp, count));
+            if (!is_error(ret)) {
+                /* Convert the dirent64 structs to target dirent.  We do this
+                 * in-place, since we can guarantee that a target_dirent is no
+                 * larger than a dirent64; however this means we have to be
+                 * careful to read everything before writing in the new format.
+                 */
+                struct linux_dirent64 *de;
+                struct target_dirent *tde;
+                int len = ret;
+                int tlen = 0;
+
+                de = dirp;
+                tde = (struct target_dirent *)dirp;
+                while (len > 0) {
+                    int namelen, treclen;
+                    int reclen = de->d_reclen;
+                    uint64_t ino = de->d_ino;
+                    int64_t off = de->d_off;
+                    uint8_t type = de->d_type;
+
+                    namelen = strlen(de->d_name);
+                    treclen = offsetof(struct target_dirent, d_name) + namelen + 2;
+                    treclen = QEMU_ALIGN_UP(treclen, sizeof(abi_long));
+
+                    tde->d_ino = tswapal(ino);
+                    tde->d_off = tswapal(off);
+                    tde->d_reclen = tswap16(treclen);
+                    memmove(tde->d_name, de->d_name, namelen + 1);
+                    /* The target_dirent type is in what was formerly a padding
+                     * byte at the end of the structure:
+                     */
+                    *(((char *)tde) + treclen - 1) = type;
+
+                    de = (struct linux_dirent64 *)((char *)de + reclen);
+                    tde = (struct target_dirent *)((char *)tde + treclen);
+                    len -= reclen;
+                    tlen += treclen;
+                }
+                ret = tlen;
+            }
+            unlock_user(dirp, arg2, ret);
+        }
+#endif
         break;
 #if defined(TARGET_NR_getdents64) && defined(__NR_getdents64)
     case TARGET_NR_getdents64:
-- 
1.7.9.5

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

[Richard Henderson]

On 06/02/2013 10:10 AM, Peter Maydell wrote:
> +                    tde->d_ino = tswapal(ino);
> +                    tde->d_off = tswapal(off);
> +                    tde->d_reclen = tswap16(treclen);
> +                    memmove(tde->d_name, de->d_name, namelen + 1);

Wouldn't it be better to do the memmove first?  Then you really are reading all
of the dirent64 data first, like your comment says.

> +                    /* The target_dirent type is in what was formerly a padding
> +                     * byte at the end of the structure:
> +                     */
> +                    *(((char *)tde) + treclen - 1) = type;

Maybe easier to read as

  ((char *)tde)[treclen - 1] = type

?


r~

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

[Peter Maydell]

On 3 June 2013 16:15, Richard Henderson <rth@twiddle.net> wrote:
> On 06/02/2013 10:10 AM, Peter Maydell wrote:
>> +                    tde->d_ino = tswapal(ino);
>> +                    tde->d_off = tswapal(off);
>> +                    tde->d_reclen = tswap16(treclen);
>> +                    memmove(tde->d_name, de->d_name, namelen + 1);
>
> Wouldn't it be better to do the memmove first?  Then you really are reading all
> of the dirent64 data first, like your comment says.

It doesn't actually make a difference, in fact, but I agree
it would be clearer that way round.

>> +                    /* The target_dirent type is in what was formerly a padding
>> +                     * byte at the end of the structure:
>> +                     */
>> +                    *(((char *)tde) + treclen - 1) = type;
>
> Maybe easier to read as
>
>   ((char *)tde)[treclen - 1] = type
>
> ?

Dunno. It's not actually a char array, so I kind of prefer
to use plain pointer arithmetic for this kind of thing.

-- PMM

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

[Richard Henderson]

On 06/03/2013 08:45 AM, Peter Maydell wrote:
>>> >> +                    /* The target_dirent type is in what was formerly a padding
>>> >> +                     * byte at the end of the structure:
>>> >> +                     */
>>> >> +                    *(((char *)tde) + treclen - 1) = type;
>> >
>> > Maybe easier to read as
>> >
>> >   ((char *)tde)[treclen - 1] = type
>> >
>> > ?
> Dunno. It's not actually a char array, so I kind of prefer
> to use plain pointer arithmetic for this kind of thing.

Then drop the unnecessary parenthesis

  *((char *)tde + trelen - 1) = type;

?


r~

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

[Peter Maydell]

On 3 June 2013 16:58, Richard Henderson <rth@twiddle.net> wrote:
> On 06/03/2013 08:45 AM, Peter Maydell wrote:
>> Dunno. It's not actually a char array, so I kind of prefer
>> to use plain pointer arithmetic for this kind of thing.
>
> Then drop the unnecessary parenthesis
>
>   *((char *)tde + trelen - 1) = type;
>
> ?

That parenthesis is there under my "if you can't immediately
say what the relative precedence of two operators is then
add brackets" rule. (ie I don't offhand know precedence
of the cast operator).

-- PMM

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

[Laurent Vivier]

[-- Attachment #1: Type: text/plain, Size: 3947 bytes --]

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().

See comment below.

> Le 2 juin 2013 à 19:10, Peter Maydell <peter.maydell@linaro.org> a écrit :
>
>
> Newer architectures may only implement the getdents64 syscall, not
> getdents. Provide an implementation of getdents in terms of getdents64
> so that we can run getdents-using targets on a getdents64-only host.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Guess which exciting new architecture doesn't have getdents :-)
> Tested on i386 by temporarily nobbling the #ifdefs so we use the
> via-getdents64 path rather than via-getdents, and with the test program
> from the getdents(2) manpage as the guest binary.
>
> linux-user/syscall.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 053fa14..15c771a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -208,8 +208,11 @@ static int gettid(void) {
> return -ENOSYS;
> }
> #endif
> +#ifdef __NR_getdents
> _syscall3(int, sys_getdents, uint, fd, struct linux_dirent *, dirp, uint,
> count);
> -#if defined(TARGET_NR_getdents64) && defined(__NR_getdents64)
> +#endif
> +#if !defined(__NR_getdents) || \
> + (defined(TARGET_NR_getdents64) && defined(__NR_getdents64))
> _syscall3(int, sys_getdents64, uint, fd, struct linux_dirent64 *, dirp, uint,
> count);
> #endif
> #if defined(TARGET_NR__llseek) && defined(__NR_llseek)
> @@ -6963,6 +6966,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
> arg1,
> break;
> #endif
> case TARGET_NR_getdents:
> +#ifdef __NR_getdents
> #if TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64
> {
> struct target_dirent *target_dirp;
> @@ -7035,6 +7039,60 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
> arg1,
> unlock_user(dirp, arg2, ret);
> }
> #endif
> +#else
> + /* Implement getdents in terms of getdents64 */
> + {
> + struct linux_dirent64 *dirp;
> + abi_long count = arg3;
> +
> + dirp = lock_user(VERIFY_WRITE, arg2, count, 0);
> + if (!dirp) {
> + goto efault;
> + }
> + ret = get_errno(sys_getdents64(arg1, dirp, count));
> + if (!is_error(ret)) {
> + /* Convert the dirent64 structs to target dirent. We do this
> + * in-place, since we can guarantee that a target_dirent is no
> + * larger than a dirent64; however this means we have to be
> + * careful to read everything before writing in the new format.
> + */
> + struct linux_dirent64 *de;
> + struct target_dirent *tde;
> + int len = ret;
> + int tlen = 0;
> +
> + de = dirp;
> + tde = (struct target_dirent *)dirp;
> + while (len > 0) {
> + int namelen, treclen;
> + int reclen = de->d_reclen;
> + uint64_t ino = de->d_ino;
> + int64_t off = de->d_off;
> + uint8_t type = de->d_type;
> +
> + 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.

> +
> + tde->d_ino = tswapal(ino);
> + tde->d_off = tswapal(off);
> + tde->d_reclen = tswap16(treclen);
> + memmove(tde->d_name, de->d_name, namelen + 1);
> + /* The target_dirent type is in what was formerly a padding
> + * byte at the end of the structure:
> + */
> + *(((char *)tde) + treclen - 1) = type;
> +
> + de = (struct linux_dirent64 *)((char *)de + reclen);
> + tde = (struct target_dirent *)((char *)tde + treclen);
> + len -= reclen;
> + tlen += treclen;
> + }
> + ret = tlen;
> + }
> + unlock_user(dirp, arg2, ret);
> + }
> +#endif
> break;
> #if defined(TARGET_NR_getdents64) && defined(__NR_getdents64)
> case TARGET_NR_getdents64:

[-- Attachment #2: Type: text/html, Size: 6143 bytes --]

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

[Peter Maydell]

On 3 June 2013 12:11, Laurent Vivier <laurent@vivier.eu> 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 <peter.maydell@linaro.org> 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

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

[Laurent Vivier]

[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]

I agree with all your comments.

Tested-by: Laurent Vivier <Laurent@Vivier.EU>
Reviewed-by: Laurent Vivier <Laurent@Vivier.EU>

> Le 3 juin 2013 à 13:28, Peter Maydell <peter.maydell@linaro.org> a écrit :
>
>
> On 3 June 2013 12:11, Laurent Vivier <laurent@vivier.eu> 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 <peter.maydell@linaro.org> 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

[-- Attachment #2: Type: text/html, Size: 4099 bytes --]