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

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|