* [RFC PATCH] linux-user: glib-ify is_proc_myself
@ 2021-05-24 11:23 Alex Bennée
2021-05-24 11:27 ` no-reply
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Alex Bennée @ 2021-05-24 11:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Bennée, Laurent Vivier, yamamoto
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] linux-user: glib-ify is_proc_myself
2021-05-24 11:23 [RFC PATCH] linux-user: glib-ify is_proc_myself Alex Bennée
@ 2021-05-24 11:27 ` no-reply
2021-05-24 11:35 ` Daniel P. Berrangé
2021-05-24 19:12 ` Richard Henderson
2 siblings, 0 replies; 5+ messages in thread
From: no-reply @ 2021-05-24 11:27 UTC (permalink / raw)
To: alex.bennee; +Cc: yamamoto, alex.bennee, qemu-devel, laurent
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] linux-user: glib-ify is_proc_myself
2021-05-24 11:23 [RFC PATCH] linux-user: glib-ify is_proc_myself Alex Bennée
2021-05-24 11:27 ` no-reply
@ 2021-05-24 11:35 ` Daniel P. Berrangé
2021-05-24 12:35 ` Alex Bennée
2021-05-24 19:12 ` Richard Henderson
2 siblings, 1 reply; 5+ messages in thread
From: Daniel P. Berrangé @ 2021-05-24 11:35 UTC (permalink / raw)
To: Alex Bennée; +Cc: yamamoto, qemu-devel, Laurent Vivier
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 :|
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] linux-user: glib-ify is_proc_myself
2021-05-24 11:35 ` Daniel P. Berrangé
@ 2021-05-24 12:35 ` Alex Bennée
0 siblings, 0 replies; 5+ messages in thread
From: Alex Bennée @ 2021-05-24 12:35 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: yamamoto, qemu-devel, Laurent Vivier
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] linux-user: glib-ify is_proc_myself
2021-05-24 11:23 [RFC PATCH] linux-user: glib-ify is_proc_myself Alex Bennée
2021-05-24 11:27 ` no-reply
2021-05-24 11:35 ` Daniel P. Berrangé
@ 2021-05-24 19:12 ` Richard Henderson
2 siblings, 0 replies; 5+ messages in thread
From: Richard Henderson @ 2021-05-24 19:12 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: Laurent Vivier, yamamoto
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~
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-24 19:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-24 11:23 [RFC PATCH] linux-user: glib-ify is_proc_myself Alex Bennée
2021-05-24 11:27 ` no-reply
2021-05-24 11:35 ` Daniel P. Berrangé
2021-05-24 12:35 ` Alex Bennée
2021-05-24 19:12 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).