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.ben...@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 :|
