Hi On Mon, Jul 12, 2021 at 11:19 PM Steven Sistare wrote: > On 7/8/2021 11:10 AM, Marc-André Lureau wrote: > > Hi > > > > On Wed, Jul 7, 2021 at 9:30 PM Steve Sistare > 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. > Why does it need to be "observable" from outside the process? I meant memory to be shared between the qemu instances (without concurrency etc). You would only need that memory fd to be passed as argument to the next qemu instance, to restore the rest of the contexts/fds I suppose. I think we need to do this right, as it may have consequences for future updates. It's effectively a kind of protocol. We have better chances to handle different versions correctly by reusing VMState imho. > > Some comments below. These new utils could also have some unit tests. > > OK. > > > Signed-off-by: Steve Sistare 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>> > > 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 > Or just *environ@N ? > 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 > -- Marc-André Lureau