Re: [PATCH V5 10/25] util: env var helpers

[Steven Sistare <steven.sistare@oracle.com>]

On 7/8/2021 11:10 AM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 7, 2021 at 9:30 PM Steve Sistare <steven.sistare@oracle.com <mailto:steven.sistare@oracle.com>> wrote:
> 
>     Add functions for saving fd's and other values in the environment via
>     setenv, and for reading them back via getenv.
> 
> 
> I understand that the rest of the series will rely on environment variables to associate and recover the child-passed FDs, but I am not really convinced that it is a good idea.
> 
> Environment variables have a number of issues that we may encounter down the road: namespace, limits, concurrency, observability etc.. I wonder if the VMState couldn't have a section about the FD to recover. Or maybe just another shared memory region?

They also have some advantages.  Their post-exec value can be observed via /proc/$pid/environ,
and modified values can be observed by calling printenv() in a debugger.  They are naturally carried
across exec, with no external file to create and potentially lose.  Lastly, libcs already defines
put and get methods, so the additional layered code is small and simple.  The number of variables
is small, and I would rather not over-engineer an alternate solution until the env proves
inadequate.  The limits on env size are huge on Linux.  The limits are smaller on Windows, but
that is just one of multiple issues to be addressed to support live update on windows.

For the alternatives, shared memory is no more observable (maybe less) and also has no concurrency
protection.  VMstate does not help because the descriptors are needed before the vmstate file
is opened.
 
> Some comments below. These new utils could also have some unit tests.

OK.

>     Signed-off-by: Steve Sistare <steven.sistare@oracle.com <mailto:steven.sistare@oracle.com>>
>     ---
>      MAINTAINERS        |  2 ++
>      include/qemu/env.h | 23 +++++++++++++
>      util/env.c         | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>      util/meson.build   |  1 +
>      4 files changed, 121 insertions(+)
>      create mode 100644 include/qemu/env.h
>      create mode 100644 util/env.c
> 
>     diff --git a/MAINTAINERS b/MAINTAINERS
>     index c48dd37..8647a97 100644
>     --- a/MAINTAINERS
>     +++ b/MAINTAINERS
>     @@ -2865,6 +2865,8 @@ S: Maintained
>      F: include/migration/cpr.h
>      F: migration/cpr.c
>      F: qapi/cpr.json
>     +F: include/qemu/env.h
>     +F: util/env.c
> 
>      Record/replay
>      M: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru <mailto:pavel.dovgaluk@ispras.ru>>
>     diff --git a/include/qemu/env.h b/include/qemu/env.h
>     new file mode 100644
>     index 0000000..3dad503
>     --- /dev/null
>     +++ b/include/qemu/env.h
>     @@ -0,0 +1,23 @@
>     +/*
>     + * Copyright (c) 2021 Oracle and/or its affiliates.
>     + *
>     + * This work is licensed under the terms of the GNU GPL, version 2.
>     + * See the COPYING file in the top-level directory.
>     + *
>     + */
>     +
>     +#ifndef QEMU_ENV_H
>     +#define QEMU_ENV_H
>     +
>     +#define FD_PREFIX "QEMU_FD_"
>     +
>     +typedef int (*walkenv_cb)(const char *name, const char *val, void *handle);
>     +
>     +int getenv_fd(const char *name);
>     +void setenv_fd(const char *name, int fd);
>     +void unsetenv_fd(const char *name);
>     +void unsetenv_fdv(const char *fmt, ...);
>     +int walkenv(const char *prefix, walkenv_cb cb, void *handle);
>     +void printenv(void);
> 
> 
> Please use qemu prefix, that avoids potential confusion with system libraries.
> 
>     +
>     +#endif
>     diff --git a/util/env.c b/util/env.c
>     new file mode 100644
>     index 0000000..863678d
>     --- /dev/null
>     +++ b/util/env.c
>     @@ -0,0 +1,95 @@
>     +/*
>     + * Copyright (c) 2021 Oracle and/or its affiliates.
>     + *
>     + * This work is licensed under the terms of the GNU GPL, version 2.
>     + * See the COPYING file in the top-level directory.
>     + */
>     +
>     +#include "qemu/osdep.h"
>     +#include "qemu/cutils.h"
>     +#include "qemu/env.h"
>     +
>     +static uint64_t getenv_ulong(const char *prefix, const char *name, int *err)
>     +{
>     +    char var[80], *val;
>     +    uint64_t res = 0;
>     +
>     +    snprintf(var, sizeof(var), "%s%s", prefix, name);
> 
> 
> No check for success / truncation...
> 
> Please use g_autofree char *var = g_strdup_printf()..
> 
>     +    val = getenv(var);
> 
> 
> For consistency, I'd use g_getenv()
> 
>     +    if (val) {
>     +        *err = qemu_strtoul(val, NULL, 10, &res);
>     +    } else {
>     +        *err = -ENOENT;
>     +    }
>     +    return res;
>     +}
>     +
>     +static void setenv_ulong(const char *prefix, const char *name, uint64_t val)
>     +{
>     +    char var[80], val_str[80];
>     +    snprintf(var, sizeof(var), "%s%s", prefix, name);
>     +    snprintf(val_str, sizeof(val_str), "%"PRIu64, val);
> 
> 
> g_strdup_printf
> 
>     +    setenv(var, val_str, 1);
> 
> 
> g_setenv(), and return error value (or assert() if that makes more sense)
> 
>     +}
>     +
>     +static void unsetenv_ulong(const char *prefix, const char *name)
>     +{
>     +    char var[80];
>     +    snprintf(var, sizeof(var), "%s%s", prefix, name);
> 
> 
> g_strdup_printf
>  
> 
>     +    unsetenv(var);
> 
> 
> g_unsetenv
> 
>     +}
>     +
>     +int getenv_fd(const char *name)
>     +{
>     +    int err;
>     +    int fd = getenv_ulong(FD_PREFIX, name, &err);
> 
> 
> I'd try to use qemu_parse_fd() instead.
> 
>     +    return err ? -1 : fd;
>     +}
>     +
>     +void setenv_fd(const char *name, int fd)
>     +{
> 
> 
> Maybe check fd >= 0 ?
> 
>     +    setenv_ulong(FD_PREFIX, name, fd);
>     +}
>     +
>     +void unsetenv_fd(const char *name)
>     +{
>     +    unsetenv_ulong(FD_PREFIX, name);
>     +}
>     +
>     +void unsetenv_fdv(const char *fmt, ...)
>     +{
>     +    va_list args;
>     +    char buf[80];
>     +    va_start(args, fmt);
>     +    vsnprintf(buf, sizeof(buf), fmt, args);
>     +    va_end(args);
> 
> 
> That seems to be a leftover.

It is called in the subsequent vfio cpr patches.

>     +}
>     +
>     +int walkenv(const char *prefix, walkenv_cb cb, void *handle)
> 
>     +{
>     +    char *str, name[128];
>     +    char **envp = environ;
>     +    size_t prefix_len = strlen(prefix);
>     +
>     +    while (*envp) {
>     +        str = *envp++;
>     +        if (!strncmp(str, prefix, prefix_len)) {
> 
>     +            char *val = strchr(str, '=');
>     +            str += prefix_len;
>     +            strncpy(name, str, val - str);
> 
> 
> g_strndup() to avoid potential buffer overflow.
> 
>     +            name[val - str] = 0;
>     +            if (cb(name, val + 1, handle)) {
>     +                return 1;
>     +            }
>     +        }
>     +    }
>     +    return 0;
>     +}
>     +
>     +void printenv(void)
>     +{
>     +    char **ptr = environ;
>     +    while (*ptr) {
>     +        puts(*ptr++);
>     +    }
>     +}
> 
> 
> Is this really useful? I doubt it.

I call it from gdb for debugging, but I can delete it and cast g_listenv() instead:
  print *(((char ** (*)(void))g_listenv)())@100

Will do on the rest.

- Steve

>     diff --git a/util/meson.build b/util/meson.build
>     index 0ffd7f4..5e8097a 100644
>     --- a/util/meson.build
>     +++ b/util/meson.build
>     @@ -23,6 +23,7 @@ util_ss.add(files('host-utils.c'))
>      util_ss.add(files('bitmap.c', 'bitops.c'))
>      util_ss.add(files('fifo8.c'))
>      util_ss.add(files('cacheinfo.c', 'cacheflush.c'))
>     +util_ss.add(files('env.c'))
>      util_ss.add(files('error.c', 'qemu-error.c'))
>      util_ss.add(files('qemu-print.c'))
>      util_ss.add(files('id.c'))
>     -- 
>     1.8.3.1
> 
> 
> 
> 
> -- 
> Marc-André Lureau