This is the easier part. All we need to do here is put -object
pr-manager-helper,id=$alias,path=$socketPath and then just
reference the object in -drive file.pr-manager=$alias.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c                            | 94 ++++++++++++++++++++++
 src/qemu/qemu_command.h                            |  3 +
 .../disk-virtio-scsi-reservations.args             | 35 ++++++++
 tests/qemuxml2argvtest.c                           |  4 +
 4 files changed, 136 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 514c3ab2ef..81f6025207 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src)
 }
 
 
+static void
+qemuBuildDriveSourcePR(virBufferPtr buf,
+                       virStorageSourcePtr src)
+{
+    qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
+    qemuDomainDiskPRDPtr prd = srcPriv->prd;
+
+    if (!prd || !prd->alias)
+        return;
+
+    virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias);
+}
+
+
 static int
 qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
                         virQEMUCapsPtr qemuCaps,
@@ -1591,6 +1605,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
 
         if (disk->src->debug)
             virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel);
+
+        qemuBuildDriveSourcePR(buf, disk->src);
     } else {
         if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
             goto cleanup;
@@ -9872,6 +9888,81 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
 }
 
 
+/**
+ * qemuBuildPRManagerInfoProps:
+ * @prd: disk PR runtime info
+ * @propsret: JSON properties to return
+ *
+ * Build the JSON properties for the pr-manager object.
+ *
+ * Returns: 0 on success (@propsret is NULL if no properties are needed),
+ *         -1 on failure (with error message set).
+ */
+int
+qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd,
+                            virJSONValuePtr *propsret)
+{
+    *propsret = NULL;
+
+    if (!prd || !prd->alias)
+        return 0;
+
+    if (virJSONValueObjectCreate(propsret,
+                                 "s:path", prd->path,
+                                 NULL) < 0)
+        return -1;
+
+    return 0;
+}
+
+
+static int
+qemuBuildMasterPRCommandLine(virCommandPtr cmd,
+                             const virDomainDef *def)
+{
+    size_t i;
+    bool managedAdded = false;
+    virJSONValuePtr props = NULL;
+    char *tmp = NULL;
+    int ret = -1;
+
+    for (i = 0; i < def->ndisks; i++) {
+        const virDomainDiskDef *disk = def->disks[i];
+        qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
+        qemuDomainDiskPRDPtr prd = srcPriv->prd;
+
+
+        if (virStoragePRDefIsManaged(disk->src->pr)) {
+            if (managedAdded)
+                continue;
+
+            managedAdded = true;
+        }
+
+        if (qemuBuildPRManagerInfoProps(prd, &props) < 0)
+            goto cleanup;
+
+        if (!props)
+            continue;
+
+        if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper",
+                                                          prd->alias,
+                                                          props)))
+            goto cleanup;
+        virJSONValueFree(props);
+        props = NULL;
+
+        virCommandAddArgList(cmd, "-object", tmp, NULL);
+        VIR_FREE(tmp);
+    }
+
+    ret = 0;
+ cleanup:
+    virJSONValueFree(props);
+    return ret;
+}
+
+
 /**
  * qemuBuildCommandLineValidate:
  *
@@ -10024,6 +10115,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
     if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0)
         goto error;
 
+    if (qemuBuildMasterPRCommandLine(cmd, def) < 0)
+        goto error;
+
     if (enableFips)
         virCommandAddArg(cmd, "-enable-fips");
 
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 31c9da673c..32f3697181 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -54,6 +54,9 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
                                    size_t *nnicindexes,
                                    int **nicindexes);
 
+/* Generate the object properties for pr-manager */
+int qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd,
+                                virJSONValuePtr *propsret);
 
 /* Generate the object properties for a secret */
 int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
new file mode 100644
index 0000000000..195e83a048
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
@@ -0,0 +1,35 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-object pr-manager-helper,id=pr-helper0,\
+path=/tmp/lib/domain--1-QEMUGuest1/pr-helper0.sock \
+-object pr-manager-helper,id=pr-helper-scsi0-0-0-1,\
+path=/path/to/qemu-pr-helper.sock \
+-M pc \
+-m 214 \
+-smp 8,sockets=8,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-no-acpi \
+-boot c \
+-device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,file.pr-manager=pr-helper0,format=raw,\
+if=none,id=drive-scsi0-0-0-0,boot=on \
+-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\
+drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
+-drive file=/dev/HostVG/QEMUGuest2,file.pr-manager=pr-helper-scsi0-0-0-1,\
+format=raw,if=none,id=drive-scsi0-0-0-1 \
+-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=1,\
+drive=drive-scsi0-0-0-1,id=scsi0-0-0-1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 165137e93c..fdaeb473d0 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3055,6 +3055,10 @@ mymain(void)
             QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4,
             QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_PCI_MULTIFUNCTION);
 
+    DO_TEST("disk-virtio-scsi-reservations",
+            QEMU_CAPS_DRIVE_BOOT, QEMU_CAPS_VIRTIO_SCSI,
+            QEMU_CAPS_SCSI_BLOCK, QEMU_CAPS_PR_MANAGER_HELPER);
+
     /* Test disks with format probing enabled for legacy reasons.
      * New tests should not go in this section. */
     driver.config->allowDiskFormatProbing = true;
-- 
2.16.1

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

On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> This is the easier part. All we need to do here is put -object
> pr-manager-helper,id=$alias,path=$socketPath and then just
> reference the object in -drive file.pr-manager=$alias.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_command.c                            | 94 ++++++++++++++++++++++
>  src/qemu/qemu_command.h                            |  3 +
>  .../disk-virtio-scsi-reservations.args             | 35 ++++++++
>  tests/qemuxml2argvtest.c                           |  4 +
>  4 files changed, 136 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
> 

This patch fails qemuxml2argv for me because of commits 'cc32731a' and
'a32539de'...

1430) QEMU XML-2-ARGV disk-virtio-scsi-reservations                     ...
In
'/home/jferlan/git/libvirt.work/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args':
Offset 405
Expect [defaults -chardev
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait
-mon chardev=charmonitor,id=monitor,mode=readline]
Actual [-user-config -nodefaults -chardev
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait
-mon chardev=charmonitor,id=monitor,mode=control]

... FAILED

Simple fix...  regenerating the output gets:

diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
index 195e83a048..0bdd35a18a 100644
--- a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
+++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
@@ -16,10 +16,11 @@ path=/path/to/qemu-pr-helper.sock \
 -smp 8,sockets=8,cores=1,threads=1 \
 -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
 -nographic \
+-no-user-config \
 -nodefaults \
 -chardev
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
 server,nowait \
--mon chardev=charmonitor,id=monitor,mode=readline \
+-mon chardev=charmonitor,id=monitor,mode=control \
 -no-acpi \
 -boot c \
 -device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \


> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 514c3ab2ef..81f6025207 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src)
>  }
>  
>  
> +static void
> +qemuBuildDriveSourcePR(virBufferPtr buf,
> +                       virStorageSourcePtr src)
> +{
> +    qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +    qemuDomainDiskPRDPtr prd = srcPriv->prd;
> +
> +    if (!prd || !prd->alias)
> +        return;
> +
> +    virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias);
> +}
> +
> +
>  static int
>  qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>                          virQEMUCapsPtr qemuCaps,
> @@ -1591,6 +1605,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>  
>          if (disk->src->debug)
>              virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel);
> +
> +        qemuBuildDriveSourcePR(buf, disk->src);
>      } else {
>          if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
>              goto cleanup;
> @@ -9872,6 +9888,81 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>  }
>  
>  
> +/**
> + * qemuBuildPRManagerInfoProps:
> + * @prd: disk PR runtime info
> + * @propsret: JSON properties to return
> + *
> + * Build the JSON properties for the pr-manager object.
> + *
> + * Returns: 0 on success (@propsret is NULL if no properties are needed),
> + *         -1 on failure (with error message set).
> + */
> +int
> +qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd,
> +                            virJSONValuePtr *propsret)
> +{
> +    *propsret = NULL;
> +
> +    if (!prd || !prd->alias)
> +        return 0;
> +
> +    if (virJSONValueObjectCreate(propsret,
> +                                 "s:path", prd->path,
> +                                 NULL) < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +
> +static int
> +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
> +                             const virDomainDef *def)
> +{
> +    size_t i;
> +    bool managedAdded = false;
> +    virJSONValuePtr props = NULL;
> +    char *tmp = NULL;
> +    int ret = -1;
> +
> +    for (i = 0; i < def->ndisks; i++) {
> +        const virDomainDiskDef *disk = def->disks[i];
> +        qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +        qemuDomainDiskPRDPtr prd = srcPriv->prd;
> +
> +
> +        if (virStoragePRDefIsManaged(disk->src->pr)) {
> +            if (managedAdded)
> +                continue;
> +
> +            managedAdded = true;

As soon as we find one that needs managing we should break from the loop
and then only add pr-manager-helper if we have one to manage. No sense
going through the rest of the list of disks.  Move @disk into the top
level and then use it to decide whether or not to add the object.  Then
in some way signify that the object was added so that future hotplugs
wouldn't need to somehow guess - a simple check that it was added
already would seem to suffice.

> +        }

The next hunk would seem to be useful for the hotplug path, no?
Including perhaps setting some sort of qemu_domain level boolean that
the object was added already so that subsequent or consecutive hotplugs
wouldn't try to add it again.

John

> +
> +        if (qemuBuildPRManagerInfoProps(prd, &props) < 0)
> +            goto cleanup;
> +
> +        if (!props)
> +            continue;
> +
> +        if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper",
> +                                                          prd->alias,
> +                                                          props)))
> +            goto cleanup;
> +        virJSONValueFree(props);
> +        props = NULL;
> +
> +        virCommandAddArgList(cmd, "-object", tmp, NULL);
> +        VIR_FREE(tmp);
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    virJSONValueFree(props);
> +    return ret;
> +}
> +
> +
>  /**
>   * qemuBuildCommandLineValidate:
>   *
> @@ -10024,6 +10115,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>      if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0)
>          goto error;
>  
> +    if (qemuBuildMasterPRCommandLine(cmd, def) < 0)
> +        goto error;
> +
>      if (enableFips)
>          virCommandAddArg(cmd, "-enable-fips");
>  
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 31c9da673c..32f3697181 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -54,6 +54,9 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
>                                     size_t *nnicindexes,
>                                     int **nicindexes);
>  
> +/* Generate the object properties for pr-manager */
> +int qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd,
> +                                virJSONValuePtr *propsret);
>  
>  /* Generate the object properties for a secret */
>  int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
> diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
> new file mode 100644
> index 0000000000..195e83a048
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
> @@ -0,0 +1,35 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-i686 \
> +-name QEMUGuest1 \
> +-S \
> +-object pr-manager-helper,id=pr-helper0,\
> +path=/tmp/lib/domain--1-QEMUGuest1/pr-helper0.sock \
> +-object pr-manager-helper,id=pr-helper-scsi0-0-0-1,\
> +path=/path/to/qemu-pr-helper.sock \
> +-M pc \
> +-m 214 \
> +-smp 8,sockets=8,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-nographic \
> +-nodefaults \
> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
> +server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=readline \
> +-no-acpi \
> +-boot c \
> +-device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \
> +-usb \
> +-drive file=/dev/HostVG/QEMUGuest1,file.pr-manager=pr-helper0,format=raw,\
> +if=none,id=drive-scsi0-0-0-0,boot=on \
> +-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\
> +drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
> +-drive file=/dev/HostVG/QEMUGuest2,file.pr-manager=pr-helper-scsi0-0-0-1,\
> +format=raw,if=none,id=drive-scsi0-0-0-1 \
> +-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=1,\
> +drive=drive-scsi0-0-0-1,id=scsi0-0-0-1 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 165137e93c..fdaeb473d0 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -3055,6 +3055,10 @@ mymain(void)
>              QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4,
>              QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_PCI_MULTIFUNCTION);
>  
> +    DO_TEST("disk-virtio-scsi-reservations",
> +            QEMU_CAPS_DRIVE_BOOT, QEMU_CAPS_VIRTIO_SCSI,
> +            QEMU_CAPS_SCSI_BLOCK, QEMU_CAPS_PR_MANAGER_HELPER);
> +
>      /* Test disks with format probing enabled for legacy reasons.
>       * New tests should not go in this section. */
>      driver.config->allowDiskFormatProbing = true;
> 

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

On 04/14/2018 03:04 PM, John Ferlan wrote:
> 
> 
> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>> This is the easier part. All we need to do here is put -object
>> pr-manager-helper,id=$alias,path=$socketPath and then just
>> reference the object in -drive file.pr-manager=$alias.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_command.c                            | 94 ++++++++++++++++++++++
>>  src/qemu/qemu_command.h                            |  3 +
>>  .../disk-virtio-scsi-reservations.args             | 35 ++++++++
>>  tests/qemuxml2argvtest.c                           |  4 +
>>  4 files changed, 136 insertions(+)
>>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
>>
> 
> This patch fails qemuxml2argv for me because of commits 'cc32731a' and
> 'a32539de'...
> 

Oh yeah. There's always chance that between me posting patches and
review somebody will push something that invalidates my patches.


>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 514c3ab2ef..81f6025207 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src)
>>  }
>>  
>>  
>> +static void
>> +qemuBuildDriveSourcePR(virBufferPtr buf,
>> +                       virStorageSourcePtr src)
>> +{
>> +    qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
>> +    qemuDomainDiskPRDPtr prd = srcPriv->prd;
>> +
>> +    if (!prd || !prd->alias)
>> +        return;
>> +
>> +    virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias);
>> +}
>> +
>> +
>>  static int
>>  qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>                          virQEMUCapsPtr qemuCaps,
>> @@ -1591,6 +1605,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>  
>>          if (disk->src->debug)
>>              virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel);
>> +
>> +        qemuBuildDriveSourcePR(buf, disk->src);
>>      } else {
>>          if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
>>              goto cleanup;
>> @@ -9872,6 +9888,81 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>>  }
>>  
>>  
>> +/**
>> + * qemuBuildPRManagerInfoProps:
>> + * @prd: disk PR runtime info
>> + * @propsret: JSON properties to return
>> + *
>> + * Build the JSON properties for the pr-manager object.
>> + *
>> + * Returns: 0 on success (@propsret is NULL if no properties are needed),
>> + *         -1 on failure (with error message set).
>> + */
>> +int
>> +qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd,
>> +                            virJSONValuePtr *propsret)
>> +{
>> +    *propsret = NULL;
>> +
>> +    if (!prd || !prd->alias)
>> +        return 0;
>> +
>> +    if (virJSONValueObjectCreate(propsret,
>> +                                 "s:path", prd->path,
>> +                                 NULL) < 0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>> +
>> +static int
>> +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
>> +                             const virDomainDef *def)
>> +{
>> +    size_t i;
>> +    bool managedAdded = false;
>> +    virJSONValuePtr props = NULL;
>> +    char *tmp = NULL;
>> +    int ret = -1;
>> +
>> +    for (i = 0; i < def->ndisks; i++) {
>> +        const virDomainDiskDef *disk = def->disks[i];
>> +        qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>> +        qemuDomainDiskPRDPtr prd = srcPriv->prd;
>> +
>> +
>> +        if (virStoragePRDefIsManaged(disk->src->pr)) {
>> +            if (managedAdded)
>> +                continue;
>> +
>> +            managedAdded = true;
> 
> As soon as we find one that needs managing we should break from the loop
> and then only add pr-manager-helper if we have one to manage. No sense
> going through the rest of the list of disks.  Move @disk into the top
> level and then use it to decide whether or not to add the object.  Then
> in some way signify that the object was added so that future hotplugs
> wouldn't need to somehow guess - a simple check that it was added
> already would seem to suffice.

Not true. We need to add manager objects even for cases where libvirt is
not managing the helper process. For instance, for the following XML:

  <disk type='block' device='lun'>
    <source dev='/dev/HostVG/QEMUGuest2'>
      <reservations enabled='yes' managed='no'>
        <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/>
      </reservations>
    </source>
  </disk>

the following cmd line needs to be generated:

  -object pr-manager-helper,id=pr-helper-scsi0-0-0-1,\
          path=/path/to/qemu-pr-helper.sock \

otherwise qemu would now know where to connect. However, if libvirt is
managing the pr-helper process, all the managed disks share the same
process => object on the command line => it must be added exactly once.

> 
>> +        }
> 
> The next hunk would seem to be useful for the hotplug path, no?
> Including perhaps setting some sort of qemu_domain level boolean that
> the object was added already so that subsequent or consecutive hotplugs
> wouldn't try to add it again.

It's reused there yes. As Peter suggested in one of earlier reviews
instead of having two functions (one for generating cmd line and the
other for JSON) I can write just one and use
virQEMUBuildObjectCommandlineFromJSON() to translate JSON into cmd line.

Michal

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

On 04/16/2018 10:56 AM, Michal Privoznik wrote:
> On 04/14/2018 03:04 PM, John Ferlan wrote:
>>
>>
>> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>>> This is the easier part. All we need to do here is put -object
>>> pr-manager-helper,id=$alias,path=$socketPath and then just
>>> reference the object in -drive file.pr-manager=$alias.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  src/qemu/qemu_command.c                            | 94 ++++++++++++++++++++++
>>>  src/qemu/qemu_command.h                            |  3 +
>>>  .../disk-virtio-scsi-reservations.args             | 35 ++++++++
>>>  tests/qemuxml2argvtest.c                           |  4 +
>>>  4 files changed, 136 insertions(+)
>>>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
>>>
>>
>> This patch fails qemuxml2argv for me because of commits 'cc32731a' and
>> 'a32539de'...
>>
> 
> Oh yeah. There's always chance that between me posting patches and
> review somebody will push something that invalidates my patches.
> 
> 

True, especially with the amount/rate of change in the capabilities
area!  In some ways it's, point it out, let you know I ACKnowledge that
you'll have some merge work, but that work isn't something that'd be a
problem w/r/t the overall patch.

>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 514c3ab2ef..81f6025207 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src)
>>>  }
>>>  
>>>  
>>> +static void
>>> +qemuBuildDriveSourcePR(virBufferPtr buf,
>>> +                       virStorageSourcePtr src)
>>> +{
>>> +    qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
>>> +    qemuDomainDiskPRDPtr prd = srcPriv->prd;
>>> +
>>> +    if (!prd || !prd->alias)
>>> +        return;
>>> +
>>> +    virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias);
>>> +}
>>> +
>>> +
>>>  static int
>>>  qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>>                          virQEMUCapsPtr qemuCaps,
>>> @@ -1591,6 +1605,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>>  
>>>          if (disk->src->debug)
>>>              virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel);
>>> +
>>> +        qemuBuildDriveSourcePR(buf, disk->src);
>>>      } else {
>>>          if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
>>>              goto cleanup;
>>> @@ -9872,6 +9888,81 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>>>  }
>>>  
>>>  
>>> +/**
>>> + * qemuBuildPRManagerInfoProps:
>>> + * @prd: disk PR runtime info
>>> + * @propsret: JSON properties to return
>>> + *
>>> + * Build the JSON properties for the pr-manager object.
>>> + *
>>> + * Returns: 0 on success (@propsret is NULL if no properties are needed),
>>> + *         -1 on failure (with error message set).
>>> + */
>>> +int
>>> +qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd,
>>> +                            virJSONValuePtr *propsret)
>>> +{
>>> +    *propsret = NULL;
>>> +
>>> +    if (!prd || !prd->alias)
>>> +        return 0;
>>> +
>>> +    if (virJSONValueObjectCreate(propsret,
>>> +                                 "s:path", prd->path,
>>> +                                 NULL) < 0)
>>> +        return -1;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +
>>> +static int
>>> +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
>>> +                             const virDomainDef *def)
>>> +{
>>> +    size_t i;
>>> +    bool managedAdded = false;
>>> +    virJSONValuePtr props = NULL;
>>> +    char *tmp = NULL;
>>> +    int ret = -1;
>>> +
>>> +    for (i = 0; i < def->ndisks; i++) {
>>> +        const virDomainDiskDef *disk = def->disks[i];
>>> +        qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>>> +        qemuDomainDiskPRDPtr prd = srcPriv->prd;
>>> +
>>> +
>>> +        if (virStoragePRDefIsManaged(disk->src->pr)) {
>>> +            if (managedAdded)
>>> +                continue;
>>> +
>>> +            managedAdded = true;
>>
>> As soon as we find one that needs managing we should break from the loop
>> and then only add pr-manager-helper if we have one to manage. No sense
>> going through the rest of the list of disks.  Move @disk into the top
>> level and then use it to decide whether or not to add the object.  Then
>> in some way signify that the object was added so that future hotplugs
>> wouldn't need to somehow guess - a simple check that it was added
>> already would seem to suffice.
> 
> Not true. We need to add manager objects even for cases where libvirt is
> not managing the helper process. For instance, for the following XML:
> 
>   <disk type='block' device='lun'>
>     <source dev='/dev/HostVG/QEMUGuest2'>
>       <reservations enabled='yes' managed='no'>
>         <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/>
>       </reservations>
>     </source>
>   </disk>
> 
> the following cmd line needs to be generated:
> 
>   -object pr-manager-helper,id=pr-helper-scsi0-0-0-1,\
>           path=/path/to/qemu-pr-helper.sock \
> 
> otherwise qemu would now know where to connect. However, if libvirt is
> managing the pr-helper process, all the managed disks share the same
> process => object on the command line => it must be added exactly once.
> 

oh, right... "pr-manager-helper" is the object name, <sigh>

>>
>>> +        }
>>
>> The next hunk would seem to be useful for the hotplug path, no?
>> Including perhaps setting some sort of qemu_domain level boolean that
>> the object was added already so that subsequent or consecutive hotplugs
>> wouldn't try to add it again.
> 
> It's reused there yes. As Peter suggested in one of earlier reviews
> instead of having two functions (one for generating cmd line and the
> other for JSON) I can write just one and use
> virQEMUBuildObjectCommandlineFromJSON() to translate JSON into cmd line.
> 

OK - I had scanned that review and had that comment in mind, but didn't
correlate the two when going through this (nor go back and check here
once I looked at hotplug later).

John

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

On 04/16/2018 04:56 PM, Michal Privoznik wrote:
> On 04/14/2018 03:04 PM, John Ferlan wrote:
>>
>>
>> On 04/10/2018 10:58 AM, Michal Privoznik wrote:
>>> This is the easier part. All we need to do here is put -object
>>> pr-manager-helper,id=$alias,path=$socketPath and then just
>>> reference the object in -drive file.pr-manager=$alias.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  src/qemu/qemu_command.c                            | 94 ++++++++++++++++++++++
>>>  src/qemu/qemu_command.h                            |  3 +
>>>  .../disk-virtio-scsi-reservations.args             | 35 ++++++++
>>>  tests/qemuxml2argvtest.c                           |  4 +
>>>  4 files changed, 136 insertions(+)
>>>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args

So just to give everybody an idea what is this patch going to look like,
I'm attaching it here. I'd like to ask if this is really way we want to
go. Frankly, I find the patch ugly, because in order to not strdup() a
static string, I have to differentiate every time I need an alias. Just
look at qemuBuildDriveSourcePR(). I hate it so much, that I made
qemuBuildPRManagerInfoProps() to stdup() the string. Just think how it
would look like if I did no such thing.

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