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

["Alex Bennée" <alex.bennee@linaro.org>]

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