Re: [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
On Fri, Sep 06, 2019 at 12:12:23PM +0100, Dr. David Alan Gilbert wrote: > * Daniel P. Berrangé (berra...@redhat.com) wrote: > > On Fri, Sep 06, 2019 at 11:23:28AM +0100, Stefan Hajnoczi wrote: > > > On Thu, Sep 05, 2019 at 06:27:32PM +0100, Dr. David Alan Gilbert wrote: > > > > * Stefan Hajnoczi (stefa...@redhat.com) wrote: > > > > > Introduce a DBus server thread that runs alongside the other virtiofsd > > > > > threads. It processes changes to the /org/qemu/virtiofsd object which > > > > > can be accessed at the org.qemu.virtiofsd location on the bus. > > > > > > > > > > This code does not use locking because we are the only writer to the > > > > > int current_log_level variable. More advanced management commands > > > > > would > > > > > require locking to prevent race conditions with the other threads. > > > > > > > > > > Signed-off-by: Stefan Hajnoczi > > > > > > > > OK, that is less complex than I'd feared. > > > > I guess there's something probably nice to do with name/integer mapping > > > > for warning levels that we could use from one of the libraries. > > > > > > I used a free-form string because it's what systemd's LogLevel property > > > also does. But I can investigate the cleanest approach for limiting it > > > to a set of string constants. > > > > There's no concept of "enums" at the DBus protocol level. Sending enums > > in string form is the normal practice - avoiding integer values means > > you are not vulnerable to enum values changing if someone inserts a new > > constant in the middlle of the enum. This same reason is why QAPI uses > > strings for enums instead of ints. > > Oh, I wasn't talking aobut changing protocol; I just meant there was > probably a neater way of doing the string look up than the opencoded way > it was done. Oh sure, you can declare the enum in a header, and then run glib-mkenums with that header file and it will spit out the code neccessary to register a GEnum class. This gives you ability to do int/string conversions with a simple api https://developer.gnome.org/gobject/stable/gobject-Enumeration-and-Flag-Types.html https://developer.gnome.org/gobject/stable/glib-mkenums.html It makes sense to use this, since virtiofsd is already using GObject via GDbus. 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 :|
Re: [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
* Daniel P. Berrangé (berra...@redhat.com) wrote: > On Fri, Sep 06, 2019 at 11:23:28AM +0100, Stefan Hajnoczi wrote: > > On Thu, Sep 05, 2019 at 06:27:32PM +0100, Dr. David Alan Gilbert wrote: > > > * Stefan Hajnoczi (stefa...@redhat.com) wrote: > > > > Introduce a DBus server thread that runs alongside the other virtiofsd > > > > threads. It processes changes to the /org/qemu/virtiofsd object which > > > > can be accessed at the org.qemu.virtiofsd location on the bus. > > > > > > > > This code does not use locking because we are the only writer to the > > > > int current_log_level variable. More advanced management commands would > > > > require locking to prevent race conditions with the other threads. > > > > > > > > Signed-off-by: Stefan Hajnoczi > > > > > > OK, that is less complex than I'd feared. > > > I guess there's something probably nice to do with name/integer mapping > > > for warning levels that we could use from one of the libraries. > > > > I used a free-form string because it's what systemd's LogLevel property > > also does. But I can investigate the cleanest approach for limiting it > > to a set of string constants. > > There's no concept of "enums" at the DBus protocol level. Sending enums > in string form is the normal practice - avoiding integer values means > you are not vulnerable to enum values changing if someone inserts a new > constant in the middlle of the enum. This same reason is why QAPI uses > strings for enums instead of ints. Oh, I wasn't talking aobut changing protocol; I just meant there was probably a neater way of doing the string look up than the opencoded way it was done. Dave > 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 :| -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
On Fri, Sep 06, 2019 at 11:23:28AM +0100, Stefan Hajnoczi wrote: > On Thu, Sep 05, 2019 at 06:27:32PM +0100, Dr. David Alan Gilbert wrote: > > * Stefan Hajnoczi (stefa...@redhat.com) wrote: > > > Introduce a DBus server thread that runs alongside the other virtiofsd > > > threads. It processes changes to the /org/qemu/virtiofsd object which > > > can be accessed at the org.qemu.virtiofsd location on the bus. > > > > > > This code does not use locking because we are the only writer to the > > > int current_log_level variable. More advanced management commands would > > > require locking to prevent race conditions with the other threads. > > > > > > Signed-off-by: Stefan Hajnoczi > > > > OK, that is less complex than I'd feared. > > I guess there's something probably nice to do with name/integer mapping > > for warning levels that we could use from one of the libraries. > > I used a free-form string because it's what systemd's LogLevel property > also does. But I can investigate the cleanest approach for limiting it > to a set of string constants. There's no concept of "enums" at the DBus protocol level. Sending enums in string form is the normal practice - avoiding integer values means you are not vulnerable to enum values changing if someone inserts a new constant in the middlle of the enum. This same reason is why QAPI uses strings for enums instead of ints. 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 :|
Re: [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
On Thu, Sep 05, 2019 at 06:27:32PM +0100, Dr. David Alan Gilbert wrote: > * Stefan Hajnoczi (stefa...@redhat.com) wrote: > > Introduce a DBus server thread that runs alongside the other virtiofsd > > threads. It processes changes to the /org/qemu/virtiofsd object which > > can be accessed at the org.qemu.virtiofsd location on the bus. > > > > This code does not use locking because we are the only writer to the > > int current_log_level variable. More advanced management commands would > > require locking to prevent race conditions with the other threads. > > > > Signed-off-by: Stefan Hajnoczi > > OK, that is less complex than I'd feared. > I guess there's something probably nice to do with name/integer mapping > for warning levels that we could use from one of the libraries. I used a free-form string because it's what systemd's LogLevel property also does. But I can investigate the cleanest approach for limiting it to a set of string constants. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
* Stefan Hajnoczi (stefa...@redhat.com) wrote:
> Introduce a DBus server thread that runs alongside the other virtiofsd
> threads. It processes changes to the /org/qemu/virtiofsd object which
> can be accessed at the org.qemu.virtiofsd location on the bus.
>
> This code does not use locking because we are the only writer to the
> int current_log_level variable. More advanced management commands would
> require locking to prevent race conditions with the other threads.
>
> Signed-off-by: Stefan Hajnoczi
OK, that is less complex than I'd feared.
I guess there's something probably nice to do with name/integer mapping
for warning levels that we could use from one of the libraries.
Dave
> ---
> contrib/virtiofsd/Makefile.objs| 3 +-
> contrib/virtiofsd/dbus.h | 9 ++
> contrib/virtiofsd/dbus.c | 162 +
> contrib/virtiofsd/passthrough_ll.c | 8 +-
> 4 files changed, 180 insertions(+), 2 deletions(-)
> create mode 100644 contrib/virtiofsd/dbus.h
> create mode 100644 contrib/virtiofsd/dbus.c
>
> diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
> index 9b2af1bc23..d59ab60f3d 100644
> --- a/contrib/virtiofsd/Makefile.objs
> +++ b/contrib/virtiofsd/Makefile.objs
> @@ -8,7 +8,8 @@ virtiofsd-obj-y = buffer.o \
>helper.o \
>passthrough_ll.o \
>seccomp.o \
> - gdbus_generated.o
> + gdbus_generated.o \
> + dbus.o
>
> seccomp.o-cflags := $(SECCOMP_CFLAGS)
> seccomp.o-libs := $(SECCOMP_LIBS)
> diff --git a/contrib/virtiofsd/dbus.h b/contrib/virtiofsd/dbus.h
> new file mode 100644
> index 00..aa18e47408
> --- /dev/null
> +++ b/contrib/virtiofsd/dbus.h
> @@ -0,0 +1,9 @@
> +#ifndef DBUS_H
> +#define DBUS_H
> +
> +#include
> +
> +bool setup_dbus(void);
> +void cleanup_dbus(void);
> +
> +#endif /* DBUS_H */
> diff --git a/contrib/virtiofsd/dbus.c b/contrib/virtiofsd/dbus.c
> new file mode 100644
> index 00..bc2308e34b
> --- /dev/null
> +++ b/contrib/virtiofsd/dbus.c
> @@ -0,0 +1,162 @@
> +#include
> +#include
> +#include
> +#include "fuse_log.h"
> +#include "dbus.h"
> +#include "gdbus_generated.h"
> +
> +static GThread *the_dbus_thread;
> +static GMainContext *the_dbus_context;
> +static GMainLoop *the_dbus_loop;
> +
> +/* Set the string property based on the current log level */
> +static void refresh_log_level(Virtiofsd *virtiofsd)
> +{
> +switch (current_log_level) {
> +case LOG_ERR:
> +virtiofsd_set_log_level(virtiofsd, "err");
> +break;
> +case LOG_WARNING:
> +virtiofsd_set_log_level(virtiofsd, "warning");
> +break;
> +case LOG_INFO:
> +virtiofsd_set_log_level(virtiofsd, "info");
> +break;
> +case LOG_DEBUG:
> +virtiofsd_set_log_level(virtiofsd, "debug");
> +break;
> +}
> +}
> +
> +/* Handle changes to Virtiofsd object properties */
> +static void notify(GObject *object, GParamSpec *pspec)
> +{
> +Virtiofsd *virtiofsd = VIRTIOFSD(object);
> +
> +fprintf(stderr, "%s %s\n", __func__, pspec->name);
> +
> +if (strcmp(pspec->name, "log-level") == 0) {
> +const char *s = virtiofsd_get_log_level(virtiofsd);
> +
> +if (strcmp(s, "err") == 0) {
> +current_log_level = LOG_ERR;
> +} else if (strcmp(s, "warning") == 0) {
> +current_log_level = LOG_WARNING;
> +} else if (strcmp(s, "info") == 0) {
> +current_log_level = LOG_INFO;
> +} else if (strcmp(s, "debug") == 0) {
> +current_log_level = LOG_DEBUG;
> +} else {
> +/* Invalid, reset the log level property */
> +refresh_log_level(virtiofsd);
> +}
> +}
> +}
> +
> +typedef struct {
> +Virtiofsd *virtiofsd;
> +pthread_barrier_t *started;
> +} GBusOwnNameData;
> +
> +static void bus_acquired(GDBusConnection *connection, const gchar *name,
> +gpointer user_data)
> +{
> +GBusOwnNameData *data = user_data;
> +GError *error = NULL;
> +
> +if (!g_dbus_interface_skeleton_export(
> +G_DBUS_INTERFACE_SKELETON(data->virtiofsd),
> +connection, "/org/qemu/virtiofsd", )) {
> +fuse_err("g_dbus_interface_skeleton_export: %s\n", error->message);
> +g_error_free(error);
> +exit(EXIT_FAILURE);
> +}
> +}
> +
> +static void name_acquired(GDBusConnection *connection, const gchar *name,
> +gpointer user_data)
> +{
> +GBusOwnNameData *data = user_data;
> +
> +pthread_barrier_wait(data->started);
> +}
> +
> +static void name_lost(GDBusConnection *connection, const gchar *name,
> +gpointer user_data)
> +{
> +if (connection) {
> +fuse_err("unable to own dbus name\n");
> +} else {
> +fuse_err("unable to connect to dbus\n");
> +}
> +exit(EXIT_FAILURE);
> +}
> +
>
