[libvirt] [dbus RFC 00/11] improve libvirt connections handling
This patch series is a proposal to how to implement libvirt connections in the libvirt D-Bus binding. The design is based on a discussion with Daniel and it should follow D-Bus convention. Each connection is represented as existing object where the object path contains the driver name: /org/libvirt/qemu and there is no need to call any connect API but simply use the existing object directly to operate with that connection. This patch series also implements strict difference between system and user bus. On the user bus only drivers with session capability are available and on the system bus only drivers with system capability are available. Pavel Hrdina (11): util: introduce VIRT_ARRAY_CARDINALITY macro util: introduce virtDBusUtilSetError helper src: rename manager to connect connect: implement lazy connection connect: don't use default libvirt authentication callback connect: implement reconnect functionality to libvirt connect: move domain related code to domain.c connect: store connect path in connect structure domain: derive domain path from connect path main: implement multiple connection within one daemon main: add support for all libvirt drivers src/Makefile.am | 2 +- src/connect.c | 308 ++++++++++++++++++++++++++++++ src/connect.h | 27 +++ src/domain.c | 131 ++++++++----- src/domain.h | 4 +- src/events.c | 64 +++---- src/events.h | 4 +- src/main.c | 75 ++++++-- src/manager.c | 216 --------------------- src/manager.h | 20 -- src/util.c | 17 +- src/util.h | 12 +- test/Makefile.am | 2 +- test/libvirttest.py | 6 +- test/{test_manager.py => test_connect.py} | 12 +- test/test_domain.py | 6 +- 16 files changed, 543 insertions(+), 363 deletions(-) create mode 100644 src/connect.c create mode 100644 src/connect.h delete mode 100644 src/manager.c delete mode 100644 src/manager.h rename test/{test_manager.py => test_connect.py} (79%) -- 2.14.3
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util.h b/src/util.h index 0c9f3d7..6d99841 100644 --- a/src/util.h +++ b/src/util.h @@ -8,6 +8,8 @@ #define _cleanup_(_x) __attribute__((__cleanup__(_x))) #define VIR_ATTR_UNUSED __attribute__((__unused__)) +#define VIRT_ARRAY_CARDINALITY(array) (sizeof(array) / sizeof(*(array))) + int virtDBusUtilMessageAppendTypedParameters(sd_bus_message *message, -- 2.14.3
On Mon, Jan 22, 2018 at 06:15:59PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/util.h b/src/util.h index 0c9f3d7..6d99841 100644 --- a/src/util.h +++ b/src/util.h @@ -8,6 +8,8 @@ #define _cleanup_(_x) __attribute__((__cleanup__(_x))) #define VIR_ATTR_UNUSED __attribute__((__unused__))
+#define VIRT_ARRAY_CARDINALITY(array) (sizeof(array) / sizeof(*(array))) +
int virtDBusUtilMessageAppendTypedParameters(sd_bus_message *message,
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Though if I was naming this again in libvirt I would follow glib and call it VIR_N_ELEMENTS as it way shorter & easier to spell than the word CARDINALITY :-) Your call ! 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 :|
On Tue, Jan 23, 2018 at 09:48:51AM +0000, Daniel P. Berrange wrote:
On Mon, Jan 22, 2018 at 06:15:59PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/util.h b/src/util.h index 0c9f3d7..6d99841 100644 --- a/src/util.h +++ b/src/util.h @@ -8,6 +8,8 @@ #define _cleanup_(_x) __attribute__((__cleanup__(_x))) #define VIR_ATTR_UNUSED __attribute__((__unused__))
+#define VIRT_ARRAY_CARDINALITY(array) (sizeof(array) / sizeof(*(array))) +
int virtDBusUtilMessageAppendTypedParameters(sd_bus_message *message,
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Though if I was naming this again in libvirt I would follow glib and call it VIR_N_ELEMENTS as it way shorter & easier to spell than the word CARDINALITY :-) Your call !
That's definitely better name, I'll use that one, thanks. Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util.c | 7 +++++++ src/util.h | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/src/util.c b/src/util.c index 2445ce0..6636d0a 100644 --- a/src/util.c +++ b/src/util.c @@ -69,6 +69,13 @@ virtDBusUtilSetLastVirtError(sd_bus_error *error) return sd_bus_error_set(error, "org.libvirt.Error", vir_error->message); } +int +virtDBusUtilSetError(sd_bus_error *error, + const char *message) +{ + return sd_bus_error_set(error, "org.libvirt.Error", message); +} + char * virtDBusUtilBusPathForVirDomain(virDomainPtr domain) { diff --git a/src/util.h b/src/util.h index 6d99841..cb8447d 100644 --- a/src/util.h +++ b/src/util.h @@ -19,6 +19,10 @@ virtDBusUtilMessageAppendTypedParameters(sd_bus_message *message, int virtDBusUtilSetLastVirtError(sd_bus_error *error); +int +virtDBusUtilSetError(sd_bus_error *error, + const char *message); + char * virtDBusUtilBusPathForVirDomain(virDomainPtr domain); -- 2.14.3
On Mon, Jan 22, 2018 at 06:16:00PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util.c | 7 +++++++ src/util.h | 4 ++++ 2 files changed, 11 insertions(+)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> 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 :|
Manager was a good name for the original design. However, the design will be changed that for now libvirt-dbus will be using only local connection and will support multiple drivers. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/Makefile.am | 2 +- src/{manager.c => connect.c} | 93 ++++++++++++++++--------------- src/{manager.h => connect.h} | 10 ++-- src/domain.c | 80 +++++++++++++------------- src/domain.h | 4 +- src/events.c | 54 +++++++++--------- src/events.h | 4 +- src/main.c | 8 +-- test/Makefile.am | 2 +- test/libvirttest.py | 4 +- test/{test_manager.py => test_connect.py} | 12 ++-- test/test_domain.py | 6 +- 12 files changed, 140 insertions(+), 139 deletions(-) rename src/{manager.c => connect.c} (64%) rename src/{manager.h => connect.h} (53%) rename test/{test_manager.py => test_connect.py} (79%) diff --git a/src/Makefile.am b/src/Makefile.am index 5d4cb04..1a5b50b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -3,7 +3,7 @@ AM_CPPFLAGS = \ DAEMON_SOURCES = \ main.c \ - manager.c manager.h \ + connect.c connect.h \ util.c util.h \ domain.c domain.h \ events.c events.h diff --git a/src/manager.c b/src/connect.c similarity index 64% rename from src/manager.c rename to src/connect.c index f289a7a..cb19c39 100644 --- a/src/manager.c +++ b/src/connect.c @@ -1,24 +1,24 @@ #include "domain.h" #include "events.h" -#include "manager.h" +#include "connect.h" #include "util.h" #include <errno.h> #include <stdlib.h> static int -virtDBusManagerEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED, +virtDBusConnectEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED, const char *path VIR_ATTR_UNUSED, void *userdata, char ***nodes, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainListFreep) virDomainPtr *domains = NULL; _cleanup_(virtDBusUtilStrvFreep) char **paths = NULL; int n_domains; - n_domains = virConnectListAllDomains(manager->connection, &domains, 0); + n_domains = virConnectListAllDomains(connect->connection, &domains, 0); if (n_domains < 0) return virtDBusUtilSetLastVirtError(error); @@ -34,11 +34,11 @@ virtDBusManagerEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED, } static int -virtDBusManagerListDomains(sd_bus_message *message, +virtDBusConnectListDomains(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; _cleanup_(virtDBusUtilVirDomainListFreep) virDomainPtr *domains = NULL; uint32_t flags; @@ -48,7 +48,7 @@ virtDBusManagerListDomains(sd_bus_message *message, if (r < 0) return r; - r = virConnectListAllDomains(manager->connection, &domains, flags); + r = virConnectListAllDomains(connect->connection, &domains, flags); if (r < 0) return virtDBusUtilSetLastVirtError(error); @@ -78,11 +78,11 @@ virtDBusManagerListDomains(sd_bus_message *message, } static int -virtDBusManagerCreateXML(sd_bus_message *message, +virtDBusConnectCreateXML(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; const char *xml; uint32_t flags; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; @@ -93,7 +93,7 @@ virtDBusManagerCreateXML(sd_bus_message *message, if (r < 0) return r; - domain = virDomainCreateXML(manager->connection, xml, flags); + domain = virDomainCreateXML(connect->connection, xml, flags); if (!domain) return virtDBusUtilSetLastVirtError(error); @@ -103,11 +103,11 @@ virtDBusManagerCreateXML(sd_bus_message *message, } static int -virtDBusManagerDefineXML(sd_bus_message *message, +virtDBusConnectDefineXML(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; const char *xml; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; _cleanup_(virtDBusUtilFreep) char *path = NULL; @@ -117,7 +117,7 @@ virtDBusManagerDefineXML(sd_bus_message *message, if (r < 0) return r; - domain = virDomainDefineXML(manager->connection, xml); + domain = virDomainDefineXML(connect->connection, xml); if (!domain) return virtDBusUtilSetLastVirtError(error); @@ -126,12 +126,12 @@ virtDBusManagerDefineXML(sd_bus_message *message, return sd_bus_reply_method_return(message, "o", path); } -static const sd_bus_vtable virt_manager_vtable[] = { +static const sd_bus_vtable virt_connect_vtable[] = { SD_BUS_VTABLE_START(0), - SD_BUS_METHOD("ListDomains", "u", "ao", virtDBusManagerListDomains, SD_BUS_VTABLE_UNPRIVILEGED), - SD_BUS_METHOD("CreateXML", "su", "o", virtDBusManagerCreateXML, SD_BUS_VTABLE_UNPRIVILEGED), - SD_BUS_METHOD("DefineXML", "s", "o", virtDBusManagerDefineXML, SD_BUS_VTABLE_UNPRIVILEGED), + SD_BUS_METHOD("ListDomains", "u", "ao", virtDBusConnectListDomains, SD_BUS_VTABLE_UNPRIVILEGED), + SD_BUS_METHOD("CreateXML", "su", "o", virtDBusConnectCreateXML, SD_BUS_VTABLE_UNPRIVILEGED), + SD_BUS_METHOD("DefineXML", "s", "o", virtDBusConnectDefineXML, SD_BUS_VTABLE_UNPRIVILEGED), SD_BUS_SIGNAL("DomainDefined", "so", 0), SD_BUS_SIGNAL("DomainUndefined", "so", 0), @@ -147,70 +147,71 @@ static const sd_bus_vtable virt_manager_vtable[] = { }; int -virtDBusManagerNew(virtDBusManager **managerp, +virtDBusConnectNew(virtDBusConnect **connectp, sd_bus *bus, const char *uri) { - _cleanup_(virtDBusManagerFreep) virtDBusManager *manager = NULL; + _cleanup_(virtDBusConnectFreep) virtDBusConnect *connect = NULL; int r; - manager = calloc(1, sizeof(virtDBusManager)); + connect = calloc(1, sizeof(virtDBusConnect)); for (int i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i += 1) - manager->callback_ids[i] = -1; + connect->callback_ids[i] = -1; - manager->bus = sd_bus_ref(bus); + connect->bus = sd_bus_ref(bus); - manager->connection = virConnectOpenAuth(uri, virConnectAuthPtrDefault, 0); - if (!manager->connection) + connect->connection = virConnectOpenAuth(uri, virConnectAuthPtrDefault, 0); + if (!connect->connection) return -EINVAL; - virtDBusEventsRegister(manager); + virtDBusEventsRegister(connect); - r = sd_bus_add_object_vtable(manager->bus, + r = sd_bus_add_object_vtable(connect->bus, NULL, - "/org/libvirt/Manager", - "org.libvirt.Manager", - virt_manager_vtable, - manager); + "/org/libvirt/Connect", + "org.libvirt.Connect", + virt_connect_vtable, + connect); if (r < 0) return r; - r = sd_bus_add_node_enumerator(bus, NULL, "/org/libvirt/domain", virtDBusManagerEnumarateDomains, manager); + r = sd_bus_add_node_enumerator(bus, NULL, "/org/libvirt/domain", + virtDBusConnectEnumarateDomains, connect); if (r < 0) return r; - if ((r = virtDBusDomainRegister(manager, bus) < 0)) + if ((r = virtDBusDomainRegister(connect, bus) < 0)) return r; - *managerp = manager; - manager = NULL; + *connectp = connect; + connect = NULL; return 0; } -virtDBusManager * -virtDBusManagerFree(virtDBusManager *manager) +virtDBusConnect * +virtDBusConnectFree(virtDBusConnect *connect) { - if (manager->bus) - sd_bus_unref(manager->bus); + if (connect->bus) + sd_bus_unref(connect->bus); - if (manager->connection) { + if (connect->connection) { for (int i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i += 1) { - if (manager->callback_ids[i] >= 0) - virConnectDomainEventDeregisterAny(manager->connection, manager->callback_ids[i]); + if (connect->callback_ids[i] >= 0) + virConnectDomainEventDeregisterAny(connect->connection, connect->callback_ids[i]); } - virConnectClose(manager->connection); + virConnectClose(connect->connection); } - free(manager); + free(connect); return NULL; } void -virtDBusManagerFreep(virtDBusManager **managerp) +virtDBusConnectFreep(virtDBusConnect **connectp) { - if (*managerp) - virtDBusManagerFree(*managerp); + if (*connectp) + virtDBusConnectFree(*connectp); } diff --git a/src/manager.h b/src/connect.h similarity index 53% rename from src/manager.h rename to src/connect.h index a1fefa6..5d64a10 100644 --- a/src/manager.h +++ b/src/connect.h @@ -5,16 +5,16 @@ #include <libvirt/libvirt.h> #include <systemd/sd-bus.h> -struct virtDBusManager { +struct virtDBusConnect { sd_bus *bus; virConnectPtr connection; int callback_ids[VIR_DOMAIN_EVENT_ID_LAST]; }; -typedef struct virtDBusManager virtDBusManager; +typedef struct virtDBusConnect virtDBusConnect; -int virtDBusManagerNew(virtDBusManager **managerp, +int virtDBusConnectNew(virtDBusConnect **connectp, sd_bus *bus, const char *uri); -virtDBusManager *virtDBusManagerFree(virtDBusManager *manager); -void virtDBusManagerFreep(virtDBusManager **managerp); +virtDBusConnect *virtDBusConnectFree(virtDBusConnect *connect); +void virtDBusConnectFreep(virtDBusConnect **connectp); diff --git a/src/domain.c b/src/domain.c index 5527778..a3e1d0b 100644 --- a/src/domain.c +++ b/src/domain.c @@ -10,11 +10,11 @@ virtDBusDomainGetName(sd_bus *bus VIR_ATTR_UNUSED, void *userdata, sd_bus_error *error VIR_ATTR_UNUSED) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; const char *name = ""; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); if (domain == NULL) return sd_bus_message_append(reply, "s", ""); @@ -34,11 +34,11 @@ virtDBusDomainGetUUID(sd_bus *bus VIR_ATTR_UNUSED, void *userdata, sd_bus_error *error VIR_ATTR_UNUSED) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; char uuid[VIR_UUID_STRING_BUFLEN] = ""; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); if (domain == NULL) return sd_bus_message_append(reply, "s", ""); @@ -56,10 +56,10 @@ virtDBusDomainGetId(sd_bus *bus VIR_ATTR_UNUSED, void *userdata, sd_bus_error *error VIR_ATTR_UNUSED) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); if (domain == NULL) return sd_bus_message_append(reply, "u", 0); @@ -75,10 +75,10 @@ virtDBusDomainGetVcpus(sd_bus *bus VIR_ATTR_UNUSED, void *userdata, sd_bus_error *error VIR_ATTR_UNUSED) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); if (domain == NULL) return sd_bus_message_append(reply, "u", 0); @@ -94,11 +94,11 @@ virtDBusDomainGetOsType(sd_bus *bus VIR_ATTR_UNUSED, void *userdata, sd_bus_error *error VIR_ATTR_UNUSED) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; _cleanup_(virtDBusUtilFreep) char *os_type = NULL; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); if (domain == NULL) return sd_bus_message_append(reply, "s", ""); @@ -118,11 +118,11 @@ virtDBusDomainGetActive(sd_bus *bus VIR_ATTR_UNUSED, void *userdata, sd_bus_error *error VIR_ATTR_UNUSED) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int active; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); if (domain == NULL) return sd_bus_message_append(reply, "b", 0); @@ -142,11 +142,11 @@ virtDBusDomainGetPersistent(sd_bus *bus VIR_ATTR_UNUSED, void *userdata, sd_bus_error *error VIR_ATTR_UNUSED) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int persistent; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); if (domain == NULL) return sd_bus_message_append(reply, "b", 0); @@ -166,12 +166,12 @@ virtDBusDomainGetState(sd_bus *bus VIR_ATTR_UNUSED, void *userdata, sd_bus_error *error VIR_ATTR_UNUSED) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int state = 0; const char *string; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); if (domain == NULL) return sd_bus_message_append(reply, "s", ""); @@ -217,11 +217,11 @@ virtDBusDomainGetAutostart(sd_bus *bus VIR_ATTR_UNUSED, void *userdata, sd_bus_error *error VIR_ATTR_UNUSED) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int autostart = 0; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); if (domain == NULL) return sd_bus_message_append(reply, "b", 0); @@ -235,13 +235,13 @@ virtDBusDomainGetXMLDesc(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; _cleanup_(virtDBusUtilFreep) char *description = NULL; uint32_t flags; int r; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, sd_bus_message_get_path(message)); if (domain == NULL) { return sd_bus_reply_method_errorf(message, @@ -273,7 +273,7 @@ virtDBusDomainGetStats(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; virDomainPtr domains[2]; _cleanup_(virtDBusDomainStatsRecordListFreep) virDomainStatsRecordPtr *records = NULL; @@ -285,7 +285,7 @@ virtDBusDomainGetStats(sd_bus_message *message, if (r < 0) return r; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, sd_bus_message_get_path(message)); if (domain == NULL) { return sd_bus_reply_method_errorf(message, @@ -316,11 +316,11 @@ virtDBusDomainShutdown(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int r; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, sd_bus_message_get_path(message)); if (domain == NULL) { return sd_bus_reply_method_errorf(message, @@ -341,11 +341,11 @@ virtDBusDomainDestroy(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int r; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, sd_bus_message_get_path(message)); if (domain == NULL) { return sd_bus_reply_method_errorf(message, @@ -366,7 +366,7 @@ virtDBusDomainReboot(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; uint32_t flags; int r; @@ -375,7 +375,7 @@ virtDBusDomainReboot(sd_bus_message *message, if (r < 0) return r; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, sd_bus_message_get_path(message)); if (domain == NULL) { return sd_bus_reply_method_errorf(message, @@ -396,7 +396,7 @@ virtDBusDomainReset(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; uint32_t flags; int r; @@ -405,7 +405,7 @@ virtDBusDomainReset(sd_bus_message *message, if (r < 0) return r; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, sd_bus_message_get_path(message)); if (domain == NULL) { return sd_bus_reply_method_errorf(message, @@ -426,11 +426,11 @@ virtDBusDomainCreate(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int r; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, sd_bus_message_get_path(message)); if (domain == NULL) { return sd_bus_reply_method_errorf(message, @@ -451,11 +451,11 @@ virtDBusDomainUndefine(sd_bus_message *message, void *userdata, sd_bus_error *error) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int r; - domain = virtDBusUtilVirDomainFromBusPath(manager->connection, + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, sd_bus_message_get_path(message)); if (domain == NULL) { return sd_bus_reply_method_errorf(message, @@ -509,7 +509,7 @@ virtDBusDomainLookup(sd_bus *bus VIR_ATTR_UNUSED, void **found, sd_bus_error *error VIR_ATTR_UNUSED) { - virtDBusManager *manager = userdata; + virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilFreep) char *name = NULL; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int r; @@ -521,22 +521,22 @@ virtDBusDomainLookup(sd_bus *bus VIR_ATTR_UNUSED, if (*name == '\0') return 0; - domain = virDomainLookupByUUIDString(manager->connection, name); + domain = virDomainLookupByUUIDString(connect->connection, name); if (!domain) return 0; /* * There's no way to unref the pointer we're returning here. So, - * return the manager object and look up the domain again in the + * return the connect object and look up the domain again in the * domain_* callbacks. */ - *found = manager; + *found = connect; return 1; } int -virtDBusDomainRegister(virtDBusManager *manager, +virtDBusDomainRegister(virtDBusConnect *connect, sd_bus *bus) { return sd_bus_add_fallback_vtable(bus, @@ -545,5 +545,5 @@ virtDBusDomainRegister(virtDBusManager *manager, "org.libvirt.Domain", virt_domain_vtable, virtDBusDomainLookup, - manager); + connect); } diff --git a/src/domain.h b/src/domain.h index 165e452..adadc6a 100644 --- a/src/domain.h +++ b/src/domain.h @@ -1,10 +1,10 @@ #pragma once -#include "manager.h" +#include "connect.h" #include <libvirt/libvirt.h> #include <systemd/sd-bus.h> int -virtDBusDomainRegister(virtDBusManager *manager, +virtDBusDomainRegister(virtDBusConnect *connect, sd_bus *bus); diff --git a/src/events.c b/src/events.c index 731ef02..92f280d 100644 --- a/src/events.c +++ b/src/events.c @@ -12,7 +12,7 @@ virtDBusEventsDomainLifecycle(virConnectPtr connection VIR_ATTR_UNUSED, int detail VIR_ATTR_UNUSED, void *opaque) { - virtDBusManager *manager = opaque; + virtDBusConnect *connect = opaque; _cleanup_(sd_bus_message_unrefp) sd_bus_message *message = NULL; const char *signal = NULL; const char *name; @@ -51,10 +51,10 @@ virtDBusEventsDomainLifecycle(virConnectPtr connection VIR_ATTR_UNUSED, return 0; } - r = sd_bus_message_new_signal(manager->bus, + r = sd_bus_message_new_signal(connect->bus, &message, - "/org/libvirt/Manager", - "org.libvirt.Manager", + "/org/libvirt/Connect", + "org.libvirt.connect", signal); if (r < 0) return r; @@ -66,7 +66,7 @@ virtDBusEventsDomainLifecycle(virConnectPtr connection VIR_ATTR_UNUSED, if (r < 0) return r; - return sd_bus_send(manager->bus, message, NULL); + return sd_bus_send(connect->bus, message, NULL); } static int @@ -75,14 +75,14 @@ virtDBusEventsDomainDeviceAdded(virConnectPtr connection VIR_ATTR_UNUSED, const char *device, void *opaque) { - virtDBusManager *manager = opaque; + virtDBusConnect *connect = opaque; _cleanup_(sd_bus_message_unrefp) sd_bus_message *message = NULL; _cleanup_(virtDBusUtilFreep) char *path = NULL; int r; path = virtDBusUtilBusPathForVirDomain(domain); - r = sd_bus_message_new_signal(manager->bus, + r = sd_bus_message_new_signal(connect->bus, &message, path, "org.libvirt.Domain", @@ -94,7 +94,7 @@ virtDBusEventsDomainDeviceAdded(virConnectPtr connection VIR_ATTR_UNUSED, if (r < 0) return r; - return sd_bus_send(manager->bus, message, NULL); + return sd_bus_send(connect->bus, message, NULL); } static int @@ -103,14 +103,14 @@ virtDBusEventsDomainDeviceRemoved(virConnectPtr connection VIR_ATTR_UNUSED, const char *device, void *opaque) { - virtDBusManager *manager = opaque; + virtDBusConnect *connect = opaque; _cleanup_(sd_bus_message_unrefp) sd_bus_message *message = NULL; _cleanup_(virtDBusUtilFreep) char *path = NULL; int r; path = virtDBusUtilBusPathForVirDomain(domain); - r = sd_bus_message_new_signal(manager->bus, + r = sd_bus_message_new_signal(connect->bus, &message, path, "org.libvirt.Domain", @@ -122,7 +122,7 @@ virtDBusEventsDomainDeviceRemoved(virConnectPtr connection VIR_ATTR_UNUSED, if (r < 0) return r; - return sd_bus_send(manager->bus, message, NULL); + return sd_bus_send(connect->bus, message, NULL); } static int @@ -132,7 +132,7 @@ virtDBusEventsDomainDiskChange(virConnectPtr connection VIR_ATTR_UNUSED, int reason, void *opaque) { - virtDBusManager *manager = opaque; + virtDBusConnect *connect = opaque; _cleanup_(sd_bus_message_unrefp) sd_bus_message *message = NULL; _cleanup_(virtDBusUtilFreep) char *path = NULL; const char *reasonstr; @@ -140,7 +140,7 @@ virtDBusEventsDomainDiskChange(virConnectPtr connection VIR_ATTR_UNUSED, path = virtDBusUtilBusPathForVirDomain(domain); - r = sd_bus_message_new_signal(manager->bus, + r = sd_bus_message_new_signal(connect->bus, &message, path, "org.libvirt.Domain", @@ -164,7 +164,7 @@ virtDBusEventsDomainDiskChange(virConnectPtr connection VIR_ATTR_UNUSED, if (r < 0) return r; - return sd_bus_send(manager->bus, message, NULL); + return sd_bus_send(connect->bus, message, NULL); } static int @@ -176,7 +176,7 @@ virtDBusEventsDomainTrayChange(virConnectPtr connection VIR_ATTR_UNUSED, int reason, void *opaque) { - virtDBusManager *manager = opaque; + virtDBusConnect *connect = opaque; _cleanup_(sd_bus_message_unrefp) sd_bus_message *message = NULL; _cleanup_(virtDBusUtilFreep) char *path = NULL; const char *reasonstr; @@ -184,7 +184,7 @@ virtDBusEventsDomainTrayChange(virConnectPtr connection VIR_ATTR_UNUSED, path = virtDBusUtilBusPathForVirDomain(domain); - r = sd_bus_message_new_signal(manager->bus, + r = sd_bus_message_new_signal(connect->bus, &message, path, "org.libvirt.Domain", @@ -208,44 +208,44 @@ virtDBusEventsDomainTrayChange(virConnectPtr connection VIR_ATTR_UNUSED, if (r < 0) return r; - return sd_bus_send(manager->bus, message, NULL); + return sd_bus_send(connect->bus, message, NULL); } static void -virtDBusEventsRegisterEvent(virtDBusManager *manager, +virtDBusEventsRegisterEvent(virtDBusConnect *connect, int id, virConnectDomainEventGenericCallback callback) { - assert(manager->callback_ids[id] == -1); + assert(connect->callback_ids[id] == -1); - manager->callback_ids[id] = virConnectDomainEventRegisterAny(manager->connection, + connect->callback_ids[id] = virConnectDomainEventRegisterAny(connect->connection, NULL, id, VIR_DOMAIN_EVENT_CALLBACK(callback), - manager, + connect, NULL); } void -virtDBusEventsRegister(virtDBusManager *manager) +virtDBusEventsRegister(virtDBusConnect *connect) { - virtDBusEventsRegisterEvent(manager, + virtDBusEventsRegisterEvent(connect, VIR_DOMAIN_EVENT_ID_LIFECYCLE, VIR_DOMAIN_EVENT_CALLBACK(virtDBusEventsDomainLifecycle)); - virtDBusEventsRegisterEvent(manager, + virtDBusEventsRegisterEvent(connect, VIR_DOMAIN_EVENT_ID_DEVICE_ADDED, VIR_DOMAIN_EVENT_CALLBACK(virtDBusEventsDomainDeviceAdded)); - virtDBusEventsRegisterEvent(manager, + virtDBusEventsRegisterEvent(connect, VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED, VIR_DOMAIN_EVENT_CALLBACK(virtDBusEventsDomainDeviceRemoved)); - virtDBusEventsRegisterEvent(manager, + virtDBusEventsRegisterEvent(connect, VIR_DOMAIN_EVENT_ID_DISK_CHANGE, VIR_DOMAIN_EVENT_CALLBACK(virtDBusEventsDomainTrayChange)); - virtDBusEventsRegisterEvent(manager, + virtDBusEventsRegisterEvent(connect, VIR_DOMAIN_EVENT_ID_TRAY_CHANGE, VIR_DOMAIN_EVENT_CALLBACK(virtDBusEventsDomainDiskChange)); diff --git a/src/events.h b/src/events.h index d64bd94..f588c85 100644 --- a/src/events.h +++ b/src/events.h @@ -1,9 +1,9 @@ #pragma once -#include "manager.h" +#include "connect.h" #include <libvirt/libvirt.h> void -virtDBusEventsRegister(virtDBusManager *manager); +virtDBusEventsRegister(virtDBusConnect *connect); diff --git a/src/main.c b/src/main.c index 225fb46..95522f5 100644 --- a/src/main.c +++ b/src/main.c @@ -1,6 +1,6 @@ #include "config.h" -#include "manager.h" +#include "connect.h" #include "util.h" #include <errno.h> @@ -98,7 +98,7 @@ main(int argc, char *argv[]) bool system_bus; const char *uri = NULL; - _cleanup_(virtDBusManagerFreep) virtDBusManager *manager = NULL; + _cleanup_(virtDBusConnectFreep) virtDBusConnect *connect = NULL; _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; _cleanup_(virtDBusUtilClosep) int signal_fd = -1; _cleanup_(virtDBusVirEventRemoveHandlep) int bus_watch = -1; @@ -170,9 +170,9 @@ main(int argc, char *argv[]) return EXIT_FAILURE; } - r = virtDBusManagerNew(&manager, bus, uri); + r = virtDBusConnectNew(&connect, bus, uri); if (r < 0) { - fprintf(stderr, "Failed to connect to libvirt"); + fprintf(stderr, "Failed to connect to libvirt."); return EXIT_FAILURE; } diff --git a/test/Makefile.am b/test/Makefile.am index a7f22d0..4651d08 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -2,7 +2,7 @@ test_helpers = \ libvirttest.py test_programs = \ - test_manager.py \ + test_connect.py \ test_domain.py TESTS = $(test_programs) diff --git a/test/libvirttest.py b/test/libvirttest.py index f5c2020..2c735e9 100644 --- a/test/libvirttest.py +++ b/test/libvirttest.py @@ -38,8 +38,8 @@ class TestCase(unittest.TestCase): else: raise TimeoutError('error starting libvirt-dbus') - obj = self.bus.get_object('org.libvirt', '/org/libvirt/Manager') - self.manager = dbus.Interface(obj, 'org.libvirt.Manager') + obj = self.bus.get_object('org.libvirt', '/org/libvirt/Connect') + self.connect = dbus.Interface(obj, 'org.libvirt.Connect') def tearDown(self): self.daemon.terminate() diff --git a/test/test_manager.py b/test/test_connect.py similarity index 79% rename from test/test_manager.py rename to test/test_connect.py index 1f58b9d..4ec3fe0 100755 --- a/test/test_manager.py +++ b/test/test_connect.py @@ -14,9 +14,9 @@ minimal_xml = ''' </domain> ''' -class TestManager(libvirttest.TestCase): +class TestConnect(libvirttest.TestCase): def test_list_domains(self): - domains = self.manager.ListDomains(0) + domains = self.connect.ListDomains(0) self.assertEqual(type(domains), dbus.Array) self.assertEqual(len(domains), 1) @@ -33,9 +33,9 @@ class TestManager(libvirttest.TestCase): self.assertEqual(type(path), dbus.ObjectPath) self.loop.quit() - self.manager.connect_to_signal('DomainStarted', domain_started) + self.connect.connect_to_signal('DomainStarted', domain_started) - path = self.manager.CreateXML(minimal_xml, 0) + path = self.connect.CreateXML(minimal_xml, 0) self.assertEqual(type(path), dbus.ObjectPath) self.main_loop() @@ -46,9 +46,9 @@ class TestManager(libvirttest.TestCase): self.assertEqual(type(path), dbus.ObjectPath) self.loop.quit() - self.manager.connect_to_signal('DomainDefined', domain_defined) + self.connect.connect_to_signal('DomainDefined', domain_defined) - path = self.manager.DefineXML(minimal_xml) + path = self.connect.DefineXML(minimal_xml) self.assertEqual(type(path), dbus.ObjectPath) self.main_loop() diff --git a/test/test_domain.py b/test/test_domain.py index 2b15285..b1ab7a5 100755 --- a/test/test_domain.py +++ b/test/test_domain.py @@ -6,7 +6,7 @@ import unittest class TestDomain(libvirttest.TestCase): def domain(self): - path = self.manager.ListDomains(0)[0] + path = self.connect.ListDomains(0)[0] obj = self.bus.get_object('org.libvirt', path) return obj, dbus.Interface(obj, 'org.libvirt.Domain') @@ -42,7 +42,7 @@ class TestDomain(libvirttest.TestCase): self.assertEqual(type(path), dbus.ObjectPath) self.loop.quit() - self.manager.connect_to_signal('DomainStopped', domain_stopped) + self.connect.connect_to_signal('DomainStopped', domain_stopped) obj, domain = self.domain() domain.Shutdown() @@ -58,7 +58,7 @@ class TestDomain(libvirttest.TestCase): self.assertEqual(type(path), dbus.ObjectPath) self.loop.quit() - self.manager.connect_to_signal('DomainUndefined', domain_undefined) + self.connect.connect_to_signal('DomainUndefined', domain_undefined) _, domain = self.domain() domain.Shutdown() -- 2.14.3
On Mon, Jan 22, 2018 at 06:16:01PM +0100, Pavel Hrdina wrote:
Manager was a good name for the original design. However, the design will be changed that for now libvirt-dbus will be using only local connection and will support multiple drivers.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/Makefile.am | 2 +- src/{manager.c => connect.c} | 93 ++++++++++++++++--------------- src/{manager.h => connect.h} | 10 ++-- src/domain.c | 80 +++++++++++++------------- src/domain.h | 4 +- src/events.c | 54 +++++++++--------- src/events.h | 4 +- src/main.c | 8 +-- test/Makefile.am | 2 +- test/libvirttest.py | 4 +- test/{test_manager.py => test_connect.py} | 12 ++-- test/test_domain.py | 6 +- 12 files changed, 140 insertions(+), 139 deletions(-) rename src/{manager.c => connect.c} (64%) rename src/{manager.h => connect.h} (53%) rename test/{test_manager.py => test_connect.py} (79%)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> 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 :|
Open a connection to libvirt only when it is required. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 41 +++++++++++++++++++++++++++++++++++------ src/connect.h | 1 + 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/connect.c b/src/connect.c index cb19c39..8d958c2 100644 --- a/src/connect.c +++ b/src/connect.c @@ -6,6 +6,23 @@ #include <errno.h> #include <stdlib.h> +static int +virtDBusConnectOpen(virtDBusConnect *connect, + sd_bus_error *error) +{ + if (connect->connection) + return 0; + + connect->connection = virConnectOpenAuth(connect->uri, + virConnectAuthPtrDefault, 0); + if (!connect->connection) + return virtDBusUtilSetLastVirtError(error); + + virtDBusEventsRegister(connect); + + return 0; +} + static int virtDBusConnectEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED, const char *path VIR_ATTR_UNUSED, @@ -17,6 +34,11 @@ virtDBusConnectEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED, _cleanup_(virtDBusUtilVirDomainListFreep) virDomainPtr *domains = NULL; _cleanup_(virtDBusUtilStrvFreep) char **paths = NULL; int n_domains; + int r; + + r = virtDBusConnectOpen(connect, error); + if (r < 0) + return r; n_domains = virConnectListAllDomains(connect->connection, &domains, 0); if (n_domains < 0) @@ -44,6 +66,10 @@ virtDBusConnectListDomains(sd_bus_message *message, uint32_t flags; int r; + r = virtDBusConnectOpen(connect, error); + if (r < 0) + return r; + r = sd_bus_message_read(message, "u", &flags); if (r < 0) return r; @@ -89,6 +115,10 @@ virtDBusConnectCreateXML(sd_bus_message *message, _cleanup_(virtDBusUtilFreep) char *path = NULL; int r; + r = virtDBusConnectOpen(connect, error); + if (r < 0) + return r; + r = sd_bus_message_read(message, "su", &xml, &flags); if (r < 0) return r; @@ -113,6 +143,10 @@ virtDBusConnectDefineXML(sd_bus_message *message, _cleanup_(virtDBusUtilFreep) char *path = NULL; int r; + r = virtDBusConnectOpen(connect, error); + if (r < 0) + return r; + r = sd_bus_message_read(message, "s", &xml); if (r < 0) return r; @@ -159,12 +193,7 @@ virtDBusConnectNew(virtDBusConnect **connectp, connect->callback_ids[i] = -1; connect->bus = sd_bus_ref(bus); - - connect->connection = virConnectOpenAuth(uri, virConnectAuthPtrDefault, 0); - if (!connect->connection) - return -EINVAL; - - virtDBusEventsRegister(connect); + connect->uri = uri; r = sd_bus_add_object_vtable(connect->bus, NULL, diff --git a/src/connect.h b/src/connect.h index 5d64a10..52e8279 100644 --- a/src/connect.h +++ b/src/connect.h @@ -7,6 +7,7 @@ struct virtDBusConnect { sd_bus *bus; + const char *uri; virConnectPtr connection; int callback_ids[VIR_DOMAIN_EVENT_ID_LAST]; -- 2.14.3
On Mon, Jan 22, 2018 at 06:16:02PM +0100, Pavel Hrdina wrote:
Open a connection to libvirt only when it is required.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 41 +++++++++++++++++++++++++++++++++++------ src/connect.h | 1 + 2 files changed, 36 insertions(+), 6 deletions(-)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> 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 :|
We need to implement our own authentication callback because the default one ask for credentials using STDIO. This is not suitable behavior for daemon. For now we will require usage of client configuration file for libvirt to provide credentials for drivers that require authentication [1]. [1] <https://libvirt.org/auth.html#Auth_client_config> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/connect.c b/src/connect.c index 8d958c2..9de764c 100644 --- a/src/connect.c +++ b/src/connect.c @@ -6,6 +6,34 @@ #include <errno.h> #include <stdlib.h> +static int virtDBusConnectCredType[] = { + VIR_CRED_AUTHNAME, + VIR_CRED_ECHOPROMPT, + VIR_CRED_REALM, + VIR_CRED_PASSPHRASE, + VIR_CRED_NOECHOPROMPT, + VIR_CRED_EXTERNAL, +}; + +static int +virtDBusConnectAuthCallback(virConnectCredentialPtr cred VIR_ATTR_UNUSED, + unsigned int ncred VIR_ATTR_UNUSED, + void *cbdata) +{ + sd_bus_error *error = cbdata; + + return virtDBusUtilSetError(error, + "Interactive authentication is not supported. " + "Use client configuration file for libvirt."); +} + +static virConnectAuth virtDBusConnectAuth = { + virtDBusConnectCredType, + VIRT_ARRAY_CARDINALITY(virtDBusConnectCredType), + virtDBusConnectAuthCallback, + NULL, +}; + static int virtDBusConnectOpen(virtDBusConnect *connect, sd_bus_error *error) @@ -13,8 +41,10 @@ virtDBusConnectOpen(virtDBusConnect *connect, if (connect->connection) return 0; + virtDBusConnectAuth.cbdata = error; + connect->connection = virConnectOpenAuth(connect->uri, - virConnectAuthPtrDefault, 0); + &virtDBusConnectAuth, 0); if (!connect->connection) return virtDBusUtilSetLastVirtError(error); -- 2.14.3
On Mon, Jan 22, 2018 at 06:16:03PM +0100, Pavel Hrdina wrote:
We need to implement our own authentication callback because the default one ask for credentials using STDIO. This is not suitable behavior for daemon.
For now we will require usage of client configuration file for libvirt to provide credentials for drivers that require authentication [1].
This is a long standing bug we should look at fixing one day. We should make it possible to rely on the auth config file from a simple call to virConnectOpen(), so people only need to use OpenAuth() when they have a genuine callback to provide....
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> 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 :|
If the connection dies for some reason between D-Bus calls we need to properly reconnect because the current connection is not usable anymore. Without this the only solution would be to restart the libvirt-dbus daemon. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/src/connect.c b/src/connect.c index 9de764c..10183f3 100644 --- a/src/connect.c +++ b/src/connect.c @@ -4,6 +4,7 @@ #include "util.h" #include <errno.h> +#include <stdbool.h> #include <stdlib.h> static int virtDBusConnectCredType[] = { @@ -34,12 +35,34 @@ static virConnectAuth virtDBusConnectAuth = { NULL, }; +static void +virtDBusConnectClose(virtDBusConnect *connect, + bool deregisterEvents) +{ + + for (int i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i += 1) { + if (connect->callback_ids[i] >= 0) { + if (deregisterEvents) { + virConnectDomainEventDeregisterAny(connect->connection, + connect->callback_ids[i]); + } + connect->callback_ids[i] = -1; + } + } + + virConnectClose(connect->connection); +} + static int virtDBusConnectOpen(virtDBusConnect *connect, sd_bus_error *error) { - if (connect->connection) - return 0; + if (connect->connection) { + if (virConnectIsAlive(connect->connection)) + return 0; + else + virtDBusConnectClose(connect, false); + } virtDBusConnectAuth.cbdata = error; @@ -254,14 +277,8 @@ virtDBusConnectFree(virtDBusConnect *connect) if (connect->bus) sd_bus_unref(connect->bus); - if (connect->connection) { - for (int i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i += 1) { - if (connect->callback_ids[i] >= 0) - virConnectDomainEventDeregisterAny(connect->connection, connect->callback_ids[i]); - } - - virConnectClose(connect->connection); - } + if (connect->connection) + virtDBusConnectClose(connect, true); free(connect); -- 2.14.3
On Mon, Jan 22, 2018 at 06:16:04PM +0100, Pavel Hrdina wrote:
If the connection dies for some reason between D-Bus calls we need to properly reconnect because the current connection is not usable anymore. Without this the only solution would be to restart the libvirt-dbus daemon.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/src/connect.c b/src/connect.c index 9de764c..10183f3 100644 --- a/src/connect.c +++ b/src/connect.c @@ -4,6 +4,7 @@ #include "util.h"
#include <errno.h> +#include <stdbool.h> #include <stdlib.h>
static int virtDBusConnectCredType[] = { @@ -34,12 +35,34 @@ static virConnectAuth virtDBusConnectAuth = { NULL, };
+static void +virtDBusConnectClose(virtDBusConnect *connect, + bool deregisterEvents) +{ + + for (int i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i += 1) { + if (connect->callback_ids[i] >= 0) { + if (deregisterEvents) { + virConnectDomainEventDeregisterAny(connect->connection, + connect->callback_ids[i]); + } + connect->callback_ids[i] = -1; + } + } + + virConnectClose(connect->connection);
I think it is prudent to set connect->connection = NULL at this point.
static int virtDBusConnectOpen(virtDBusConnect *connect, sd_bus_error *error) { - if (connect->connection) - return 0; + if (connect->connection) { + if (virConnectIsAlive(connect->connection))
This means that every single dbus call runs an extra RPC call to ping the server for liveliness. That's not a huge problem, but at some point I'd recommend that just use the close callback to immediately detect connection failure and close the connection & run background job to re-open it. Presumably you're going to issue dbus signals for domain lifecycle events. Using the close callback & job to re-open means your lifecycle events will start working again in the shortest amount of time, instead of waiting for the next methd call to detect the failure.
+ return 0; + else + virtDBusConnectClose(connect, false); + }
virtDBusConnectAuth.cbdata = error;
@@ -254,14 +277,8 @@ virtDBusConnectFree(virtDBusConnect *connect) if (connect->bus) sd_bus_unref(connect->bus);
- if (connect->connection) { - for (int i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i += 1) { - if (connect->callback_ids[i] >= 0) - virConnectDomainEventDeregisterAny(connect->connection, connect->callback_ids[i]); - } - - virConnectClose(connect->connection); - } + if (connect->connection) + virtDBusConnectClose(connect, true);
free(connect);
With the 1 small change Reviewed-by: Daniel P. Berrange <berrange@redhat.com> 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 :|
On Tue, Jan 23, 2018 at 10:02:24AM +0000, Daniel P. Berrange wrote:
On Mon, Jan 22, 2018 at 06:16:04PM +0100, Pavel Hrdina wrote:
If the connection dies for some reason between D-Bus calls we need to properly reconnect because the current connection is not usable anymore. Without this the only solution would be to restart the libvirt-dbus daemon.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/src/connect.c b/src/connect.c index 9de764c..10183f3 100644 --- a/src/connect.c +++ b/src/connect.c @@ -4,6 +4,7 @@ #include "util.h"
#include <errno.h> +#include <stdbool.h> #include <stdlib.h>
static int virtDBusConnectCredType[] = { @@ -34,12 +35,34 @@ static virConnectAuth virtDBusConnectAuth = { NULL, };
+static void +virtDBusConnectClose(virtDBusConnect *connect, + bool deregisterEvents) +{ + + for (int i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i += 1) { + if (connect->callback_ids[i] >= 0) { + if (deregisterEvents) { + virConnectDomainEventDeregisterAny(connect->connection, + connect->callback_ids[i]); + } + connect->callback_ids[i] = -1; + } + } + + virConnectClose(connect->connection);
I think it is prudent to set connect->connection = NULL at this point.
Right, I'll fix that.
static int virtDBusConnectOpen(virtDBusConnect *connect, sd_bus_error *error) { - if (connect->connection) - return 0; + if (connect->connection) { + if (virConnectIsAlive(connect->connection))
This means that every single dbus call runs an extra RPC call to ping the server for liveliness.
That's not a huge problem, but at some point I'd recommend that just use the close callback to immediately detect connection failure and close the connection & run background job to re-open it.
Presumably you're going to issue dbus signals for domain lifecycle events. Using the close callback & job to re-open means your lifecycle events will start working again in the shortest amount of time, instead of waiting for the next methd call to detect the failure.
I knew that this implementation was too easy. I'll prepare a followup patch to use close callback instead. Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 7 ++----- src/connect.h | 2 ++ src/domain.c | 7 +++++++ 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/connect.c b/src/connect.c index 10183f3..314277f 100644 --- a/src/connect.c +++ b/src/connect.c @@ -248,6 +248,8 @@ virtDBusConnectNew(virtDBusConnect **connectp, connect->bus = sd_bus_ref(bus); connect->uri = uri; + connect->enumerateDomains = virtDBusConnectEnumarateDomains; + r = sd_bus_add_object_vtable(connect->bus, NULL, "/org/libvirt/Connect", @@ -257,11 +259,6 @@ virtDBusConnectNew(virtDBusConnect **connectp, if (r < 0) return r; - r = sd_bus_add_node_enumerator(bus, NULL, "/org/libvirt/domain", - virtDBusConnectEnumarateDomains, connect); - if (r < 0) - return r; - if ((r = virtDBusDomainRegister(connect, bus) < 0)) return r; diff --git a/src/connect.h b/src/connect.h index 52e8279..de1aae7 100644 --- a/src/connect.h +++ b/src/connect.h @@ -10,6 +10,8 @@ struct virtDBusConnect { const char *uri; virConnectPtr connection; + sd_bus_node_enumerator_t enumerateDomains; + int callback_ids[VIR_DOMAIN_EVENT_ID_LAST]; }; typedef struct virtDBusConnect virtDBusConnect; diff --git a/src/domain.c b/src/domain.c index a3e1d0b..bc465b0 100644 --- a/src/domain.c +++ b/src/domain.c @@ -539,6 +539,13 @@ int virtDBusDomainRegister(virtDBusConnect *connect, sd_bus *bus) { + int r; + + r = sd_bus_add_node_enumerator(bus, NULL, "/org/libvirt/domain", + connect->enumerateDomains, connect); + if (r < 0) + return r; + return sd_bus_add_fallback_vtable(bus, NULL, "/org/libvirt/domain", -- 2.14.3
On Mon, Jan 22, 2018 at 06:16:05PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 7 ++----- src/connect.h | 2 ++ src/domain.c | 7 +++++++ 3 files changed, 11 insertions(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> 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 :|
This prepares for multiple connection objects since each object will have different path based on the driver name. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 3 ++- src/connect.h | 1 + src/events.c | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/connect.c b/src/connect.c index 314277f..3aec99e 100644 --- a/src/connect.c +++ b/src/connect.c @@ -247,12 +247,13 @@ virtDBusConnectNew(virtDBusConnect **connectp, connect->bus = sd_bus_ref(bus); connect->uri = uri; + connect->connectPath = "/org/libvirt/Connect"; connect->enumerateDomains = virtDBusConnectEnumarateDomains; r = sd_bus_add_object_vtable(connect->bus, NULL, - "/org/libvirt/Connect", + connect->connectPath, "org.libvirt.Connect", virt_connect_vtable, connect); diff --git a/src/connect.h b/src/connect.h index de1aae7..489ce84 100644 --- a/src/connect.h +++ b/src/connect.h @@ -8,6 +8,7 @@ struct virtDBusConnect { sd_bus *bus; const char *uri; + const char *connectPath; virConnectPtr connection; sd_bus_node_enumerator_t enumerateDomains; diff --git a/src/events.c b/src/events.c index 92f280d..d2d7f6b 100644 --- a/src/events.c +++ b/src/events.c @@ -53,7 +53,7 @@ virtDBusEventsDomainLifecycle(virConnectPtr connection VIR_ATTR_UNUSED, r = sd_bus_message_new_signal(connect->bus, &message, - "/org/libvirt/Connect", + connect->connectPath, "org.libvirt.connect", signal); if (r < 0) -- 2.14.3
On Mon, Jan 22, 2018 at 06:16:06PM +0100, Pavel Hrdina wrote:
This prepares for multiple connection objects since each object will have different path based on the driver name.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 3 ++- src/connect.h | 1 + src/events.c | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> 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 :|
Each connection have its own domains so we must make the domain path as a sub-path of the specific connection. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 12 +++++++---- src/connect.h | 1 + src/domain.c | 64 ++++++++++++++++++++++++++++++++++++++++------------------- src/events.c | 10 +++++----- src/util.c | 10 ++++++---- src/util.h | 6 ++++-- 6 files changed, 68 insertions(+), 35 deletions(-) diff --git a/src/connect.c b/src/connect.c index 3aec99e..5249eba 100644 --- a/src/connect.c +++ b/src/connect.c @@ -100,7 +100,8 @@ virtDBusConnectEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED, paths = calloc(n_domains + 1, sizeof(char *)); for (int i = 0; i < n_domains; i += 1) - paths[i] = virtDBusUtilBusPathForVirDomain(domains[i]); + paths[i] = virtDBusUtilBusPathForVirDomain(domains[i], + connect->domainPath); *nodes = paths; paths = NULL; @@ -142,7 +143,8 @@ virtDBusConnectListDomains(sd_bus_message *message, for (int i = 0; domains[i] != NULL; i += 1) { _cleanup_(virtDBusUtilFreep) char *path = NULL; - path = virtDBusUtilBusPathForVirDomain(domains[i]); + path = virtDBusUtilBusPathForVirDomain(domains[i], + connect->domainPath); r = sd_bus_message_append(reply, "o", path); if (r < 0) @@ -180,7 +182,7 @@ virtDBusConnectCreateXML(sd_bus_message *message, if (!domain) return virtDBusUtilSetLastVirtError(error); - path = virtDBusUtilBusPathForVirDomain(domain); + path = virtDBusUtilBusPathForVirDomain(domain, connect->domainPath); return sd_bus_reply_method_return(message, "o", path); } @@ -208,7 +210,7 @@ virtDBusConnectDefineXML(sd_bus_message *message, if (!domain) return virtDBusUtilSetLastVirtError(error); - path = virtDBusUtilBusPathForVirDomain(domain); + path = virtDBusUtilBusPathForVirDomain(domain, connect->domainPath); return sd_bus_reply_method_return(message, "o", path); } @@ -278,6 +280,8 @@ virtDBusConnectFree(virtDBusConnect *connect) if (connect->connection) virtDBusConnectClose(connect, true); + free(connect->domainPath); + free(connect); return NULL; diff --git a/src/connect.h b/src/connect.h index 489ce84..bed2c7e 100644 --- a/src/connect.h +++ b/src/connect.h @@ -9,6 +9,7 @@ struct virtDBusConnect { sd_bus *bus; const char *uri; const char *connectPath; + char *domainPath; virConnectPtr connection; sd_bus_node_enumerator_t enumerateDomains; diff --git a/src/domain.c b/src/domain.c index bc465b0..33ed2b3 100644 --- a/src/domain.c +++ b/src/domain.c @@ -1,6 +1,9 @@ +#define _GNU_SOURCE #include "domain.h" #include "util.h" +#include <stdio.h> + static int virtDBusDomainGetName(sd_bus *bus VIR_ATTR_UNUSED, const char *path, @@ -14,7 +17,8 @@ virtDBusDomainGetName(sd_bus *bus VIR_ATTR_UNUSED, _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; const char *name = ""; - domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path, + connect->domainPath); if (domain == NULL) return sd_bus_message_append(reply, "s", ""); @@ -38,7 +42,8 @@ virtDBusDomainGetUUID(sd_bus *bus VIR_ATTR_UNUSED, _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; char uuid[VIR_UUID_STRING_BUFLEN] = ""; - domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path, + connect->domainPath); if (domain == NULL) return sd_bus_message_append(reply, "s", ""); @@ -59,7 +64,8 @@ virtDBusDomainGetId(sd_bus *bus VIR_ATTR_UNUSED, virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; - domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path, + connect->domainPath); if (domain == NULL) return sd_bus_message_append(reply, "u", 0); @@ -78,7 +84,8 @@ virtDBusDomainGetVcpus(sd_bus *bus VIR_ATTR_UNUSED, virtDBusConnect *connect = userdata; _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; - domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path, + connect->domainPath); if (domain == NULL) return sd_bus_message_append(reply, "u", 0); @@ -98,7 +105,8 @@ virtDBusDomainGetOsType(sd_bus *bus VIR_ATTR_UNUSED, _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; _cleanup_(virtDBusUtilFreep) char *os_type = NULL; - domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path, + connect->domainPath); if (domain == NULL) return sd_bus_message_append(reply, "s", ""); @@ -122,7 +130,8 @@ virtDBusDomainGetActive(sd_bus *bus VIR_ATTR_UNUSED, _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int active; - domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path, + connect->domainPath); if (domain == NULL) return sd_bus_message_append(reply, "b", 0); @@ -146,7 +155,8 @@ virtDBusDomainGetPersistent(sd_bus *bus VIR_ATTR_UNUSED, _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int persistent; - domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path, + connect->domainPath); if (domain == NULL) return sd_bus_message_append(reply, "b", 0); @@ -171,7 +181,8 @@ virtDBusDomainGetState(sd_bus *bus VIR_ATTR_UNUSED, int state = 0; const char *string; - domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path, + connect->domainPath); if (domain == NULL) return sd_bus_message_append(reply, "s", ""); @@ -221,7 +232,8 @@ virtDBusDomainGetAutostart(sd_bus *bus VIR_ATTR_UNUSED, _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int autostart = 0; - domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path); + domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path, + connect->domainPath); if (domain == NULL) return sd_bus_message_append(reply, "b", 0); @@ -242,7 +254,8 @@ virtDBusDomainGetXMLDesc(sd_bus_message *message, int r; domain = virtDBusUtilVirDomainFromBusPath(connect->connection, - sd_bus_message_get_path(message)); + sd_bus_message_get_path(message), + connect->domainPath); if (domain == NULL) { return sd_bus_reply_method_errorf(message, SD_BUS_ERROR_UNKNOWN_OBJECT, @@ -286,7 +299,8 @@ virtDBusDomainGetStats(sd_bus_message *message, return r; domain = virtDBusUtilVirDomainFromBusPath(connect->connection, - sd_bus_message_get_path(message)); + sd_bus_message_get_path(message), + connect->domainPath); if (domain == NULL) { return sd_bus_reply_method_errorf(message, SD_BUS_ERROR_UNKNOWN_OBJECT, @@ -321,7 +335,8 @@ virtDBusDomainShutdown(sd_bus_message *message, int r; domain = virtDBusUtilVirDomainFromBusPath(connect->connection, - sd_bus_message_get_path(message)); + sd_bus_message_get_path(message), + connect->domainPath); if (domain == NULL) { return sd_bus_reply_method_errorf(message, SD_BUS_ERROR_UNKNOWN_OBJECT, @@ -346,7 +361,8 @@ virtDBusDomainDestroy(sd_bus_message *message, int r; domain = virtDBusUtilVirDomainFromBusPath(connect->connection, - sd_bus_message_get_path(message)); + sd_bus_message_get_path(message), + connect->domainPath); if (domain == NULL) { return sd_bus_reply_method_errorf(message, SD_BUS_ERROR_UNKNOWN_OBJECT, @@ -376,7 +392,8 @@ virtDBusDomainReboot(sd_bus_message *message, return r; domain = virtDBusUtilVirDomainFromBusPath(connect->connection, - sd_bus_message_get_path(message)); + sd_bus_message_get_path(message), + connect->domainPath); if (domain == NULL) { return sd_bus_reply_method_errorf(message, SD_BUS_ERROR_UNKNOWN_OBJECT, @@ -406,7 +423,8 @@ virtDBusDomainReset(sd_bus_message *message, return r; domain = virtDBusUtilVirDomainFromBusPath(connect->connection, - sd_bus_message_get_path(message)); + sd_bus_message_get_path(message), + connect->domainPath); if (domain == NULL) { return sd_bus_reply_method_errorf(message, SD_BUS_ERROR_UNKNOWN_OBJECT, @@ -431,7 +449,8 @@ virtDBusDomainCreate(sd_bus_message *message, int r; domain = virtDBusUtilVirDomainFromBusPath(connect->connection, - sd_bus_message_get_path(message)); + sd_bus_message_get_path(message), + connect->domainPath); if (domain == NULL) { return sd_bus_reply_method_errorf(message, SD_BUS_ERROR_UNKNOWN_OBJECT, @@ -456,7 +475,8 @@ virtDBusDomainUndefine(sd_bus_message *message, int r; domain = virtDBusUtilVirDomainFromBusPath(connect->connection, - sd_bus_message_get_path(message)); + sd_bus_message_get_path(message), + connect->domainPath); if (domain == NULL) { return sd_bus_reply_method_errorf(message, SD_BUS_ERROR_UNKNOWN_OBJECT, @@ -514,7 +534,7 @@ virtDBusDomainLookup(sd_bus *bus VIR_ATTR_UNUSED, _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL; int r; - r = sd_bus_path_decode(path, "/org/libvirt/domain", &name); + r = sd_bus_path_decode(path, connect->domainPath, &name); if (r < 0) return r; @@ -541,14 +561,18 @@ virtDBusDomainRegister(virtDBusConnect *connect, { int r; - r = sd_bus_add_node_enumerator(bus, NULL, "/org/libvirt/domain", + r = asprintf(&connect->domainPath, "%s/domain", connect->connectPath); + if (r < 0) + return r; + + r = sd_bus_add_node_enumerator(bus, NULL, connect->domainPath, connect->enumerateDomains, connect); if (r < 0) return r; return sd_bus_add_fallback_vtable(bus, NULL, - "/org/libvirt/domain", + connect->domainPath, "org.libvirt.Domain", virt_domain_vtable, virtDBusDomainLookup, diff --git a/src/events.c b/src/events.c index d2d7f6b..05d5940 100644 --- a/src/events.c +++ b/src/events.c @@ -60,7 +60,7 @@ virtDBusEventsDomainLifecycle(virConnectPtr connection VIR_ATTR_UNUSED, return r; name = virDomainGetName(domain); - path = virtDBusUtilBusPathForVirDomain(domain); + path = virtDBusUtilBusPathForVirDomain(domain, connect->domainPath); r = sd_bus_message_append(message, "so", name ? : "", path); if (r < 0) @@ -80,7 +80,7 @@ virtDBusEventsDomainDeviceAdded(virConnectPtr connection VIR_ATTR_UNUSED, _cleanup_(virtDBusUtilFreep) char *path = NULL; int r; - path = virtDBusUtilBusPathForVirDomain(domain); + path = virtDBusUtilBusPathForVirDomain(domain, connect->domainPath); r = sd_bus_message_new_signal(connect->bus, &message, @@ -108,7 +108,7 @@ virtDBusEventsDomainDeviceRemoved(virConnectPtr connection VIR_ATTR_UNUSED, _cleanup_(virtDBusUtilFreep) char *path = NULL; int r; - path = virtDBusUtilBusPathForVirDomain(domain); + path = virtDBusUtilBusPathForVirDomain(domain, connect->domainPath); r = sd_bus_message_new_signal(connect->bus, &message, @@ -138,7 +138,7 @@ virtDBusEventsDomainDiskChange(virConnectPtr connection VIR_ATTR_UNUSED, const char *reasonstr; int r; - path = virtDBusUtilBusPathForVirDomain(domain); + path = virtDBusUtilBusPathForVirDomain(domain, connect->domainPath); r = sd_bus_message_new_signal(connect->bus, &message, @@ -182,7 +182,7 @@ virtDBusEventsDomainTrayChange(virConnectPtr connection VIR_ATTR_UNUSED, const char *reasonstr; int r; - path = virtDBusUtilBusPathForVirDomain(domain); + path = virtDBusUtilBusPathForVirDomain(domain, connect->domainPath); r = sd_bus_message_new_signal(connect->bus, &message, diff --git a/src/util.c b/src/util.c index 6636d0a..d0649fa 100644 --- a/src/util.c +++ b/src/util.c @@ -77,25 +77,27 @@ virtDBusUtilSetError(sd_bus_error *error, } char * -virtDBusUtilBusPathForVirDomain(virDomainPtr domain) +virtDBusUtilBusPathForVirDomain(virDomainPtr domain, + const char *domainPath) { char *path = NULL; char uuid[VIR_UUID_STRING_BUFLEN] = ""; virDomainGetUUIDString(domain, uuid); - sd_bus_path_encode("/org/libvirt/domain", uuid, &path); + sd_bus_path_encode(domainPath, uuid, &path); return path; } virDomainPtr virtDBusUtilVirDomainFromBusPath(virConnectPtr connection, - const char *path) + const char *path, + const char *domainPath) { _cleanup_(virtDBusUtilFreep) char *name = NULL; int r; - r = sd_bus_path_decode(path, "/org/libvirt/domain", &name); + r = sd_bus_path_decode(path, domainPath, &name); if (r < 0) return NULL; diff --git a/src/util.h b/src/util.h index cb8447d..14b1e9a 100644 --- a/src/util.h +++ b/src/util.h @@ -24,11 +24,13 @@ virtDBusUtilSetError(sd_bus_error *error, const char *message); char * -virtDBusUtilBusPathForVirDomain(virDomainPtr domain); +virtDBusUtilBusPathForVirDomain(virDomainPtr domain, + const char *domainPath); virDomainPtr virtDBusUtilVirDomainFromBusPath(virConnectPtr connection, - const char *path); + const char *path, + const char *domainPath); static inline void virtDBusUtilFreep(void *p) -- 2.14.3
On Mon, Jan 22, 2018 at 06:16:07PM +0100, Pavel Hrdina wrote:
Each connection have its own domains so we must make the domain path as a sub-path of the specific connection.
Might be nice to illustrate the new scheme which, IIUC, will be /org/libvirt/Connect/domain/{uuid} ?
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 12 +++++++---- src/connect.h | 1 + src/domain.c | 64 ++++++++++++++++++++++++++++++++++++++++------------------- src/events.c | 10 +++++----- src/util.c | 10 ++++++---- src/util.h | 6 ++++-- 6 files changed, 68 insertions(+), 35 deletions(-)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> 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 :|
This effectively removes --connect parameter from daemon, for now we will support only local connection. Instead of specifying URI for different drivers we will use connect object which allows using multiple drivers from one daemon. In the future if that feature will be requested we can extend libvirt-dbus to have some configuration file where we will have an option to specify remote host instead of local connection. This change also defines the difference between session and system bus, and limits the list of drivers available for that specific bus because not all drivers support session mode. Currently only QEMU driver has session mode. For test purposes we can allow TEST driver to be available for session bus. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 17 +++++++++++++++-- src/connect.h | 4 +++- src/main.c | 55 ++++++++++++++++++++++++++++++++++------------------- test/libvirttest.py | 4 ++-- 4 files changed, 55 insertions(+), 25 deletions(-) diff --git a/src/connect.c b/src/connect.c index 5249eba..878cf91 100644 --- a/src/connect.c +++ b/src/connect.c @@ -238,7 +238,8 @@ static const sd_bus_vtable virt_connect_vtable[] = { int virtDBusConnectNew(virtDBusConnect **connectp, sd_bus *bus, - const char *uri) + const char *uri, + const char *connectPath) { _cleanup_(virtDBusConnectFreep) virtDBusConnect *connect = NULL; int r; @@ -249,7 +250,7 @@ virtDBusConnectNew(virtDBusConnect **connectp, connect->bus = sd_bus_ref(bus); connect->uri = uri; - connect->connectPath = "/org/libvirt/Connect"; + connect->connectPath = connectPath; connect->enumerateDomains = virtDBusConnectEnumarateDomains; @@ -293,3 +294,15 @@ virtDBusConnectFreep(virtDBusConnect **connectp) if (*connectp) virtDBusConnectFree(*connectp); } + +void +virtDBusConnectListFree(virtDBusConnect ***connectList) +{ + if (!*connectList) + return; + + for (int i = 0; (*connectList)[i]; i += 1) + virtDBusConnectFree((*connectList)[i]); + + free(*connectList); +} diff --git a/src/connect.h b/src/connect.h index bed2c7e..46e8c9a 100644 --- a/src/connect.h +++ b/src/connect.h @@ -20,6 +20,8 @@ typedef struct virtDBusConnect virtDBusConnect; int virtDBusConnectNew(virtDBusConnect **connectp, sd_bus *bus, - const char *uri); + const char *uri, + const char *connectPath); virtDBusConnect *virtDBusConnectFree(virtDBusConnect *connect); void virtDBusConnectFreep(virtDBusConnect **connectp); +void virtDBusConnectListFree(virtDBusConnect ***connectList); diff --git a/src/main.c b/src/main.c index 95522f5..14c7c18 100644 --- a/src/main.c +++ b/src/main.c @@ -79,6 +79,21 @@ virtDBusHandleBusEvent(int watch, virEventUpdateHandle(watch, virtDBusGetLibvirtEvents(bus)); } +struct virtDBusDriver { + const char *uri; + const char *object; +}; + +static const struct virtDBusDriver sessionDrivers[] = { + { "qemu:///session", "/org/libvirt/qemu" }, + { "test:///default", "/org/libvirt/test" }, +}; + +static const struct virtDBusDriver systemDrivers[] = { + { "qemu:///system", "/org/libvirt/qemu" }, + { "test:///default", "/org/libvirt/test" }, +}; + int main(int argc, char *argv[]) { @@ -89,16 +104,16 @@ main(int argc, char *argv[]) static const struct option options[] = { { "help", no_argument, NULL, 'h' }, - { "connect", required_argument, NULL, 'c' }, { "system", no_argument, NULL, ARG_SYSTEM }, { "session", no_argument, NULL, ARG_SESSION }, {} }; bool system_bus; - const char *uri = NULL; + const struct virtDBusDriver *drivers = NULL; + int ndrivers = 0; - _cleanup_(virtDBusConnectFreep) virtDBusConnect *connect = NULL; + _cleanup_(virtDBusConnectListFree) virtDBusConnect **connect = NULL; _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; _cleanup_(virtDBusUtilClosep) int signal_fd = -1; _cleanup_(virtDBusVirEventRemoveHandlep) int bus_watch = -1; @@ -121,15 +136,10 @@ main(int argc, char *argv[]) printf("Provide a D-Bus interface to a libvirtd.\n"); printf("\n"); printf(" -h, --help Display this help text and exit\n"); - printf(" -c, --connect URI Connect to the specified libvirt URI\n"); printf(" --session Connect to the session bus\n"); printf(" --system Connect to the system bus\n"); return 0; - case 'c': - uri = optarg; - break; - case ARG_SYSTEM: system_bus = true; break; @@ -143,14 +153,6 @@ main(int argc, char *argv[]) } } - if (uri == NULL) { - if (system_bus) { - uri = "qemu:///system"; - } else { - uri = "qemu:///session"; - } - } - sigemptyset(&mask); sigaddset(&mask, SIGTERM); sigaddset(&mask, SIGINT); @@ -170,10 +172,23 @@ main(int argc, char *argv[]) return EXIT_FAILURE; } - r = virtDBusConnectNew(&connect, bus, uri); - if (r < 0) { - fprintf(stderr, "Failed to connect to libvirt."); - return EXIT_FAILURE; + if (system_bus) { + drivers = systemDrivers; + ndrivers = VIRT_ARRAY_CARDINALITY(systemDrivers); + } else { + drivers = sessionDrivers; + ndrivers = VIRT_ARRAY_CARDINALITY(sessionDrivers); + } + + connect = calloc(ndrivers + 1, sizeof(virtDBusConnect *)); + + for (int i = 0; i < ndrivers; i += 1) { + r = virtDBusConnectNew(&connect[i], bus, + drivers[i].uri, drivers[i].object); + if (r < 0) { + fprintf(stderr, "Failed to register libvirt connection."); + return EXIT_FAILURE; + } } r = virtDBusProcessEvents(bus); diff --git a/test/libvirttest.py b/test/libvirttest.py index 2c735e9..579485f 100644 --- a/test/libvirttest.py +++ b/test/libvirttest.py @@ -28,7 +28,7 @@ class TestCase(unittest.TestCase): def setUp(self): os.environ['LIBVIRT_DEBUG'] = '3' - self.daemon = subprocess.Popen([exe, '--connect', 'test:///default']) + self.daemon = subprocess.Popen([exe]) self.bus = dbus.SessionBus() for i in range(10): @@ -38,7 +38,7 @@ class TestCase(unittest.TestCase): else: raise TimeoutError('error starting libvirt-dbus') - obj = self.bus.get_object('org.libvirt', '/org/libvirt/Connect') + obj = self.bus.get_object('org.libvirt', '/org/libvirt/test') self.connect = dbus.Interface(obj, 'org.libvirt.Connect') def tearDown(self): -- 2.14.3
On Mon, Jan 22, 2018 at 06:16:08PM +0100, Pavel Hrdina wrote:
This effectively removes --connect parameter from daemon, for now we will support only local connection. Instead of specifying URI for different drivers we will use connect object which allows using multiple drivers from one daemon. In the future if that feature will be requested we can extend libvirt-dbus to have some configuration file where we will have an option to specify remote host instead of local connection.
This change also defines the difference between session and system bus, and limits the list of drivers available for that specific bus because not all drivers support session mode. Currently only QEMU driver has session mode. For test purposes we can allow TEST driver to be available for session bus.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/connect.c | 17 +++++++++++++++-- src/connect.h | 4 +++- src/main.c | 55 ++++++++++++++++++++++++++++++++++------------------- test/libvirttest.py | 4 ++-- 4 files changed, 55 insertions(+), 25 deletions(-)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> 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 :|
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/main.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/main.c b/src/main.c index 14c7c18..6421919 100644 --- a/src/main.c +++ b/src/main.c @@ -85,13 +85,31 @@ struct virtDBusDriver { }; static const struct virtDBusDriver sessionDrivers[] = { - { "qemu:///session", "/org/libvirt/qemu" }, - { "test:///default", "/org/libvirt/test" }, + { "qemu:///session", "/org/libvirt/qemu" }, + { "test:///default", "/org/libvirt/test" }, + { "uml:///session", "/org/libvirt/uml" }, + { "vbox:///session", "/org/libvirt/vbox" }, + { "vmwarefusion:///session", "/org/libvirt/vmwarefusion" }, + { "vmwareplayer:///session", "/org/libvirt/vmwareplayer" }, + { "vmwarews:///session", "/org/libvirt/vmwarews" }, }; static const struct virtDBusDriver systemDrivers[] = { - { "qemu:///system", "/org/libvirt/qemu" }, - { "test:///default", "/org/libvirt/test" }, + { "XenApi://localhost/", "/org/libvirt/XenApi" }, + { "bhyve:///system", "/org/libvirt/bhyve" }, + { "esx://localhost/", "/org/libvirt/esx" }, + { "gsx://localhost/", "/org/libvirt/gsx" }, + { "hyperv://localhost/", "/org/libvirt/hyperv" }, + { "lxc:///", "/org/libvirt/lxc" }, + { "openvz:///system", "/org/libvirt/openvz" }, + { "phyp://localhost/", "/org/libvirt/phyp" }, + { "qemu:///system", "/org/libvirt/qemu" }, + { "test:///default", "/org/libvirt/test" }, + { "uml:///system", "/org/libvirt/uml" }, + { "vbox:///system", "/org/libvirt/vbox" }, + { "vpx://localhost/", "/org/libvirt/vpx" }, + { "vz:///system", "/org/libvirt/vz" }, + { "xen:///", "/org/libvirt/xen" }, }; int -- 2.14.3
On Mon, Jan 22, 2018 at 06:16:09PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/main.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/main.c b/src/main.c index 14c7c18..6421919 100644 --- a/src/main.c +++ b/src/main.c @@ -85,13 +85,31 @@ struct virtDBusDriver { };
static const struct virtDBusDriver sessionDrivers[] = { - { "qemu:///session", "/org/libvirt/qemu" }, - { "test:///default", "/org/libvirt/test" }, + { "qemu:///session", "/org/libvirt/qemu" }, + { "test:///default", "/org/libvirt/test" }, + { "uml:///session", "/org/libvirt/uml" }, + { "vbox:///session", "/org/libvirt/vbox" }, + { "vmwarefusion:///session", "/org/libvirt/vmwarefusion" }, + { "vmwareplayer:///session", "/org/libvirt/vmwareplayer" }, + { "vmwarews:///session", "/org/libvirt/vmwarews" }, };
static const struct virtDBusDriver systemDrivers[] = { - { "qemu:///system", "/org/libvirt/qemu" }, - { "test:///default", "/org/libvirt/test" }, + { "XenApi://localhost/", "/org/libvirt/XenApi" },
Hmm, lowercase 'xenapi' would match all other paths, but uppercase matches the URI scheme.
+ { "bhyve:///system", "/org/libvirt/bhyve" }, + { "esx://localhost/", "/org/libvirt/esx" },
Not sure this is useful, as I don't think there's any practical way to manage ESX from the host it runs on, as that would imply being able to run custom software inside ESX.
+ { "gsx://localhost/", "/org/libvirt/gsx" }, + { "hyperv://localhost/", "/org/libvirt/hyperv" },
I don't think this is useful either.
+ { "lxc:///", "/org/libvirt/lxc" }, + { "openvz:///system", "/org/libvirt/openvz" }, + { "phyp://localhost/", "/org/libvirt/phyp" },
IIUC, phyp is also always a remotely executing driver, which needs to ssh into the hypervisor.
+ { "qemu:///system", "/org/libvirt/qemu" }, + { "test:///default", "/org/libvirt/test" }, + { "uml:///system", "/org/libvirt/uml" }, + { "vbox:///system", "/org/libvirt/vbox" }, + { "vpx://localhost/", "/org/libvirt/vpx" },
Same as ESX - remote only.
+ { "vz:///system", "/org/libvirt/vz" }, + { "xen:///", "/org/libvirt/xen" }, };
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 :|
On Tue, Jan 23, 2018 at 10:12:26AM +0000, Daniel P. Berrange wrote:
On Mon, Jan 22, 2018 at 06:16:09PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/main.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/main.c b/src/main.c index 14c7c18..6421919 100644 --- a/src/main.c +++ b/src/main.c @@ -85,13 +85,31 @@ struct virtDBusDriver { };
static const struct virtDBusDriver sessionDrivers[] = { - { "qemu:///session", "/org/libvirt/qemu" }, - { "test:///default", "/org/libvirt/test" }, + { "qemu:///session", "/org/libvirt/qemu" }, + { "test:///default", "/org/libvirt/test" }, + { "uml:///session", "/org/libvirt/uml" }, + { "vbox:///session", "/org/libvirt/vbox" }, + { "vmwarefusion:///session", "/org/libvirt/vmwarefusion" }, + { "vmwareplayer:///session", "/org/libvirt/vmwareplayer" }, + { "vmwarews:///session", "/org/libvirt/vmwarews" }, };
static const struct virtDBusDriver systemDrivers[] = { - { "qemu:///system", "/org/libvirt/qemu" }, - { "test:///default", "/org/libvirt/test" }, + { "XenApi://localhost/", "/org/libvirt/XenApi" },
Hmm, lowercase 'xenapi' would match all other paths, but uppercase matches the URI scheme.
There is another "issue" with the naming, best practices for D-Bus object names is to have the first letter uppercase, e.g. "/org/libvirt/Qemu", but that feels strange.
+ { "bhyve:///system", "/org/libvirt/bhyve" }, + { "esx://localhost/", "/org/libvirt/esx" },
Not sure this is useful, as I don't think there's any practical way to manage ESX from the host it runs on, as that would imply being able to run custom software inside ESX.
+ { "gsx://localhost/", "/org/libvirt/gsx" }, + { "hyperv://localhost/", "/org/libvirt/hyperv" },
I don't think this is useful either.
+ { "lxc:///", "/org/libvirt/lxc" }, + { "openvz:///system", "/org/libvirt/openvz" }, + { "phyp://localhost/", "/org/libvirt/phyp" },
IIUC, phyp is also always a remotely executing driver, which needs to ssh into the hypervisor.
+ { "qemu:///system", "/org/libvirt/qemu" }, + { "test:///default", "/org/libvirt/test" }, + { "uml:///system", "/org/libvirt/uml" }, + { "vbox:///system", "/org/libvirt/vbox" }, + { "vpx://localhost/", "/org/libvirt/vpx" },
Same as ESX - remote only.
I've basically included all the drivers because I don't know how exactly some of them works. I'm OK with dropping the ones that are not useful (esx, gsx, vpx, hyperv and phyp). I'll send a v2 of this patch. Thanks, Pavel
On Tue, Jan 23, 2018 at 12:37:53PM +0100, Pavel Hrdina wrote:
On Tue, Jan 23, 2018 at 10:12:26AM +0000, Daniel P. Berrange wrote:
On Mon, Jan 22, 2018 at 06:16:09PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/main.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/main.c b/src/main.c index 14c7c18..6421919 100644 --- a/src/main.c +++ b/src/main.c @@ -85,13 +85,31 @@ struct virtDBusDriver { };
static const struct virtDBusDriver sessionDrivers[] = { - { "qemu:///session", "/org/libvirt/qemu" }, - { "test:///default", "/org/libvirt/test" }, + { "qemu:///session", "/org/libvirt/qemu" }, + { "test:///default", "/org/libvirt/test" }, + { "uml:///session", "/org/libvirt/uml" }, + { "vbox:///session", "/org/libvirt/vbox" }, + { "vmwarefusion:///session", "/org/libvirt/vmwarefusion" }, + { "vmwareplayer:///session", "/org/libvirt/vmwareplayer" }, + { "vmwarews:///session", "/org/libvirt/vmwarews" }, };
static const struct virtDBusDriver systemDrivers[] = { - { "qemu:///system", "/org/libvirt/qemu" }, - { "test:///default", "/org/libvirt/test" }, + { "XenApi://localhost/", "/org/libvirt/XenApi" },
Hmm, lowercase 'xenapi' would match all other paths, but uppercase matches the URI scheme.
There is another "issue" with the naming, best practices for D-Bus object names is to have the first letter uppercase, e.g. "/org/libvirt/Qemu", but that feels strange.
Agree that Qemu is wierd, but I think it would be ok if we fully uppercased acronyms in the "natural" manner. eg QEMU rather than Qemu. So we could have XenAPI, BHyve, ESX, GSX, HyperV, LXC, OpenVZ, PHyp, QEMU, Test, UML, VBox, VZ 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 :|
On Mon, Jan 22, 2018 at 06:15:58PM +0100, Pavel Hrdina wrote:
This patch series is a proposal to how to implement libvirt connections in the libvirt D-Bus binding. The design is based on a discussion with Daniel and it should follow D-Bus convention.
Each connection is represented as existing object where the object path contains the driver name:
/org/libvirt/qemu
and there is no need to call any connect API but simply use the existing object directly to operate with that connection.
This patch series also implements strict difference between system and user bus. On the user bus only drivers with session capability are available and on the system bus only drivers with system capability are available.
Aside from the last patch, this all looks great to me. 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 :|
On Tue, Jan 23, 2018 at 10:13:07AM +0000, Daniel P. Berrange wrote:
On Mon, Jan 22, 2018 at 06:15:58PM +0100, Pavel Hrdina wrote:
This patch series is a proposal to how to implement libvirt connections in the libvirt D-Bus binding. The design is based on a discussion with Daniel and it should follow D-Bus convention.
Each connection is represented as existing object where the object path contains the driver name:
/org/libvirt/qemu
and there is no need to call any connect API but simply use the existing object directly to operate with that connection.
This patch series also implements strict difference between system and user bus. On the user bus only drivers with session capability are available and on the system bus only drivers with system capability are available.
Aside from the last patch, this all looks great to me.
Thanks, I've pushed the patch series except for the last patch. V2 of the last patch is on its way. Pavel
participants (2)
-
Daniel P. Berrange -
Pavel Hrdina