Extracting the code logic for writing a test image to disk from
testDomainSaveFlags into a separate function, allows us to reuse this
code in other functions such as testDomainSaveImageDefineXML.

Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
---
 src/test/test_driver.c | 114 +++++++++++++++++++++++++----------------
 1 file changed, 69 insertions(+), 45 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2f58a1da95..e71b931790 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1974,75 +1974,106 @@ testDomainGetTime(virDomainPtr dom ATTRIBUTE_UNUSED,

 #define TEST_SAVE_MAGIC "TestGuestMagic"

-static int
-testDomainSaveFlags(virDomainPtr domain, const char *path,
-                    const char *dxml, unsigned int flags)
+
+/**
+ * testDomainSaveImageWrite:
+ * @driver: test driver data
+ * @def: domain definition whose XML will be stored in the image
+ * @path: path of the save image
+ *
+ * Returns true on success, else false.
+ */
+static bool
+testDomainSaveImageWrite(testDriverPtr driver,
+                         virDomainDefPtr def,
+                         const char *path)
 {
-    testDriverPtr privconn = domain->conn->privateData;
-    int fd = -1;
     int len;
-    virDomainObjPtr privdom;
-    virObjectEventPtr event = NULL;
-    int ret = -1;
+    int fd = -1;
     VIR_AUTOFREE(char *) xml = NULL;

-    virCheckFlags(0, -1);
-    if (dxml) {
-        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
-                       _("xml modification unsupported"));
-        return -1;
-    }
-
-
-    if (!(privdom = testDomObjFromDomain(domain)))
-        goto cleanup;
-
-    if (virDomainObjCheckActive(privdom) < 0)
-        goto cleanup;
-
-    xml = virDomainDefFormat(privdom->def, privconn->caps,
-                             VIR_DOMAIN_DEF_FORMAT_SECURE);
+    xml = virDomainDefFormat(def, driver->caps, VIR_DOMAIN_DEF_FORMAT_SECURE);

     if (xml == NULL) {
         virReportSystemError(errno,
                              _("saving domain '%s' failed to allocate space for metadata"),
-                             domain->name);
-        goto cleanup;
+                             def->name);
+        goto error;
     }

     if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) {
         virReportSystemError(errno,
                              _("saving domain '%s' to '%s': open failed"),
-                             domain->name, path);
-        goto cleanup;
+                             def->name, path);
+        goto error;
     }
-    len = strlen(xml);
+
     if (safewrite(fd, TEST_SAVE_MAGIC, sizeof(TEST_SAVE_MAGIC)) < 0) {
         virReportSystemError(errno,
                              _("saving domain '%s' to '%s': write failed"),
-                             domain->name, path);
-        goto cleanup;
+                             def->name, path);
+        goto error;
     }
+
+    len = strlen(xml);
     if (safewrite(fd, (char*)&len, sizeof(len)) < 0) {
         virReportSystemError(errno,
                              _("saving domain '%s' to '%s': write failed"),
-                             domain->name, path);
-        goto cleanup;
+                             def->name, path);
+        goto error;
     }
+
     if (safewrite(fd, xml, len) < 0) {
         virReportSystemError(errno,
                              _("saving domain '%s' to '%s': write failed"),
-                             domain->name, path);
-        goto cleanup;
+                             def->name, path);
+        goto error;
     }

     if (VIR_CLOSE(fd) < 0) {
         virReportSystemError(errno,
                              _("saving domain '%s' to '%s': write failed"),
-                             domain->name, path);
-        goto cleanup;
+                             def->name, path);
+        goto error;
     }
-    fd = -1;
+
+    return true;
+
+ error:
+    /* Don't report failure in close or unlink, because
+     * in either case we're already in a failure scenario
+     * and have reported an earlier error */
+    VIR_FORCE_CLOSE(fd);
+    unlink(path);
+
+    return false;
+}
+
+static int
+testDomainSaveFlags(virDomainPtr domain, const char *path,
+                    const char *dxml, unsigned int flags)
+{
+    testDriverPtr privconn = domain->conn->privateData;
+    virDomainObjPtr privdom;
+    virObjectEventPtr event = NULL;
+    int ret = -1;
+
+    virCheckFlags(0, -1);
+
+    if (dxml) {
+        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+                       _("xml modification unsupported"));
+        return -1;
+    }
+
+    if (!(privdom = testDomObjFromDomain(domain)))
+        goto cleanup;
+
+    if (virDomainObjCheckActive(privdom) < 0)
+        goto cleanup;
+
+    if (!testDomainSaveImageWrite(privconn, privdom->def, path))
+        goto cleanup;

     testDomainShutdownState(domain, privdom, VIR_DOMAIN_SHUTOFF_SAVED);
     event = virDomainEventLifecycleNewFromObj(privdom,
@@ -2054,13 +2085,6 @@ testDomainSaveFlags(virDomainPtr domain, const char *path,

     ret = 0;
  cleanup:
-    /* Don't report failure in close or unlink, because
-     * in either case we're already in a failure scenario
-     * and have reported an earlier error */
-    if (ret != 0) {
-        VIR_FORCE_CLOSE(fd);
-        unlink(path);
-    }
     virDomainObjEndAPI(&privdom);
     virObjectEventStateQueue(privconn->eventState, event);
     return ret;
--
2.21.0

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

On Wed, May 29, 2019 at 02:22:56PM +0200, Ilias Stamatis wrote:
> Extracting the code logic for writing a test image to disk from
> testDomainSaveFlags into a separate function, allows us to reuse this
> code in other functions such as testDomainSaveImageDefineXML.
>
> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> ---
>  src/test/test_driver.c | 114 +++++++++++++++++++++++++----------------
>  1 file changed, 69 insertions(+), 45 deletions(-)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 2f58a1da95..e71b931790 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -1974,75 +1974,106 @@ testDomainGetTime(virDomainPtr dom ATTRIBUTE_UNUSED,
>
>  #define TEST_SAVE_MAGIC "TestGuestMagic"
>
> -static int
> -testDomainSaveFlags(virDomainPtr domain, const char *path,
> -                    const char *dxml, unsigned int flags)
> +
> +/**
> + * testDomainSaveImageWrite:
> + * @driver: test driver data
> + * @def: domain definition whose XML will be stored in the image
> + * @path: path of the save image
> + *
> + * Returns true on success, else false.
> + */
> +static bool

A minor nitpick, I'd probably prefer 'int' rather than 'bool', feels more
natural for the given function.

Reviewed-by: Erik Skultety <eskultet@redhat.com>

> +testDomainSaveImageWrite(testDriverPtr driver,
> +                         virDomainDefPtr def,
> +                         const char *path)
>  {

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

On Mon, Jun 3, 2019 at 10:01 AM Erik Skultety <eskultet@redhat.com> wrote:
>
> On Wed, May 29, 2019 at 02:22:56PM +0200, Ilias Stamatis wrote:
> > Extracting the code logic for writing a test image to disk from
> > testDomainSaveFlags into a separate function, allows us to reuse this
> > code in other functions such as testDomainSaveImageDefineXML.
> >
> > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > ---
> >  src/test/test_driver.c | 114 +++++++++++++++++++++++++----------------
> >  1 file changed, 69 insertions(+), 45 deletions(-)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 2f58a1da95..e71b931790 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -1974,75 +1974,106 @@ testDomainGetTime(virDomainPtr dom ATTRIBUTE_UNUSED,
> >
> >  #define TEST_SAVE_MAGIC "TestGuestMagic"
> >
> > -static int
> > -testDomainSaveFlags(virDomainPtr domain, const char *path,
> > -                    const char *dxml, unsigned int flags)
> > +
> > +/**
> > + * testDomainSaveImageWrite:
> > + * @driver: test driver data
> > + * @def: domain definition whose XML will be stored in the image
> > + * @path: path of the save image
> > + *
> > + * Returns true on success, else false.
> > + */
> > +static bool
>
> A minor nitpick, I'd probably prefer 'int' rather than 'bool', feels more
> natural for the given function.

I was skeptical about the return value type tbh. Initially I had it as
an int as well because I wasn't sure if the bool type is used in
general.

But probably you're right, int makes more sense since this function
could potentially return different error codes for different error
cases.

>
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
>
> > +testDomainSaveImageWrite(testDriverPtr driver,
> > +                         virDomainDefPtr def,
> > +                         const char *path)
> >  {

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