Create a new "Prepare" function and move the drive add code into the new
helpers. This will eventually allow to simplify and unify the attaching
code for use with blockdev at the same time as providing compatibility
with older qemus.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_block.c   | 18 ++++++++++++++++++
 src/qemu/qemu_block.h   |  4 ++++
 src/qemu/qemu_command.c | 29 ++++++++++++++++++++++++++++-
 src/qemu/qemu_command.h |  8 ++++----
 src/qemu/qemu_hotplug.c | 26 +++++++++-----------------
 5 files changed, 63 insertions(+), 22 deletions(-)

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 85176925c9..73aab9d73a 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -24,9 +24,12 @@

 #include "viralloc.h"
 #include "virstring.h"
+#include "virlog.h"

 #define VIR_FROM_THIS VIR_FROM_QEMU

+VIR_LOG_INIT("qemu.qemu_block");
+
 /* qemu declares the buffer for node names as a 32 byte array */
 static const size_t qemuBlockNodeNameBufSize = 32;

@@ -1482,6 +1485,8 @@ qemuBlockStorageSourceAttachDataFree(qemuBlockStorageSourceAttachDataPtr data)

     virJSONValueFree(data->storageProps);
     virJSONValueFree(data->formatProps);
+    VIR_FREE(data->driveCmd);
+    VIR_FREE(data->driveAlias);
     VIR_FREE(data);
 }

@@ -1563,6 +1568,13 @@ qemuBlockStorageSourceAttachApply(qemuMonitorPtr mon,
         data->formatAttached = true;
     }

+    if (data->driveCmd) {
+        if (qemuMonitorAddDrive(mon, data->driveCmd) < 0)
+            return -1;
+
+        data->driveAdded = true;
+    }
+
     return 0;
 }

@@ -1591,6 +1603,12 @@ qemuBlockStorageSourceAttachRollback(qemuMonitorPtr mon,
     if (data->storageAttached)
         ignore_value(qemuMonitorBlockdevDel(mon, data->storageNodeName));

+    if (data->driveAdded) {
+        if (qemuMonitorDriveDel(mon, data->driveAlias) < 0)
+            VIR_WARN("Unable to remove drive %s (%s) after failed "
+                     "qemuMonitorAddDevice", data->driveAlias, data->driveCmd);
+    }
+
     virErrorRestore(&orig_err);
 }

diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index bbaf9ec0f1..5c7791ee72 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -78,6 +78,10 @@ struct qemuBlockStorageSourceAttachData {
     virJSONValuePtr formatProps;
     const char *formatNodeName;
     bool formatAttached;
+
+    char *driveCmd;
+    char *driveAlias;
+    bool driveAdded;
 };


diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 484ac261b0..84bb857507 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1639,7 +1639,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
 }


-char *
+static char *
 qemuBuildDriveStr(virDomainDiskDefPtr disk,
                   bool bootable,
                   virQEMUCapsPtr qemuCaps)
@@ -10436,3 +10436,30 @@ qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu)
     virJSONValueFree(ret);
     return NULL;
 }
+
+
+/**
+ * qemuBuildStorageSourceAttachPrepareDrive:
+ * @disk: disk object to prepare
+ * @qemuCaps: qemu capabilities object
+ *
+ * Prepare qemuBlockStorageSourceAttachDataPtr for use with the old approach
+ * using -drive/drive_add. See qemuBlockStorageSourceAttachPrepareBlockdev.
+ */
+qemuBlockStorageSourceAttachDataPtr
+qemuBuildStorageSourceAttachPrepareDrive(virDomainDiskDefPtr disk,
+                                         virQEMUCapsPtr qemuCaps)
+{
+    qemuBlockStorageSourceAttachDataPtr data = NULL;
+
+    if (VIR_ALLOC(data) < 0)
+        return NULL;
+
+    if (!(data->driveCmd = qemuBuildDriveStr(disk, false, qemuCaps)) ||
+        !(data->driveAlias = qemuAliasDiskDriveFromDisk(disk))) {
+        qemuBlockStorageSourceAttachDataFree(data);
+        return NULL;
+    }
+
+    return data;
+}
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index b7580b4796..04f6245bc7 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -28,6 +28,7 @@
 # include "domain_conf.h"
 # include "vircommand.h"
 # include "capabilities.h"
+# include "qemu_block.h"
 # include "qemu_conf.h"
 # include "qemu_domain.h"
 # include "qemu_domain_address.h"
@@ -102,10 +103,9 @@ char *qemuBuildNicDevStr(virDomainDefPtr def,

 char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk);

-/* Both legacy & current support */
-char *qemuBuildDriveStr(virDomainDiskDefPtr disk,
-                        bool bootable,
-                        virQEMUCapsPtr qemuCaps);
+qemuBlockStorageSourceAttachDataPtr
+qemuBuildStorageSourceAttachPrepareDrive(virDomainDiskDefPtr disk,
+                                         virQEMUCapsPtr qemuCaps);

 /* Current, best practice */
 char *qemuBuildDriveDevStr(const virDomainDef *def,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 62fb652093..ece9a82562 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -35,6 +35,7 @@
 #include "qemu_interface.h"
 #include "qemu_process.h"
 #include "qemu_security.h"
+#include "qemu_block.h"
 #include "domain_audit.h"
 #include "netdev_bandwidth_conf.h"
 #include "domain_nwfilter.h"
@@ -390,15 +391,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
 {
     int ret = -1;
     qemuDomainObjPrivatePtr priv = vm->privateData;
+    qemuBlockStorageSourceAttachDataPtr data = NULL;
     virErrorPtr orig_err;
     char *devstr = NULL;
-    char *drivestr = NULL;
-    char *drivealias = NULL;
     char *unmanagedPrmgrAlias = NULL;
     char *managedPrmgrAlias = NULL;
     char *encobjAlias = NULL;
     char *secobjAlias = NULL;
-    bool driveAdded = false;
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     virJSONValuePtr secobjProps = NULL;
     virJSONValuePtr encobjProps = NULL;
@@ -439,14 +438,11 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
         !(unmanagedPrmgrProps = qemuBuildPRManagerInfoProps(disk->src)))
         goto error;

-    if (disk->src->haveTLS == VIR_TRISTATE_BOOL_YES &&
-        qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src) < 0)
-        goto error;
-
-    if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps)))
+    if (!(data = qemuBuildStorageSourceAttachPrepareDrive(disk, priv->qemuCaps)))
         goto error;

-    if (!(drivealias = qemuAliasDiskDriveFromDisk(disk)))
+    if (disk->src->haveTLS == VIR_TRISTATE_BOOL_YES &&
+        qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src) < 0)
         goto error;

     if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps)))
@@ -473,9 +469,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
         qemuMonitorAddObject(priv->mon, &unmanagedPrmgrProps, &unmanagedPrmgrAlias) < 0)
         goto exit_monitor;

-    if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
+    if (qemuBlockStorageSourceAttachApply(priv->mon, data) < 0)
         goto exit_monitor;
-    driveAdded = true;

     if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
         goto exit_monitor;
@@ -491,6 +486,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
     ret = 0;

  cleanup:
+    qemuBlockStorageSourceAttachDataFree(data);
     virJSONValueFree(managedPrmgrProps);
     virJSONValueFree(unmanagedPrmgrProps);
     virJSONValueFree(encobjProps);
@@ -500,18 +496,14 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
     VIR_FREE(unmanagedPrmgrAlias);
     VIR_FREE(secobjAlias);
     VIR_FREE(encobjAlias);
-    VIR_FREE(drivealias);
-    VIR_FREE(drivestr);
     VIR_FREE(devstr);
     virObjectUnref(cfg);
     return ret;

  exit_monitor:
+    qemuBlockStorageSourceAttachRollback(priv->mon, data);
+
     virErrorPreserveLast(&orig_err);
-    if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) {
-        VIR_WARN("Unable to remove drive %s (%s) after failed "
-                 "qemuMonitorAddDevice", drivealias, drivestr);
-    }
     if (secobjAlias)
         ignore_value(qemuMonitorDelObject(priv->mon, secobjAlias));
     if (encobjAlias)
-- 
2.16.2

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

On Fri, Jun 01, 2018 at 05:51:06PM +0200, Peter Krempa wrote:
>Create a new "Prepare" function and move the drive add code into the new
>helpers. This will eventually allow to simplify and unify the attaching
>code for use with blockdev at the same time as providing compatibility
>with older qemus.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_block.c   | 18 ++++++++++++++++++
> src/qemu/qemu_block.h   |  4 ++++
> src/qemu/qemu_command.c | 29 ++++++++++++++++++++++++++++-
> src/qemu/qemu_command.h |  8 ++++----
> src/qemu/qemu_hotplug.c | 26 +++++++++-----------------
> 5 files changed, 63 insertions(+), 22 deletions(-)
>
>diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
>index 85176925c9..73aab9d73a 100644
>--- a/src/qemu/qemu_block.c
>+++ b/src/qemu/qemu_block.c
>@@ -24,9 +24,12 @@
>
> #include "viralloc.h"
> #include "virstring.h"
>+#include "virlog.h"
>
> #define VIR_FROM_THIS VIR_FROM_QEMU
>
>+VIR_LOG_INIT("qemu.qemu_block");
>+
> /* qemu declares the buffer for node names as a 32 byte array */
> static const size_t qemuBlockNodeNameBufSize = 32;
>
>@@ -1482,6 +1485,8 @@ qemuBlockStorageSourceAttachDataFree(qemuBlockStorageSourceAttachDataPtr data)
>
>     virJSONValueFree(data->storageProps);
>     virJSONValueFree(data->formatProps);
>+    VIR_FREE(data->driveCmd);
>+    VIR_FREE(data->driveAlias);
>     VIR_FREE(data);
> }
>
>@@ -1563,6 +1568,13 @@ qemuBlockStorageSourceAttachApply(qemuMonitorPtr mon,
>         data->formatAttached = true;
>     }
>
>+    if (data->driveCmd) {
>+        if (qemuMonitorAddDrive(mon, data->driveCmd) < 0)
>+            return -1;
>+
>+        data->driveAdded = true;
>+    }
>+
>     return 0;
> }
>
>@@ -1591,6 +1603,12 @@ qemuBlockStorageSourceAttachRollback(qemuMonitorPtr mon,
>     if (data->storageAttached)
>         ignore_value(qemuMonitorBlockdevDel(mon, data->storageNodeName));
>
>+    if (data->driveAdded) {
>+        if (qemuMonitorDriveDel(mon, data->driveAlias) < 0)
>+            VIR_WARN("Unable to remove drive %s (%s) after failed "
>+                     "qemuMonitorAddDevice", data->driveAlias, data->driveCmd);
>+    }
>+
>     virErrorRestore(&orig_err);

Even though this call is unrelated to the other two, shouldn't rollback
be in reverse order?

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

On Sat, Jun 02, 2018 at 17:39:51 +0200, Ján Tomko wrote:
> On Fri, Jun 01, 2018 at 05:51:06PM +0200, Peter Krempa wrote:
> > Create a new "Prepare" function and move the drive add code into the new
> > helpers. This will eventually allow to simplify and unify the attaching
> > code for use with blockdev at the same time as providing compatibility
> > with older qemus.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > src/qemu/qemu_block.c   | 18 ++++++++++++++++++
> > src/qemu/qemu_block.h   |  4 ++++
> > src/qemu/qemu_command.c | 29 ++++++++++++++++++++++++++++-
> > src/qemu/qemu_command.h |  8 ++++----
> > src/qemu/qemu_hotplug.c | 26 +++++++++-----------------
> > 5 files changed, 63 insertions(+), 22 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> > index 85176925c9..73aab9d73a 100644
> > --- a/src/qemu/qemu_block.c
> > +++ b/src/qemu/qemu_block.c

[...]

> > @@ -1591,6 +1603,12 @@ qemuBlockStorageSourceAttachRollback(qemuMonitorPtr mon,
> >     if (data->storageAttached)
> >         ignore_value(qemuMonitorBlockdevDel(mon, data->storageNodeName));
> > 
> > +    if (data->driveAdded) {
> > +        if (qemuMonitorDriveDel(mon, data->driveAlias) < 0)
> > +            VIR_WARN("Unable to remove drive %s (%s) after failed "
> > +                     "qemuMonitorAddDevice", data->driveAlias, data->driveCmd);
> > +    }
> > +
> >     virErrorRestore(&orig_err);
> 
> Even though this call is unrelated to the other two, shouldn't rollback
> be in reverse order?

I'll put it so that it's strictly in reverse order, but I made it
impossible to create the data structure in a way that would allow
'drive_add' and 'blockdev_add' be used at the same time.

> 
> 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

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