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
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
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 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~
© 2016 - 2024 Red Hat, Inc.