[Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus
@ 2019-09-05 15:21 ` Stefan Hajnoczi
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-05 15:21 UTC (permalink / raw)
  To: qemu-devel, virtio-fs
  Cc: Marc-André Lureau, Eryu Guan, Dr. David Alan Gilbert,
	Stefan Hajnoczi

It is likely that virtiofsd will need to support "management commands" for
reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
getting/setting the current log level.

I promised to try out DBus as the management interface because it has a rich
feature set and is accessible from most programming languages.  It should be
able to support all the use cases we come up with.

This patch series is a prototype that implements the get-log-level and
set-log-level management commands via DBus.  Use the new virtiofsctl tool to
talk to a running virtiofsd process:

  # dbus-run-session ./virtiofsd ...
  ...
  Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
  # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
  # ./virtiofsctl set-log-level err

Most of the work is done by gdbus-codegen(1).  It generates code for the
org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.

I'm pretty happy with this approach because the code is straightforward.  It
hasn't even triggered seccomp failures yet :).

Error handling is a little problematic.  I noticed that virtiofsctl silently
returns success even if it cannot talk to virtiofsd.  This is due to the code
generated by gdbus-codegen(1) which has no error reporting :(.  This can be
solved by writing more low-level GDBus code instead of using the high-level
generated bindings.

What do you think about this approach?

Stefan Hajnoczi (3):
  virtiofsd: add org.qemu.Virtiofsd interface
  virtiofsd: add DBus server to handle log level changes
  virtiofsd: add virtiofsctl command-line management tool

 configure                                |   7 +
 Makefile                                 |  16 +++
 Makefile.objs                            |   1 +
 contrib/virtiofsd/Makefile.objs          |  10 +-
 contrib/virtiofsd/dbus.h                 |   9 ++
 contrib/virtiofsd/dbus.c                 | 162 +++++++++++++++++++++++
 contrib/virtiofsd/passthrough_ll.c       |   8 +-
 contrib/virtiofsd/virtiofsctl.c          |  55 ++++++++
 .gitignore                               |   1 +
 contrib/virtiofsd/org.qemu.Virtiofsd.xml |   7 +
 10 files changed, 274 insertions(+), 2 deletions(-)
 create mode 100644 contrib/virtiofsd/dbus.h
 create mode 100644 contrib/virtiofsd/dbus.c
 create mode 100644 contrib/virtiofsd/virtiofsctl.c
 create mode 100644 contrib/virtiofsd/org.qemu.Virtiofsd.xml

-- 
2.21.0

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [Virtio-fs] [RFC 0/3] virtiofsd: get/set log level via DBus
@ 2019-09-05 15:21 ` Stefan Hajnoczi
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-05 15:21 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: Marc-André Lureau

It is likely that virtiofsd will need to support "management commands" for
reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
getting/setting the current log level.

I promised to try out DBus as the management interface because it has a rich
feature set and is accessible from most programming languages.  It should be
able to support all the use cases we come up with.

This patch series is a prototype that implements the get-log-level and
set-log-level management commands via DBus.  Use the new virtiofsctl tool to
talk to a running virtiofsd process:

  # dbus-run-session ./virtiofsd ...
  ...
  Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
  # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
  # ./virtiofsctl set-log-level err

Most of the work is done by gdbus-codegen(1).  It generates code for the
org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.

I'm pretty happy with this approach because the code is straightforward.  It
hasn't even triggered seccomp failures yet :).

Error handling is a little problematic.  I noticed that virtiofsctl silently
returns success even if it cannot talk to virtiofsd.  This is due to the code
generated by gdbus-codegen(1) which has no error reporting :(.  This can be
solved by writing more low-level GDBus code instead of using the high-level
generated bindings.

What do you think about this approach?

Stefan Hajnoczi (3):
  virtiofsd: add org.qemu.Virtiofsd interface
  virtiofsd: add DBus server to handle log level changes
  virtiofsd: add virtiofsctl command-line management tool

 configure                                |   7 +
 Makefile                                 |  16 +++
 Makefile.objs                            |   1 +
 contrib/virtiofsd/Makefile.objs          |  10 +-
 contrib/virtiofsd/dbus.h                 |   9 ++
 contrib/virtiofsd/dbus.c                 | 162 +++++++++++++++++++++++
 contrib/virtiofsd/passthrough_ll.c       |   8 +-
 contrib/virtiofsd/virtiofsctl.c          |  55 ++++++++
 .gitignore                               |   1 +
 contrib/virtiofsd/org.qemu.Virtiofsd.xml |   7 +
 10 files changed, 274 insertions(+), 2 deletions(-)
 create mode 100644 contrib/virtiofsd/dbus.h
 create mode 100644 contrib/virtiofsd/dbus.c
 create mode 100644 contrib/virtiofsd/virtiofsctl.c
 create mode 100644 contrib/virtiofsd/org.qemu.Virtiofsd.xml

-- 
2.21.0

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [Qemu-devel] [RFC 1/3] virtiofsd: add org.qemu.Virtiofsd interface
  2019-09-05 15:21 ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-05 15:21   ` Stefan Hajnoczi
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-05 15:21 UTC (permalink / raw)
  To: qemu-devel, virtio-fs
  Cc: Marc-André Lureau, Eryu Guan, Dr. David Alan Gilbert,
	Stefan Hajnoczi

Define a DBus interface for virtiofsd management.  It only allows
querying and changing the log level at the moment.

In the future more methods and properties could be added.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 configure                                |  7 +++++++
 Makefile                                 | 13 +++++++++++++
 contrib/virtiofsd/Makefile.objs          |  6 +++++-
 .gitignore                               |  1 +
 contrib/virtiofsd/org.qemu.Virtiofsd.xml |  7 +++++++
 5 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100644 contrib/virtiofsd/org.qemu.Virtiofsd.xml

diff --git a/configure b/configure
index 2976e64b9b..17b789d772 100755
--- a/configure
+++ b/configure
@@ -3674,10 +3674,16 @@ if $pkg_config --atleast-version=$glib_req_ver gio-2.0; then
     gio=yes
     gio_cflags=$($pkg_config --cflags gio-2.0)
     gio_libs=$($pkg_config --libs gio-2.0)
+    gdbus_codegen=$($pkg_config --variable=gdbus_codegen gio-2.0)
 else
     gio=no
 fi
 
+if $pkg_config --atleast-version=$glib_req_ver gio-unix-2.0; then
+    gio_cflags="$gio_cflags $($pkg_config --cflags gio-unix-2.0)"
+    gio_libs="$gio_libs $($pkg_config --libs gio-unix-2.0)"
+fi
+
 # Sanity check that the current size_t matches the
 # size that glib thinks it should be. This catches
 # problems on multi-arch where people try to build
@@ -6856,6 +6862,7 @@ if test "$gio" = "yes" ; then
     echo "CONFIG_GIO=y" >> $config_host_mak
     echo "GIO_CFLAGS=$gio_cflags" >> $config_host_mak
     echo "GIO_LIBS=$gio_libs" >> $config_host_mak
+    echo "GDBUS_CODEGEN=$gdbus_codegen" >> $config_host_mak
 fi
 echo "CONFIG_TLS_PRIORITY=\"$tls_priority\"" >> $config_host_mak
 if test "$gnutls" = "yes" ; then
diff --git a/Makefile b/Makefile
index a3dfdd6fa8..6b1af33348 100644
--- a/Makefile
+++ b/Makefile
@@ -124,6 +124,11 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
 
 generated-files-y += $(GENERATED_QAPI_FILES)
 
+ifdef CONFIG_LINUX
+generated-files-y += contrib/virtiofsd/gdbus_generated.h
+generated-files-y += contrib/virtiofsd/gdbus_generated.c
+endif
+
 generated-files-y += trace/generated-tcg-tracers.h
 
 generated-files-y += trace/generated-helpers-wrappers.h
@@ -646,6 +651,14 @@ rdmacm-mux$(EXESUF): $(rdmacm-mux-obj-y) $(COMMON_LDADDS)
 	$(call LINK, $^)
 
 ifdef CONFIG_LINUX # relies on Linux-specific syscalls
+contrib/virtiofsd/gdbus_generated.h contrib/virtiofsd/gdbus_generated.c: contrib/virtiofsd/gdbus_generated.c-timestamp ;
+contrib/virtiofsd/gdbus_generated.c-timestamp: $(SRC_PATH)/contrib/virtiofsd/org.qemu.Virtiofsd.xml
+	$(call quiet-command,$(GDBUS_CODEGEN) $< \
+		--interface-prefix org.qemu \
+		--generate-c-code contrib/virtiofsd/gdbus_generated, \
+		"GEN","$(@:%-timestamp=%)")
+	@>$@
+
 virtiofsd$(EXESUF): $(virtiofsd-obj-y) libvhost-user.a $(COMMON_LDADDS)
 	$(call LINK, $^)
 endif
diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
index 25f1e8dd73..9b2af1bc23 100644
--- a/contrib/virtiofsd/Makefile.objs
+++ b/contrib/virtiofsd/Makefile.objs
@@ -7,9 +7,13 @@ virtiofsd-obj-y = buffer.o \
                   fuse_virtio.o \
                   helper.o \
                   passthrough_ll.o \
-                  seccomp.o
+                  seccomp.o \
+                  gdbus_generated.o
 
 seccomp.o-cflags := $(SECCOMP_CFLAGS)
 seccomp.o-libs := $(SECCOMP_LIBS)
 
+gdbus_generated.o-cflags := $(GIO_CFLAGS)
+gdbus_generated.o-libs := $(GIO_LIBS)
+
 passthrough_ll.o-libs += -lcap
diff --git a/.gitignore b/.gitignore
index fd6e6c38c7..c04d5dd0b1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -6,6 +6,7 @@
 /config-target.*
 /config.status
 /config-temp
+/contrib/virtiofsd/gdbus_generated.*
 /elf2dmp
 /trace-events-all
 /trace/generated-events.h
diff --git a/contrib/virtiofsd/org.qemu.Virtiofsd.xml b/contrib/virtiofsd/org.qemu.Virtiofsd.xml
new file mode 100644
index 0000000000..efc0c59020
--- /dev/null
+++ b/contrib/virtiofsd/org.qemu.Virtiofsd.xml
@@ -0,0 +1,7 @@
+<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN"
+         "http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd">
+<node>
+    <interface name="org.qemu.Virtiofsd">
+        <property name="LogLevel" type="s" access="readwrite"/>
+    </interface>
+</node>
-- 
2.21.0



^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Virtio-fs] [RFC 1/3] virtiofsd: add org.qemu.Virtiofsd interface
@ 2019-09-05 15:21   ` Stefan Hajnoczi
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-05 15:21 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: Marc-André Lureau

Define a DBus interface for virtiofsd management.  It only allows
querying and changing the log level at the moment.

In the future more methods and properties could be added.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 configure                                |  7 +++++++
 Makefile                                 | 13 +++++++++++++
 contrib/virtiofsd/Makefile.objs          |  6 +++++-
 .gitignore                               |  1 +
 contrib/virtiofsd/org.qemu.Virtiofsd.xml |  7 +++++++
 5 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100644 contrib/virtiofsd/org.qemu.Virtiofsd.xml

diff --git a/configure b/configure
index 2976e64b9b..17b789d772 100755
--- a/configure
+++ b/configure
@@ -3674,10 +3674,16 @@ if $pkg_config --atleast-version=$glib_req_ver gio-2.0; then
     gio=yes
     gio_cflags=$($pkg_config --cflags gio-2.0)
     gio_libs=$($pkg_config --libs gio-2.0)
+    gdbus_codegen=$($pkg_config --variable=gdbus_codegen gio-2.0)
 else
     gio=no
 fi
 
+if $pkg_config --atleast-version=$glib_req_ver gio-unix-2.0; then
+    gio_cflags="$gio_cflags $($pkg_config --cflags gio-unix-2.0)"
+    gio_libs="$gio_libs $($pkg_config --libs gio-unix-2.0)"
+fi
+
 # Sanity check that the current size_t matches the
 # size that glib thinks it should be. This catches
 # problems on multi-arch where people try to build
@@ -6856,6 +6862,7 @@ if test "$gio" = "yes" ; then
     echo "CONFIG_GIO=y" >> $config_host_mak
     echo "GIO_CFLAGS=$gio_cflags" >> $config_host_mak
     echo "GIO_LIBS=$gio_libs" >> $config_host_mak
+    echo "GDBUS_CODEGEN=$gdbus_codegen" >> $config_host_mak
 fi
 echo "CONFIG_TLS_PRIORITY=\"$tls_priority\"" >> $config_host_mak
 if test "$gnutls" = "yes" ; then
diff --git a/Makefile b/Makefile
index a3dfdd6fa8..6b1af33348 100644
--- a/Makefile
+++ b/Makefile
@@ -124,6 +124,11 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
 
 generated-files-y += $(GENERATED_QAPI_FILES)
 
+ifdef CONFIG_LINUX
+generated-files-y += contrib/virtiofsd/gdbus_generated.h
+generated-files-y += contrib/virtiofsd/gdbus_generated.c
+endif
+
 generated-files-y += trace/generated-tcg-tracers.h
 
 generated-files-y += trace/generated-helpers-wrappers.h
@@ -646,6 +651,14 @@ rdmacm-mux$(EXESUF): $(rdmacm-mux-obj-y) $(COMMON_LDADDS)
 	$(call LINK, $^)
 
 ifdef CONFIG_LINUX # relies on Linux-specific syscalls
+contrib/virtiofsd/gdbus_generated.h contrib/virtiofsd/gdbus_generated.c: contrib/virtiofsd/gdbus_generated.c-timestamp ;
+contrib/virtiofsd/gdbus_generated.c-timestamp: $(SRC_PATH)/contrib/virtiofsd/org.qemu.Virtiofsd.xml
+	$(call quiet-command,$(GDBUS_CODEGEN) $< \
+		--interface-prefix org.qemu \
+		--generate-c-code contrib/virtiofsd/gdbus_generated, \
+		"GEN","$(@:%-timestamp=%)")
+	@>$@
+
 virtiofsd$(EXESUF): $(virtiofsd-obj-y) libvhost-user.a $(COMMON_LDADDS)
 	$(call LINK, $^)
 endif
diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
index 25f1e8dd73..9b2af1bc23 100644
--- a/contrib/virtiofsd/Makefile.objs
+++ b/contrib/virtiofsd/Makefile.objs
@@ -7,9 +7,13 @@ virtiofsd-obj-y = buffer.o \
                   fuse_virtio.o \
                   helper.o \
                   passthrough_ll.o \
-                  seccomp.o
+                  seccomp.o \
+                  gdbus_generated.o
 
 seccomp.o-cflags := $(SECCOMP_CFLAGS)
 seccomp.o-libs := $(SECCOMP_LIBS)
 
+gdbus_generated.o-cflags := $(GIO_CFLAGS)
+gdbus_generated.o-libs := $(GIO_LIBS)
+
 passthrough_ll.o-libs += -lcap
diff --git a/.gitignore b/.gitignore
index fd6e6c38c7..c04d5dd0b1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -6,6 +6,7 @@
 /config-target.*
 /config.status
 /config-temp
+/contrib/virtiofsd/gdbus_generated.*
 /elf2dmp
 /trace-events-all
 /trace/generated-events.h
diff --git a/contrib/virtiofsd/org.qemu.Virtiofsd.xml b/contrib/virtiofsd/org.qemu.Virtiofsd.xml
new file mode 100644
index 0000000000..efc0c59020
--- /dev/null
+++ b/contrib/virtiofsd/org.qemu.Virtiofsd.xml
@@ -0,0 +1,7 @@
+<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN"
+         "http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd">
+<node>
+    <interface name="org.qemu.Virtiofsd">
+        <property name="LogLevel" type="s" access="readwrite"/>
+    </interface>
+</node>
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
  2019-09-05 15:21 ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-05 15:21   ` Stefan Hajnoczi
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-05 15:21 UTC (permalink / raw)
  To: qemu-devel, virtio-fs
  Cc: Marc-André Lureau, Eryu Guan, Dr. David Alan Gilbert,
	Stefan Hajnoczi

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 <stefanha@redhat.com>
---
 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 0000000000..aa18e47408
--- /dev/null
+++ b/contrib/virtiofsd/dbus.h
@@ -0,0 +1,9 @@
+#ifndef DBUS_H
+#define DBUS_H
+
+#include <stdbool.h>
+
+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 0000000000..bc2308e34b
--- /dev/null
+++ b/contrib/virtiofsd/dbus.c
@@ -0,0 +1,162 @@
+#include <assert.h>
+#include <stdio.h>
+#include <glib.h>
+#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", &error)) {
+        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);
+}
+
+static gpointer dbus_thread(gpointer opaque)
+{
+    GBusOwnNameData data;
+    Virtiofsd *virtiofsd;
+    guint owner_id;
+
+    g_main_context_push_thread_default(the_dbus_context);
+
+    virtiofsd = virtiofsd_skeleton_new();
+    refresh_log_level(virtiofsd);
+    g_signal_connect(virtiofsd, "notify", G_CALLBACK(notify), NULL);
+
+    data.virtiofsd = virtiofsd;
+    data.started = opaque;
+
+    owner_id = g_bus_own_name(G_BUS_TYPE_SESSION, "org.qemu.virtiofsd",
+            G_BUS_NAME_OWNER_FLAGS_DO_NOT_QUEUE, bus_acquired, name_acquired,
+            name_lost, &data, NULL);
+
+    g_main_loop_run(the_dbus_loop);
+    g_bus_unown_name(owner_id);
+    g_object_unref(virtiofsd);
+
+    g_main_context_pop_thread_default(the_dbus_context);
+    return NULL;
+}
+
+/**
+ * Start DBus server thread.
+ *
+ * Returns: true on success, false on failure
+ */
+bool setup_dbus(void)
+{
+    pthread_barrier_t started;
+
+    assert(!the_dbus_thread);
+
+    fuse_info("Using dbus address %s\n",
+              getenv("DBUS_SESSION_BUS_ADDRESS") ?: "(null)");
+
+    pthread_barrier_init(&started, NULL, 2);
+
+    the_dbus_context = g_main_context_new();
+    the_dbus_loop = g_main_loop_new(the_dbus_context, FALSE);
+    the_dbus_thread = g_thread_new("dbus-thread", dbus_thread, &started);
+
+    pthread_barrier_wait(&started);
+    pthread_barrier_destroy(&started);
+
+    return true;
+}
+
+/**
+ * Stop DBus server thread.
+ */
+void cleanup_dbus(void)
+{
+    g_main_loop_quit(the_dbus_loop);
+    g_thread_join(the_dbus_thread);
+    the_dbus_thread = NULL;
+
+    g_main_loop_unref(the_dbus_loop);
+    the_dbus_loop = NULL;
+
+    g_main_context_unref(the_dbus_context);
+    the_dbus_context = NULL;
+}
diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 0ef01b7e3f..0ddd7d280a 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -66,6 +66,7 @@
 #include <gmodule.h>
 #include "fuse_log.h"
 #include "seccomp.h"
+#include "dbus.h"
 
 /* Keep track of inode posix locks for each owner. */
 struct lo_inode_plock {
@@ -2989,6 +2990,9 @@ int main(int argc, char *argv[])
 	if (fuse_session_mount(se) != 0)
 	    goto err_out3;
 
+	if (!setup_dbus())
+	    goto err_out4;
+
 	fuse_daemonize(opts.foreground);
 
 	if (lo.ireg_sock != -1) {
@@ -2998,7 +3002,7 @@ int main(int argc, char *argv[])
 		if (ret) {
 			warnx("pthread_create: %s", strerror(ret));
 			ret = 1;
-			goto err_out4;
+			goto err_out5;
 		}
 
 		get_shared(&lo, &lo.root);
@@ -3014,6 +3018,8 @@ int main(int argc, char *argv[])
 	/* Block until ctrl+c or fusermount -u */
         ret = virtio_loop(se);
 
+err_out5:
+	cleanup_dbus();
 err_out4:
 	fuse_session_unmount(se);
 err_out3:
-- 
2.21.0



^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Virtio-fs] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
@ 2019-09-05 15:21   ` Stefan Hajnoczi
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-05 15:21 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: Marc-André Lureau

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 <stefanha@redhat.com>
---
 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 0000000000..aa18e47408
--- /dev/null
+++ b/contrib/virtiofsd/dbus.h
@@ -0,0 +1,9 @@
+#ifndef DBUS_H
+#define DBUS_H
+
+#include <stdbool.h>
+
+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 0000000000..bc2308e34b
--- /dev/null
+++ b/contrib/virtiofsd/dbus.c
@@ -0,0 +1,162 @@
+#include <assert.h>
+#include <stdio.h>
+#include <glib.h>
+#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", &error)) {
+        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);
+}
+
+static gpointer dbus_thread(gpointer opaque)
+{
+    GBusOwnNameData data;
+    Virtiofsd *virtiofsd;
+    guint owner_id;
+
+    g_main_context_push_thread_default(the_dbus_context);
+
+    virtiofsd = virtiofsd_skeleton_new();
+    refresh_log_level(virtiofsd);
+    g_signal_connect(virtiofsd, "notify", G_CALLBACK(notify), NULL);
+
+    data.virtiofsd = virtiofsd;
+    data.started = opaque;
+
+    owner_id = g_bus_own_name(G_BUS_TYPE_SESSION, "org.qemu.virtiofsd",
+            G_BUS_NAME_OWNER_FLAGS_DO_NOT_QUEUE, bus_acquired, name_acquired,
+            name_lost, &data, NULL);
+
+    g_main_loop_run(the_dbus_loop);
+    g_bus_unown_name(owner_id);
+    g_object_unref(virtiofsd);
+
+    g_main_context_pop_thread_default(the_dbus_context);
+    return NULL;
+}
+
+/**
+ * Start DBus server thread.
+ *
+ * Returns: true on success, false on failure
+ */
+bool setup_dbus(void)
+{
+    pthread_barrier_t started;
+
+    assert(!the_dbus_thread);
+
+    fuse_info("Using dbus address %s\n",
+              getenv("DBUS_SESSION_BUS_ADDRESS") ?: "(null)");
+
+    pthread_barrier_init(&started, NULL, 2);
+
+    the_dbus_context = g_main_context_new();
+    the_dbus_loop = g_main_loop_new(the_dbus_context, FALSE);
+    the_dbus_thread = g_thread_new("dbus-thread", dbus_thread, &started);
+
+    pthread_barrier_wait(&started);
+    pthread_barrier_destroy(&started);
+
+    return true;
+}
+
+/**
+ * Stop DBus server thread.
+ */
+void cleanup_dbus(void)
+{
+    g_main_loop_quit(the_dbus_loop);
+    g_thread_join(the_dbus_thread);
+    the_dbus_thread = NULL;
+
+    g_main_loop_unref(the_dbus_loop);
+    the_dbus_loop = NULL;
+
+    g_main_context_unref(the_dbus_context);
+    the_dbus_context = NULL;
+}
diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 0ef01b7e3f..0ddd7d280a 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -66,6 +66,7 @@
 #include <gmodule.h>
 #include "fuse_log.h"
 #include "seccomp.h"
+#include "dbus.h"
 
 /* Keep track of inode posix locks for each owner. */
 struct lo_inode_plock {
@@ -2989,6 +2990,9 @@ int main(int argc, char *argv[])
 	if (fuse_session_mount(se) != 0)
 	    goto err_out3;
 
+	if (!setup_dbus())
+	    goto err_out4;
+
 	fuse_daemonize(opts.foreground);
 
 	if (lo.ireg_sock != -1) {
@@ -2998,7 +3002,7 @@ int main(int argc, char *argv[])
 		if (ret) {
 			warnx("pthread_create: %s", strerror(ret));
 			ret = 1;
-			goto err_out4;
+			goto err_out5;
 		}
 
 		get_shared(&lo, &lo.root);
@@ -3014,6 +3018,8 @@ int main(int argc, char *argv[])
 	/* Block until ctrl+c or fusermount -u */
         ret = virtio_loop(se);
 
+err_out5:
+	cleanup_dbus();
 err_out4:
 	fuse_session_unmount(se);
 err_out3:
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
  2019-09-05 15:21   ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-05 17:27     ` Dr. David Alan Gilbert
  -1 siblings, 0 replies; 36+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-05 17:27 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, Eryu Guan, qemu-devel

* Stefan Hajnoczi (stefanha@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 <stefanha@redhat.com>

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 0000000000..aa18e47408
> --- /dev/null
> +++ b/contrib/virtiofsd/dbus.h
> @@ -0,0 +1,9 @@
> +#ifndef DBUS_H
> +#define DBUS_H
> +
> +#include <stdbool.h>
> +
> +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 0000000000..bc2308e34b
> --- /dev/null
> +++ b/contrib/virtiofsd/dbus.c
> @@ -0,0 +1,162 @@
> +#include <assert.h>
> +#include <stdio.h>
> +#include <glib.h>
> +#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", &error)) {
> +        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);
> +}
> +
> +static gpointer dbus_thread(gpointer opaque)
> +{
> +    GBusOwnNameData data;
> +    Virtiofsd *virtiofsd;
> +    guint owner_id;
> +
> +    g_main_context_push_thread_default(the_dbus_context);
> +
> +    virtiofsd = virtiofsd_skeleton_new();
> +    refresh_log_level(virtiofsd);
> +    g_signal_connect(virtiofsd, "notify", G_CALLBACK(notify), NULL);
> +
> +    data.virtiofsd = virtiofsd;
> +    data.started = opaque;
> +
> +    owner_id = g_bus_own_name(G_BUS_TYPE_SESSION, "org.qemu.virtiofsd",
> +            G_BUS_NAME_OWNER_FLAGS_DO_NOT_QUEUE, bus_acquired, name_acquired,
> +            name_lost, &data, NULL);
> +
> +    g_main_loop_run(the_dbus_loop);
> +    g_bus_unown_name(owner_id);
> +    g_object_unref(virtiofsd);
> +
> +    g_main_context_pop_thread_default(the_dbus_context);
> +    return NULL;
> +}
> +
> +/**
> + * Start DBus server thread.
> + *
> + * Returns: true on success, false on failure
> + */
> +bool setup_dbus(void)
> +{
> +    pthread_barrier_t started;
> +
> +    assert(!the_dbus_thread);
> +
> +    fuse_info("Using dbus address %s\n",
> +              getenv("DBUS_SESSION_BUS_ADDRESS") ?: "(null)");
> +
> +    pthread_barrier_init(&started, NULL, 2);
> +
> +    the_dbus_context = g_main_context_new();
> +    the_dbus_loop = g_main_loop_new(the_dbus_context, FALSE);
> +    the_dbus_thread = g_thread_new("dbus-thread", dbus_thread, &started);
> +
> +    pthread_barrier_wait(&started);
> +    pthread_barrier_destroy(&started);
> +
> +    return true;
> +}
> +
> +/**
> + * Stop DBus server thread.
> + */
> +void cleanup_dbus(void)
> +{
> +    g_main_loop_quit(the_dbus_loop);
> +    g_thread_join(the_dbus_thread);
> +    the_dbus_thread = NULL;
> +
> +    g_main_loop_unref(the_dbus_loop);
> +    the_dbus_loop = NULL;
> +
> +    g_main_context_unref(the_dbus_context);
> +    the_dbus_context = NULL;
> +}
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index 0ef01b7e3f..0ddd7d280a 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -66,6 +66,7 @@
>  #include <gmodule.h>
>  #include "fuse_log.h"
>  #include "seccomp.h"
> +#include "dbus.h"
>  
>  /* Keep track of inode posix locks for each owner. */
>  struct lo_inode_plock {
> @@ -2989,6 +2990,9 @@ int main(int argc, char *argv[])
>  	if (fuse_session_mount(se) != 0)
>  	    goto err_out3;
>  
> +	if (!setup_dbus())
> +	    goto err_out4;
> +
>  	fuse_daemonize(opts.foreground);
>  
>  	if (lo.ireg_sock != -1) {
> @@ -2998,7 +3002,7 @@ int main(int argc, char *argv[])
>  		if (ret) {
>  			warnx("pthread_create: %s", strerror(ret));
>  			ret = 1;
> -			goto err_out4;
> +			goto err_out5;
>  		}
>  
>  		get_shared(&lo, &lo.root);
> @@ -3014,6 +3018,8 @@ int main(int argc, char *argv[])
>  	/* Block until ctrl+c or fusermount -u */
>          ret = virtio_loop(se);
>  
> +err_out5:
> +	cleanup_dbus();
>  err_out4:
>  	fuse_session_unmount(se);
>  err_out3:
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
@ 2019-09-05 17:27     ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 36+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-05 17:27 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

* Stefan Hajnoczi (stefanha@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 <stefanha@redhat.com>

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 0000000000..aa18e47408
> --- /dev/null
> +++ b/contrib/virtiofsd/dbus.h
> @@ -0,0 +1,9 @@
> +#ifndef DBUS_H
> +#define DBUS_H
> +
> +#include <stdbool.h>
> +
> +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 0000000000..bc2308e34b
> --- /dev/null
> +++ b/contrib/virtiofsd/dbus.c
> @@ -0,0 +1,162 @@
> +#include <assert.h>
> +#include <stdio.h>
> +#include <glib.h>
> +#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", &error)) {
> +        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);
> +}
> +
> +static gpointer dbus_thread(gpointer opaque)
> +{
> +    GBusOwnNameData data;
> +    Virtiofsd *virtiofsd;
> +    guint owner_id;
> +
> +    g_main_context_push_thread_default(the_dbus_context);
> +
> +    virtiofsd = virtiofsd_skeleton_new();
> +    refresh_log_level(virtiofsd);
> +    g_signal_connect(virtiofsd, "notify", G_CALLBACK(notify), NULL);
> +
> +    data.virtiofsd = virtiofsd;
> +    data.started = opaque;
> +
> +    owner_id = g_bus_own_name(G_BUS_TYPE_SESSION, "org.qemu.virtiofsd",
> +            G_BUS_NAME_OWNER_FLAGS_DO_NOT_QUEUE, bus_acquired, name_acquired,
> +            name_lost, &data, NULL);
> +
> +    g_main_loop_run(the_dbus_loop);
> +    g_bus_unown_name(owner_id);
> +    g_object_unref(virtiofsd);
> +
> +    g_main_context_pop_thread_default(the_dbus_context);
> +    return NULL;
> +}
> +
> +/**
> + * Start DBus server thread.
> + *
> + * Returns: true on success, false on failure
> + */
> +bool setup_dbus(void)
> +{
> +    pthread_barrier_t started;
> +
> +    assert(!the_dbus_thread);
> +
> +    fuse_info("Using dbus address %s\n",
> +              getenv("DBUS_SESSION_BUS_ADDRESS") ?: "(null)");
> +
> +    pthread_barrier_init(&started, NULL, 2);
> +
> +    the_dbus_context = g_main_context_new();
> +    the_dbus_loop = g_main_loop_new(the_dbus_context, FALSE);
> +    the_dbus_thread = g_thread_new("dbus-thread", dbus_thread, &started);
> +
> +    pthread_barrier_wait(&started);
> +    pthread_barrier_destroy(&started);
> +
> +    return true;
> +}
> +
> +/**
> + * Stop DBus server thread.
> + */
> +void cleanup_dbus(void)
> +{
> +    g_main_loop_quit(the_dbus_loop);
> +    g_thread_join(the_dbus_thread);
> +    the_dbus_thread = NULL;
> +
> +    g_main_loop_unref(the_dbus_loop);
> +    the_dbus_loop = NULL;
> +
> +    g_main_context_unref(the_dbus_context);
> +    the_dbus_context = NULL;
> +}
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index 0ef01b7e3f..0ddd7d280a 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -66,6 +66,7 @@
>  #include <gmodule.h>
>  #include "fuse_log.h"
>  #include "seccomp.h"
> +#include "dbus.h"
>  
>  /* Keep track of inode posix locks for each owner. */
>  struct lo_inode_plock {
> @@ -2989,6 +2990,9 @@ int main(int argc, char *argv[])
>  	if (fuse_session_mount(se) != 0)
>  	    goto err_out3;
>  
> +	if (!setup_dbus())
> +	    goto err_out4;
> +
>  	fuse_daemonize(opts.foreground);
>  
>  	if (lo.ireg_sock != -1) {
> @@ -2998,7 +3002,7 @@ int main(int argc, char *argv[])
>  		if (ret) {
>  			warnx("pthread_create: %s", strerror(ret));
>  			ret = 1;
> -			goto err_out4;
> +			goto err_out5;
>  		}
>  
>  		get_shared(&lo, &lo.root);
> @@ -3014,6 +3018,8 @@ int main(int argc, char *argv[])
>  	/* Block until ctrl+c or fusermount -u */
>          ret = virtio_loop(se);
>  
> +err_out5:
> +	cleanup_dbus();
>  err_out4:
>  	fuse_session_unmount(se);
>  err_out3:
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
  2019-09-05 17:27     ` [Virtio-fs] " Dr. David Alan Gilbert
@ 2019-09-06 10:23       ` Stefan Hajnoczi
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:23 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: virtio-fs, Marc-André Lureau, Eryu Guan, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Thu, Sep 05, 2019 at 06:27:32PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@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 <stefanha@redhat.com>
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
@ 2019-09-06 10:23       ` Stefan Hajnoczi
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:23 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Thu, Sep 05, 2019 at 06:27:32PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@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 <stefanha@redhat.com>
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
  2019-09-06 10:23       ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-06 10:49         ` Daniel P. Berrangé
  -1 siblings, 0 replies; 36+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 10:49 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: virtio-fs, Marc-André Lureau, Eryu Guan,
	Dr. David Alan Gilbert, qemu-devel

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 (stefanha@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 <stefanha@redhat.com>
> > 
> > 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 :|


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
@ 2019-09-06 10:49         ` Daniel P. Berrangé
  0 siblings, 0 replies; 36+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 10:49 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

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 (stefanha@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 <stefanha@redhat.com>
> > 
> > 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 :|


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
  2019-09-06 10:49         ` [Virtio-fs] " Daniel P. Berrangé
@ 2019-09-06 11:12           ` Dr. David Alan Gilbert
  -1 siblings, 0 replies; 36+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-06 11:12 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: virtio-fs, Marc-André Lureau, Eryu Guan, qemu-devel,
	Stefan Hajnoczi

* Daniel P. Berrangé (berrange@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 (stefanha@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 <stefanha@redhat.com>
> > > 
> > > 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 / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
@ 2019-09-06 11:12           ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 36+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-06 11:12 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

* Daniel P. Berrangé (berrange@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 (stefanha@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 <stefanha@redhat.com>
> > > 
> > > 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 / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
  2019-09-06 11:12           ` [Virtio-fs] " Dr. David Alan Gilbert
@ 2019-09-06 11:48             ` Daniel P. Berrangé
  -1 siblings, 0 replies; 36+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 11:48 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: virtio-fs, Marc-André Lureau, Eryu Guan, qemu-devel,
	Stefan Hajnoczi

On Fri, Sep 06, 2019 at 12:12:23PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@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 (stefanha@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 <stefanha@redhat.com>
> > > > 
> > > > 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 :|


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes
@ 2019-09-06 11:48             ` Daniel P. Berrangé
  0 siblings, 0 replies; 36+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 11:48 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

On Fri, Sep 06, 2019 at 12:12:23PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@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 (stefanha@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 <stefanha@redhat.com>
> > > > 
> > > > 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 :|


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [Qemu-devel] [RFC 3/3] virtiofsd: add virtiofsctl command-line management tool
  2019-09-05 15:21 ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-05 15:21   ` Stefan Hajnoczi
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-05 15:21 UTC (permalink / raw)
  To: qemu-devel, virtio-fs
  Cc: Marc-André Lureau, Eryu Guan, Dr. David Alan Gilbert,
	Stefan Hajnoczi

virtiofsctl can control a running virtiofsd process:

  usage: ./virtiofsctl COMMAND [args...]

  Commands:
    get-log-level       - show current log level
    set-log-level LEVEL - set current log level to one of
                          "err", "warning", "info", "debug"

Make sure it is running in the same DBus session as virtiofsd.  This may
require setting the DBUS_SESSION_BUS_ADDRESS environment variable to the
same value as used by virtiofsd.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 Makefile                        |  3 ++
 Makefile.objs                   |  1 +
 contrib/virtiofsd/Makefile.objs |  3 ++
 contrib/virtiofsd/virtiofsctl.c | 55 +++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+)
 create mode 100644 contrib/virtiofsd/virtiofsctl.c

diff --git a/Makefile b/Makefile
index 6b1af33348..d7ed9e7669 100644
--- a/Makefile
+++ b/Makefile
@@ -419,6 +419,7 @@ dummy := $(call unnest-vars,, \
                 ivshmem-client-obj-y \
                 ivshmem-server-obj-y \
                 virtiofsd-obj-y \
+                virtiofsctl-obj-y \
                 rdmacm-mux-obj-y \
                 libvhost-user-obj-y \
                 vhost-user-scsi-obj-y \
@@ -661,6 +662,8 @@ contrib/virtiofsd/gdbus_generated.c-timestamp: $(SRC_PATH)/contrib/virtiofsd/org
 
 virtiofsd$(EXESUF): $(virtiofsd-obj-y) libvhost-user.a $(COMMON_LDADDS)
 	$(call LINK, $^)
+virtiofsctl$(EXESUF): $(virtiofsctl-obj-y)
+	$(call LINK, $^)
 endif
 
 vhost-user-gpu$(EXESUF): $(vhost-user-gpu-obj-y) $(libvhost-user-obj-y) libqemuutil.a libqemustub.a
diff --git a/Makefile.objs b/Makefile.objs
index dfdd7d56ea..326a8abb8e 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -126,6 +126,7 @@ rdmacm-mux-obj-y = contrib/rdmacm-mux/
 vhost-user-input-obj-y = contrib/vhost-user-input/
 vhost-user-gpu-obj-y = contrib/vhost-user-gpu/
 virtiofsd-obj-y = contrib/virtiofsd/
+virtiofsctl-obj-y = contrib/virtiofsd/
 
 ######################################################################
 trace-events-subdirs =
diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
index d59ab60f3d..3f944d493e 100644
--- a/contrib/virtiofsd/Makefile.objs
+++ b/contrib/virtiofsd/Makefile.objs
@@ -11,6 +11,9 @@ virtiofsd-obj-y = buffer.o \
                   gdbus_generated.o \
                   dbus.o
 
+virtiofsctl-obj-y = virtiofsctl.o \
+                    gdbus_generated.o
+
 seccomp.o-cflags := $(SECCOMP_CFLAGS)
 seccomp.o-libs := $(SECCOMP_LIBS)
 
diff --git a/contrib/virtiofsd/virtiofsctl.c b/contrib/virtiofsd/virtiofsctl.c
new file mode 100644
index 0000000000..39bee2b881
--- /dev/null
+++ b/contrib/virtiofsd/virtiofsctl.c
@@ -0,0 +1,55 @@
+#include <stdio.h>
+#include "gdbus_generated.h"
+
+static void get_log_level(Virtiofsd *virtiofsd)
+{
+    const char *value = virtiofsd_get_log_level(virtiofsd);
+
+    printf("%s\n", value ? value : "(null)");
+}
+
+static void set_log_level(Virtiofsd *virtiofsd, const char *value)
+{
+    virtiofsd_set_log_level(virtiofsd, value);
+}
+
+static void usage(const char *progname)
+{
+    printf("usage: %s COMMAND [args...]\n", progname);
+    printf("\n");
+    printf("Commands:\n");
+    printf("  get-log-level       - show current log level\n");
+    printf("  set-log-level LEVEL - set current log level to one of\n");
+    printf("                        \"err\", \"warning\", \"info\", \"debug\"\n");
+    exit(0);
+}
+
+int main(int argc, char **argv)
+{
+    Virtiofsd *virtiofsd;
+    GError *error = NULL;
+
+    if (argc < 2) {
+        usage(argv[0]);
+    }
+
+    virtiofsd = virtiofsd_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION,
+            G_DBUS_PROXY_FLAGS_NONE, "org.qemu.virtiofsd",
+            "/org/qemu/virtiofsd", NULL, &error);
+    if (error) {
+        fprintf(stderr, "%s\n", error->message);
+        g_error_free(error);
+        return 1;
+    }
+
+    if (strcmp(argv[0], "get-log-level") == 0) {
+        get_log_level(virtiofsd);
+    } else if (strcmp(argv[0], "set-log-level") == 0) {
+        if (argc != 3) {
+            usage(argv[0]);
+        }
+        set_log_level(virtiofsd, argv[2]);
+    }
+    g_object_unref(virtiofsd);
+    return 0;
+}
-- 
2.21.0



^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Virtio-fs] [RFC 3/3] virtiofsd: add virtiofsctl command-line management tool
@ 2019-09-05 15:21   ` Stefan Hajnoczi
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-05 15:21 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: Marc-André Lureau

virtiofsctl can control a running virtiofsd process:

  usage: ./virtiofsctl COMMAND [args...]

  Commands:
    get-log-level       - show current log level
    set-log-level LEVEL - set current log level to one of
                          "err", "warning", "info", "debug"

Make sure it is running in the same DBus session as virtiofsd.  This may
require setting the DBUS_SESSION_BUS_ADDRESS environment variable to the
same value as used by virtiofsd.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 Makefile                        |  3 ++
 Makefile.objs                   |  1 +
 contrib/virtiofsd/Makefile.objs |  3 ++
 contrib/virtiofsd/virtiofsctl.c | 55 +++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+)
 create mode 100644 contrib/virtiofsd/virtiofsctl.c

diff --git a/Makefile b/Makefile
index 6b1af33348..d7ed9e7669 100644
--- a/Makefile
+++ b/Makefile
@@ -419,6 +419,7 @@ dummy := $(call unnest-vars,, \
                 ivshmem-client-obj-y \
                 ivshmem-server-obj-y \
                 virtiofsd-obj-y \
+                virtiofsctl-obj-y \
                 rdmacm-mux-obj-y \
                 libvhost-user-obj-y \
                 vhost-user-scsi-obj-y \
@@ -661,6 +662,8 @@ contrib/virtiofsd/gdbus_generated.c-timestamp: $(SRC_PATH)/contrib/virtiofsd/org
 
 virtiofsd$(EXESUF): $(virtiofsd-obj-y) libvhost-user.a $(COMMON_LDADDS)
 	$(call LINK, $^)
+virtiofsctl$(EXESUF): $(virtiofsctl-obj-y)
+	$(call LINK, $^)
 endif
 
 vhost-user-gpu$(EXESUF): $(vhost-user-gpu-obj-y) $(libvhost-user-obj-y) libqemuutil.a libqemustub.a
diff --git a/Makefile.objs b/Makefile.objs
index dfdd7d56ea..326a8abb8e 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -126,6 +126,7 @@ rdmacm-mux-obj-y = contrib/rdmacm-mux/
 vhost-user-input-obj-y = contrib/vhost-user-input/
 vhost-user-gpu-obj-y = contrib/vhost-user-gpu/
 virtiofsd-obj-y = contrib/virtiofsd/
+virtiofsctl-obj-y = contrib/virtiofsd/
 
 ######################################################################
 trace-events-subdirs =
diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
index d59ab60f3d..3f944d493e 100644
--- a/contrib/virtiofsd/Makefile.objs
+++ b/contrib/virtiofsd/Makefile.objs
@@ -11,6 +11,9 @@ virtiofsd-obj-y = buffer.o \
                   gdbus_generated.o \
                   dbus.o
 
+virtiofsctl-obj-y = virtiofsctl.o \
+                    gdbus_generated.o
+
 seccomp.o-cflags := $(SECCOMP_CFLAGS)
 seccomp.o-libs := $(SECCOMP_LIBS)
 
diff --git a/contrib/virtiofsd/virtiofsctl.c b/contrib/virtiofsd/virtiofsctl.c
new file mode 100644
index 0000000000..39bee2b881
--- /dev/null
+++ b/contrib/virtiofsd/virtiofsctl.c
@@ -0,0 +1,55 @@
+#include <stdio.h>
+#include "gdbus_generated.h"
+
+static void get_log_level(Virtiofsd *virtiofsd)
+{
+    const char *value = virtiofsd_get_log_level(virtiofsd);
+
+    printf("%s\n", value ? value : "(null)");
+}
+
+static void set_log_level(Virtiofsd *virtiofsd, const char *value)
+{
+    virtiofsd_set_log_level(virtiofsd, value);
+}
+
+static void usage(const char *progname)
+{
+    printf("usage: %s COMMAND [args...]\n", progname);
+    printf("\n");
+    printf("Commands:\n");
+    printf("  get-log-level       - show current log level\n");
+    printf("  set-log-level LEVEL - set current log level to one of\n");
+    printf("                        \"err\", \"warning\", \"info\", \"debug\"\n");
+    exit(0);
+}
+
+int main(int argc, char **argv)
+{
+    Virtiofsd *virtiofsd;
+    GError *error = NULL;
+
+    if (argc < 2) {
+        usage(argv[0]);
+    }
+
+    virtiofsd = virtiofsd_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION,
+            G_DBUS_PROXY_FLAGS_NONE, "org.qemu.virtiofsd",
+            "/org/qemu/virtiofsd", NULL, &error);
+    if (error) {
+        fprintf(stderr, "%s\n", error->message);
+        g_error_free(error);
+        return 1;
+    }
+
+    if (strcmp(argv[0], "get-log-level") == 0) {
+        get_log_level(virtiofsd);
+    } else if (strcmp(argv[0], "set-log-level") == 0) {
+        if (argc != 3) {
+            usage(argv[0]);
+        }
+        set_log_level(virtiofsd, argv[2]);
+    }
+    g_object_unref(virtiofsd);
+    return 0;
+}
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 3/3] virtiofsd: add virtiofsctl command-line management tool
  2019-09-05 15:21   ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-05 17:12     ` Dr. David Alan Gilbert
  -1 siblings, 0 replies; 36+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-05 17:12 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, Eryu Guan, qemu-devel

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> virtiofsctl can control a running virtiofsd process:
> 
>   usage: ./virtiofsctl COMMAND [args...]
> 
>   Commands:
>     get-log-level       - show current log level
>     set-log-level LEVEL - set current log level to one of
>                           "err", "warning", "info", "debug"
> 
> Make sure it is running in the same DBus session as virtiofsd.  This may
> require setting the DBUS_SESSION_BUS_ADDRESS environment variable to the
> same value as used by virtiofsd.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  Makefile                        |  3 ++
>  Makefile.objs                   |  1 +
>  contrib/virtiofsd/Makefile.objs |  3 ++
>  contrib/virtiofsd/virtiofsctl.c | 55 +++++++++++++++++++++++++++++++++
>  4 files changed, 62 insertions(+)
>  create mode 100644 contrib/virtiofsd/virtiofsctl.c
> 
> diff --git a/Makefile b/Makefile
> index 6b1af33348..d7ed9e7669 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -419,6 +419,7 @@ dummy := $(call unnest-vars,, \
>                  ivshmem-client-obj-y \
>                  ivshmem-server-obj-y \
>                  virtiofsd-obj-y \
> +                virtiofsctl-obj-y \
>                  rdmacm-mux-obj-y \
>                  libvhost-user-obj-y \
>                  vhost-user-scsi-obj-y \
> @@ -661,6 +662,8 @@ contrib/virtiofsd/gdbus_generated.c-timestamp: $(SRC_PATH)/contrib/virtiofsd/org
>  
>  virtiofsd$(EXESUF): $(virtiofsd-obj-y) libvhost-user.a $(COMMON_LDADDS)
>  	$(call LINK, $^)
> +virtiofsctl$(EXESUF): $(virtiofsctl-obj-y)
> +	$(call LINK, $^)
>  endif
>  
>  vhost-user-gpu$(EXESUF): $(vhost-user-gpu-obj-y) $(libvhost-user-obj-y) libqemuutil.a libqemustub.a
> diff --git a/Makefile.objs b/Makefile.objs
> index dfdd7d56ea..326a8abb8e 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -126,6 +126,7 @@ rdmacm-mux-obj-y = contrib/rdmacm-mux/
>  vhost-user-input-obj-y = contrib/vhost-user-input/
>  vhost-user-gpu-obj-y = contrib/vhost-user-gpu/
>  virtiofsd-obj-y = contrib/virtiofsd/
> +virtiofsctl-obj-y = contrib/virtiofsd/
>  
>  ######################################################################
>  trace-events-subdirs =
> diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
> index d59ab60f3d..3f944d493e 100644
> --- a/contrib/virtiofsd/Makefile.objs
> +++ b/contrib/virtiofsd/Makefile.objs
> @@ -11,6 +11,9 @@ virtiofsd-obj-y = buffer.o \
>                    gdbus_generated.o \
>                    dbus.o
>  
> +virtiofsctl-obj-y = virtiofsctl.o \
> +                    gdbus_generated.o
> +
>  seccomp.o-cflags := $(SECCOMP_CFLAGS)
>  seccomp.o-libs := $(SECCOMP_LIBS)
>  
> diff --git a/contrib/virtiofsd/virtiofsctl.c b/contrib/virtiofsd/virtiofsctl.c
> new file mode 100644
> index 0000000000..39bee2b881
> --- /dev/null
> +++ b/contrib/virtiofsd/virtiofsctl.c
> @@ -0,0 +1,55 @@
> +#include <stdio.h>
> +#include "gdbus_generated.h"
> +
> +static void get_log_level(Virtiofsd *virtiofsd)
> +{
> +    const char *value = virtiofsd_get_log_level(virtiofsd);
> +
> +    printf("%s\n", value ? value : "(null)");
> +}
> +
> +static void set_log_level(Virtiofsd *virtiofsd, const char *value)
> +{
> +    virtiofsd_set_log_level(virtiofsd, value);
> +}
> +
> +static void usage(const char *progname)
> +{
> +    printf("usage: %s COMMAND [args...]\n", progname);
> +    printf("\n");
> +    printf("Commands:\n");
> +    printf("  get-log-level       - show current log level\n");
> +    printf("  set-log-level LEVEL - set current log level to one of\n");
> +    printf("                        \"err\", \"warning\", \"info\", \"debug\"\n");
> +    exit(0);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    Virtiofsd *virtiofsd;
> +    GError *error = NULL;
> +
> +    if (argc < 2) {
> +        usage(argv[0]);
> +    }
> +
> +    virtiofsd = virtiofsd_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION,
> +            G_DBUS_PROXY_FLAGS_NONE, "org.qemu.virtiofsd",
> +            "/org/qemu/virtiofsd", NULL, &error);
> +    if (error) {
> +        fprintf(stderr, "%s\n", error->message);
> +        g_error_free(error);
> +        return 1;
> +    }
> +
> +    if (strcmp(argv[0], "get-log-level") == 0) {

This and the one below works a lot better with argv[1] !

(I wonder if a little python script would be better for these type of
wrappers).

Dave

> +        get_log_level(virtiofsd);
> +    } else if (strcmp(argv[0], "set-log-level") == 0) {

> +        if (argc != 3) {
> +            usage(argv[0]);
> +        }
> +        set_log_level(virtiofsd, argv[2]);
> +    }
> +    g_object_unref(virtiofsd);
> +    return 0;
> +}
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [RFC 3/3] virtiofsd: add virtiofsctl command-line management tool
@ 2019-09-05 17:12     ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 36+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-05 17:12 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> virtiofsctl can control a running virtiofsd process:
> 
>   usage: ./virtiofsctl COMMAND [args...]
> 
>   Commands:
>     get-log-level       - show current log level
>     set-log-level LEVEL - set current log level to one of
>                           "err", "warning", "info", "debug"
> 
> Make sure it is running in the same DBus session as virtiofsd.  This may
> require setting the DBUS_SESSION_BUS_ADDRESS environment variable to the
> same value as used by virtiofsd.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  Makefile                        |  3 ++
>  Makefile.objs                   |  1 +
>  contrib/virtiofsd/Makefile.objs |  3 ++
>  contrib/virtiofsd/virtiofsctl.c | 55 +++++++++++++++++++++++++++++++++
>  4 files changed, 62 insertions(+)
>  create mode 100644 contrib/virtiofsd/virtiofsctl.c
> 
> diff --git a/Makefile b/Makefile
> index 6b1af33348..d7ed9e7669 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -419,6 +419,7 @@ dummy := $(call unnest-vars,, \
>                  ivshmem-client-obj-y \
>                  ivshmem-server-obj-y \
>                  virtiofsd-obj-y \
> +                virtiofsctl-obj-y \
>                  rdmacm-mux-obj-y \
>                  libvhost-user-obj-y \
>                  vhost-user-scsi-obj-y \
> @@ -661,6 +662,8 @@ contrib/virtiofsd/gdbus_generated.c-timestamp: $(SRC_PATH)/contrib/virtiofsd/org
>  
>  virtiofsd$(EXESUF): $(virtiofsd-obj-y) libvhost-user.a $(COMMON_LDADDS)
>  	$(call LINK, $^)
> +virtiofsctl$(EXESUF): $(virtiofsctl-obj-y)
> +	$(call LINK, $^)
>  endif
>  
>  vhost-user-gpu$(EXESUF): $(vhost-user-gpu-obj-y) $(libvhost-user-obj-y) libqemuutil.a libqemustub.a
> diff --git a/Makefile.objs b/Makefile.objs
> index dfdd7d56ea..326a8abb8e 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -126,6 +126,7 @@ rdmacm-mux-obj-y = contrib/rdmacm-mux/
>  vhost-user-input-obj-y = contrib/vhost-user-input/
>  vhost-user-gpu-obj-y = contrib/vhost-user-gpu/
>  virtiofsd-obj-y = contrib/virtiofsd/
> +virtiofsctl-obj-y = contrib/virtiofsd/
>  
>  ######################################################################
>  trace-events-subdirs =
> diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
> index d59ab60f3d..3f944d493e 100644
> --- a/contrib/virtiofsd/Makefile.objs
> +++ b/contrib/virtiofsd/Makefile.objs
> @@ -11,6 +11,9 @@ virtiofsd-obj-y = buffer.o \
>                    gdbus_generated.o \
>                    dbus.o
>  
> +virtiofsctl-obj-y = virtiofsctl.o \
> +                    gdbus_generated.o
> +
>  seccomp.o-cflags := $(SECCOMP_CFLAGS)
>  seccomp.o-libs := $(SECCOMP_LIBS)
>  
> diff --git a/contrib/virtiofsd/virtiofsctl.c b/contrib/virtiofsd/virtiofsctl.c
> new file mode 100644
> index 0000000000..39bee2b881
> --- /dev/null
> +++ b/contrib/virtiofsd/virtiofsctl.c
> @@ -0,0 +1,55 @@
> +#include <stdio.h>
> +#include "gdbus_generated.h"
> +
> +static void get_log_level(Virtiofsd *virtiofsd)
> +{
> +    const char *value = virtiofsd_get_log_level(virtiofsd);
> +
> +    printf("%s\n", value ? value : "(null)");
> +}
> +
> +static void set_log_level(Virtiofsd *virtiofsd, const char *value)
> +{
> +    virtiofsd_set_log_level(virtiofsd, value);
> +}
> +
> +static void usage(const char *progname)
> +{
> +    printf("usage: %s COMMAND [args...]\n", progname);
> +    printf("\n");
> +    printf("Commands:\n");
> +    printf("  get-log-level       - show current log level\n");
> +    printf("  set-log-level LEVEL - set current log level to one of\n");
> +    printf("                        \"err\", \"warning\", \"info\", \"debug\"\n");
> +    exit(0);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    Virtiofsd *virtiofsd;
> +    GError *error = NULL;
> +
> +    if (argc < 2) {
> +        usage(argv[0]);
> +    }
> +
> +    virtiofsd = virtiofsd_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION,
> +            G_DBUS_PROXY_FLAGS_NONE, "org.qemu.virtiofsd",
> +            "/org/qemu/virtiofsd", NULL, &error);
> +    if (error) {
> +        fprintf(stderr, "%s\n", error->message);
> +        g_error_free(error);
> +        return 1;
> +    }
> +
> +    if (strcmp(argv[0], "get-log-level") == 0) {

This and the one below works a lot better with argv[1] !

(I wonder if a little python script would be better for these type of
wrappers).

Dave

> +        get_log_level(virtiofsd);
> +    } else if (strcmp(argv[0], "set-log-level") == 0) {

> +        if (argc != 3) {
> +            usage(argv[0]);
> +        }
> +        set_log_level(virtiofsd, argv[2]);
> +    }
> +    g_object_unref(virtiofsd);
> +    return 0;
> +}
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 3/3] virtiofsd: add virtiofsctl command-line management tool
  2019-09-05 17:12     ` [Virtio-fs] " Dr. David Alan Gilbert
@ 2019-09-05 20:03       ` Marc-André Lureau
  -1 siblings, 0 replies; 36+ messages in thread
From: Marc-André Lureau @ 2019-09-05 20:03 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: virtio-fs, Eryu Guan, qemu-devel, Stefan Hajnoczi

Hi

On Thu, Sep 5, 2019 at 9:13 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > virtiofsctl can control a running virtiofsd process:
> >
> >   usage: ./virtiofsctl COMMAND [args...]
> >
> >   Commands:
> >     get-log-level       - show current log level
> >     set-log-level LEVEL - set current log level to one of
> >                           "err", "warning", "info", "debug"
> >
> > Make sure it is running in the same DBus session as virtiofsd.  This may
> > require setting the DBUS_SESSION_BUS_ADDRESS environment variable to the
> > same value as used by virtiofsd.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  Makefile                        |  3 ++
> >  Makefile.objs                   |  1 +
> >  contrib/virtiofsd/Makefile.objs |  3 ++
> >  contrib/virtiofsd/virtiofsctl.c | 55 +++++++++++++++++++++++++++++++++
> >  4 files changed, 62 insertions(+)
> >  create mode 100644 contrib/virtiofsd/virtiofsctl.c
> >
> > diff --git a/Makefile b/Makefile
> > index 6b1af33348..d7ed9e7669 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -419,6 +419,7 @@ dummy := $(call unnest-vars,, \
> >                  ivshmem-client-obj-y \
> >                  ivshmem-server-obj-y \
> >                  virtiofsd-obj-y \
> > +                virtiofsctl-obj-y \
> >                  rdmacm-mux-obj-y \
> >                  libvhost-user-obj-y \
> >                  vhost-user-scsi-obj-y \
> > @@ -661,6 +662,8 @@ contrib/virtiofsd/gdbus_generated.c-timestamp: $(SRC_PATH)/contrib/virtiofsd/org
> >
> >  virtiofsd$(EXESUF): $(virtiofsd-obj-y) libvhost-user.a $(COMMON_LDADDS)
> >       $(call LINK, $^)
> > +virtiofsctl$(EXESUF): $(virtiofsctl-obj-y)
> > +     $(call LINK, $^)
> >  endif
> >
> >  vhost-user-gpu$(EXESUF): $(vhost-user-gpu-obj-y) $(libvhost-user-obj-y) libqemuutil.a libqemustub.a
> > diff --git a/Makefile.objs b/Makefile.objs
> > index dfdd7d56ea..326a8abb8e 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -126,6 +126,7 @@ rdmacm-mux-obj-y = contrib/rdmacm-mux/
> >  vhost-user-input-obj-y = contrib/vhost-user-input/
> >  vhost-user-gpu-obj-y = contrib/vhost-user-gpu/
> >  virtiofsd-obj-y = contrib/virtiofsd/
> > +virtiofsctl-obj-y = contrib/virtiofsd/
> >
> >  ######################################################################
> >  trace-events-subdirs =
> > diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
> > index d59ab60f3d..3f944d493e 100644
> > --- a/contrib/virtiofsd/Makefile.objs
> > +++ b/contrib/virtiofsd/Makefile.objs
> > @@ -11,6 +11,9 @@ virtiofsd-obj-y = buffer.o \
> >                    gdbus_generated.o \
> >                    dbus.o
> >
> > +virtiofsctl-obj-y = virtiofsctl.o \
> > +                    gdbus_generated.o
> > +
> >  seccomp.o-cflags := $(SECCOMP_CFLAGS)
> >  seccomp.o-libs := $(SECCOMP_LIBS)
> >
> > diff --git a/contrib/virtiofsd/virtiofsctl.c b/contrib/virtiofsd/virtiofsctl.c
> > new file mode 100644
> > index 0000000000..39bee2b881
> > --- /dev/null
> > +++ b/contrib/virtiofsd/virtiofsctl.c
> > @@ -0,0 +1,55 @@
> > +#include <stdio.h>
> > +#include "gdbus_generated.h"
> > +
> > +static void get_log_level(Virtiofsd *virtiofsd)
> > +{
> > +    const char *value = virtiofsd_get_log_level(virtiofsd);
> > +
> > +    printf("%s\n", value ? value : "(null)");
> > +}
> > +
> > +static void set_log_level(Virtiofsd *virtiofsd, const char *value)
> > +{
> > +    virtiofsd_set_log_level(virtiofsd, value);
> > +}
> > +
> > +static void usage(const char *progname)
> > +{
> > +    printf("usage: %s COMMAND [args...]\n", progname);
> > +    printf("\n");
> > +    printf("Commands:\n");
> > +    printf("  get-log-level       - show current log level\n");
> > +    printf("  set-log-level LEVEL - set current log level to one of\n");
> > +    printf("                        \"err\", \"warning\", \"info\", \"debug\"\n");
> > +    exit(0);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +    Virtiofsd *virtiofsd;
> > +    GError *error = NULL;
> > +
> > +    if (argc < 2) {
> > +        usage(argv[0]);
> > +    }
> > +
> > +    virtiofsd = virtiofsd_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION,
> > +            G_DBUS_PROXY_FLAGS_NONE, "org.qemu.virtiofsd",
> > +            "/org/qemu/virtiofsd", NULL, &error);
> > +    if (error) {
> > +        fprintf(stderr, "%s\n", error->message);
> > +        g_error_free(error);
> > +        return 1;
> > +    }
> > +
> > +    if (strcmp(argv[0], "get-log-level") == 0) {
>
> This and the one below works a lot better with argv[1] !
>
> (I wonder if a little python script would be better for these type of
> wrappers).

Or just plain gdbus/busctl calls (which already has bash completion
fwiw), and/or eventually a shell script.

>
> Dave
>
> > +        get_log_level(virtiofsd);
> > +    } else if (strcmp(argv[0], "set-log-level") == 0) {
>
> > +        if (argc != 3) {
> > +            usage(argv[0]);
> > +        }
> > +        set_log_level(virtiofsd, argv[2]);
> > +    }
> > +    g_object_unref(virtiofsd);
> > +    return 0;
> > +}
> > --
> > 2.21.0
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [RFC 3/3] virtiofsd: add virtiofsctl command-line management tool
@ 2019-09-05 20:03       ` Marc-André Lureau
  0 siblings, 0 replies; 36+ messages in thread
From: Marc-André Lureau @ 2019-09-05 20:03 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: virtio-fs, qemu-devel

Hi

On Thu, Sep 5, 2019 at 9:13 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > virtiofsctl can control a running virtiofsd process:
> >
> >   usage: ./virtiofsctl COMMAND [args...]
> >
> >   Commands:
> >     get-log-level       - show current log level
> >     set-log-level LEVEL - set current log level to one of
> >                           "err", "warning", "info", "debug"
> >
> > Make sure it is running in the same DBus session as virtiofsd.  This may
> > require setting the DBUS_SESSION_BUS_ADDRESS environment variable to the
> > same value as used by virtiofsd.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  Makefile                        |  3 ++
> >  Makefile.objs                   |  1 +
> >  contrib/virtiofsd/Makefile.objs |  3 ++
> >  contrib/virtiofsd/virtiofsctl.c | 55 +++++++++++++++++++++++++++++++++
> >  4 files changed, 62 insertions(+)
> >  create mode 100644 contrib/virtiofsd/virtiofsctl.c
> >
> > diff --git a/Makefile b/Makefile
> > index 6b1af33348..d7ed9e7669 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -419,6 +419,7 @@ dummy := $(call unnest-vars,, \
> >                  ivshmem-client-obj-y \
> >                  ivshmem-server-obj-y \
> >                  virtiofsd-obj-y \
> > +                virtiofsctl-obj-y \
> >                  rdmacm-mux-obj-y \
> >                  libvhost-user-obj-y \
> >                  vhost-user-scsi-obj-y \
> > @@ -661,6 +662,8 @@ contrib/virtiofsd/gdbus_generated.c-timestamp: $(SRC_PATH)/contrib/virtiofsd/org
> >
> >  virtiofsd$(EXESUF): $(virtiofsd-obj-y) libvhost-user.a $(COMMON_LDADDS)
> >       $(call LINK, $^)
> > +virtiofsctl$(EXESUF): $(virtiofsctl-obj-y)
> > +     $(call LINK, $^)
> >  endif
> >
> >  vhost-user-gpu$(EXESUF): $(vhost-user-gpu-obj-y) $(libvhost-user-obj-y) libqemuutil.a libqemustub.a
> > diff --git a/Makefile.objs b/Makefile.objs
> > index dfdd7d56ea..326a8abb8e 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -126,6 +126,7 @@ rdmacm-mux-obj-y = contrib/rdmacm-mux/
> >  vhost-user-input-obj-y = contrib/vhost-user-input/
> >  vhost-user-gpu-obj-y = contrib/vhost-user-gpu/
> >  virtiofsd-obj-y = contrib/virtiofsd/
> > +virtiofsctl-obj-y = contrib/virtiofsd/
> >
> >  ######################################################################
> >  trace-events-subdirs =
> > diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
> > index d59ab60f3d..3f944d493e 100644
> > --- a/contrib/virtiofsd/Makefile.objs
> > +++ b/contrib/virtiofsd/Makefile.objs
> > @@ -11,6 +11,9 @@ virtiofsd-obj-y = buffer.o \
> >                    gdbus_generated.o \
> >                    dbus.o
> >
> > +virtiofsctl-obj-y = virtiofsctl.o \
> > +                    gdbus_generated.o
> > +
> >  seccomp.o-cflags := $(SECCOMP_CFLAGS)
> >  seccomp.o-libs := $(SECCOMP_LIBS)
> >
> > diff --git a/contrib/virtiofsd/virtiofsctl.c b/contrib/virtiofsd/virtiofsctl.c
> > new file mode 100644
> > index 0000000000..39bee2b881
> > --- /dev/null
> > +++ b/contrib/virtiofsd/virtiofsctl.c
> > @@ -0,0 +1,55 @@
> > +#include <stdio.h>
> > +#include "gdbus_generated.h"
> > +
> > +static void get_log_level(Virtiofsd *virtiofsd)
> > +{
> > +    const char *value = virtiofsd_get_log_level(virtiofsd);
> > +
> > +    printf("%s\n", value ? value : "(null)");
> > +}
> > +
> > +static void set_log_level(Virtiofsd *virtiofsd, const char *value)
> > +{
> > +    virtiofsd_set_log_level(virtiofsd, value);
> > +}
> > +
> > +static void usage(const char *progname)
> > +{
> > +    printf("usage: %s COMMAND [args...]\n", progname);
> > +    printf("\n");
> > +    printf("Commands:\n");
> > +    printf("  get-log-level       - show current log level\n");
> > +    printf("  set-log-level LEVEL - set current log level to one of\n");
> > +    printf("                        \"err\", \"warning\", \"info\", \"debug\"\n");
> > +    exit(0);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +    Virtiofsd *virtiofsd;
> > +    GError *error = NULL;
> > +
> > +    if (argc < 2) {
> > +        usage(argv[0]);
> > +    }
> > +
> > +    virtiofsd = virtiofsd_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION,
> > +            G_DBUS_PROXY_FLAGS_NONE, "org.qemu.virtiofsd",
> > +            "/org/qemu/virtiofsd", NULL, &error);
> > +    if (error) {
> > +        fprintf(stderr, "%s\n", error->message);
> > +        g_error_free(error);
> > +        return 1;
> > +    }
> > +
> > +    if (strcmp(argv[0], "get-log-level") == 0) {
>
> This and the one below works a lot better with argv[1] !
>
> (I wonder if a little python script would be better for these type of
> wrappers).

Or just plain gdbus/busctl calls (which already has bash completion
fwiw), and/or eventually a shell script.

>
> Dave
>
> > +        get_log_level(virtiofsd);
> > +    } else if (strcmp(argv[0], "set-log-level") == 0) {
>
> > +        if (argc != 3) {
> > +            usage(argv[0]);
> > +        }
> > +        set_log_level(virtiofsd, argv[2]);
> > +    }
> > +    g_object_unref(virtiofsd);
> > +    return 0;
> > +}
> > --
> > 2.21.0
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 3/3] virtiofsd: add virtiofsctl command-line management tool
  2019-09-05 20:03       ` [Virtio-fs] " Marc-André Lureau
@ 2019-09-06 10:33         ` Stefan Hajnoczi
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:33 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: virtio-fs, Eryu Guan, Dr. David Alan Gilbert, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 5438 bytes --]

On Fri, Sep 06, 2019 at 12:03:15AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Sep 5, 2019 at 9:13 PM Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > virtiofsctl can control a running virtiofsd process:
> > >
> > >   usage: ./virtiofsctl COMMAND [args...]
> > >
> > >   Commands:
> > >     get-log-level       - show current log level
> > >     set-log-level LEVEL - set current log level to one of
> > >                           "err", "warning", "info", "debug"
> > >
> > > Make sure it is running in the same DBus session as virtiofsd.  This may
> > > require setting the DBUS_SESSION_BUS_ADDRESS environment variable to the
> > > same value as used by virtiofsd.
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  Makefile                        |  3 ++
> > >  Makefile.objs                   |  1 +
> > >  contrib/virtiofsd/Makefile.objs |  3 ++
> > >  contrib/virtiofsd/virtiofsctl.c | 55 +++++++++++++++++++++++++++++++++
> > >  4 files changed, 62 insertions(+)
> > >  create mode 100644 contrib/virtiofsd/virtiofsctl.c
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 6b1af33348..d7ed9e7669 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -419,6 +419,7 @@ dummy := $(call unnest-vars,, \
> > >                  ivshmem-client-obj-y \
> > >                  ivshmem-server-obj-y \
> > >                  virtiofsd-obj-y \
> > > +                virtiofsctl-obj-y \
> > >                  rdmacm-mux-obj-y \
> > >                  libvhost-user-obj-y \
> > >                  vhost-user-scsi-obj-y \
> > > @@ -661,6 +662,8 @@ contrib/virtiofsd/gdbus_generated.c-timestamp: $(SRC_PATH)/contrib/virtiofsd/org
> > >
> > >  virtiofsd$(EXESUF): $(virtiofsd-obj-y) libvhost-user.a $(COMMON_LDADDS)
> > >       $(call LINK, $^)
> > > +virtiofsctl$(EXESUF): $(virtiofsctl-obj-y)
> > > +     $(call LINK, $^)
> > >  endif
> > >
> > >  vhost-user-gpu$(EXESUF): $(vhost-user-gpu-obj-y) $(libvhost-user-obj-y) libqemuutil.a libqemustub.a
> > > diff --git a/Makefile.objs b/Makefile.objs
> > > index dfdd7d56ea..326a8abb8e 100644
> > > --- a/Makefile.objs
> > > +++ b/Makefile.objs
> > > @@ -126,6 +126,7 @@ rdmacm-mux-obj-y = contrib/rdmacm-mux/
> > >  vhost-user-input-obj-y = contrib/vhost-user-input/
> > >  vhost-user-gpu-obj-y = contrib/vhost-user-gpu/
> > >  virtiofsd-obj-y = contrib/virtiofsd/
> > > +virtiofsctl-obj-y = contrib/virtiofsd/
> > >
> > >  ######################################################################
> > >  trace-events-subdirs =
> > > diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
> > > index d59ab60f3d..3f944d493e 100644
> > > --- a/contrib/virtiofsd/Makefile.objs
> > > +++ b/contrib/virtiofsd/Makefile.objs
> > > @@ -11,6 +11,9 @@ virtiofsd-obj-y = buffer.o \
> > >                    gdbus_generated.o \
> > >                    dbus.o
> > >
> > > +virtiofsctl-obj-y = virtiofsctl.o \
> > > +                    gdbus_generated.o
> > > +
> > >  seccomp.o-cflags := $(SECCOMP_CFLAGS)
> > >  seccomp.o-libs := $(SECCOMP_LIBS)
> > >
> > > diff --git a/contrib/virtiofsd/virtiofsctl.c b/contrib/virtiofsd/virtiofsctl.c
> > > new file mode 100644
> > > index 0000000000..39bee2b881
> > > --- /dev/null
> > > +++ b/contrib/virtiofsd/virtiofsctl.c
> > > @@ -0,0 +1,55 @@
> > > +#include <stdio.h>
> > > +#include "gdbus_generated.h"
> > > +
> > > +static void get_log_level(Virtiofsd *virtiofsd)
> > > +{
> > > +    const char *value = virtiofsd_get_log_level(virtiofsd);
> > > +
> > > +    printf("%s\n", value ? value : "(null)");
> > > +}
> > > +
> > > +static void set_log_level(Virtiofsd *virtiofsd, const char *value)
> > > +{
> > > +    virtiofsd_set_log_level(virtiofsd, value);
> > > +}
> > > +
> > > +static void usage(const char *progname)
> > > +{
> > > +    printf("usage: %s COMMAND [args...]\n", progname);
> > > +    printf("\n");
> > > +    printf("Commands:\n");
> > > +    printf("  get-log-level       - show current log level\n");
> > > +    printf("  set-log-level LEVEL - set current log level to one of\n");
> > > +    printf("                        \"err\", \"warning\", \"info\", \"debug\"\n");
> > > +    exit(0);
> > > +}
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > +    Virtiofsd *virtiofsd;
> > > +    GError *error = NULL;
> > > +
> > > +    if (argc < 2) {
> > > +        usage(argv[0]);
> > > +    }
> > > +
> > > +    virtiofsd = virtiofsd_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION,
> > > +            G_DBUS_PROXY_FLAGS_NONE, "org.qemu.virtiofsd",
> > > +            "/org/qemu/virtiofsd", NULL, &error);
> > > +    if (error) {
> > > +        fprintf(stderr, "%s\n", error->message);
> > > +        g_error_free(error);
> > > +        return 1;
> > > +    }
> > > +
> > > +    if (strcmp(argv[0], "get-log-level") == 0) {
> >
> > This and the one below works a lot better with argv[1] !
> >
> > (I wonder if a little python script would be better for these type of
> > wrappers).
> 
> Or just plain gdbus/busctl calls (which already has bash completion
> fwiw), and/or eventually a shell script.

A virtiofsctl program is useful because the sub-commands are easily
discoverable and no DBus knowledge is required.  Writing it in Python 3
would be nice.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [RFC 3/3] virtiofsd: add virtiofsctl command-line management tool
@ 2019-09-06 10:33         ` Stefan Hajnoczi
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:33 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: virtio-fs, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 5438 bytes --]

On Fri, Sep 06, 2019 at 12:03:15AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Sep 5, 2019 at 9:13 PM Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > virtiofsctl can control a running virtiofsd process:
> > >
> > >   usage: ./virtiofsctl COMMAND [args...]
> > >
> > >   Commands:
> > >     get-log-level       - show current log level
> > >     set-log-level LEVEL - set current log level to one of
> > >                           "err", "warning", "info", "debug"
> > >
> > > Make sure it is running in the same DBus session as virtiofsd.  This may
> > > require setting the DBUS_SESSION_BUS_ADDRESS environment variable to the
> > > same value as used by virtiofsd.
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  Makefile                        |  3 ++
> > >  Makefile.objs                   |  1 +
> > >  contrib/virtiofsd/Makefile.objs |  3 ++
> > >  contrib/virtiofsd/virtiofsctl.c | 55 +++++++++++++++++++++++++++++++++
> > >  4 files changed, 62 insertions(+)
> > >  create mode 100644 contrib/virtiofsd/virtiofsctl.c
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 6b1af33348..d7ed9e7669 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -419,6 +419,7 @@ dummy := $(call unnest-vars,, \
> > >                  ivshmem-client-obj-y \
> > >                  ivshmem-server-obj-y \
> > >                  virtiofsd-obj-y \
> > > +                virtiofsctl-obj-y \
> > >                  rdmacm-mux-obj-y \
> > >                  libvhost-user-obj-y \
> > >                  vhost-user-scsi-obj-y \
> > > @@ -661,6 +662,8 @@ contrib/virtiofsd/gdbus_generated.c-timestamp: $(SRC_PATH)/contrib/virtiofsd/org
> > >
> > >  virtiofsd$(EXESUF): $(virtiofsd-obj-y) libvhost-user.a $(COMMON_LDADDS)
> > >       $(call LINK, $^)
> > > +virtiofsctl$(EXESUF): $(virtiofsctl-obj-y)
> > > +     $(call LINK, $^)
> > >  endif
> > >
> > >  vhost-user-gpu$(EXESUF): $(vhost-user-gpu-obj-y) $(libvhost-user-obj-y) libqemuutil.a libqemustub.a
> > > diff --git a/Makefile.objs b/Makefile.objs
> > > index dfdd7d56ea..326a8abb8e 100644
> > > --- a/Makefile.objs
> > > +++ b/Makefile.objs
> > > @@ -126,6 +126,7 @@ rdmacm-mux-obj-y = contrib/rdmacm-mux/
> > >  vhost-user-input-obj-y = contrib/vhost-user-input/
> > >  vhost-user-gpu-obj-y = contrib/vhost-user-gpu/
> > >  virtiofsd-obj-y = contrib/virtiofsd/
> > > +virtiofsctl-obj-y = contrib/virtiofsd/
> > >
> > >  ######################################################################
> > >  trace-events-subdirs =
> > > diff --git a/contrib/virtiofsd/Makefile.objs b/contrib/virtiofsd/Makefile.objs
> > > index d59ab60f3d..3f944d493e 100644
> > > --- a/contrib/virtiofsd/Makefile.objs
> > > +++ b/contrib/virtiofsd/Makefile.objs
> > > @@ -11,6 +11,9 @@ virtiofsd-obj-y = buffer.o \
> > >                    gdbus_generated.o \
> > >                    dbus.o
> > >
> > > +virtiofsctl-obj-y = virtiofsctl.o \
> > > +                    gdbus_generated.o
> > > +
> > >  seccomp.o-cflags := $(SECCOMP_CFLAGS)
> > >  seccomp.o-libs := $(SECCOMP_LIBS)
> > >
> > > diff --git a/contrib/virtiofsd/virtiofsctl.c b/contrib/virtiofsd/virtiofsctl.c
> > > new file mode 100644
> > > index 0000000000..39bee2b881
> > > --- /dev/null
> > > +++ b/contrib/virtiofsd/virtiofsctl.c
> > > @@ -0,0 +1,55 @@
> > > +#include <stdio.h>
> > > +#include "gdbus_generated.h"
> > > +
> > > +static void get_log_level(Virtiofsd *virtiofsd)
> > > +{
> > > +    const char *value = virtiofsd_get_log_level(virtiofsd);
> > > +
> > > +    printf("%s\n", value ? value : "(null)");
> > > +}
> > > +
> > > +static void set_log_level(Virtiofsd *virtiofsd, const char *value)
> > > +{
> > > +    virtiofsd_set_log_level(virtiofsd, value);
> > > +}
> > > +
> > > +static void usage(const char *progname)
> > > +{
> > > +    printf("usage: %s COMMAND [args...]\n", progname);
> > > +    printf("\n");
> > > +    printf("Commands:\n");
> > > +    printf("  get-log-level       - show current log level\n");
> > > +    printf("  set-log-level LEVEL - set current log level to one of\n");
> > > +    printf("                        \"err\", \"warning\", \"info\", \"debug\"\n");
> > > +    exit(0);
> > > +}
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > +    Virtiofsd *virtiofsd;
> > > +    GError *error = NULL;
> > > +
> > > +    if (argc < 2) {
> > > +        usage(argv[0]);
> > > +    }
> > > +
> > > +    virtiofsd = virtiofsd_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION,
> > > +            G_DBUS_PROXY_FLAGS_NONE, "org.qemu.virtiofsd",
> > > +            "/org/qemu/virtiofsd", NULL, &error);
> > > +    if (error) {
> > > +        fprintf(stderr, "%s\n", error->message);
> > > +        g_error_free(error);
> > > +        return 1;
> > > +    }
> > > +
> > > +    if (strcmp(argv[0], "get-log-level") == 0) {
> >
> > This and the one below works a lot better with argv[1] !
> >
> > (I wonder if a little python script would be better for these type of
> > wrappers).
> 
> Or just plain gdbus/busctl calls (which already has bash completion
> fwiw), and/or eventually a shell script.

A virtiofsctl program is useful because the sub-commands are easily
discoverable and no DBus knowledge is required.  Writing it in Python 3
would be nice.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus
  2019-09-05 15:21 ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-05 17:40   ` Dr. David Alan Gilbert
  -1 siblings, 0 replies; 36+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-05 17:40 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, Eryu Guan, qemu-devel

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> It is likely that virtiofsd will need to support "management commands" for
> reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> getting/setting the current log level.
> 
> I promised to try out DBus as the management interface because it has a rich
> feature set and is accessible from most programming languages.  It should be
> able to support all the use cases we come up with.
> 
> This patch series is a prototype that implements the get-log-level and
> set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> talk to a running virtiofsd process:
> 
>   # dbus-run-session ./virtiofsd ...
>   ...
>   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # ./virtiofsctl set-log-level err
> 
> Most of the work is done by gdbus-codegen(1).  It generates code for the
> org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> 
> I'm pretty happy with this approach because the code is straightforward.  It
> hasn't even triggered seccomp failures yet :).

Yes it's less complex than I'd worried.
Now, I do think we've got to think about how qemu in general is going to
use dbus as people were discussing it, because then we have to think
what the security aspects are - do we need to look at some calls only
available to some clients etc.

Dave

> Error handling is a little problematic.  I noticed that virtiofsctl silently
> returns success even if it cannot talk to virtiofsd.  This is due to the code
> generated by gdbus-codegen(1) which has no error reporting :(.  This can be
> solved by writing more low-level GDBus code instead of using the high-level
> generated bindings.
> 
> What do you think about this approach?
> 
> Stefan Hajnoczi (3):
>   virtiofsd: add org.qemu.Virtiofsd interface
>   virtiofsd: add DBus server to handle log level changes
>   virtiofsd: add virtiofsctl command-line management tool
> 
>  configure                                |   7 +
>  Makefile                                 |  16 +++
>  Makefile.objs                            |   1 +
>  contrib/virtiofsd/Makefile.objs          |  10 +-
>  contrib/virtiofsd/dbus.h                 |   9 ++
>  contrib/virtiofsd/dbus.c                 | 162 +++++++++++++++++++++++
>  contrib/virtiofsd/passthrough_ll.c       |   8 +-
>  contrib/virtiofsd/virtiofsctl.c          |  55 ++++++++
>  .gitignore                               |   1 +
>  contrib/virtiofsd/org.qemu.Virtiofsd.xml |   7 +
>  10 files changed, 274 insertions(+), 2 deletions(-)
>  create mode 100644 contrib/virtiofsd/dbus.h
>  create mode 100644 contrib/virtiofsd/dbus.c
>  create mode 100644 contrib/virtiofsd/virtiofsctl.c
>  create mode 100644 contrib/virtiofsd/org.qemu.Virtiofsd.xml
> 
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [RFC 0/3] virtiofsd: get/set log level via DBus
@ 2019-09-05 17:40   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 36+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-05 17:40 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> It is likely that virtiofsd will need to support "management commands" for
> reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> getting/setting the current log level.
> 
> I promised to try out DBus as the management interface because it has a rich
> feature set and is accessible from most programming languages.  It should be
> able to support all the use cases we come up with.
> 
> This patch series is a prototype that implements the get-log-level and
> set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> talk to a running virtiofsd process:
> 
>   # dbus-run-session ./virtiofsd ...
>   ...
>   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # ./virtiofsctl set-log-level err
> 
> Most of the work is done by gdbus-codegen(1).  It generates code for the
> org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> 
> I'm pretty happy with this approach because the code is straightforward.  It
> hasn't even triggered seccomp failures yet :).

Yes it's less complex than I'd worried.
Now, I do think we've got to think about how qemu in general is going to
use dbus as people were discussing it, because then we have to think
what the security aspects are - do we need to look at some calls only
available to some clients etc.

Dave

> Error handling is a little problematic.  I noticed that virtiofsctl silently
> returns success even if it cannot talk to virtiofsd.  This is due to the code
> generated by gdbus-codegen(1) which has no error reporting :(.  This can be
> solved by writing more low-level GDBus code instead of using the high-level
> generated bindings.
> 
> What do you think about this approach?
> 
> Stefan Hajnoczi (3):
>   virtiofsd: add org.qemu.Virtiofsd interface
>   virtiofsd: add DBus server to handle log level changes
>   virtiofsd: add virtiofsctl command-line management tool
> 
>  configure                                |   7 +
>  Makefile                                 |  16 +++
>  Makefile.objs                            |   1 +
>  contrib/virtiofsd/Makefile.objs          |  10 +-
>  contrib/virtiofsd/dbus.h                 |   9 ++
>  contrib/virtiofsd/dbus.c                 | 162 +++++++++++++++++++++++
>  contrib/virtiofsd/passthrough_ll.c       |   8 +-
>  contrib/virtiofsd/virtiofsctl.c          |  55 ++++++++
>  .gitignore                               |   1 +
>  contrib/virtiofsd/org.qemu.Virtiofsd.xml |   7 +
>  10 files changed, 274 insertions(+), 2 deletions(-)
>  create mode 100644 contrib/virtiofsd/dbus.h
>  create mode 100644 contrib/virtiofsd/dbus.c
>  create mode 100644 contrib/virtiofsd/virtiofsctl.c
>  create mode 100644 contrib/virtiofsd/org.qemu.Virtiofsd.xml
> 
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus
  2019-09-05 17:40   ` [Virtio-fs] " Dr. David Alan Gilbert
@ 2019-09-06 10:29     ` Stefan Hajnoczi
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:29 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: virtio-fs, Marc-André Lureau, Eryu Guan, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 2304 bytes --]

On Thu, Sep 05, 2019 at 06:40:21PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > It is likely that virtiofsd will need to support "management commands" for
> > reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> > getting/setting the current log level.
> > 
> > I promised to try out DBus as the management interface because it has a rich
> > feature set and is accessible from most programming languages.  It should be
> > able to support all the use cases we come up with.
> > 
> > This patch series is a prototype that implements the get-log-level and
> > set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> > talk to a running virtiofsd process:
> > 
> >   # dbus-run-session ./virtiofsd ...
> >   ...
> >   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> >   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> >   # ./virtiofsctl set-log-level err
> > 
> > Most of the work is done by gdbus-codegen(1).  It generates code for the
> > org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> > virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> > 
> > I'm pretty happy with this approach because the code is straightforward.  It
> > hasn't even triggered seccomp failures yet :).
> 
> Yes it's less complex than I'd worried.
> Now, I do think we've got to think about how qemu in general is going to
> use dbus as people were discussing it, because then we have to think
> what the security aspects are - do we need to look at some calls only
> available to some clients etc.

The approach I took in this patch series is to launch a session bus just
for this virtiofsd.  The abstract socket unix(7) namespace used by GDBus
by default does not offer any security.  I think any process on the host
can connect to it, regardless of uid/gid.

A path like unix:path=/tmp/foo would allow us to use UNIX Domain Socket
permissions as the main security mechanism.  I'm not enthusiastic about
using SELinux or some kind of DBus-specific policy language if we can
avoid it because it's complex and obscure.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [RFC 0/3] virtiofsd: get/set log level via DBus
@ 2019-09-06 10:29     ` Stefan Hajnoczi
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-06 10:29 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 2304 bytes --]

On Thu, Sep 05, 2019 at 06:40:21PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > It is likely that virtiofsd will need to support "management commands" for
> > reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> > getting/setting the current log level.
> > 
> > I promised to try out DBus as the management interface because it has a rich
> > feature set and is accessible from most programming languages.  It should be
> > able to support all the use cases we come up with.
> > 
> > This patch series is a prototype that implements the get-log-level and
> > set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> > talk to a running virtiofsd process:
> > 
> >   # dbus-run-session ./virtiofsd ...
> >   ...
> >   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> >   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> >   # ./virtiofsctl set-log-level err
> > 
> > Most of the work is done by gdbus-codegen(1).  It generates code for the
> > org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> > virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> > 
> > I'm pretty happy with this approach because the code is straightforward.  It
> > hasn't even triggered seccomp failures yet :).
> 
> Yes it's less complex than I'd worried.
> Now, I do think we've got to think about how qemu in general is going to
> use dbus as people were discussing it, because then we have to think
> what the security aspects are - do we need to look at some calls only
> available to some clients etc.

The approach I took in this patch series is to launch a session bus just
for this virtiofsd.  The abstract socket unix(7) namespace used by GDBus
by default does not offer any security.  I think any process on the host
can connect to it, regardless of uid/gid.

A path like unix:path=/tmp/foo would allow us to use UNIX Domain Socket
permissions as the main security mechanism.  I'm not enthusiastic about
using SELinux or some kind of DBus-specific policy language if we can
avoid it because it's complex and obscure.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus
  2019-09-06 10:29     ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-06 10:35       ` Dr. David Alan Gilbert
  -1 siblings, 0 replies; 36+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-06 10:35 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, Eryu Guan, qemu-devel

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Thu, Sep 05, 2019 at 06:40:21PM +0100, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > It is likely that virtiofsd will need to support "management commands" for
> > > reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> > > getting/setting the current log level.
> > > 
> > > I promised to try out DBus as the management interface because it has a rich
> > > feature set and is accessible from most programming languages.  It should be
> > > able to support all the use cases we come up with.
> > > 
> > > This patch series is a prototype that implements the get-log-level and
> > > set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> > > talk to a running virtiofsd process:
> > > 
> > >   # dbus-run-session ./virtiofsd ...
> > >   ...
> > >   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> > >   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> > >   # ./virtiofsctl set-log-level err
> > > 
> > > Most of the work is done by gdbus-codegen(1).  It generates code for the
> > > org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> > > virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> > > 
> > > I'm pretty happy with this approach because the code is straightforward.  It
> > > hasn't even triggered seccomp failures yet :).
> > 
> > Yes it's less complex than I'd worried.
> > Now, I do think we've got to think about how qemu in general is going to
> > use dbus as people were discussing it, because then we have to think
> > what the security aspects are - do we need to look at some calls only
> > available to some clients etc.
> 
> The approach I took in this patch series is to launch a session bus just
> for this virtiofsd.  The abstract socket unix(7) namespace used by GDBus
> by default does not offer any security.  I think any process on the host
> can connect to it, regardless of uid/gid.
> 
> A path like unix:path=/tmp/foo would allow us to use UNIX Domain Socket
> permissions as the main security mechanism.

Yes, that I'm fine with; my worry is that there was talk of much more
complex dbus setups coming out of the qemu world with many things being
connected; and then we have to think about security upfront.

>  I'm not enthusiastic about
> using SELinux or some kind of DBus-specific policy language if we can
> avoid it because it's complex and obscure.

Right.

Dave

> Stefan


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [RFC 0/3] virtiofsd: get/set log level via DBus
@ 2019-09-06 10:35       ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 36+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-06 10:35 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Thu, Sep 05, 2019 at 06:40:21PM +0100, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > It is likely that virtiofsd will need to support "management commands" for
> > > reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> > > getting/setting the current log level.
> > > 
> > > I promised to try out DBus as the management interface because it has a rich
> > > feature set and is accessible from most programming languages.  It should be
> > > able to support all the use cases we come up with.
> > > 
> > > This patch series is a prototype that implements the get-log-level and
> > > set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> > > talk to a running virtiofsd process:
> > > 
> > >   # dbus-run-session ./virtiofsd ...
> > >   ...
> > >   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> > >   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> > >   # ./virtiofsctl set-log-level err
> > > 
> > > Most of the work is done by gdbus-codegen(1).  It generates code for the
> > > org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> > > virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> > > 
> > > I'm pretty happy with this approach because the code is straightforward.  It
> > > hasn't even triggered seccomp failures yet :).
> > 
> > Yes it's less complex than I'd worried.
> > Now, I do think we've got to think about how qemu in general is going to
> > use dbus as people were discussing it, because then we have to think
> > what the security aspects are - do we need to look at some calls only
> > available to some clients etc.
> 
> The approach I took in this patch series is to launch a session bus just
> for this virtiofsd.  The abstract socket unix(7) namespace used by GDBus
> by default does not offer any security.  I think any process on the host
> can connect to it, regardless of uid/gid.
> 
> A path like unix:path=/tmp/foo would allow us to use UNIX Domain Socket
> permissions as the main security mechanism.

Yes, that I'm fine with; my worry is that there was talk of much more
complex dbus setups coming out of the qemu world with many things being
connected; and then we have to think about security upfront.

>  I'm not enthusiastic about
> using SELinux or some kind of DBus-specific policy language if we can
> avoid it because it's complex and obscure.

Right.

Dave

> Stefan


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus
  2019-09-06 10:29     ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-06 11:03       ` Daniel P. Berrangé
  -1 siblings, 0 replies; 36+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 11:03 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: virtio-fs, Marc-André Lureau, Eryu Guan,
	Dr. David Alan Gilbert, qemu-devel

On Fri, Sep 06, 2019 at 11:29:26AM +0100, Stefan Hajnoczi wrote:
> On Thu, Sep 05, 2019 at 06:40:21PM +0100, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > It is likely that virtiofsd will need to support "management commands" for
> > > reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> > > getting/setting the current log level.
> > > 
> > > I promised to try out DBus as the management interface because it has a rich
> > > feature set and is accessible from most programming languages.  It should be
> > > able to support all the use cases we come up with.
> > > 
> > > This patch series is a prototype that implements the get-log-level and
> > > set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> > > talk to a running virtiofsd process:
> > > 
> > >   # dbus-run-session ./virtiofsd ...
> > >   ...
> > >   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> > >   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> > >   # ./virtiofsctl set-log-level err
> > > 
> > > Most of the work is done by gdbus-codegen(1).  It generates code for the
> > > org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> > > virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> > > 
> > > I'm pretty happy with this approach because the code is straightforward.  It
> > > hasn't even triggered seccomp failures yet :).
> > 
> > Yes it's less complex than I'd worried.
> > Now, I do think we've got to think about how qemu in general is going to
> > use dbus as people were discussing it, because then we have to think
> > what the security aspects are - do we need to look at some calls only
> > available to some clients etc.
> 
> The approach I took in this patch series is to launch a session bus just
> for this virtiofsd.  The abstract socket unix(7) namespace used by GDBus
> by default does not offer any security.  I think any process on the host
> can connect to it, regardless of uid/gid.

Other users will be able to connect(), but you'll find that the dbus
policy causes their connections to be dropped immediately - even if
they are the root user in fact.

> A path like unix:path=/tmp/foo would allow us to use UNIX Domain Socket
> permissions as the main security mechanism.  I'm not enthusiastic about
> using SELinux or some kind of DBus-specific policy language if we can
> avoid it because it's complex and obscure.

Yep, that just needs you to supply a config file when launching to
specify a desired filesystem path.

I don't think it is an either/or matter - I think we'll  want all
three in general - DAC controls on the socket, and DBus policy and
SELinux policy. DAC controls on the socket alone are not sufficient
if you want to separate each QEMU from each other and they're running
the same UID which is common.

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 :|


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus
@ 2019-09-06 11:03       ` Daniel P. Berrangé
  0 siblings, 0 replies; 36+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 11:03 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

On Fri, Sep 06, 2019 at 11:29:26AM +0100, Stefan Hajnoczi wrote:
> On Thu, Sep 05, 2019 at 06:40:21PM +0100, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > It is likely that virtiofsd will need to support "management commands" for
> > > reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> > > getting/setting the current log level.
> > > 
> > > I promised to try out DBus as the management interface because it has a rich
> > > feature set and is accessible from most programming languages.  It should be
> > > able to support all the use cases we come up with.
> > > 
> > > This patch series is a prototype that implements the get-log-level and
> > > set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> > > talk to a running virtiofsd process:
> > > 
> > >   # dbus-run-session ./virtiofsd ...
> > >   ...
> > >   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> > >   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
> > >   # ./virtiofsctl set-log-level err
> > > 
> > > Most of the work is done by gdbus-codegen(1).  It generates code for the
> > > org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> > > virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> > > 
> > > I'm pretty happy with this approach because the code is straightforward.  It
> > > hasn't even triggered seccomp failures yet :).
> > 
> > Yes it's less complex than I'd worried.
> > Now, I do think we've got to think about how qemu in general is going to
> > use dbus as people were discussing it, because then we have to think
> > what the security aspects are - do we need to look at some calls only
> > available to some clients etc.
> 
> The approach I took in this patch series is to launch a session bus just
> for this virtiofsd.  The abstract socket unix(7) namespace used by GDBus
> by default does not offer any security.  I think any process on the host
> can connect to it, regardless of uid/gid.

Other users will be able to connect(), but you'll find that the dbus
policy causes their connections to be dropped immediately - even if
they are the root user in fact.

> A path like unix:path=/tmp/foo would allow us to use UNIX Domain Socket
> permissions as the main security mechanism.  I'm not enthusiastic about
> using SELinux or some kind of DBus-specific policy language if we can
> avoid it because it's complex and obscure.

Yep, that just needs you to supply a config file when launching to
specify a desired filesystem path.

I don't think it is an either/or matter - I think we'll  want all
three in general - DAC controls on the socket, and DBus policy and
SELinux policy. DAC controls on the socket alone are not sufficient
if you want to separate each QEMU from each other and they're running
the same UID which is common.

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 :|


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus
  2019-09-05 15:21 ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-06 10:47   ` Daniel P. Berrangé
  -1 siblings, 0 replies; 36+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 10:47 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: virtio-fs, Marc-André Lureau, Eryu Guan, qemu-devel,
	Dr. David Alan Gilbert

On Thu, Sep 05, 2019 at 04:21:33PM +0100, Stefan Hajnoczi wrote:
> It is likely that virtiofsd will need to support "management commands" for
> reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> getting/setting the current log level.
> 
> I promised to try out DBus as the management interface because it has a rich
> feature set and is accessible from most programming languages.  It should be
> able to support all the use cases we come up with.
> 
> This patch series is a prototype that implements the get-log-level and
> set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> talk to a running virtiofsd process:
> 
>   # dbus-run-session ./virtiofsd ...
>   ...
>   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # ./virtiofsctl set-log-level err
> 
> Most of the work is done by gdbus-codegen(1).  It generates code for the
> org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> 
> I'm pretty happy with this approach because the code is straightforward.  It
> hasn't even triggered seccomp failures yet :).
> 
> Error handling is a little problematic.  I noticed that virtiofsctl silently
> returns success even if it cannot talk to virtiofsd.  This is due to the code
> generated by gdbus-codegen(1) which has no error reporting :(.  This can be
> solved by writing more low-level GDBus code instead of using the high-level
> generated bindings.

You declared a simple property for the log level, which gets mapped into
GObject properties, and hence hit the error reporting limitations they
have.  DBus at the protocol level can report errors for properties.

For log level settings I'm not worried, but more generally we may well
hit cases where error reporting is functionally critical. So we have a
choice I think

 - Enhance gdbus-codegen so that it can map DBus properties to a different
   impl, with explicit getters/setters, ignoring GObject's properties. This
   would allow for a GError ** parameter to handle/report errors

 - Don't use properties at the DBus protocol level. Instead simply define
   explicit setter & getter methods in DBus. These woudl again bypass
   GObject's properties

Option 2 is probably easier to be honest, especially since in the code
you end up calling what are setters & getters on the client side anyway.

> What do you think about this approach?

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 :|


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus
@ 2019-09-06 10:47   ` Daniel P. Berrangé
  0 siblings, 0 replies; 36+ messages in thread
From: Daniel P. Berrangé @ 2019-09-06 10:47 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

On Thu, Sep 05, 2019 at 04:21:33PM +0100, Stefan Hajnoczi wrote:
> It is likely that virtiofsd will need to support "management commands" for
> reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> getting/setting the current log level.
> 
> I promised to try out DBus as the management interface because it has a rich
> feature set and is accessible from most programming languages.  It should be
> able to support all the use cases we come up with.
> 
> This patch series is a prototype that implements the get-log-level and
> set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> talk to a running virtiofsd process:
> 
>   # dbus-run-session ./virtiofsd ...
>   ...
>   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # ./virtiofsctl set-log-level err
> 
> Most of the work is done by gdbus-codegen(1).  It generates code for the
> org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> 
> I'm pretty happy with this approach because the code is straightforward.  It
> hasn't even triggered seccomp failures yet :).
> 
> Error handling is a little problematic.  I noticed that virtiofsctl silently
> returns success even if it cannot talk to virtiofsd.  This is due to the code
> generated by gdbus-codegen(1) which has no error reporting :(.  This can be
> solved by writing more low-level GDBus code instead of using the high-level
> generated bindings.

You declared a simple property for the log level, which gets mapped into
GObject properties, and hence hit the error reporting limitations they
have.  DBus at the protocol level can report errors for properties.

For log level settings I'm not worried, but more generally we may well
hit cases where error reporting is functionally critical. So we have a
choice I think

 - Enhance gdbus-codegen so that it can map DBus properties to a different
   impl, with explicit getters/setters, ignoring GObject's properties. This
   would allow for a GError ** parameter to handle/report errors

 - Don't use properties at the DBus protocol level. Instead simply define
   explicit setter & getter methods in DBus. These woudl again bypass
   GObject's properties

Option 2 is probably easier to be honest, especially since in the code
you end up calling what are setters & getters on the client side anyway.

> What do you think about this approach?

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 :|


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus
  2019-09-05 15:21 ` [Virtio-fs] " Stefan Hajnoczi
@ 2019-09-09 12:37   ` Stefan Hajnoczi
  -1 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-09 12:37 UTC (permalink / raw)
  To: Eryu Guan
  Cc: virtio-fs, Marc-André Lureau, Dr. David Alan Gilbert, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1906 bytes --]

On Thu, Sep 05, 2019 at 04:21:33PM +0100, Stefan Hajnoczi wrote:
> It is likely that virtiofsd will need to support "management commands" for
> reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> getting/setting the current log level.
> 
> I promised to try out DBus as the management interface because it has a rich
> feature set and is accessible from most programming languages.  It should be
> able to support all the use cases we come up with.
> 
> This patch series is a prototype that implements the get-log-level and
> set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> talk to a running virtiofsd process:
> 
>   # dbus-run-session ./virtiofsd ...
>   ...
>   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # ./virtiofsctl set-log-level err
> 
> Most of the work is done by gdbus-codegen(1).  It generates code for the
> org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> 
> I'm pretty happy with this approach because the code is straightforward.  It
> hasn't even triggered seccomp failures yet :).
> 
> Error handling is a little problematic.  I noticed that virtiofsctl silently
> returns success even if it cannot talk to virtiofsd.  This is due to the code
> generated by gdbus-codegen(1) which has no error reporting :(.  This can be
> solved by writing more low-level GDBus code instead of using the high-level
> generated bindings.
> 
> What do you think about this approach?

Hi Eryu Guan,
Have you had a chance to try these patches?  Do they meet your
requirements for being able to get/set the log level at runtime?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Virtio-fs] [RFC 0/3] virtiofsd: get/set log level via DBus
@ 2019-09-09 12:37   ` Stefan Hajnoczi
  0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2019-09-09 12:37 UTC (permalink / raw)
  To: Eryu Guan; +Cc: virtio-fs, Marc-André Lureau, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1906 bytes --]

On Thu, Sep 05, 2019 at 04:21:33PM +0100, Stefan Hajnoczi wrote:
> It is likely that virtiofsd will need to support "management commands" for
> reconfiguring it at runtime.  The first use case was proposed by Eryu Guan for
> getting/setting the current log level.
> 
> I promised to try out DBus as the management interface because it has a rich
> feature set and is accessible from most programming languages.  It should be
> able to support all the use cases we come up with.
> 
> This patch series is a prototype that implements the get-log-level and
> set-log-level management commands via DBus.  Use the new virtiofsctl tool to
> talk to a running virtiofsd process:
> 
>   # dbus-run-session ./virtiofsd ...
>   ...
>   Using dbus address unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # export DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-H9WBbpjk3O,guid=0be16acefb868e6025a8737f5d7124d2
>   # ./virtiofsctl set-log-level err
> 
> Most of the work is done by gdbus-codegen(1).  It generates code for the
> org.qemu.Virtiofsd.xml interface definition.  Our code can use the simple
> virtiofsd_get/set_log_level() APIs and it will make corresponding DBus calls.
> 
> I'm pretty happy with this approach because the code is straightforward.  It
> hasn't even triggered seccomp failures yet :).
> 
> Error handling is a little problematic.  I noticed that virtiofsctl silently
> returns success even if it cannot talk to virtiofsd.  This is due to the code
> generated by gdbus-codegen(1) which has no error reporting :(.  This can be
> solved by writing more low-level GDBus code instead of using the high-level
> generated bindings.
> 
> What do you think about this approach?

Hi Eryu Guan,
Have you had a chance to try these patches?  Do they meet your
requirements for being able to get/set the log level at runtime?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2019-09-09 12:38 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05 15:21 [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus Stefan Hajnoczi
2019-09-05 15:21 ` [Virtio-fs] " Stefan Hajnoczi
2019-09-05 15:21 ` [Qemu-devel] [RFC 1/3] virtiofsd: add org.qemu.Virtiofsd interface Stefan Hajnoczi
2019-09-05 15:21   ` [Virtio-fs] " Stefan Hajnoczi
2019-09-05 15:21 ` [Qemu-devel] [RFC 2/3] virtiofsd: add DBus server to handle log level changes Stefan Hajnoczi
2019-09-05 15:21   ` [Virtio-fs] " Stefan Hajnoczi
2019-09-05 17:27   ` [Qemu-devel] " Dr. David Alan Gilbert
2019-09-05 17:27     ` [Virtio-fs] " Dr. David Alan Gilbert
2019-09-06 10:23     ` [Qemu-devel] " Stefan Hajnoczi
2019-09-06 10:23       ` [Virtio-fs] " Stefan Hajnoczi
2019-09-06 10:49       ` [Qemu-devel] " Daniel P. Berrangé
2019-09-06 10:49         ` [Virtio-fs] " Daniel P. Berrangé
2019-09-06 11:12         ` Dr. David Alan Gilbert
2019-09-06 11:12           ` [Virtio-fs] " Dr. David Alan Gilbert
2019-09-06 11:48           ` Daniel P. Berrangé
2019-09-06 11:48             ` [Virtio-fs] " Daniel P. Berrangé
2019-09-05 15:21 ` [Qemu-devel] [RFC 3/3] virtiofsd: add virtiofsctl command-line management tool Stefan Hajnoczi
2019-09-05 15:21   ` [Virtio-fs] " Stefan Hajnoczi
2019-09-05 17:12   ` [Qemu-devel] " Dr. David Alan Gilbert
2019-09-05 17:12     ` [Virtio-fs] " Dr. David Alan Gilbert
2019-09-05 20:03     ` [Qemu-devel] " Marc-André Lureau
2019-09-05 20:03       ` [Virtio-fs] " Marc-André Lureau
2019-09-06 10:33       ` [Qemu-devel] " Stefan Hajnoczi
2019-09-06 10:33         ` [Virtio-fs] " Stefan Hajnoczi
2019-09-05 17:40 ` [Qemu-devel] [RFC 0/3] virtiofsd: get/set log level via DBus Dr. David Alan Gilbert
2019-09-05 17:40   ` [Virtio-fs] " Dr. David Alan Gilbert
2019-09-06 10:29   ` [Qemu-devel] " Stefan Hajnoczi
2019-09-06 10:29     ` [Virtio-fs] " Stefan Hajnoczi
2019-09-06 10:35     ` [Qemu-devel] " Dr. David Alan Gilbert
2019-09-06 10:35       ` [Virtio-fs] " Dr. David Alan Gilbert
2019-09-06 11:03     ` [Qemu-devel] " Daniel P. Berrangé
2019-09-06 11:03       ` [Virtio-fs] " Daniel P. Berrangé
2019-09-06 10:47 ` Daniel P. Berrangé
2019-09-06 10:47   ` [Virtio-fs] " Daniel P. Berrangé
2019-09-09 12:37 ` Stefan Hajnoczi
2019-09-09 12:37   ` [Virtio-fs] " Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.