[libvirt] [dbus PATCH 3/4] Merge all Domain lifecycle signals into one signal called Domain.

Katerina Koukiou posted 4 patches 7 years, 10 months ago
There is a newer version of this series
[libvirt] [dbus PATCH 3/4] Merge all Domain lifecycle signals into one signal called Domain.
Posted by Katerina Koukiou 7 years, 10 months ago
Instead of having multiple signals regarding to domain events,
like DomainStarted, DomainUndefined etc, we will have only one
called Domain, and the specific event type will be specified in
the signals arguments.

The domain name argument in not needed in the signal since we can
fetch it from path.

The tests are adjusted to call correctly connect_to_signal method
on the new signal.

Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
---
 data/org.libvirt.Connect.xml | 54 +++-----------------------------------------
 src/events.c                 | 26 ++++++++++-----------
 test/test_connect.py         | 10 ++++----
 test/test_domain.py          | 20 +++++++---------
 4 files changed, 27 insertions(+), 83 deletions(-)

diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
index f24dff4..6810422 100644
--- a/data/org.libvirt.Connect.xml
+++ b/data/org.libvirt.Connect.xml
@@ -44,59 +44,11 @@
       <arg name="uuid" type="s" direction="in"/>
       <arg name="domain" type="o" direction="out"/>
     </method>
-    <signal name="DomainCrashed">
+    <signal name="Domain">
       <annotation name="org.gtk.GDBus.DocString"
-        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_CRASHED"/>
-      <arg name="reason" type="s"/>
-      <arg name="domain" type="o"/>
-    </signal>
-    <signal name="DomainDefined">
-      <annotation name="org.gtk.GDBus.DocString"
-        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_DEFINED"/>
-      <arg name="reason" type="s"/>
-      <arg name="domain" type="o"/>
-    </signal>
-    <signal name="DomainPMSuspended">
-      <annotation name="org.gtk.GDBus.DocString"
-        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_SUSPENDED"/>
-      <arg name="reason" type="s"/>
-      <arg name="domain" type="o"/>
-    </signal>
-    <signal name="DomainResumed">
-      <annotation name="org.gtk.GDBus.DocString"
-        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_RESUMED"/>
-      <arg name="reason" type="s"/>
-      <arg name="domain" type="o"/>
-    </signal>
-    <signal name="DomainShutdown">
-      <annotation name="org.gtk.GDBus.DocString"
-        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_SHUTDOWN"/>
-      <arg name="reason" type="s"/>
-      <arg name="domain" type="o"/>
-    </signal>
-    <signal name="DomainStarted">
-      <annotation name="org.gtk.GDBus.DocString"
-        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_STARTED"/>
-      <arg name="reason" type="s"/>
-      <arg name="domain" type="o"/>
-    </signal>
-    <signal name="DomainStopped">
-      <annotation name="org.gtk.GDBus.DocString"
-        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_STOPPED"/>
-      <arg name="reason" type="s"/>
-      <arg name="domain" type="o"/>
-    </signal>
-    <signal name="DomainSuspended">
-      <annotation name="org.gtk.GDBus.DocString"
-        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_SUSPENDED"/>
-      <arg name="reason" type="s"/>
-      <arg name="domain" type="o"/>
-    </signal>
-    <signal name="DomainUndefined">
-      <annotation name="org.gtk.GDBus.DocString"
-        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_UNDEFINED"/>
-      <arg name="reason" type="s"/>
+        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectDomainEventCallback"/>
       <arg name="domain" type="o"/>
+      <arg name="event" type="s"/>
     </signal>
   </interface>
 </node>
diff --git a/src/events.c b/src/events.c
index dada55f..62f3729 100644
--- a/src/events.c
+++ b/src/events.c
@@ -12,51 +12,49 @@ virtDBusEventsDomainLifecycle(virConnectPtr connection G_GNUC_UNUSED,
                               gpointer opaque)
 {
     virtDBusConnect *connect = opaque;
-    const gchar *signal = NULL;
-    const gchar *name;
+    const gchar *event_type = NULL;
     g_autofree gchar *path = NULL;
 
     switch (event) {
     case VIR_DOMAIN_EVENT_DEFINED:
-        signal = "DomainDefined";
+        event_type = "DomainDefined";
         break;
     case VIR_DOMAIN_EVENT_UNDEFINED:
-        signal = "DomainUndefined";
+        event_type = "DomainUndefined";
         break;
     case VIR_DOMAIN_EVENT_STARTED:
-        signal = "DomainStarted";
+        event_type = "DomainStarted";
         break;
     case VIR_DOMAIN_EVENT_SUSPENDED:
-        signal = "DomainSuspended";
+        event_type = "DomainSuspended";
         break;
     case VIR_DOMAIN_EVENT_RESUMED:
-        signal = "DomainResumed";
+        event_type = "DomainResumed";
         break;
     case VIR_DOMAIN_EVENT_STOPPED:
-        signal = "DomainStopped";
+        event_type = "DomainStopped";
         break;
     case VIR_DOMAIN_EVENT_SHUTDOWN:
-        signal = "DomainShutdown";
+        event_type = "DomainShutdown";
         break;
     case VIR_DOMAIN_EVENT_PMSUSPENDED:
-        signal = "DomainPMSuspended";
+        event_type = "DomainPMSuspended";
         break;
     case VIR_DOMAIN_EVENT_CRASHED:
-        signal = "DomainCrashed";
+        event_type = "DomainCrashed";
         break;
     default:
         return 0;
     }
 
-    name = virDomainGetName(domain);
     path = virtDBusUtilBusPathForVirDomain(domain, connect->domainPath);
 
     g_dbus_connection_emit_signal(connect->bus,
                                   NULL,
                                   connect->connectPath,
                                   VIRT_DBUS_CONNECT_INTERFACE,
-                                  signal,
-                                  g_variant_new("(so)", name, path),
+                                  "Domain",
+                                  g_variant_new("(os)", path, event_type),
                                   NULL);
 
     return 0;
diff --git a/test/test_connect.py b/test/test_connect.py
index 48e25cb..c7830c5 100755
--- a/test/test_connect.py
+++ b/test/test_connect.py
@@ -29,12 +29,11 @@ class TestConnect(libvirttest.BaseTestClass):
             domain.Introspect(dbus_interface=dbus.INTROSPECTABLE_IFACE)
 
     def test_create(self):
-        def domain_started(name, path):
-            assert name == 'foo'
+        def domain_started(path, _event):
             assert isinstance(path, dbus.ObjectPath)
             self.loop.quit()
 
-        self.connect.connect_to_signal('DomainStarted', domain_started)
+        self.connect.connect_to_signal('Domain', domain_started, arg2='DomainStarted')
 
         path = self.connect.CreateXML(self.minimal_xml, 0)
         assert isinstance(path, dbus.ObjectPath)
@@ -42,12 +41,11 @@ class TestConnect(libvirttest.BaseTestClass):
         self.main_loop()
 
     def test_define(self):
-        def domain_defined(name, path):
-            assert name == 'foo'
+        def domain_defined(path, _event):
             assert isinstance(path, dbus.ObjectPath)
             self.loop.quit()
 
-        self.connect.connect_to_signal('DomainDefined', domain_defined)
+        self.connect.connect_to_signal('Domain', domain_defined, arg2='DomainDefined')
 
         path = self.connect.DefineXML(self.minimal_xml)
         assert isinstance(path, dbus.ObjectPath)
diff --git a/test/test_domain.py b/test/test_domain.py
index 1433ff5..7dbc971 100755
--- a/test/test_domain.py
+++ b/test/test_domain.py
@@ -38,12 +38,11 @@ class TestDomain(libvirttest.BaseTestClass):
         domain.Undefine(0)
 
     def test_shutdown(self):
-        def domain_stopped(name, path):
-            assert name == 'test'
+        def domain_stopped(path, _event):
             assert isinstance(path, dbus.ObjectPath)
             self.loop.quit()
 
-        self.connect.connect_to_signal('DomainStopped', domain_stopped)
+        self.connect.connect_to_signal('Domain', domain_stopped, arg2='DomainStopped')
 
         obj, domain = self.domain()
         domain.Shutdown(0)
@@ -54,12 +53,11 @@ class TestDomain(libvirttest.BaseTestClass):
         self.main_loop()
 
     def test_undefine(self):
-        def domain_undefined(name, path):
-            assert name == 'test'
+        def domain_undefined(path, _event):
             assert isinstance(path, dbus.ObjectPath)
             self.loop.quit()
 
-        self.connect.connect_to_signal('DomainUndefined', domain_undefined)
+        self.connect.connect_to_signal('Domain', domain_undefined, arg2='DomainUndefined')
 
         _, domain = self.domain()
         domain.Shutdown(0)
@@ -68,12 +66,11 @@ class TestDomain(libvirttest.BaseTestClass):
         self.main_loop()
 
     def test_suspend(self):
-        def domain_suspended(name, path):
-            assert name == 'test'
+        def domain_suspended(path, _event):
             assert isinstance(path, dbus.ObjectPath)
             self.loop.quit()
 
-        self.connect.connect_to_signal('DomainSuspended', domain_suspended)
+        self.connect.connect_to_signal('Domain', domain_suspended, arg2='DomainSuspended')
 
         obj, domain = self.domain()
         domain.Suspend()
@@ -84,12 +81,11 @@ class TestDomain(libvirttest.BaseTestClass):
         self.main_loop()
 
     def test_resume(self):
-        def domain_resumed(name, path):
-            assert name == 'test'
+        def domain_resumed(path, _event):
             assert isinstance(path, dbus.ObjectPath)
             self.loop.quit()
 
-        self.connect.connect_to_signal('DomainResumed', domain_resumed)
+        self.connect.connect_to_signal('Domain', domain_resumed, arg2='DomainResumed')
 
         obj, domain = self.domain()
         domain.Suspend()
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 3/4] Merge all Domain lifecycle signals into one signal called Domain.
Posted by Ján Tomko 7 years, 10 months ago
On Thu, Mar 29, 2018 at 01:07:57PM +0200, Katerina Koukiou wrote:
>Instead of having multiple signals regarding to domain events,
>like DomainStarted, DomainUndefined etc, we will have only one
>called Domain, and the specific event type will be specified in
>the signals arguments.
>
>The domain name argument in not needed in the signal since we can
>fetch it from path.
>

Any plans to add support for the 'detail' field of
virConnectDomainEventCallback?

>The tests are adjusted to call correctly connect_to_signal method
>on the new signal.
>
>Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
>---
> data/org.libvirt.Connect.xml | 54 +++-----------------------------------------
> src/events.c                 | 26 ++++++++++-----------
> test/test_connect.py         | 10 ++++----
> test/test_domain.py          | 20 +++++++---------
> 4 files changed, 27 insertions(+), 83 deletions(-)
>

>diff --git a/src/events.c b/src/events.c
>index dada55f..62f3729 100644
>--- a/src/events.c
>+++ b/src/events.c
>@@ -12,51 +12,49 @@ virtDBusEventsDomainLifecycle(virConnectPtr connection G_GNUC_UNUSED,
>                               gpointer opaque)
> {
>     virtDBusConnect *connect = opaque;
>-    const gchar *signal = NULL;
>-    const gchar *name;
>+    const gchar *event_type = NULL;
>     g_autofree gchar *path = NULL;
>
>     switch (event) {
>     case VIR_DOMAIN_EVENT_DEFINED:
>-        signal = "DomainDefined";
>+        event_type = "DomainDefined";
>         break;
>     case VIR_DOMAIN_EVENT_UNDEFINED:
>-        signal = "DomainUndefined";
>+        event_type = "DomainUndefined";
>         break;
>     case VIR_DOMAIN_EVENT_STARTED:
>-        signal = "DomainStarted";
>+        event_type = "DomainStarted";
>         break;
>     case VIR_DOMAIN_EVENT_SUSPENDED:
>-        signal = "DomainSuspended";
>+        event_type = "DomainSuspended";
>         break;
>     case VIR_DOMAIN_EVENT_RESUMED:
>-        signal = "DomainResumed";
>+        event_type = "DomainResumed";
>         break;
>     case VIR_DOMAIN_EVENT_STOPPED:
>-        signal = "DomainStopped";
>+        event_type = "DomainStopped";
>         break;
>     case VIR_DOMAIN_EVENT_SHUTDOWN:
>-        signal = "DomainShutdown";
>+        event_type = "DomainShutdown";
>         break;
>     case VIR_DOMAIN_EVENT_PMSUSPENDED:
>-        signal = "DomainPMSuspended";
>+        event_type = "DomainPMSuspended";
>         break;
>     case VIR_DOMAIN_EVENT_CRASHED:
>-        signal = "DomainCrashed";
>+        event_type = "DomainCrashed";
>         break;
>     default:
>         return 0;

Converting this to an EnumToString call first would make the commit replacing
the char* variable name and the commit dropping the Domain prefix nicer.

Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 3/4] Merge all Domain lifecycle signals into one signal called Domain.
Posted by Katerina Koukiou 7 years, 10 months ago
On Thu, 2018-03-29 at 15:05 +0200, Ján Tomko wrote:
> On Thu, Mar 29, 2018 at 01:07:57PM +0200, Katerina Koukiou wrote:
> > Instead of having multiple signals regarding to domain events,
> > like DomainStarted, DomainUndefined etc, we will have only one
> > called Domain, and the specific event type will be specified in
> > the signals arguments.
> > 
> > The domain name argument in not needed in the signal since we can
> > fetch it from path.
> > 
> 
> Any plans to add support for the 'detail' field of
> virConnectDomainEventCallback?

Yes, I guess it's ok to be in different patchset though.

> 
> > The tests are adjusted to call correctly connect_to_signal method
> > on the new signal.
> > 
> > Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
> > ---
> > data/org.libvirt.Connect.xml | 54 +++----------------------------
> > -------------
> > src/events.c                 | 26 ++++++++++-----------
> > test/test_connect.py         | 10 ++++----
> > test/test_domain.py          | 20 +++++++---------
> > 4 files changed, 27 insertions(+), 83 deletions(-)
> > 
> > diff --git a/src/events.c b/src/events.c
> > index dada55f..62f3729 100644
> > --- a/src/events.c
> > +++ b/src/events.c
> > @@ -12,51 +12,49 @@ virtDBusEventsDomainLifecycle(virConnectPtr
> > connection G_GNUC_UNUSED,
> >                               gpointer opaque)
> > {
> >     virtDBusConnect *connect = opaque;
> > -    const gchar *signal = NULL;
> > -    const gchar *name;
> > +    const gchar *event_type = NULL;
> >     g_autofree gchar *path = NULL;
> > 
> >     switch (event) {
> >     case VIR_DOMAIN_EVENT_DEFINED:
> > -        signal = "DomainDefined";
> > +        event_type = "DomainDefined";
> >         break;
> >     case VIR_DOMAIN_EVENT_UNDEFINED:
> > -        signal = "DomainUndefined";
> > +        event_type = "DomainUndefined";
> >         break;
> >     case VIR_DOMAIN_EVENT_STARTED:
> > -        signal = "DomainStarted";
> > +        event_type = "DomainStarted";
> >         break;
> >     case VIR_DOMAIN_EVENT_SUSPENDED:
> > -        signal = "DomainSuspended";
> > +        event_type = "DomainSuspended";
> >         break;
> >     case VIR_DOMAIN_EVENT_RESUMED:
> > -        signal = "DomainResumed";
> > +        event_type = "DomainResumed";
> >         break;
> >     case VIR_DOMAIN_EVENT_STOPPED:
> > -        signal = "DomainStopped";
> > +        event_type = "DomainStopped";
> >         break;
> >     case VIR_DOMAIN_EVENT_SHUTDOWN:
> > -        signal = "DomainShutdown";
> > +        event_type = "DomainShutdown";
> >         break;
> >     case VIR_DOMAIN_EVENT_PMSUSPENDED:
> > -        signal = "DomainPMSuspended";
> > +        event_type = "DomainPMSuspended";
> >         break;
> >     case VIR_DOMAIN_EVENT_CRASHED:
> > -        signal = "DomainCrashed";
> > +        event_type = "DomainCrashed";
> >         break;
> >     default:
> >         return 0;
> 
> Converting this to an EnumToString call first would make the commit
> replacing
> the char* variable name and the commit dropping the Domain prefix
> nicer.

Good point. I reverted the order of the two commits.

> 
> Jan

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 3/4] Merge all Domain lifecycle signals into one signal called Domain.
Posted by Ján Tomko 7 years, 10 months ago
On Thu, Mar 29, 2018 at 05:15:05PM +0200, Katerina Koukiou wrote:
>On Thu, 2018-03-29 at 15:05 +0200, Ján Tomko wrote:
>> On Thu, Mar 29, 2018 at 01:07:57PM +0200, Katerina Koukiou wrote:
>> > Instead of having multiple signals regarding to domain events,
>> > like DomainStarted, DomainUndefined etc, we will have only one
>> > called Domain, and the specific event type will be specified in
>> > the signals arguments.
>> >
>> > The domain name argument in not needed in the signal since we can
>> > fetch it from path.
>> >
>>
>> Any plans to add support for the 'detail' field of
>> virConnectDomainEventCallback?
>
>Yes, I guess it's ok to be in different patchset though.
>

Given the warning in README.md:
NB: at this time, libvirt-dbus is *NOT* considered API/ABI stable. Future
releases may still include API/ABI incompatible changes.

It's ok for it to be in a different release.

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 3/4] Merge all Domain lifecycle signals into one signal called Domain.
Posted by Pavel Hrdina 7 years, 10 months ago
On Thu, Mar 29, 2018 at 01:07:57PM +0200, Katerina Koukiou wrote:
> Instead of having multiple signals regarding to domain events,
> like DomainStarted, DomainUndefined etc, we will have only one
> called Domain, and the specific event type will be specified in
> the signals arguments.
> 
> The domain name argument in not needed in the signal since we can
> fetch it from path.
> 
> The tests are adjusted to call correctly connect_to_signal method
> on the new signal.
> 
> Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
> ---
>  data/org.libvirt.Connect.xml | 54 +++-----------------------------------------
>  src/events.c                 | 26 ++++++++++-----------
>  test/test_connect.py         | 10 ++++----
>  test/test_domain.py          | 20 +++++++---------
>  4 files changed, 27 insertions(+), 83 deletions(-)
> 
> diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
> index f24dff4..6810422 100644
> --- a/data/org.libvirt.Connect.xml
> +++ b/data/org.libvirt.Connect.xml
> @@ -44,59 +44,11 @@
>        <arg name="uuid" type="s" direction="in"/>
>        <arg name="domain" type="o" direction="out"/>
>      </method>
> -    <signal name="DomainCrashed">
> +    <signal name="Domain">

I'm not sure about using "Domain" as the signal name, how about
"DomainEvent", "DomainChanded" or "DomainState"?

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 3/4] Merge all Domain lifecycle signals into one signal called Domain.
Posted by Katerina Koukiou 7 years, 10 months ago
On Thu, 2018-03-29 at 15:14 +0200, Pavel Hrdina wrote:
> On Thu, Mar 29, 2018 at 01:07:57PM +0200, Katerina Koukiou wrote:
> > Instead of having multiple signals regarding to domain events,
> > like DomainStarted, DomainUndefined etc, we will have only one
> > called Domain, and the specific event type will be specified in
> > the signals arguments.
> > 
> > The domain name argument in not needed in the signal since we can
> > fetch it from path.
> > 
> > The tests are adjusted to call correctly connect_to_signal method
> > on the new signal.
> > 
> > Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
> > ---
> >  data/org.libvirt.Connect.xml | 54 +++-----------------------------
> > ------------
> >  src/events.c                 | 26 ++++++++++-----------
> >  test/test_connect.py         | 10 ++++----
> >  test/test_domain.py          | 20 +++++++---------
> >  4 files changed, 27 insertions(+), 83 deletions(-)
> > 
> > diff --git a/data/org.libvirt.Connect.xml
> > b/data/org.libvirt.Connect.xml
> > index f24dff4..6810422 100644
> > --- a/data/org.libvirt.Connect.xml
> > +++ b/data/org.libvirt.Connect.xml
> > @@ -44,59 +44,11 @@
> >        <arg name="uuid" type="s" direction="in"/>
> >        <arg name="domain" type="o" direction="out"/>
> >      </method>
> > -    <signal name="DomainCrashed">
> > +    <signal name="Domain">
> 
> I'm not sure about using "Domain" as the signal name, how about
> "DomainEvent", "DomainChanded" or "DomainState"?

DomainEvent sounds good. Changed.

> 
> Pavel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list