| Message ID | 20210524112323.2310-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] linux-user: glib-ify is_proc_myself | expand |
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
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 :|
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
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~
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[] = {
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(-) -- 2.20.1