[RFC PATCH] linux-user: glib-ify is_proc_myself

[RFC PATCH] linux-user: glib-ify is_proc_myself

[Alex Bennée]

I'm not sure if this is neater than the original code but it does
remove a bunch of the !strcmp's in favour of glib's more natural bool
results. While we are at it make the function a bool return and fixup
the fake_open function prototypes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/syscall.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e739921e86..18e953de9d 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7987,33 +7987,27 @@ static int open_self_auxv(void *cpu_env, int fd)
     return 0;
 }
 
-static int is_proc_myself(const char *filename, const char *entry)
+static bool is_proc_myself(const char *filename, const char *entry)
 {
-    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
+    if (g_str_has_prefix(filename, "/proc/")) {
         filename += strlen("/proc/");
-        if (!strncmp(filename, "self/", strlen("self/"))) {
+        if (g_str_has_prefix(filename, "self/")) {
             filename += strlen("self/");
-        } else if (*filename >= '1' && *filename <= '9') {
-            char myself[80];
-            snprintf(myself, sizeof(myself), "%d/", getpid());
-            if (!strncmp(filename, myself, strlen(myself))) {
-                filename += strlen(myself);
-            } else {
-                return 0;
+        } else if (g_ascii_isdigit(*filename)) {
+            g_autofree char * myself = g_strdup_printf("%d/", getpid());
+            if (!g_str_has_prefix(filename, myself)) {
+                return false;
             }
-        } else {
-            return 0;
-        }
-        if (!strcmp(filename, entry)) {
-            return 1;
+            filename += strlen(myself);
         }
+        return g_str_has_prefix(filename, entry);
     }
-    return 0;
+    return false;
 }
 
 #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN) || \
     defined(TARGET_SPARC) || defined(TARGET_M68K) || defined(TARGET_HPPA)
-static int is_proc(const char *filename, const char *entry)
+static bool is_proc(const char *filename, const char *entry)
 {
     return strcmp(filename, entry) == 0;
 }
@@ -8097,7 +8091,7 @@ static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags,
     struct fake_open {
         const char *filename;
         int (*fill)(void *cpu_env, int fd);
-        int (*cmp)(const char *s1, const char *s2);
+        bool (*cmp)(const char *s1, const char *s2);
     };
     const struct fake_open *fake_open;
     static const struct fake_open fakes[] = {
-- 
2.20.1

Re: [RFC PATCH] linux-user: glib-ify is_proc_myself

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210524112323.2310-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210524112323.2310-1-alex.bennee@linaro.org
Subject: [RFC PATCH] linux-user: glib-ify is_proc_myself

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210524112323.2310-1-alex.bennee@linaro.org -> patchew/20210524112323.2310-1-alex.bennee@linaro.org
Switched to a new branch 'test'
4e0076f linux-user: glib-ify is_proc_myself

=== OUTPUT BEGIN ===
ERROR: "foo * bar" should be "foo *bar"
#43: FILE: linux-user/syscall.c:7996:
+            g_autofree char * myself = g_strdup_printf("%d/", getpid());

total: 1 errors, 0 warnings, 52 lines checked

Commit 4e0076fad27e (linux-user: glib-ify is_proc_myself) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210524112323.2310-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC PATCH] linux-user: glib-ify is_proc_myself

[Daniel P. Berrangé]

On Mon, May 24, 2021 at 12:23:23PM +0100, Alex Bennée wrote:
> I'm not sure if this is neater than the original code but it does
> remove a bunch of the !strcmp's in favour of glib's more natural bool
> results. While we are at it make the function a bool return and fixup
> the fake_open function prototypes.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  linux-user/syscall.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index e739921e86..18e953de9d 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7987,33 +7987,27 @@ static int open_self_auxv(void *cpu_env, int fd)
>      return 0;
>  }
>  
> -static int is_proc_myself(const char *filename, const char *entry)
> +static bool is_proc_myself(const char *filename, const char *entry)
>  {
> -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> +    if (g_str_has_prefix(filename, "/proc/")) {
>          filename += strlen("/proc/");
> -        if (!strncmp(filename, "self/", strlen("self/"))) {
> +        if (g_str_has_prefix(filename, "self/")) {
>              filename += strlen("self/");
> -        } else if (*filename >= '1' && *filename <= '9') {
> -            char myself[80];
> -            snprintf(myself, sizeof(myself), "%d/", getpid());
> -            if (!strncmp(filename, myself, strlen(myself))) {
> -                filename += strlen(myself);
> -            } else {
> -                return 0;
> +        } else if (g_ascii_isdigit(*filename)) {
> +            g_autofree char * myself = g_strdup_printf("%d/", getpid());
> +            if (!g_str_has_prefix(filename, myself)) {
> +                return false;
>              }
> -        } else {
> -            return 0;
> -        }
> -        if (!strcmp(filename, entry)) {
> -            return 1;
> +            filename += strlen(myself);
>          }
> +        return g_str_has_prefix(filename, entry);
>      }
> -    return 0;
> +    return false;
>  }

Diff is hard to compare, so this is what it looks like:

 static bool is_proc_myself(const char *filename, const char *entry)
 {
     if (g_str_has_prefix(filename, "/proc/")) {
         filename += strlen("/proc/");
         if (g_str_has_prefix(filename, "self/")) {
             filename += strlen("self/");
         } else if (g_ascii_isdigit(*filename)) {
             g_autofree char * myself = g_strdup_printf("%d/", getpid());
             if (!g_str_has_prefix(filename, myself)) {
                 return false;
             }
             filename += strlen(myself);
         }
         return g_str_has_prefix(filename, entry);
     }
     return false;
 }

I think if we don't mind two heap allocs it can be simplified to:

  static int is_proc_myself(const char *filename, const char *entry)
  {
    g_autofree char *procself = g_strdup_printf("/proc/self/%s", entry);
    g_autofree char *procpid = g_strdup_printf("/proc/%d/%s", getpid(), entry);
    return g_str_equal(filename, procself) || g_str_equal(filename, procpid);
  }


This makes me wonder if the code needs to cope with non-canonicalized
filenames though. eg /proc///self/maps  or /proc/./self/maps

Is something further up ensuring canonicalization of 'filename' ?


>  
>  #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN) || \
>      defined(TARGET_SPARC) || defined(TARGET_M68K) || defined(TARGET_HPPA)
> -static int is_proc(const char *filename, const char *entry)
> +static bool is_proc(const char *filename, const char *entry)
>  {
>      return strcmp(filename, entry) == 0;
>  }
> @@ -8097,7 +8091,7 @@ static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags,
>      struct fake_open {
>          const char *filename;
>          int (*fill)(void *cpu_env, int fd);
> -        int (*cmp)(const char *s1, const char *s2);
> +        bool (*cmp)(const char *s1, const char *s2);
>      };
>      const struct fake_open *fake_open;
>      static const struct fake_open fakes[] = {
> -- 
> 2.20.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH] linux-user: glib-ify is_proc_myself

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, May 24, 2021 at 12:23:23PM +0100, Alex Bennée wrote:
>> I'm not sure if this is neater than the original code but it does
>> remove a bunch of the !strcmp's in favour of glib's more natural bool
>> results. While we are at it make the function a bool return and fixup
>> the fake_open function prototypes.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  linux-user/syscall.c | 30 ++++++++++++------------------
>>  1 file changed, 12 insertions(+), 18 deletions(-)
>> 
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index e739921e86..18e953de9d 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -7987,33 +7987,27 @@ static int open_self_auxv(void *cpu_env, int fd)
>>      return 0;
>>  }
>>  
>> -static int is_proc_myself(const char *filename, const char *entry)
>> +static bool is_proc_myself(const char *filename, const char *entry)
>>  {
>> -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
>> +    if (g_str_has_prefix(filename, "/proc/")) {
>>          filename += strlen("/proc/");
>> -        if (!strncmp(filename, "self/", strlen("self/"))) {
>> +        if (g_str_has_prefix(filename, "self/")) {
>>              filename += strlen("self/");
>> -        } else if (*filename >= '1' && *filename <= '9') {
>> -            char myself[80];
>> -            snprintf(myself, sizeof(myself), "%d/", getpid());
>> -            if (!strncmp(filename, myself, strlen(myself))) {
>> -                filename += strlen(myself);
>> -            } else {
>> -                return 0;
>> +        } else if (g_ascii_isdigit(*filename)) {
>> +            g_autofree char * myself = g_strdup_printf("%d/", getpid());
>> +            if (!g_str_has_prefix(filename, myself)) {
>> +                return false;
>>              }
>> -        } else {
>> -            return 0;
>> -        }
>> -        if (!strcmp(filename, entry)) {
>> -            return 1;
>> +            filename += strlen(myself);
>>          }
>> +        return g_str_has_prefix(filename, entry);
>>      }
>> -    return 0;
>> +    return false;
>>  }
>
> Diff is hard to compare, so this is what it looks like:
>
>  static bool is_proc_myself(const char *filename, const char *entry)
>  {
>      if (g_str_has_prefix(filename, "/proc/")) {
>          filename += strlen("/proc/");
>          if (g_str_has_prefix(filename, "self/")) {
>              filename += strlen("self/");
>          } else if (g_ascii_isdigit(*filename)) {
>              g_autofree char * myself = g_strdup_printf("%d/", getpid());
>              if (!g_str_has_prefix(filename, myself)) {
>                  return false;
>              }
>              filename += strlen(myself);
>          }
>          return g_str_has_prefix(filename, entry);
>      }
>      return false;
>  }
>
> I think if we don't mind two heap allocs it can be simplified to:
>
>   static int is_proc_myself(const char *filename, const char *entry)
>   {
>     g_autofree char *procself = g_strdup_printf("/proc/self/%s", entry);
>     g_autofree char *procpid = g_strdup_printf("/proc/%d/%s", getpid(), entry);
>     return g_str_equal(filename, procself) || g_str_equal(filename, procpid);
>   }

Ahh nice, even simpler and easy to follow. I don't think the double
alloc is too much of a concern because we are typically on a syscall
path anyway so have a bunch of stuff to check.

> This makes me wonder if the code needs to cope with non-canonicalized
> filenames though. eg /proc///self/maps  or /proc/./self/maps
>
> Is something further up ensuring canonicalization of 'filename' ?

It seems so from my cursory pokes but I'm not convinced all paths do. We
could throw in a g_canonicalize_filename to be sure.

>
>
>>  
>>  #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN) || \
>>      defined(TARGET_SPARC) || defined(TARGET_M68K) || defined(TARGET_HPPA)
>> -static int is_proc(const char *filename, const char *entry)
>> +static bool is_proc(const char *filename, const char *entry)
>>  {
>>      return strcmp(filename, entry) == 0;
>>  }
>> @@ -8097,7 +8091,7 @@ static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags,
>>      struct fake_open {
>>          const char *filename;
>>          int (*fill)(void *cpu_env, int fd);
>> -        int (*cmp)(const char *s1, const char *s2);
>> +        bool (*cmp)(const char *s1, const char *s2);
>>      };
>>      const struct fake_open *fake_open;
>>      static const struct fake_open fakes[] = {
>> -- 
>> 2.20.1
>> 
>> 
>
> Regards,
> Daniel


-- 
Alex Bennée

Re: [RFC PATCH] linux-user: glib-ify is_proc_myself

[Richard Henderson]

On 5/24/21 4:23 AM, Alex Bennée wrote:
> +        } else if (g_ascii_isdigit(*filename)) {
> +            g_autofree char * myself = g_strdup_printf("%d/", getpid());
> +            if (!g_str_has_prefix(filename, myself)) {

This seems roundabout.  Wouldn't it be better to qemu_strtoul and compare the 
integers?


r~