Re: [libvirt] [dbus PATCH 1/8] Implement Capabilities property for connect Interface
On Mon, 2018-04-09 at 13:45 +0100, Daniel P. Berrangé wrote: > On Mon, Apr 09, 2018 at 02:25:04PM +0200, Pavel Hrdina wrote: > > On Mon, Apr 09, 2018 at 01:47:32PM +0200, Katerina Koukiou wrote: > > > Signed-off-by: Katerina Koukiou > > > --- > > > data/org.libvirt.Connect.xml | 4 > > > src/connect.c| 20 > > > test/test_connect.py | 1 + > > > 3 files changed, 25 insertions(+) > > > > Adding Dan to CC because I'm not that familiar with D-Bus > > conventions. > > This API can be exported as property, however it returns rather > > large > > string and not a simple value so I'm not sure whether method would > > be > > better in this case. > > The concept of properties is essentially a convenience to make live > easier > for mapping into language bindings / higher level object systems. > > At the protocol level, the properties are handling via a standard > interface > with a handful of methods > > https://dbus.freedesktop.org/doc/dbus-specification.html#standard-int > erfaces-properties > > So whether you have a "Capabilities" property, or a "GetCapabilities" > method, you'll still have a method call on the wire protocol, with > the > full string transmitted. There is also a signal that servers should > emit when a property value changes so that clients know that if they > have cached the property value, they should refetch it. > > So potential issues are > > - If we don't emit PropertyChange signals, clients may cache out >of date capabilities > - If clients pre-fetch properties there'll be a small perf hit > - If we have lots of properties, then the overall size of all >properties combined might cause problems for GetAll > > The first item is the one that would concern me in this instance. > The libvirt capabilities data can change between calls, and libvirt > has no way to notify you of such changes. Thus IMHO it is better > exposed as a dynamic method call than a property. Leave properties > for libvirt things that are esentially static data, unless we're > able to provide PropertyChange signals. > Thanks for the detailed explanation. I will adjust it and repost. > > > Regards, > Daniel Regards, Katerina -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 1/8] Implement Capabilities property for connect Interface
On Mon, Apr 09, 2018 at 02:25:04PM +0200, Pavel Hrdina wrote: > On Mon, Apr 09, 2018 at 01:47:32PM +0200, Katerina Koukiou wrote: > > Signed-off-by: Katerina Koukiou > > --- > > data/org.libvirt.Connect.xml | 4 > > src/connect.c| 20 > > test/test_connect.py | 1 + > > 3 files changed, 25 insertions(+) > > Adding Dan to CC because I'm not that familiar with D-Bus conventions. > This API can be exported as property, however it returns rather large > string and not a simple value so I'm not sure whether method would be > better in this case. The concept of properties is essentially a convenience to make live easier for mapping into language bindings / higher level object systems. At the protocol level, the properties are handling via a standard interface with a handful of methods https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties So whether you have a "Capabilities" property, or a "GetCapabilities" method, you'll still have a method call on the wire protocol, with the full string transmitted. There is also a signal that servers should emit when a property value changes so that clients know that if they have cached the property value, they should refetch it. So potential issues are - If we don't emit PropertyChange signals, clients may cache out of date capabilities - If clients pre-fetch properties there'll be a small perf hit - If we have lots of properties, then the overall size of all properties combined might cause problems for GetAll The first item is the one that would concern me in this instance. The libvirt capabilities data can change between calls, and libvirt has no way to notify you of such changes. Thus IMHO it is better exposed as a dynamic method call than a property. Leave properties for libvirt things that are esentially static data, unless we're able to provide PropertyChange signals. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 1/8] Implement Capabilities property for connect Interface
On Mon, Apr 09, 2018 at 01:47:32PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou
> ---
> data/org.libvirt.Connect.xml | 4
> src/connect.c| 20
> test/test_connect.py | 1 +
> 3 files changed, 25 insertions(+)
Adding Dan to CC because I'm not that familiar with D-Bus conventions.
This API can be exported as property, however it returns rather large
string and not a simple value so I'm not sure whether method would be
better in this case.
Pavel
>
> diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
> index e36b7f6..caf0a47 100644
> --- a/data/org.libvirt.Connect.xml
> +++ b/data/org.libvirt.Connect.xml
> @@ -3,6 +3,10 @@
>
>
>
> +
> + +value="See
> https://libvirt.org/html/libvirt-libvirt-host.html#virConnectGetCapabilities"/>
> +
>
> value="See
> https://libvirt.org/html/libvirt-libvirt-host.html#virConnectGetVersion"/>
> diff --git a/src/connect.c b/src/connect.c
> index ce9055a..af61cc8 100644
> --- a/src/connect.c
> +++ b/src/connect.c
> @@ -91,6 +91,25 @@ virtDBusConnectOpen(virtDBusConnect *connect,
> return TRUE;
> }
>
> +static void
> +virtDBusConnectGetCapabilities(const gchar *objectPath G_GNUC_UNUSED,
> + gpointer userData,
> + GVariant **value,
> + GError **error)
> +{
> +virtDBusConnect *connect = userData;
> +g_autofree gchar *capabilities = NULL;
> +
> +if (!virtDBusConnectOpen(connect, error))
> +return;
> +
> +capabilities = virConnectGetCapabilities(connect->connection);
> +if (!capabilities)
> +return virtDBusUtilSetLastVirtError(error);
> +
> +*value = g_variant_new("s", capabilities);
> +}
> +
> static void
> virtDBusConnectGetVersion(const gchar *objectPath G_GNUC_UNUSED,
>gpointer userData,
> @@ -443,6 +462,7 @@ virtDBusNetworkLookupByUUID(GVariant *inArgs,
> }
>
> static virtDBusGDBusPropertyTable virtDBusConnectPropertyTable[] = {
> +{ "Capabilities", virtDBusConnectGetCapabilities, NULL },
> { "Version", virtDBusConnectGetVersion, NULL },
> { 0 }
> };
> diff --git a/test/test_connect.py b/test/test_connect.py
> index c411eeb..d7c1e20 100755
> --- a/test/test_connect.py
> +++ b/test/test_connect.py
> @@ -81,6 +81,7 @@ class TestConnect(libvirttest.BaseTestClass):
> assert original_path == path
>
> @pytest.mark.parametrize("property_name,expected_type", [
> +("Capabilities", dbus.String),
> ("Version", dbus.UInt64),
> ])
> def test_connect_properties_return_type(self, property_name,
> expected_type):
> --
> 2.15.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dbus PATCH 1/8] Implement Capabilities property for connect Interface
Signed-off-by: Katerina Koukiou
---
data/org.libvirt.Connect.xml | 4
src/connect.c| 20
test/test_connect.py | 1 +
3 files changed, 25 insertions(+)
diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
index e36b7f6..caf0a47 100644
--- a/data/org.libvirt.Connect.xml
+++ b/data/org.libvirt.Connect.xml
@@ -3,6 +3,10 @@
+
+ https://libvirt.org/html/libvirt-libvirt-host.html#virConnectGetCapabilities"/>
+
https://libvirt.org/html/libvirt-libvirt-host.html#virConnectGetVersion"/>
diff --git a/src/connect.c b/src/connect.c
index ce9055a..af61cc8 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -91,6 +91,25 @@ virtDBusConnectOpen(virtDBusConnect *connect,
return TRUE;
}
+static void
+virtDBusConnectGetCapabilities(const gchar *objectPath G_GNUC_UNUSED,
+ gpointer userData,
+ GVariant **value,
+ GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autofree gchar *capabilities = NULL;
+
+if (!virtDBusConnectOpen(connect, error))
+return;
+
+capabilities = virConnectGetCapabilities(connect->connection);
+if (!capabilities)
+return virtDBusUtilSetLastVirtError(error);
+
+*value = g_variant_new("s", capabilities);
+}
+
static void
virtDBusConnectGetVersion(const gchar *objectPath G_GNUC_UNUSED,
gpointer userData,
@@ -443,6 +462,7 @@ virtDBusNetworkLookupByUUID(GVariant *inArgs,
}
static virtDBusGDBusPropertyTable virtDBusConnectPropertyTable[] = {
+{ "Capabilities", virtDBusConnectGetCapabilities, NULL },
{ "Version", virtDBusConnectGetVersion, NULL },
{ 0 }
};
diff --git a/test/test_connect.py b/test/test_connect.py
index c411eeb..d7c1e20 100755
--- a/test/test_connect.py
+++ b/test/test_connect.py
@@ -81,6 +81,7 @@ class TestConnect(libvirttest.BaseTestClass):
assert original_path == path
@pytest.mark.parametrize("property_name,expected_type", [
+("Capabilities", dbus.String),
("Version", dbus.UInt64),
])
def test_connect_properties_return_type(self, property_name,
expected_type):
--
2.15.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
