[libvirt] [dbus PATCH v2 1/3] Implement FSFreeze method for Domain Interface

Katerina Koukiou posted 3 patches 7 years ago
[libvirt] [dbus PATCH v2 1/3] Implement FSFreeze method for Domain Interface
Posted by Katerina Koukiou 7 years ago
Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
---
 data/org.libvirt.Domain.xml |  7 +++++++
 src/domain.c                | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index ffb40d1..1d2d760 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -136,6 +136,13 @@
       <arg name="xml" type="s" direction="in"/>
       <arg name="flags" type="u" direction="in"/>
     </method>
+    <method name="FSFreeze">
+      <annotation name="org.gtk.GDBus.DocString"
+        value="See https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSFreeze"/>
+      <arg name="mountpoints" type="as" direction="in"/>
+      <arg name="flags" type="u" direction="in"/>
+      <arg name="frozenFilesystems" type="u" direction="out"/>
+    </method>
     <method name="FSTrim">
       <annotation name="org.gtk.GDBus.DocString"
           value="See https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSTrim
diff --git a/src/domain.c b/src/domain.c
index 5e59094..9a6ff1d 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -677,6 +677,46 @@ virtDBusDomainDetachDevice(GVariant *inArgs,
         virtDBusUtilSetLastVirtError(error);
 }
 
+static void
+virtDBusDomainFSFreeze(GVariant *inArgs,
+                       GUnixFDList *inFDs G_GNUC_UNUSED,
+                       const gchar *objectPath,
+                       gpointer userData,
+                       GVariant **outArgs,
+                       GUnixFDList **outFDs G_GNUC_UNUSED,
+                       GError **error)
+{
+    virtDBusConnect *connect = userData;
+    g_autoptr(virDomain) domain = NULL;
+    g_autofree const gchar **mountpoints = NULL;
+    const gchar **tmp;
+    GVariantIter *iter;
+    gsize nmountpoints = 0;
+    guint flags;
+    gint ret;
+
+    g_variant_get(inArgs, "(asu)", &iter, &flags);
+
+    nmountpoints = g_variant_iter_n_children(iter);
+    if (nmountpoints > 0) {
+        mountpoints = g_new0(const gchar*, nmountpoints);
+        tmp = mountpoints;
+        while (g_variant_iter_loop(iter, "&s", tmp))
+            tmp++;
+        g_variant_iter_free(iter);
+    }
+
+    domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+    if (!domain)
+        return;
+
+    ret = virDomainFSFreeze(domain, mountpoints, nmountpoints, flags);
+    if (ret < 0)
+        return virtDBusUtilSetLastVirtError(error);
+
+    *outArgs = g_variant_new("(u)", ret);
+}
+
 static void
 virtDBusDomainFSTrim(GVariant *inArgs,
                      GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -1582,6 +1622,7 @@ static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] = {
     { "DelIOThread", virtDBusDomainDelIOThread },
     { "Destroy", virtDBusDomainDestroy },
     { "DetachDevice", virtDBusDomainDetachDevice },
+    { "FSFreeze", virtDBusDomainFSFreeze },
     { "FSTrim", virtDBusDomainFSTrim },
     { "GetBlkioParameters", virtDBusDomainGetBlkioParameters },
     { "GetJobInfo", virtDBusDomainGetJobInfo },
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH v2 1/3] Implement FSFreeze method for Domain Interface
Posted by Ján Tomko 7 years ago
On Wed, Apr 18, 2018 at 01:52:17PM +0200, Katerina Koukiou wrote:
>Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
>---
> data/org.libvirt.Domain.xml |  7 +++++++
> src/domain.c                | 41 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+)
>

>diff --git a/src/domain.c b/src/domain.c
>index 5e59094..9a6ff1d 100644
>--- a/src/domain.c
>+++ b/src/domain.c
>@@ -677,6 +677,46 @@ virtDBusDomainDetachDevice(GVariant *inArgs,
>         virtDBusUtilSetLastVirtError(error);
> }
>
>+static void
>+virtDBusDomainFSFreeze(GVariant *inArgs,
>+                       GUnixFDList *inFDs G_GNUC_UNUSED,
>+                       const gchar *objectPath,
>+                       gpointer userData,
>+                       GVariant **outArgs,
>+                       GUnixFDList **outFDs G_GNUC_UNUSED,
>+                       GError **error)
>+{
>+    virtDBusConnect *connect = userData;
>+    g_autoptr(virDomain) domain = NULL;
>+    g_autofree const gchar **mountpoints = NULL;
>+    const gchar **tmp;
>+    GVariantIter *iter;
>+    gsize nmountpoints = 0;
>+    guint flags;
>+    gint ret;
>+
>+    g_variant_get(inArgs, "(asu)", &iter, &flags);
>+
>+    nmountpoints = g_variant_iter_n_children(iter);
>+    if (nmountpoints > 0) {
>+        mountpoints = g_new0(const gchar*, nmountpoints);
>+        tmp = mountpoints;
>+        while (g_variant_iter_loop(iter, "&s", tmp))

g_variant_iter_loop seems to access tmp even if it returns 0.
For an array with two strings, valgrind reports:

==27339== Invalid read of size 8
==27339==    at 0x54B2CB3: g_variant_valist_get (in /usr/lib64/libglib-2.0.so.0.5200.3)
==27339==    by 0x54B4AAB: g_variant_iter_loop (in /usr/lib64/libglib-2.0.so.0.5200.3)
==27339==    by 0x407AD1: virtDBusDomainFSFreeze (domain.c:720)
==27339==    by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224)
==27339==    by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263)
==27339==    by 0x54A146F: g_thread_pool_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3)
==27339==    by 0x54A0AA4: g_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3)
==27339==    by 0x611E636: start_thread (in /lib64/libpthread-2.25.so)
==27339==    by 0x642FBCE: clone (in /lib64/libc-2.25.so)
==27339==  Address 0x1064f980 is 0 bytes after a block of size 16 alloc'd
==27339==    at 0x4C2CEE6: calloc (vg_replace_malloc.c:711)
==27339==    by 0x547EAF0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5200.3)
==27339==    by 0x407AAA: virtDBusDomainFSFreeze (domain.c:718)
==27339==    by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224)
==27339==    by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263)

==27339== Invalid write of size 8
==27339==    at 0x54B2CF0: g_variant_valist_get (in /usr/lib64/libglib-2.0.so.0.5200.3)
==27339==    by 0x54B4AAB: g_variant_iter_loop (in /usr/lib64/libglib-2.0.so.0.5200.3)
==27339==    by 0x407AD1: virtDBusDomainFSFreeze (domain.c:720)
==27339==    by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224)
==27339==    by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263)
==27339==    by 0x54A146F: g_thread_pool_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3)
==27339==    by 0x54A0AA4: g_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3)
==27339==    by 0x611E636: start_thread (in /lib64/libpthread-2.25.so)
==27339==    by 0x642FBCE: clone (in /lib64/libc-2.25.so)
==27339==  Address 0x1064f980 is 0 bytes after a block of size 16 alloc'd
==27339==    at 0x4C2CEE6: calloc (vg_replace_malloc.c:711)
==27339==    by 0x547EAF0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5200.3)
==27339==    by 0x407AAA: virtDBusDomainFSFreeze (domain.c:718)
==27339==    by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224)
==27339==    by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263)

So yes, you should allocate nmountpoints + 1:
https://www.redhat.com/archives/libvir-list/2018-April/msg01647.html

>+            tmp++;
>+        g_variant_iter_free(iter);

This should be moved after the if (n > 0) condition.
g_variant_get initialized the iterator even if the string array has zero
elements.

>+    }
>+
>+    domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
>+    if (!domain)
>+        return;
>+
>+    ret = virDomainFSFreeze(domain, mountpoints, nmountpoints, flags);
>+    if (ret < 0)
>+        return virtDBusUtilSetLastVirtError(error);
>+
>+    *outArgs = g_variant_new("(u)", ret);
>+}
>+
> static void
> virtDBusDomainFSTrim(GVariant *inArgs,
>                      GUnixFDList *inFDs G_GNUC_UNUSED,

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH v2 1/3] Implement FSFreeze method for Domain Interface
Posted by Pavel Hrdina 7 years ago
On Wed, Apr 18, 2018 at 05:12:23PM +0200, Ján Tomko wrote:
> On Wed, Apr 18, 2018 at 01:52:17PM +0200, Katerina Koukiou wrote:
> > Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
> > ---
> > data/org.libvirt.Domain.xml |  7 +++++++
> > src/domain.c                | 41 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 48 insertions(+)
> > 
> 
> > diff --git a/src/domain.c b/src/domain.c
> > index 5e59094..9a6ff1d 100644
> > --- a/src/domain.c
> > +++ b/src/domain.c
> > @@ -677,6 +677,46 @@ virtDBusDomainDetachDevice(GVariant *inArgs,
> >         virtDBusUtilSetLastVirtError(error);
> > }
> > 
> > +static void
> > +virtDBusDomainFSFreeze(GVariant *inArgs,
> > +                       GUnixFDList *inFDs G_GNUC_UNUSED,
> > +                       const gchar *objectPath,
> > +                       gpointer userData,
> > +                       GVariant **outArgs,
> > +                       GUnixFDList **outFDs G_GNUC_UNUSED,
> > +                       GError **error)
> > +{
> > +    virtDBusConnect *connect = userData;
> > +    g_autoptr(virDomain) domain = NULL;
> > +    g_autofree const gchar **mountpoints = NULL;
> > +    const gchar **tmp;
> > +    GVariantIter *iter;
> > +    gsize nmountpoints = 0;
> > +    guint flags;
> > +    gint ret;
> > +
> > +    g_variant_get(inArgs, "(asu)", &iter, &flags);
> > +
> > +    nmountpoints = g_variant_iter_n_children(iter);
> > +    if (nmountpoints > 0) {
> > +        mountpoints = g_new0(const gchar*, nmountpoints);
> > +        tmp = mountpoints;
> > +        while (g_variant_iter_loop(iter, "&s", tmp))
> 
> g_variant_iter_loop seems to access tmp even if it returns 0.
> For an array with two strings, valgrind reports:
> 
> ==27339== Invalid read of size 8
> ==27339==    at 0x54B2CB3: g_variant_valist_get (in /usr/lib64/libglib-2.0.so.0.5200.3)
> ==27339==    by 0x54B4AAB: g_variant_iter_loop (in /usr/lib64/libglib-2.0.so.0.5200.3)
> ==27339==    by 0x407AD1: virtDBusDomainFSFreeze (domain.c:720)
> ==27339==    by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224)
> ==27339==    by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263)
> ==27339==    by 0x54A146F: g_thread_pool_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3)
> ==27339==    by 0x54A0AA4: g_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3)
> ==27339==    by 0x611E636: start_thread (in /lib64/libpthread-2.25.so)
> ==27339==    by 0x642FBCE: clone (in /lib64/libc-2.25.so)
> ==27339==  Address 0x1064f980 is 0 bytes after a block of size 16 alloc'd
> ==27339==    at 0x4C2CEE6: calloc (vg_replace_malloc.c:711)
> ==27339==    by 0x547EAF0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5200.3)
> ==27339==    by 0x407AAA: virtDBusDomainFSFreeze (domain.c:718)
> ==27339==    by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224)
> ==27339==    by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263)
> 
> ==27339== Invalid write of size 8
> ==27339==    at 0x54B2CF0: g_variant_valist_get (in /usr/lib64/libglib-2.0.so.0.5200.3)
> ==27339==    by 0x54B4AAB: g_variant_iter_loop (in /usr/lib64/libglib-2.0.so.0.5200.3)
> ==27339==    by 0x407AD1: virtDBusDomainFSFreeze (domain.c:720)
> ==27339==    by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224)
> ==27339==    by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263)
> ==27339==    by 0x54A146F: g_thread_pool_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3)
> ==27339==    by 0x54A0AA4: g_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3)
> ==27339==    by 0x611E636: start_thread (in /lib64/libpthread-2.25.so)
> ==27339==    by 0x642FBCE: clone (in /lib64/libc-2.25.so)
> ==27339==  Address 0x1064f980 is 0 bytes after a block of size 16 alloc'd
> ==27339==    at 0x4C2CEE6: calloc (vg_replace_malloc.c:711)
> ==27339==    by 0x547EAF0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5200.3)
> ==27339==    by 0x407AAA: virtDBusDomainFSFreeze (domain.c:718)
> ==27339==    by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224)
> ==27339==    by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263)
> 
> So yes, you should allocate nmountpoints + 1:
> https://www.redhat.com/archives/libvir-list/2018-April/msg01647.html

Nice catch, but we still don't have to allocate more than we need to,
instead we should use g_variant_iter_next().

> 
> > +            tmp++;
> > +        g_variant_iter_free(iter);
> 
> This should be moved after the if (n > 0) condition.
> g_variant_get initialized the iterator even if the string array has zero
> elements.

Right, I would suggest 'g_autoptr(GVariantIter) iter = NULL;'

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list