Daniel P. Berrangé <berra...@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.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);
> }
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
