Re: [libvirt] [Qemu-devel] [RFC v2] libvirt vGPU QEMU integration

[Kirti Wankhede]

Ping..

Pulling the questions at the top.

>> Will libvirt report 'description' RO attribute, its output would be
>> string, so that user could be able to see the configuration of that
>> profile?
>>
>
> Daniel,
> Waiting for your input on this.
>
>> We can have 'class' as optional attribute. So Intel don't have to
>> provide 'class' attribute and they don't have to specify mandatory
>> attributes of that class. We would provide 'class' attribute and provide
>> mandatory attributes.
>

Thanks,
Kirti


On 10/3/2016 1:50 PM, Kirti Wankhede wrote:
> 
> 
> On 9/30/2016 10:49 AM, Kirti Wankhede wrote:
>>
> ...
> 
>>> Hi Daniel,
>>>
>>> Here you are proposing to add a class named "gpu", which will make all 
>>> those gpu
>>> related attributes mandatory, which libvirt can allow user to better
>>> parse/present a particular mdev configuration?
>>>
>>> I am just wondering if there is another option that we just make all 
>>> those
>>> attributes that a mdev device can have as optional but still meaningful 
>>> to
>>> libvirt, so libvirt can still parse / recognize them as an class "mdev".
>>
>> 'mdev' isn't a class - mdev is the name of the kernel module. The class
>> refers to the broad capability of the device. class would be things
>> like "gpu", "nic", "fpga" or other such things. The point of the class
>> is to identify which other attributes will be considered mandatory.
>>
>>
>
> Thanks Daniel. This class definition makes sense to me.
>
> However I'm not sure whether we should define such common mandatory 
> attributes
> of a 'gpu' class now. Intel will go with a 2's power sharing of type 
> definition... actual
> type name to be finalized, but an example looks like below:
>
> [GVTG-SKL-x2]: available instances (2)
> [GVTG-SKL-x4]: available instances (4)
> [GVTG-SKL-x8]: available instances (8)
> ...
>
> User can create different types of vGPUs simultaneously. A GVTG-SKL-x2 
> type
> vGPU will get half of the physical GPU resource, while a GVTG-SKL-x4 type 
> will
> get a quarter. However it's unclear to me how we want to enumerate those
> resources into resolution or heads. I feel it'd be more reasonable for us 
> to push
> initial libvirt mdev support w/o vgpu specific class definition, until we 
> see
> a clear value of doing so (at that time we then follow Daniel's guideline 
> to define
> mandatory attributes common to all GPU vendors).

 Libvirt won't report arbitrary vendor define attributes. So if we are not
 going to define a gpu class & associated attributes, then there will be
 no reporting of the 'heads', 'resolution', 'fb_length' data described
 above.

>>>
>>> yes, that's my point. I think nvidia may put them into the 'description' 
>>> attribute
>>> just for descriptive purpose for now.
>>
>>
>> Will libvirt report 'description' RO attribute, its output would be
>> string, so that user could be able to see the configuration of that
>> profile?
>>
> 
> Daniel,
> Waiting for your input on this.
> 
>> We can have 'class' as optional attribute. So Intel don't have to
>> provide 'class' attribute and they don't have to specify mandatory
>> attributes of that class. We would provide 'class' attribute and provide
>> mandatory attributes.
> 
> 
> Thanks,
> Kirti
> 

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

[libvirt] [PATCH v2 12/12] virsh: Add _length parameters to virsh output

[John Ferlan]

https://bugzilla.redhat.com/show_bug.cgi?id=1349898

Add the duration parameters to the virsh input/output for blkdeviotune
command and describe them in the pod file.

Signed-off-by: John Ferlan 
---
 tools/virsh-domain.c | 55 
 tools/virsh.pod  | 21 
 2 files changed, 76 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0643dfb..2a9dc77 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1263,6 +1263,54 @@ static const vshCmdOptDef opts_blkdeviotune[] = {
  .type = VSH_OT_INT,
  .help = N_("I/O size in bytes")
 },
+{.name = "total_bytes_sec_max_length",
+ .type = VSH_OT_ALIAS,
+ .help = "total-bytes-sec-max-length"
+},
+{.name = "total-bytes-sec-max-length",
+ .type = VSH_OT_INT,
+ .help = N_("duration in seconds to allow total max bytes")
+},
+{.name = "read_bytes_sec_max_length",
+ .type = VSH_OT_ALIAS,
+ .help = "read-bytes-sec-max-length"
+},
+{.name = "read-bytes-sec-max-length",
+ .type = VSH_OT_INT,
+ .help = N_("duration in seconds to allow read max bytes")
+},
+{.name = "write_bytes_sec_max_length",
+ .type = VSH_OT_ALIAS,
+ .help = "write-bytes-sec-max-length"
+},
+{.name = "write-bytes-sec-max-length",
+ .type = VSH_OT_INT,
+ .help = N_("duration in seconds to allow write max bytes")
+},
+{.name = "total_iops_sec_max_length",
+ .type = VSH_OT_ALIAS,
+ .help = "total-iops-sec-max-length"
+},
+{.name = "total-iops-sec-max-length",
+ .type = VSH_OT_INT,
+ .help = N_("duration in seconds to allow total I/O operations max")
+},
+{.name = "read_iops_sec_max_length",
+ .type = VSH_OT_ALIAS,
+ .help = "read-iops-sec-max-length"
+},
+{.name = "read-iops-sec-max-length",
+ .type = VSH_OT_INT,
+ .help = N_("duration in seconds to allow read I/O operations max")
+},
+{.name = "write_iops_sec_max_length",
+ .type = VSH_OT_ALIAS,
+ .help = "write-iops-sec-max-length"
+},
+{.name = "write-iops-sec-max-length",
+ .type = VSH_OT_INT,
+ .help = N_("duration in seconds to allow write I/O operations max")
+},
 VIRSH_COMMON_OPT_DOMAIN_CONFIG,
 VIRSH_COMMON_OPT_DOMAIN_LIVE,
 VIRSH_COMMON_OPT_DOMAIN_CURRENT,
@@ -1326,6 +1374,13 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
 VSH_ADD_IOTUNE(write-iops-sec-max, WRITE_IOPS_SEC_MAX);
 VSH_ADD_IOTUNE(size-iops-sec, SIZE_IOPS_SEC);
 
+VSH_ADD_IOTUNE(total-bytes-sec-max-length, TOTAL_BYTES_SEC_MAX_LENGTH);
+VSH_ADD_IOTUNE(read-bytes-sec-max-length, READ_BYTES_SEC_MAX_LENGTH);
+VSH_ADD_IOTUNE(write-bytes-sec-max-length, WRITE_BYTES_SEC_MAX_LENGTH);
+VSH_ADD_IOTUNE(total-iops-sec-max-length, TOTAL_IOPS_SEC_MAX_LENGTH);
+VSH_ADD_IOTUNE(read-iops-sec-max-length, READ_IOPS_SEC_MAX_LENGTH);
+VSH_ADD_IOTUNE(write-iops-sec-max-length, WRITE_IOPS_SEC_MAX_LENGTH);
+
 if (nparams == 0) {
 if (virDomainGetBlockIoTune(dom, NULL, NULL, , flags) != 0) {
 vshError(ctl, "%s",
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 227c9b2..99620b1 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1127,6 +1127,10 @@ command.
 [[I] | [I] [I]]
 [[I] | [I] [I]]
 [[I] | [I] [I]]
+[[I] |
+[I] [I]]
+[[I] |
+[I] [I]]
 [I]
 
 Set or query the block disk io parameters for a block device of I.
@@ -1154,6 +1158,18 @@ integer, the default being bytes per second if no suffix 
is specified.
 I<--total-iops-sec-max> specifies maximum total I/O operations limit per 
second.
 I<--read-iops-sec-max> specifies maximum read I/O operations limit per second.
 I<--write-iops-sec-max> specifies maximum write I/O operations limit per 
second.
+I<--total-bytes-sec-max-length> specifies duration in seconds to allow maximum
+total throughput limit.
+I<--read-bytes-sec-max-length> specifies duration in seconds to allow maximum
+read throughput limit.
+I<--write-bytes-sec-max-length> specifies duration in seconds to allow maximum
+write throughput limit.
+I<--total-iops-sec-max-length> specifies duration in seconds to allow maximum
+total I/O operations limit.
+I<--read-iops-sec-max-length> specifies duration in seconds to allow maximum
+read I/O operations limit.
+I<--write-iops-sec-max-length> specifies duration in seconds to allow maximum
+write I/O operations limit.
 I<--size-iops-sec> specifies size I/O operations limit per second.
 
 Older versions of virsh only accepted these options with underscore
@@ -1164,6 +1180,11 @@ as --read-bytes-sec) resets the other two in that 
category to unlimited.
 An explicit 0 also clears any limit.  A non-zero value for a given total
 cannot be mixed with non-zero values for read or write.
 
+It is up to the hypervisor to determine how to handle the length values.
+For the qemu hypervisor, if an I/O limit value or maximum value is set,
+then the default value of 1 

[libvirt] [PATCH v2 11/12] virsh: Create a macro to add IOTUNE values

[John Ferlan]

Rework the repetitive lines to add iotune values into easier to read macro

Signed-off-by: John Ferlan 
---
 tools/virsh-domain.c | 141 +--
 1 file changed, 25 insertions(+), 116 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 2ce0a06..0643dfb 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1300,122 +1300,31 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptStringReq(ctl, cmd, "device", ) < 0)
 goto cleanup;
 
-if ((rv = vshCommandOptScaledInt(ctl, cmd, "total-bytes-sec", , 1, 
ULLONG_MAX)) < 0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptScaledInt(ctl, cmd, "read-bytes-sec", , 1, 
ULLONG_MAX)) < 0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptScaledInt(ctl, cmd, "write-bytes-sec", , 1, 
ULLONG_MAX)) < 0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptScaledInt(ctl, cmd, "total-bytes-sec-max", , 
1, ULLONG_MAX)) < 0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-
VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptScaledInt(ctl, cmd, "read-bytes-sec-max", , 
1, ULLONG_MAX)) < 0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptScaledInt(ctl, cmd, "write-bytes-sec-max", , 
1, ULLONG_MAX)) < 0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-
VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptULongLong(ctl, cmd, "total-iops-sec", )) < 0) 
{
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptULongLong(ctl, cmd, "read-iops-sec", )) < 0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptULongLong(ctl, cmd, "write-iops-sec", )) < 0) 
{
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptULongLong(ctl, cmd, "write-iops-sec-max", )) 
< 0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptULongLong(ctl, cmd, "read-iops-sec-max", )) < 
0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptULongLong(ctl, cmd, "total-iops-sec-max", )) 
< 0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX,
-value) < 0)
-goto save_error;
-}
-
-if ((rv = vshCommandOptULongLong(ctl, cmd, "size-iops-sec", )) < 0) {
-goto interror;
-} else if (rv > 0) {
-if (virTypedParamsAddULLong(, , ,
-VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC,
-

[libvirt] [PATCH v2 03/12] qemu: Move setting of conf_disk in qemuDomainSetBlockIoTune

[John Ferlan]

Since persistent_def is the only place that uses it, let's just keep
it closer to where it's used.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_driver.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3ce3f2d..78e917e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17449,15 +17449,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 goto endjob;
 }
 
-if (persistentDef) {
-if (!(conf_disk = virDomainDiskByName(persistentDef, path, true))) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("missing persistent configuration for disk '%s'"),
-   path);
-goto endjob;
-}
-}
-
 if (def) {
 supportMaxOptions = virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_DRIVE_IOTUNE_MAX);
@@ -17550,6 +17541,12 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 }
 
 if (persistentDef) {
+if (!(conf_disk = virDomainDiskByName(persistentDef, path, true))) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("missing persistent configuration for disk '%s'"),
+   path);
+goto endjob;
+}
 oldinfo = _disk->blkdeviotune;
 if (!set_bytes) {
 info.total_bytes_sec = oldinfo->total_bytes_sec;
-- 
2.7.4

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

[libvirt] [PATCH v2 09/12] conf: Add support for blkiotune "_length" options

[John Ferlan]

Modify _virDomainBlockIoTuneInfo and rng schema to support the _length
options for bps/iops throttling values. Document the new values.

Signed-off-by: John Ferlan 
---
 docs/formatdomain.html.in  | 40 -
 docs/schemas/domaincommon.rng  | 38 +
 src/conf/domain_conf.c | 24 +++-
 .../qemuxml2argv-blkdeviotune-max-length.xml   | 65 ++
 .../qemuxml2xmlout-blkdeviotune-max-length.xml |  1 +
 tests/qemuxml2xmltest.c|  1 +
 6 files changed, 166 insertions(+), 3 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml
 create mode 12 
tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 1266e9d..274b9bb 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2617,7 +2617,45 @@
 maximum write I/O operations per second.
   size_iops_sec
   The optional size_iops_sec element is the
-size of I/O operations per second.
+size of I/O operations per second.
+  
+Throughput limits since 1.2.11 and QEMU 
1.7
+  
+  
+  total_bytes_sec_max_length
+  The optional total_bytes_sec_max_length
+element is the maximum duration in seconds for the
+total_bytes_sec_max burst period. Only valid
+when the total_bytes_sec_max is set.
+  read_bytes_sec_max_length
+  The optional read_bytes_sec_max_length
+element is the maximum duration in seconds for the
+read_bytes_sec_max burst period. Only valid
+when the read_bytes_sec_max is set.
+  write_bytes_sec_max
+  The optional write_bytes_sec_max_length
+element is the maximum duration in seconds for the
+write_bytes_sec_max burst period. Only valid
+when the if write_bytes_sec_max is set.
+  total_iops_sec_max_length
+  The optional total_iops_sec_max_length
+element is the maximum duration in seconds for the
+total_iops_sec_max burst period. Only valid
+when the total_iops_sec_max is set.
+  read_iops_sec_max_length
+  The optional read_iops_sec_max_length
+element is the maximum duration in seconds for the
+read_iops_sec_max burst period. Only valid
+when the read_iops_sec_max is set.
+  write_iops_sec_max
+  The optional write_iops_sec_max_length
+element is the maximum duration in seconds for the
+write_iops_sec_max burst period. Only valid
+when the write_iops_sec_max is set.
+  
+Throughput length since 2.4.0 and QEMU 
2.6
+  
+  
 
   
   driver
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 6eeb4e9..d648702 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4960,6 +4960,44 @@
 
   
 
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6a772c6..9dce51d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7137,7 +7137,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_XML_ERROR,  
\
_("disk iotune field '%s' must be an integer"), #val);  
\
 return -1; 
\
-}
+}  
\
 
 static int
 virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
@@ -7159,6 +7159,13 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
 
 PARSE_IOTUNE(size_iops_sec);
 
+PARSE_IOTUNE(total_bytes_sec_max_length);
+PARSE_IOTUNE(read_bytes_sec_max_length);
+PARSE_IOTUNE(write_bytes_sec_max_length);
+PARSE_IOTUNE(total_iops_sec_max_length);
+PARSE_IOTUNE(read_iops_sec_max_length);
+PARSE_IOTUNE(write_iops_sec_max_length);
+
 if ((def->blkdeviotune.total_bytes_sec &&
  def->blkdeviotune.read_bytes_sec) ||
 (def->blkdeviotune.total_bytes_sec &&
@@ -20272,7 

[libvirt] [PATCH v2 07/12] caps: Add new capability for the bps/iops throttling length

[John Ferlan]

Add the capability to detect if the qemu binary can support the feature
to use bps-max-length and friends.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_capabilities.c| 2 ++
 src/qemu/qemu_capabilities.h| 1 +
 tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml| 1 +
 7 files changed, 8 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index cc8ec58..eb78d92 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -344,6 +344,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "query-hotpluggable-cpus",
 
   "virtio-net.rx_queue_size", /* 235 */
+  "drive-iotune-max-length",
 );
 
 
@@ -2852,6 +2853,7 @@ static struct virQEMUCapsCommandLineProps 
virQEMUCapsCommandLine[] = {
 { "name", "debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS },
 { "name", "guest", QEMU_CAPS_NAME_GUEST },
 { "spice", "unix", QEMU_CAPS_SPICE_UNIX },
+{ "drive", "throttling.bps-total-max-length", 
QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH },
 };
 
 static int
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index ba0ef48..d99d61d 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -378,6 +378,7 @@ typedef enum {
 
 /* 235 */
 QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE, /* virtio-net-*.rx_queue_size */
+QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH, /* -drive bps_max_length = and friends 
*/
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
index fd14665..d5089dc 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
@@ -159,6 +159,7 @@
   
   
   
+  
   2005094
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
index eb708f8..3417996 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
@@ -159,6 +159,7 @@
   
   
   
+  
   2005094
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml 
b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
index 482b384..6a653ae 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
@@ -153,6 +153,7 @@
   
   
   
+  
   2005094
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
index 60f1fcf..cbea49e 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
@@ -196,6 +196,7 @@
   
   
   
+  
   2006000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
index 98f3762..d9e9a78 100644
--- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
@@ -197,6 +197,7 @@
   
   
   
+  
   2007000
   0
(v2.7.0)
-- 
2.7.4

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

[libvirt] [PATCH v2 02/12] qemu: Return real error message for block_set_io_throttle

[John Ferlan]

This patch will also adjust the qemuMonitorJSONSetBlockIoThrottle error
procession so that rather than returning/displaying:

"error: internal error: Unexpected error"

Fetch the actual error message from qemu and display that

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_monitor_json.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 349a64f..744c878 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -4615,15 +4615,19 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr 
mon,
 goto cleanup;
 
 if (virJSONValueObjectHasKey(result, "error")) {
-if (qemuMonitorJSONHasError(result, "DeviceNotActive"))
+if (qemuMonitorJSONHasError(result, "DeviceNotActive")) {
 virReportError(VIR_ERR_OPERATION_INVALID,
_("No active operation on device: %s"), device);
-else if (qemuMonitorJSONHasError(result, "NotSupported"))
+} else if (qemuMonitorJSONHasError(result, "NotSupported")) {
 virReportError(VIR_ERR_OPERATION_INVALID,
_("Operation is not supported for device: %s"), 
device);
-else
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Unexpected error"));
+} else {
+virJSONValuePtr error = virJSONValueObjectGet(result, "error");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("unable to execute '%s', unexpected error: '%s'"),
+   qemuMonitorJSONCommandName(cmd),
+   qemuMonitorJSONStringifyError(error));
+}
 goto cleanup;
 }
 
-- 
2.7.4

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

[libvirt] [PATCH v2 08/12] qemu: Add length for bps/iops throttling parameters to driver

[John Ferlan]

Add support for a duration/length for the bps/iops and friends.

Modify the API in order to add the "blkdeviotune." specific definitions
for the iotune throttling duration/length options

total_bytes_sec_max_length
write_bytes_sec_max_length
read_bytes_sec_max_length
total_iops_sec_max_length
write_iops_sec_max_length
read_iops_sec_max_length

Signed-off-by: John Ferlan 
---
 include/libvirt/libvirt-domain.h |  54 +
 src/conf/domain_conf.h   |   6 +++
 src/qemu/qemu_driver.c   | 100 +--
 src/qemu/qemu_monitor.c  |   7 ++-
 src/qemu/qemu_monitor.h  |   3 +-
 src/qemu/qemu_monitor_json.c |  25 +-
 src/qemu/qemu_monitor_json.h |   3 +-
 tests/qemumonitorjsontest.c  |  17 ++-
 8 files changed, 202 insertions(+), 13 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 6fe92e2..d5d7d47 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -3846,6 +3846,60 @@ typedef void 
(*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn,
 # define VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC "blkdeviotune.size_iops_sec"
 
 /**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH:
+ *
+ * Macro represents the length in seconds allowed for a burst period
+ * for the blkdeviotune.total_bytes_sec_max,
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH 
"blkdeviotune.total_bytes_sec_max_length"
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH:
+ *
+ * Macro represents the length in seconds allowed for a burst period
+ * for the blkdeviotune.read_bytes_sec_max
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH 
"blkdeviotune.read_bytes_sec_max_length"
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH:
+ *
+ * Macro represents the length in seconds allowed for a burst period
+ * for the blkdeviotune.write_bytes_sec_max
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH 
"blkdeviotune.write_bytes_sec_max_length"
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH:
+ *
+ * Macro represents the length in seconds allowed for a burst period
+ * for the blkdeviotune.total_iops_sec_max
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH 
"blkdeviotune.total_iops_sec_max_length"
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH:
+ *
+ * Macro represents the length in seconds allowed for a burst period
+ * for the blkdeviotune.read_iops_sec_max
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH 
"blkdeviotune.read_iops_sec_max_length"
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH:
+ *
+ * Macro represents the length in seconds allowed for a burst period
+ * for the blkdeviotune.write_iops_sec_max
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH 
"blkdeviotune.write_iops_sec_max_length"
+
+/**
  * virConnectDomainEventTunableCallback:
  * @conn: connection object
  * @dom: domain on which the event occurred
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a70bc21..7e182c1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -548,6 +548,12 @@ struct _virDomainBlockIoTuneInfo {
 unsigned long long read_iops_sec_max;
 unsigned long long write_iops_sec_max;
 unsigned long long size_iops_sec;
+unsigned long long total_bytes_sec_max_length;
+unsigned long long read_bytes_sec_max_length;
+unsigned long long write_bytes_sec_max_length;
+unsigned long long total_iops_sec_max_length;
+unsigned long long read_iops_sec_max_length;
+unsigned long long write_iops_sec_max_length;
 };
 typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr;
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a5f7d0c..8345d58 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -114,6 +114,7 @@ VIR_LOG_INIT("qemu.qemu_driver");
 
 #define QEMU_NB_BLOCK_IO_TUNE_PARAM  6
 #define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX  13
+#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 19
 
 #define QEMU_NB_NUMA_PARAM 2
 
@@ -17296,7 +17297,9 @@ 
qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
 bool set_iops,
 bool set_bytes_max,
 bool set_iops_max,
-bool set_size_iops)
+bool set_size_iops,
+bool set_bytes_max_length,
+bool set_iops_max_length)
 {
 if (!set_bytes) {
 newinfo->total_bytes_sec = oldinfo->total_bytes_sec;
@@ 

[libvirt] [PATCH v2 04/12] qemu: Introduce qemuDomainSetBlockIoTuneSetDefaults

[John Ferlan]

Create a helper to set the bytes/iops iotune default values based on
the current qemu setting

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_driver.c | 66 ++
 1 file changed, 40 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 78e917e..fcce3f0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17286,6 +17286,43 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom,
 return ret;
 }
 
+
+/* If the user didn't specify bytes limits, inherit previous values;
+ * likewise if the user didn't specify iops limits.  */
+static void
+qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
+virDomainBlockIoTuneInfoPtr oldinfo,
+bool set_bytes,
+bool set_iops,
+bool set_bytes_max,
+bool set_iops_max,
+bool set_size_iops)
+{
+if (!set_bytes) {
+newinfo->total_bytes_sec = oldinfo->total_bytes_sec;
+newinfo->read_bytes_sec = oldinfo->read_bytes_sec;
+newinfo->write_bytes_sec = oldinfo->write_bytes_sec;
+}
+if (!set_bytes_max) {
+newinfo->total_bytes_sec_max = oldinfo->total_bytes_sec_max;
+newinfo->read_bytes_sec_max = oldinfo->read_bytes_sec_max;
+newinfo->write_bytes_sec_max = oldinfo->write_bytes_sec_max;
+}
+if (!set_iops) {
+newinfo->total_iops_sec = oldinfo->total_iops_sec;
+newinfo->read_iops_sec = oldinfo->read_iops_sec;
+newinfo->write_iops_sec = oldinfo->write_iops_sec;
+}
+if (!set_iops_max) {
+newinfo->total_iops_sec_max = oldinfo->total_iops_sec_max;
+newinfo->read_iops_sec_max = oldinfo->read_iops_sec_max;
+newinfo->write_iops_sec_max = oldinfo->write_iops_sec_max;
+}
+if (!set_size_iops)
+newinfo->size_iops_sec = oldinfo->size_iops_sec;
+}
+
+
 static int
 qemuDomainSetBlockIoTune(virDomainPtr dom,
  const char *path,
@@ -17473,32 +17510,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 if (!(device = qemuAliasFromDisk(disk)))
 goto endjob;
 
-/* If the user didn't specify bytes limits, inherit previous
- * values; likewise if the user didn't specify iops
- * limits.  */
-oldinfo = >blkdeviotune;
-if (!set_bytes) {
-info.total_bytes_sec = oldinfo->total_bytes_sec;
-info.read_bytes_sec = oldinfo->read_bytes_sec;
-info.write_bytes_sec = oldinfo->write_bytes_sec;
-}
-if (!set_bytes_max) {
-info.total_bytes_sec_max = oldinfo->total_bytes_sec_max;
-info.read_bytes_sec_max = oldinfo->read_bytes_sec_max;
-info.write_bytes_sec_max = oldinfo->write_bytes_sec_max;
-}
-if (!set_iops) {
-info.total_iops_sec = oldinfo->total_iops_sec;
-info.read_iops_sec = oldinfo->read_iops_sec;
-info.write_iops_sec = oldinfo->write_iops_sec;
-}
-if (!set_iops_max) {
-info.total_iops_sec_max = oldinfo->total_iops_sec_max;
-info.read_iops_sec_max = oldinfo->read_iops_sec_max;
-info.write_iops_sec_max = oldinfo->write_iops_sec_max;
-}
-if (!set_size_iops)
-info.size_iops_sec = oldinfo->size_iops_sec;
+qemuDomainSetBlockIoTuneSetDefaults(, >blkdeviotune,
+set_bytes, set_iops, set_bytes_max,
+set_iops_max, set_size_iops);
 
 #define CHECK_MAX(val)  \
 do {\
-- 
2.7.4

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

[libvirt] [PATCH v2 06/12] include: Add new definitions for duration for bps/iops throttling

[John Ferlan]

Add new options to allow proving a duration/length in seconds to allow the
bps/iops (and friends) to occur:

total_bytes_sec_max_length
write_bytes_sec_max_length
read_bytes_sec_max_length
total_iops_sec_max_length
write_iops_sec_max_length
read_iops_sec_max_length

Add continue for compiler hint to return to for control

Signed-off-by: John Ferlan 
---
 include/libvirt/libvirt-domain.h | 48 
 1 file changed, 48 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index ec94cdf..6fe92e2 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2322,6 +2322,54 @@ int virDomainBlockCommit(virDomainPtr dom, const char 
*disk, const char *base,
 # define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX "write_iops_sec_max"
 
 /**
+ * VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH:
+ *
+ * Macro for the BlockIoTune tunable weight: it represents the duration in
+ * seconds for the burst allowed by total_bytes_sec_max, as a ullong.
+ */
+# define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH 
"total_bytes_sec_max_length"
+
+/**
+ * VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH:
+ *
+ * Macro for the BlockIoTune tunable weight: it represents the duration in
+ * seconds for the burst allowed by read_bytes_sec_max, as a ullong.
+ */
+# define VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH 
"read_bytes_sec_max_length"
+
+/**
+ * VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH:
+ *
+ * Macro for the BlockIoTune tunable weight: it represents the duration in
+ * seconds for the burst allowed by write_bytes_sec_max, as a ullong.
+ */
+# define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH 
"write_bytes_sec_max_length"
+
+/**
+ * VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH:
+ *
+ * Macro for the BlockIoTune tunable weight: it represents the duration in
+ * seconds for the burst allowed by total_iops_sec_max, as a ullong.
+ */
+# define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH 
"total_iops_sec_max_length"
+
+/**
+ * VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH:
+ *
+ * Macro for the BlockIoTune tunable weight: it represents the duration in
+ * seconds for the burst allowed by read_iops_sec_max, as a ullong.
+ */
+# define VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH 
"read_iops_sec_max_length"
+
+/**
+ * VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH:
+ *
+ * Macro for the BlockIoTune tunable weight: it represents the duration in
+ * seconds for the burst allowed by write_iops_sec_max, as a ullong.
+ */
+# define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH 
"write_iops_sec_max_length"
+
+/**
  * VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC:
  * Macro for the BlockIoTune tunable weight: it represents the size
  * I/O operations per second permitted through a block device, as a ullong.
-- 
2.7.4

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

[libvirt] [PATCH v2 10/12] qemu: Add the length options to the iotune command line

[John Ferlan]

Add in the block I/O throttling length/duration parameter to the command
line if supported. If not supported, fail command creation.

Add the xml2argvtest for testing.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c| 21 +
 .../qemuxml2argv-blkdeviotune-max-length.args  | 34 ++
 tests/qemuxml2argvtest.c   |  4 +++
 3 files changed, 59 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 578ff8b..6d2e15d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1747,6 +1747,20 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 goto error;
 }
 
+/* block I/O throttling length 2.6 */
+if ((disk->blkdeviotune.total_bytes_sec_max_length ||
+ disk->blkdeviotune.read_bytes_sec_max_length ||
+ disk->blkdeviotune.write_bytes_sec_max_length ||
+ disk->blkdeviotune.total_iops_sec_max_length ||
+ disk->blkdeviotune.read_iops_sec_max_length ||
+ disk->blkdeviotune.write_iops_sec_max_length) &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("there are some block I/O throttling length 
parameters "
+ "that are not supported with this QEMU binary"));
+goto error;
+}
+
 if (disk->blkdeviotune.total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
 disk->blkdeviotune.read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
 disk->blkdeviotune.write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
@@ -1788,6 +1802,13 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 
 IOTUNE_ADD(size_iops_sec, "iops-size");
 
+IOTUNE_ADD(total_bytes_sec_max_length, "bps-total-max-length");
+IOTUNE_ADD(read_bytes_sec_max_length, "bps-read-max-length");
+IOTUNE_ADD(write_bytes_sec_max_length, "bps-write-max-length");
+IOTUNE_ADD(total_iops_sec_max_length, "iops-total-max-length");
+IOTUNE_ADD(read_iops_sec_max_length, "iops-read-max-length");
+IOTUNE_ADD(write_iops_sec_max_length, "iops-write-max-length");
+
 #undef IOTUNE_ADD
 
 if (virBufferCheckError() < 0)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args 
b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args
new file mode 100644
index 000..b656aea
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args
@@ -0,0 +1,34 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\
+cache=none,throttling.bps-total=5000,throttling.iops-total=6000,\
+throttling.bps-total-max=1,throttling.iops-total-max=11000,\
+throttling.bps-total-max-length=3,throttling.iops-total-max-length=5 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-drive file=/dev/HostVG/QEMUGuest2,format=qcow2,if=none,id=drive-ide0-0-1,\
+cache=none,throttling.bps-read=5000,throttling.bps-write=5500,\
+throttling.iops-read=3500,throttling.iops-write=4000,\
+throttling.bps-read-max=6000,throttling.bps-write-max=6500,\
+throttling.iops-read-max=7000,throttling.iops-write-max=7500,\
+throttling.iops-size=2000,throttling.bps-read-max-length=3,\
+throttling.bps-write-max-length=5,throttling.iops-read-max-length=7,\
+throttling.iops-write-max-length=9 \
+-device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 4b9ecb8..6523a07 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1492,6 +1492,10 @@ mymain(void)
 DO_TEST("blkdeviotune-max",
 QEMU_CAPS_DRIVE_IOTUNE,
 QEMU_CAPS_DRIVE_IOTUNE_MAX);
+DO_TEST("blkdeviotune-max-length",
+QEMU_CAPS_DRIVE_IOTUNE,
+QEMU_CAPS_DRIVE_IOTUNE_MAX,
+QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH);
 
 DO_TEST("multifunction-pci-device",
 QEMU_CAPS_NODEFCONFIG,
-- 
2.7.4

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

[libvirt] [PATCH v2 05/12] qemu: Use qemuDomainsetBlockIoTuneSetDefaults for config

[John Ferlan]

Rather than repeat the lines in the persistent def, use the same helper
to set config values.

This also fixes a bug where config values for *_max and size_iops_sec would
be set back to 0 if a config value was set.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_driver.c | 15 +++
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fcce3f0..a5f7d0c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17336,7 +17336,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 virDomainDefPtr def = NULL;
 virDomainDefPtr persistentDef = NULL;
 virDomainBlockIoTuneInfo info;
-virDomainBlockIoTuneInfo *oldinfo;
 char *device = NULL;
 int ret = -1;
 size_t i;
@@ -17561,17 +17560,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
path);
 goto endjob;
 }
-oldinfo = _disk->blkdeviotune;
-if (!set_bytes) {
-info.total_bytes_sec = oldinfo->total_bytes_sec;
-info.read_bytes_sec = oldinfo->read_bytes_sec;
-info.write_bytes_sec = oldinfo->write_bytes_sec;
-}
-if (!set_iops) {
-info.total_iops_sec = oldinfo->total_iops_sec;
-info.read_iops_sec = oldinfo->read_iops_sec;
-info.write_iops_sec = oldinfo->write_iops_sec;
-}
+qemuDomainSetBlockIoTuneSetDefaults(, _disk->blkdeviotune,
+set_bytes, set_iops, set_bytes_max,
+set_iops_max, set_size_iops);
 conf_disk->blkdeviotune = info;
 ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef);
 if (ret < 0)
-- 
2.7.4

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

[libvirt] [PATCH v2 01/12] qemu: Create a macro to handle setting bytes/iops iotune values

[John Ferlan]

Create a macros to hide all the comparisons for each of the fields.

Add a 'continue;' for a compiler hint that we only need to find one
this should be similar enough to the if - elseif - elseif logic.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_driver.c | 153 +++--
 1 file changed, 35 insertions(+), 118 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 29a7e3f..3ce3f2d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17371,6 +17371,18 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0)
 goto endjob;
 
+#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST)   
\
+if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) {
\
+info.FIELD = param->value.ul;  
\
+BOOL = true;   
\
+if (virTypedParamsAddULLong(, ,   
\
+,   
\
+VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, 
\
+param->value.ul) < 0)  
\
+goto endjob;   
\
+continue;  
\
+}
+
 for (i = 0; i < nparams; i++) {
 virTypedParameterPtr param = [i];
 
@@ -17381,124 +17393,29 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 goto endjob;
 }
 
-if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) {
-info.total_bytes_sec = param->value.ul;
-set_bytes = true;
-if (virTypedParamsAddULLong(, ,
-,
-
VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC,
-param->value.ul) < 0)
-goto endjob;
-} else if (STREQ(param->field,
- VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) {
-info.read_bytes_sec = param->value.ul;
-set_bytes = true;
-if (virTypedParamsAddULLong(, ,
-,
-
VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC,
-param->value.ul) < 0)
-goto endjob;
-} else if (STREQ(param->field,
- VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) {
-info.write_bytes_sec = param->value.ul;
-set_bytes = true;
-if (virTypedParamsAddULLong(, ,
-,
-
VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC,
-param->value.ul) < 0)
-goto endjob;
-} else if (STREQ(param->field,
- VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) {
-info.total_iops_sec = param->value.ul;
-set_iops = true;
-if (virTypedParamsAddULLong(, ,
-,
-
VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC,
-param->value.ul) < 0)
-goto endjob;
-} else if (STREQ(param->field,
- VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) {
-info.read_iops_sec = param->value.ul;
-set_iops = true;
-if (virTypedParamsAddULLong(, ,
-,
-
VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC,
-param->value.ul) < 0)
-goto endjob;
-} else if (STREQ(param->field,
- VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) {
-info.write_iops_sec = param->value.ul;
-set_iops = true;
-if (virTypedParamsAddULLong(, ,
-,
-
VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC,
-param->value.ul) < 0)
-goto endjob;
-} else if (STREQ(param->field,
- VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX)) {
-info.total_bytes_sec_max = param->value.ul;
-set_bytes_max = true;
-if (virTypedParamsAddULLong(, ,
-,
-
VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX,
-param->value.ul) < 0)
-goto endjob;
-} else if (STREQ(param->field,
- VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX)) {
- 

[libvirt] [PATCH v2 00/12] Add length (duration) params for iotune throttling

[John Ferlan]

v1: http://www.redhat.com/archives/libvir-list/2016-September/msg01090.html

Differences since v1:

Patches 1-5 are new...
   Patch 1 is essentially the former patch 8.5 added after initial review
  based on a review comment. Erik Skultety and I went back and forth
  on this one a few times and this is the result
   Patch 2 is a result of being frustrated by a generic error message when
  I know we could return whatever qemu gives us
   Patch 3 is simple code motion - the conf_disk is closer to usage now
   Patch 4 & 5 create and use the same algorithm to set the default values
  that weren't provided.

Patches 6-7 have already been ACK'd... I've been waiting to push because
   they will interfere with libvirt-perl builds and since the 2.3.0 hasn't
   been published yet, I'm still holding onto them.

Patch 8 is the former patch 8, but reworked mostly in qemu_driver.c, but
   also a change in qemu_monitor_json.c to use "P:" instead of "U:" for
   then length values. For _length a 0 is an invalid value, so we'll use
   it to mean "nothing changed"

Patch 9 is the former patch 11 and is mostly intact, with major difference
   being the removal of the CHECK_IOTUNE_MAX_LENGTH macro (it was causing
   domain disappearance)

Patch 10 is the former patch 12 and is unchanged. It also has been ACKd

Patch 11-12 are new - it's the virsh code.

John Ferlan (12):
  qemu: Create a macro to handle setting bytes/iops iotune values
  qemu: Return real error message for block_set_io_throttle
  qemu: Move setting of conf_disk in qemuDomainSetBlockIoTune
  qemu: Introduce qemuDomainSetBlockIoTuneSetDefaults
  qemu: Use qemuDomainsetBlockIoTuneSetDefaults for config
  include: Add new definitions for duration for bps/iops throttling
  caps: Add new capability for the bps/iops throttling length
  qemu: Add length for bps/iops throttling parameters to driver
  conf: Add support for blkiotune "_length" options
  qemu: Add the length options to the iotune command line
  virsh: Create a macro to add IOTUNE values
  virsh: Add _length parameters to virsh output

 docs/formatdomain.html.in  |  40 ++-
 docs/schemas/domaincommon.rng  |  38 +++
 include/libvirt/libvirt-domain.h   | 102 ++
 src/conf/domain_conf.c |  24 +-
 src/conf/domain_conf.h |   6 +
 src/qemu/qemu_capabilities.c   |   2 +
 src/qemu/qemu_capabilities.h   |   1 +
 src/qemu/qemu_command.c|  21 ++
 src/qemu/qemu_driver.c | 341 +++--
 src/qemu/qemu_monitor.c|   7 +-
 src/qemu/qemu_monitor.h|   3 +-
 src/qemu/qemu_monitor_json.c   |  39 ++-
 src/qemu/qemu_monitor_json.h   |   3 +-
 .../caps_2.6.0-gicv2.aarch64.xml   |   1 +
 .../caps_2.6.0-gicv3.aarch64.xml   |   1 +
 tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml  |   1 +
 tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |   1 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |   1 +
 tests/qemumonitorjsontest.c|  17 +-
 .../qemuxml2argv-blkdeviotune-max-length.args  |  34 ++
 .../qemuxml2argv-blkdeviotune-max-length.xml   |  65 
 tests/qemuxml2argvtest.c   |   4 +
 .../qemuxml2xmlout-blkdeviotune-max-length.xml |   1 +
 tests/qemuxml2xmltest.c|   1 +
 tools/virsh-domain.c   | 196 +---
 tools/virsh.pod|  21 ++
 26 files changed, 673 insertions(+), 298 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml
 create mode 12 
tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml

-- 
2.7.4

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

Re: [libvirt] RFC: Exposing "ready" bool (of `query-block-jobs`) or QMP BLOCK_JOB_READY event

[Eric Blake]

On 10/06/2016 10:06 AM, Kashyap Chamarthy wrote:
> On Thu, Oct 06, 2016 at 09:25:26AM -0500, Eric Blake wrote:
>> On 10/06/2016 06:34 AM, Peter Krempa wrote:
> 
> [...]
> 
>>> We expose the state of the copy job in the XML and forward the READY
>>> event from qemu to the users.
>>
>> I was not aware of that when I was chatting on IRC yesterday; that's
>> useful to know, because virDomainGetBlockJobInfo() is NOT exposing that
>> information at the moment.
> 
> That is what this RFC was asking to consider -- whether an [I think it
> has to be a new one] API should report.

I think we've answered this one - we don't need a new API, because
parsing the domain XML already provides the current 'ready':true/false
status from qemu.

The existing virDomainGetBlockJobInfo() can't be extended, but it can be
fixed to quit reporting cur==end when ready:false.

> 
> Given the above, I've re-opened the bug here:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1382165#c3

Thanks.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 13/18] qemu: assign nec-xhci (USB3) controller to a PCIe address when appropriate

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> The nec-usb-xhci device (which is a USB3 controller) has always
> presented itself as a PCI device when plugged into a legacy PCI slot,
> and a PCIe device when plugged into a PCIe slot, but libvirt has
> always auto-assigned it to a PCI slot.
> 
> This patch changes that behavior to auto-assign to a PCIe slot on
> systems that have pcie-root (e.g. Q35 and aarch64/virt).
> 
> Since we don't yet auto-create pcie-*-port controllers on demand, this
> means a config with an nec-xhci USB controller that has no PCI address
> assigned will also need to have an otherwise-unused pcie-*-port
> controller specified:
> 
>
>
> 
> (this assumes there is an otherwise-unused slot on pcie-root to accept
> the pcie-root-port)
> ---
>  src/qemu/qemu_domain_address.c |  3 ++
>  tests/qemuxml2argvdata/qemuxml2argv-autoindex.args | 10 +++
>  tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args  | 21 ++---
>  tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml   |  2 ++
>  .../qemuxml2argv-q35-virtio-pci.args   |  7 ++---
>  tests/qemuxml2argvtest.c   |  2 ++
>  .../qemuxml2xmlout-autoindex.xml   | 10 +++
>  .../qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml | 35 
>+-
>  .../qemuxml2xmlout-q35-virtio-pci.xml  | 21 +
>  9 files changed, 49 insertions(+), 62 deletions(-)

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] private_syms: add virLogFilterListFree to libvirt_private.syms

[Erik Skultety]

Commit 660468b1 forgot to add it, so let's add it now to prevent future linker
issues.

Signed-off-by: Erik Skultety 
---
Pushed under trivial rule

 src/libvirt_private.syms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c4af57d..923afd1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1872,6 +1872,7 @@ virLockSpaceReleaseResourcesForOwner;
 virLogDefineFilter;
 virLogDefineOutput;
 virLogFilterFree;
+virLogFilterListFree;
 virLogGetDefaultPriority;
 virLogGetFilters;
 virLogGetNbFilters;
-- 
2.5.5

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

Re: [libvirt] RFC: Exposing "ready" bool (of `query-block-jobs`) or QMP BLOCK_JOB_READY event

[John Snow]

On 10/06/2016 10:25 AM, Eric Blake wrote:

On 10/06/2016 06:34 AM, Peter Krempa wrote:


Currently libvirt block APIs (& consequently higher-level applications
like Nova which use these APIs) rely on polling for job completion via


libvirt is _not_ polling the data. Libvirt relies on the events from
qemu which are also exposed as libvirt events.


Libvirt is not the one deciding when to issue the pivot command, Nova
is.  Right now, Nova is polling (rather than waiting for events), and
its polling is solely conditional on cur==end rather than on the XML
addition of ready='true'.



We expose the state of the copy job in the XML and forward the READY
event from qemu to the users.


I was not aware of that when I was chatting on IRC yesterday; that's
useful to know, because virDomainGetBlockJobInfo() is NOT exposing that
information at the moment.


The documentation suggests that block jobs should listen to the events
and act accordingly only after receiving the event.


Yes, but the documentation ALSO states that waiting for cur==end is
SUPPOSED to work.  And it doesn't.


~

libvirt finds cur==end AND sends a pivot request, all in the window
before QEMU would have sent "ready": true field [emitted as part of the


This is not true. Libvirt checks that the mirror is actually ready. It's
done by the commit you've mentioned above.


In other words, Nova sees cur==end, and requests the pivot, but libvirt
is rejecting Nova's request because 'ready' is not true yet; and Nova
then gives up rather than trying again.




QMP `query-block-jobs` command's response, indicating that the job has
actually completed], however the pivot request fails because it requires
"ready": true.


The problem is that you are polling the block job info which correctly
reports that all data was properly copied and you are inferring the
block job state from that data.


But the problem here is that qemu is NOT accurately reporting data - it
is reporting cur==end with the promise that they are only equal if the
job is stable, WHEN THE JOB IS NOT STABLE.



Do we really promise that in QEMU? I guess since jobs have existed since 
before the ready field I guess we do...




I'm against deliberately reporting false data in the block info
structure.


We are NOT falsifying any information, any more than we are falsifying
information by changing cur/end to 0/1 when ready:false and qemu
reported 0/0.  (see commit 988218ca).



The application should register handlers for the block job events and
act only if it receives such event. Additionally you may want to check
that the state is correct in the XML. The current block job information
structure can't be extended unfortunately.


Yes, changing Nova to use event handlers is a good idea.  But I'm ALSO
in favor of fixing libvirt to work around the qemu bug, by intentionally
munging the output to state cur

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

Re: [libvirt] [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised

[Ian Jackson]

Martin Kletzander writes ("Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt 
save/restore when migration not advertised"):
> Well then, unfortunately you do.
> 
> Also, looking at how the code is structured, if you have live migration
> but don't have save/restore, you won't have  there
> at all.

Right.  OK, thanks.  I will add the patch below to my osstest queue.

Ian.

>From 5330ff9222e4e611505149945eef7dc074b4f9b5 Mon Sep 17 00:00:00 2001
In-Reply-To: <20161006104255.GP16414@wheatley>
References: <20161006104255.GP16414@wheatley>
From: Ian Jackson 
Date: Thu, 6 Oct 2016 17:38:29 +0100
Subject: [OSSTEST PATCH 3/2] libvirt: Check 
/capabilities/host/migration_features/live for live migration
Cc: libvir-list@redhat.com

libvirt is capable of advertising this separately from
/capabilities/host/migration_features, so if save/restore is supported
but live migration is not, this will do the right thing.

We would have preferred libvirt to advertise
  /capabilities/host/migration_features/save
or something, but it doesn't right now, so we continue to use
  /capabilities/host/migration_features
to detect save/restore support.

If libvirt changes its feature presentation, then at some future point
we should change osstest too.

Signed-off-by: Ian Jackson 
CC: Martin Kletzander 
CC: Jim Fehlig 
---
 Osstest/Toolstack/libvirt.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 250fe47..81e724d 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -93,7 +93,8 @@ sub migrate_check ($$) {
 # local migration is not supported
 $rc = 1;
 } else {
-   $rc = $self->check_capability('/capabilities/host/migration_features');
+   $rc = $self->check_capability
+   ('/capabilities/host/migration_features/live');
 }
 
 logm("rc=$rc");
-- 
2.1.4

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

Re: [libvirt] Analysis of the effect of adding PCIe root ports

[Laine Stump]

On 10/06/2016 11:20 AM, Richard W.M. Jones wrote:

On Thu, Oct 06, 2016 at 10:00:39AM -0400, Laine Stump wrote:

*  x 4 - a set of USB2
controllers. This will turn into a single USB3 controller on a
root-port after my patches. Alternately, since it seems you don't
use it, you could eliminate it with:

Yup, this is auto-added, and a mistake.

I have sent a patch upstream adding:


  
 

...

1) virtio-scsi controller
2) virtio-serial controller

and nothing else. Manually address those two to be on bus 0
(pcie-root), and (with my patches) you've reduced your PCI
device+controller count from the current 10 down to 3 (including the
sata controller).

Interesting.  Is there any particular reason why we should or should
not use explicit PCI addresses for the remaining devices?  What would
you recommend we do?


For 440fx it makes no difference. For Q35 it can eliminate the "extra 
PCI controllers" - current upstream libvirt will add a 
dmi-to-pci-bridge, then a pci-bridge plugged into that, and add your PCI 
devices to that; after my in-progress patches, libvirt will add a 
pcie-root-port for each virtio device, and plug them into those. If you 
manually address the devices to "some unused slot on bus 0", then they 
will be directly plugged into pcie-root, and no extra controllers will 
be needed.


As far as recommendations, I guess you could manually assign addresses 
for those two devices that would otherwise be open in both 440fx and 
Q35. Generally 00:1 (chipset devices) and 00:2 (video) are in use on a 
440fx domain, and 00:1 (video) and 00:1F (chipset devices) on a Q35. If 
you don't disable USB, then USB controllers will also be added at 00:3 
(?) on 440fx) and 00:1D on Q35; but you don't use USB so you don't need 
to worry about this. So you could just manually assign the virtio-scsi 
and virtio-serial devices to have these two addresses:


   
   

(domain and function default to '0' if not specified - in older libvirt 
they were required by the RNG, but otherwise optional, in new libvirt 
they're completely optional). That should work for both machinetypes and 
not cause any of the stupid outdated "you won't be able to add a video 
device in the future!" warnings.


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

Re: [libvirt] [PATCH v3.5 14/18] qemu: only force an available legacy-PCI slot on domains with pci-root

[Andrea Bolognani]

On Thu, 2016-10-06 at 12:04 -0400, Laine Stump wrote:
> > Since now you're using info exclusively inside this block,
> > you can move its declaration here as well.
> 
> Except that patch 19/18 uses it too (it was that patch that led to me 
> re-doing this one and posting a v3.5)

Nevermind then :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] Analysis of the effect of adding PCIe root ports

[Andrea Bolognani]

On Thu, 2016-10-06 at 11:57 -0400, Laine Stump wrote:
> (BTW, isn't there something wrt aarch64 about "no pci controllers in 
> config means use mmio for devices", or something like that? (Or maybe we 
> were just *thinking* about that and didn't actually do it, I don't 
> remember). Using the lack of PCI controllers in the config to imply 
> "automatically add necessary + extra controllers" would directly 
> conflict with that.)

We were just thinking about it. mach-virt guests use MMIO
addresses by default, but you can force specific devices
to use PCI instead by adding

  

to the relevant element.

How we will make it easy for users and management
applications to create pure PCIe mach-virt guests is
still up for debate :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] Analysis of the effect of adding PCIe root ports

[Daniel P. Berrange]

On Thu, Oct 06, 2016 at 11:57:17AM -0400, Laine Stump wrote:
> On 10/06/2016 11:31 AM, Daniel P. Berrange wrote:
> > On Thu, Oct 06, 2016 at 12:58:51PM +0200, Andrea Bolognani wrote:
> > > On Wed, 2016-10-05 at 18:36 +0100, Richard W.M. Jones wrote:
> > > > > > (b) It would be nice to turn the whole thing off for people who 
> > > > > > don't
> > > > > > care about / need hotplugging.
> > > > > I had contemplated having an "availablePCIeSlots" (or something like
> > > > > that) that was either an attribute of the config, or an option in
> > > > > qemu.conf or libvirtd.conf. If we had such a setting, it could be
> > > > > set to "0".
> > > I remember some pushback when this was proposed. Maybe we
> > > should just give up on the idea of providing spare
> > > hotpluggable PCIe slots by default and ask the user to add
> > > them explicitly after all.
> > > 
> > > > Note that changes to libvirt conf files are not usable by libguestfs.
> > > > The setting would need to go into the XML, and please also make it
> > > > possible to determine if $random version of libvirt supports the
> > > > setting, either by a version check or something in capabilities.
> > > Note that you can avoid using any PCIe root port at all by
> > > assigning PCI addresses manually. It looks like the overhead
> > > for the small (I'm assuming) number of devices a libguestfs
> > > appliance will use is low enough that you will probably not
> > > want to open that can of worm, though.
> > For most apps the performance impact of the PCI enumeration
> > is not a big deal. So having libvirt ensure there's enough
> > available hotpluggable PCIe slots is reasonable, as long as
> > we leave a get-out clause for libguestfs.
> > 
> > This could be as simple as declaring that *if* we see one
> > or more  in the input XML, then libvirt
> > will honour those and not try to add new controllers to the
> > guest.
> > 
> > That way, by default libvirt will just "do the right thing"
> > and auto-create a suitable number of controllers needed to
> > boot the guest.
> > 
> > Apps that want strict control though, can specify the
> >  elements themselves.  Libvirt can still
> > auto-assign device addresses onto these controllers.
> > It simply wouldn't add any further controllers itself
> > at that point. NB I'm talking cold-boot here. So libguestfs
> > would specify  itself to the minimal set it wants
> > to optimize its boot performance.
> 
> That works for the initial definition of the domain, but as soon as you've
> saved it once, there will be controllers explicitly in the config, and since
> we don't have any way of differentiating between auto-added controllers and
> those specifically requested by the user, we have to assume they were
> explicitly added, so such a check is then meaningless because you will
> *always* have PCI controllers.

Ok, so coldplug was probably the wrong word to use. What I actually
meant was "at time of initial define", since that's when libvirt
actually does its controller auto-creation. If you later add more
devices to the guest, whether it is online or offline, that libvirt
would still be auto-adding more controllers if required (and if
possible) . I was not expecting libvirt to remember whether we
were auto-adding controllers the first time or not.

> Say you create a domain definition with no controllers, you would get enough
> for the devices in the initial config, plus "N" more empty root ports. Let's
> say you then add 4 more devices (either hotplug or coldplug, doesn't
> matter). Those devices are placed on the existing unused pcie-root-ports.
> But now all your ports are full, and since you have PCI controllers in the
> config, libvirt is going to say "Ah, this user knows what they want to do,
> so I'm not going to add any extras! I'm so smart!". This would be especially
> maddening in the case of "coldplug", where libvirt could have easily added a
> new controller to accomodate the new device, but didn't.
> 
> Unless we don't care what happens after the initial definition (and then
> adding of "N" new devices), trying to behave properly purely based on
> whether or not there are any PCI controllers present in the config isn't
> going to work.

I think that's fine.

Lets stop talking about coldplug since that's very misleading.

What I mean is that...

1. When initially defining a guest

   If no controllers are present, auto-add controllers implied
   by the machine type, sufficient to deal with all currently
   listed devices, plus "N" extra spare ports.

   Else, simply assign devices to the controllers listed in
   the XML config. If there are no extra spare ports after
   doing this, so be it. It was the application's choice
   to have not listed enough controllers to allow later
   addition of more devices.


2. When adding further devices (whether to an offline or online
   guest)

   If there's not enough slots left, add further controllers
   to host the devices.  If there were not enough slots left
   to 

Re: [libvirt] [PATCH v3.5 14/18] qemu: only force an available legacy-PCI slot on domains with pci-root

[Laine Stump]

On 10/06/2016 11:39 AM, Andrea Bolognani wrote:

On Thu, 2016-09-29 at 10:14 -0400, Laine Stump wrote:

Andrea had the right idea when he disabled the "reserve an extra
unused slot" bit for aarch64/virt. For *any* PCI Express-based
machine, it is pointless since 1) an extra legacy-PCI slot can't be
used for hotplug, since hotplug into legacy PCI slots doesn't work on
PCI Express machinetypes, and 2) even for "coldplug" expansion,
everybody will want to expand using Express controllers, not legacy
PCI.
  
This patch eliminates the extra slot reserve unless the system has a

pci-root (i.e. legacy PCI)

[...]

@@ -1704,23 +1702,35 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
addrs) < 0)
   goto cleanup;
   
-for (i = 0; i < addrs->nbuses; i++) {

-if (!qemuDomainPCIBusFullyReserved(>buses[i]))
-buses_reserved = false;
-}
-
-/* Reserve 1 extra slot for a (potential) bridge only if buses
- * are not fully reserved yet.
+/* For domains that have pci-root, reserve 1 extra slot for a
+ * (potential) bridge (for future expansion) only if buses are
+ * not fully reserved yet (if all buses are fully reserved
+ * with manually/previously assigned addresses, any attempt to
+ * reserve an extra slot would fail anyway. But if all buses
+ * are *not* fully reserved, this extra reservation might push
+ * the config to add a new pci-bridge to plug into the final
+ * available slot, thus preserving the ability to expand)
*
- * We don't reserve the extra slot for aarch64 mach-virt guests
- * either because we want to be able to have pure virtio-mmio
- * guests, and reserving this slot would force us to add at least
- * a dmi-to-pci-bridge to an otherwise PCI-free topology
+ * We only do this for those domains that have pci-root, since
+ * those with pcie-root will usually want to expand using PCIe
+ * controllers, which we will do after assigning addresses for
+ * all *actual* devices.
*/
-if (!buses_reserved &&
-!qemuDomainMachineIsVirt(def) &&
-qemuDomainPCIAddressReserveNextSlot(addrs, ) < 0)
-goto cleanup;
+
+if (qemuDomainMachineHasPCIRoot(def)) {

Since now you're using info exclusively inside this block,
you can move its declaration here as well.


Except that patch 19/18 uses it too (it was that patch that led to me 
re-doing this one and posting a v3.5)


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

Re: [libvirt] Analysis of the effect of adding PCIe root ports

[Laine Stump]

On 10/06/2016 11:31 AM, Daniel P. Berrange wrote:

On Thu, Oct 06, 2016 at 12:58:51PM +0200, Andrea Bolognani wrote:

On Wed, 2016-10-05 at 18:36 +0100, Richard W.M. Jones wrote:

(b) It would be nice to turn the whole thing off for people who don't
care about / need hotplugging.
  
I had contemplated having an "availablePCIeSlots" (or something like

that) that was either an attribute of the config, or an option in
qemu.conf or libvirtd.conf. If we had such a setting, it could be
set to "0".

I remember some pushback when this was proposed. Maybe we
should just give up on the idea of providing spare
hotpluggable PCIe slots by default and ask the user to add
them explicitly after all.


Note that changes to libvirt conf files are not usable by libguestfs.
  
The setting would need to go into the XML, and please also make it

possible to determine if $random version of libvirt supports the
setting, either by a version check or something in capabilities.

Note that you can avoid using any PCIe root port at all by
assigning PCI addresses manually. It looks like the overhead
for the small (I'm assuming) number of devices a libguestfs
appliance will use is low enough that you will probably not
want to open that can of worm, though.

For most apps the performance impact of the PCI enumeration
is not a big deal. So having libvirt ensure there's enough
available hotpluggable PCIe slots is reasonable, as long as
we leave a get-out clause for libguestfs.

This could be as simple as declaring that *if* we see one
or more  in the input XML, then libvirt
will honour those and not try to add new controllers to the
guest.

That way, by default libvirt will just "do the right thing"
and auto-create a suitable number of controllers needed to
boot the guest.

Apps that want strict control though, can specify the
 elements themselves.  Libvirt can still
auto-assign device addresses onto these controllers.
It simply wouldn't add any further controllers itself
at that point. NB I'm talking cold-boot here. So libguestfs
would specify  itself to the minimal set it wants
to optimize its boot performance.


That works for the initial definition of the domain, but as soon as 
you've saved it once, there will be controllers explicitly in the 
config, and since we don't have any way of differentiating between 
auto-added controllers and those specifically requested by the user, we 
have to assume they were explicitly added, so such a check is then 
meaningless because you will *always* have PCI controllers.


Say you create a domain definition with no controllers, you would get 
enough for the devices in the initial config, plus "N" more empty root 
ports. Let's say you then add 4 more devices (either hotplug or 
coldplug, doesn't matter). Those devices are placed on the existing 
unused pcie-root-ports. But now all your ports are full, and since you 
have PCI controllers in the config, libvirt is going to say "Ah, this 
user knows what they want to do, so I'm not going to add any extras! I'm 
so smart!". This would be especially maddening in the case of 
"coldplug", where libvirt could have easily added a new controller to 
accomodate the new device, but didn't.


Unless we don't care what happens after the initial definition (and then 
adding of "N" new devices), trying to behave properly purely based on 
whether or not there are any PCI controllers present in the config isn't 
going to work.


(BTW, isn't there something wrt aarch64 about "no pci controllers in 
config means use mmio for devices", or something like that? (Or maybe we 
were just *thinking* about that and didn't actually do it, I don't 
remember). Using the lack of PCI controllers in the config to imply 
"automatically add necessary + extra controllers" would directly 
conflict with that.)


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

Re: [libvirt] [PATCH] virCgroupDetect: Print pid as long long

[Daniel P. Berrange]

On Thu, Oct 06, 2016 at 05:18:11PM +0200, Pavel Hrdina wrote:
> On Thu, Oct 06, 2016 at 04:58:40PM +0200, Michal Privoznik wrote:
> > Pids are signed, report them as such.
> > 
> > Before:
> > Detected mount/mapping 2:cpuset at /sys/fs/cgroup/cpuset in 
> > /machine/qemu-4-fedora.libvirt-qemu/vcpu0 for pid 18446744073709551615
> > 
> > After:
> > Detected mount/mapping 2:cpuset at /sys/fs/cgroup/cpuset in 
> > /machine/qemu-4-fedora.libvirt-qemu/vcpu0 for pid -1
> > 
> > Signed-off-by: Michal Privoznik 
> > ---
> >  src/util/vircgroup.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> > index 8b52816..0a06a8a 100644
> > --- a/src/util/vircgroup.c
> > +++ b/src/util/vircgroup.c
> > @@ -732,11 +732,12 @@ virCgroupDetect(virCgroupPtr group,
> >  return -1;
> >  }
> >  
> > -VIR_DEBUG("Detected mount/mapping %zu:%s at %s in %s for pid 
> > %llu", i,
> > +VIR_DEBUG("Detected mount/mapping %zu:%s at %s in %s for pid %lld",
> > +  i,
> >virCgroupControllerTypeToString(i),
> >group->controllers[i].mountPoint,
> >group->controllers[i].placement,
> > -  (unsigned long long)pid);
> > +  (long long) pid);
> >  }
> >  
> >  return 0;
> 
> Printing -1 instead of long number is better, but there are lot of
> other places where we print pids as unsigned:
> 
>   git grep "unsigned long long.*pid"
> 
> it would be worth to change them too.

And add a syntax-check rule to detect such casts.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH v3.5 14/18] qemu: only force an available legacy-PCI slot on domains with pci-root

[Andrea Bolognani]

On Thu, 2016-09-29 at 10:14 -0400, Laine Stump wrote:
> Andrea had the right idea when he disabled the "reserve an extra
> unused slot" bit for aarch64/virt. For *any* PCI Express-based
> machine, it is pointless since 1) an extra legacy-PCI slot can't be
> used for hotplug, since hotplug into legacy PCI slots doesn't work on
> PCI Express machinetypes, and 2) even for "coldplug" expansion,
> everybody will want to expand using Express controllers, not legacy
> PCI.
> 
> This patch eliminates the extra slot reserve unless the system has a
> pci-root (i.e. legacy PCI)

[...]
> @@ -1704,23 +1702,35 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>   addrs) < 0)
>  goto cleanup;
>  
> -for (i = 0; i < addrs->nbuses; i++) {
> -if (!qemuDomainPCIBusFullyReserved(>buses[i]))
> -buses_reserved = false;
> -}
> -
> -/* Reserve 1 extra slot for a (potential) bridge only if buses
> - * are not fully reserved yet.
> +/* For domains that have pci-root, reserve 1 extra slot for a
> + * (potential) bridge (for future expansion) only if buses are
> + * not fully reserved yet (if all buses are fully reserved
> + * with manually/previously assigned addresses, any attempt to
> + * reserve an extra slot would fail anyway. But if all buses
> + * are *not* fully reserved, this extra reservation might push
> + * the config to add a new pci-bridge to plug into the final
> + * available slot, thus preserving the ability to expand)
>   *
> - * We don't reserve the extra slot for aarch64 mach-virt guests
> - * either because we want to be able to have pure virtio-mmio
> - * guests, and reserving this slot would force us to add at least
> - * a dmi-to-pci-bridge to an otherwise PCI-free topology
> + * We only do this for those domains that have pci-root, since
> + * those with pcie-root will usually want to expand using PCIe
> + * controllers, which we will do after assigning addresses for
> + * all *actual* devices.
>   */
> -if (!buses_reserved &&
> -!qemuDomainMachineIsVirt(def) &&
> -qemuDomainPCIAddressReserveNextSlot(addrs, ) < 0)
> -goto cleanup;
> +
> +if (qemuDomainMachineHasPCIRoot(def)) {

Since now you're using info exclusively inside this block,
you can move its declaration here as well.

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] Analysis of the effect of adding PCIe root ports

[Daniel P. Berrange]

On Thu, Oct 06, 2016 at 12:58:51PM +0200, Andrea Bolognani wrote:
> On Wed, 2016-10-05 at 18:36 +0100, Richard W.M. Jones wrote:
> > > > (b) It would be nice to turn the whole thing off for people who don't
> > > > care about / need hotplugging.
> > > 
> > > I had contemplated having an "availablePCIeSlots" (or something like
> > > that) that was either an attribute of the config, or an option in
> > > qemu.conf or libvirtd.conf. If we had such a setting, it could be
> > > set to "0".
> 
> I remember some pushback when this was proposed. Maybe we
> should just give up on the idea of providing spare
> hotpluggable PCIe slots by default and ask the user to add
> them explicitly after all.
> 
> > Note that changes to libvirt conf files are not usable by libguestfs.
> > 
> > The setting would need to go into the XML, and please also make it
> > possible to determine if $random version of libvirt supports the
> > setting, either by a version check or something in capabilities.
> 
> Note that you can avoid using any PCIe root port at all by
> assigning PCI addresses manually. It looks like the overhead
> for the small (I'm assuming) number of devices a libguestfs
> appliance will use is low enough that you will probably not
> want to open that can of worm, though.

For most apps the performance impact of the PCI enumeration
is not a big deal. So having libvirt ensure there's enough
available hotpluggable PCIe slots is reasonable, as long as
we leave a get-out clause for libguestfs.

This could be as simple as declaring that *if* we see one
or more  in the input XML, then libvirt
will honour those and not try to add new controllers to the
guest.

That way, by default libvirt will just "do the right thing"
and auto-create a suitable number of controllers needed to
boot the guest.

Apps that want strict control though, can specify the
 elements themselves.  Libvirt can still
auto-assign device addresses onto these controllers.
It simply wouldn't add any further controllers itself
at that point. NB I'm talking cold-boot here. So libguestfs
would specify  itself to the minimal set it wants
to optimize its boot performance.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] Analysis of the effect of adding PCIe root ports

[Richard W.M. Jones]

On Thu, Oct 06, 2016 at 10:00:39AM -0400, Laine Stump wrote:
> *  x 4 - a set of USB2
> controllers. This will turn into a single USB3 controller on a
> root-port after my patches. Alternately, since it seems you don't
> use it, you could eliminate it with:

Yup, this is auto-added, and a mistake.

I have sent a patch upstream adding:

>  
> 

...
> 1) virtio-scsi controller
> 2) virtio-serial controller
> 
> and nothing else. Manually address those two to be on bus 0
> (pcie-root), and (with my patches) you've reduced your PCI
> device+controller count from the current 10 down to 3 (including the
> sata controller).

Interesting.  Is there any particular reason why we should or should
not use explicit PCI addresses for the remaining devices?  What would
you recommend we do?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

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

Re: [libvirt] [PATCH] virCgroupDetect: Print pid as long long

[Pavel Hrdina]

On Thu, Oct 06, 2016 at 04:58:40PM +0200, Michal Privoznik wrote:
> Pids are signed, report them as such.
> 
> Before:
> Detected mount/mapping 2:cpuset at /sys/fs/cgroup/cpuset in 
> /machine/qemu-4-fedora.libvirt-qemu/vcpu0 for pid 18446744073709551615
> 
> After:
> Detected mount/mapping 2:cpuset at /sys/fs/cgroup/cpuset in 
> /machine/qemu-4-fedora.libvirt-qemu/vcpu0 for pid -1
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/vircgroup.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 8b52816..0a06a8a 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -732,11 +732,12 @@ virCgroupDetect(virCgroupPtr group,
>  return -1;
>  }
>  
> -VIR_DEBUG("Detected mount/mapping %zu:%s at %s in %s for pid %llu", 
> i,
> +VIR_DEBUG("Detected mount/mapping %zu:%s at %s in %s for pid %lld",
> +  i,
>virCgroupControllerTypeToString(i),
>group->controllers[i].mountPoint,
>group->controllers[i].placement,
> -  (unsigned long long)pid);
> +  (long long) pid);
>  }
>  
>  return 0;

Printing -1 instead of long number is better, but there are lot of
other places where we print pids as unsigned:

  git grep "unsigned long long.*pid"

it would be worth to change them too.

Pavel


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] RFC: Exposing "ready" bool (of `query-block-jobs`) or QMP BLOCK_JOB_READY event

[Kashyap Chamarthy]

On Thu, Oct 06, 2016 at 09:25:26AM -0500, Eric Blake wrote:
> On 10/06/2016 06:34 AM, Peter Krempa wrote:

[...]

> > We expose the state of the copy job in the XML and forward the READY
> > event from qemu to the users.
> 
> I was not aware of that when I was chatting on IRC yesterday; that's
> useful to know, because virDomainGetBlockJobInfo() is NOT exposing that
> information at the moment.

That is what this RFC was asking to consider -- whether an [I think it
has to be a new one] API should report.

> > The documentation suggests that block jobs should listen to the events
> > and act accordingly only after receiving the event.
> 
> Yes, but the documentation ALSO states that waiting for cur==end is
> SUPPOSED to work.  And it doesn't.

Yes.

> >> libvirt finds cur==end AND sends a pivot request, all in the window
> >> before QEMU would have sent "ready": true field [emitted as part of the
> > 
> > This is not true. Libvirt checks that the mirror is actually ready. It's
> > done by the commit you've mentioned above.
> 
> In other words, Nova sees cur==end, and requests the pivot, but libvirt
> is rejecting Nova's request because 'ready' is not true yet; and Nova
> then gives up rather than trying again.

Indeed ^ (I made this correction in my other response.)

> >> QMP `query-block-jobs` command's response, indicating that the job has
> >> actually completed], however the pivot request fails because it requires
> >> "ready": true.
> > 
> > The problem is that you are polling the block job info which correctly
> > reports that all data was properly copied and you are inferring the
> > block job state from that data.
> 
> But the problem here is that qemu is NOT accurately reporting data - it
> is reporting cur==end with the promise that they are only equal if the
> job is stable, WHEN THE JOB IS NOT STABLE.

That's precisely the source of the confusion for Nova here.
 
> > I'm against deliberately reporting false data in the block info
> > structure.
> 
> We are NOT falsifying any information, any more than we are falsifying
> information by changing cur/end to 0/1 when ready:false and qemu
> reported 0/0.  (see commit 988218ca).

Indeed, it seems inconsistent to allow it in one case (like the above
commit ID) to adjust (& _not_ falsify, as you accurately point out)
libvirt reporting, but not the other case (cur==end, "ready": false case
when cur != 0).

> > The application should register handlers for the block job events and
> > act only if it receives such event. Additionally you may want to check
> > that the state is correct in the XML. The current block job information
> > structure can't be extended unfortunately.
> 
> Yes, changing Nova to use event handlers is a good idea.  But I'm ALSO
> in favor of fixing libvirt to work around the qemu bug, by intentionally
> munging the output to state cur qemu reports ready:false.

Given the above, I've re-opened the bug here:

https://bugzilla.redhat.com/show_bug.cgi?id=1382165#c3




-- 
/kashyap

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

[libvirt] [PATCH] virCgroupDetect: Print pid as long long

[Michal Privoznik]

Pids are signed, report them as such.

Before:
Detected mount/mapping 2:cpuset at /sys/fs/cgroup/cpuset in 
/machine/qemu-4-fedora.libvirt-qemu/vcpu0 for pid 18446744073709551615

After:
Detected mount/mapping 2:cpuset at /sys/fs/cgroup/cpuset in 
/machine/qemu-4-fedora.libvirt-qemu/vcpu0 for pid -1

Signed-off-by: Michal Privoznik 
---
 src/util/vircgroup.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 8b52816..0a06a8a 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -732,11 +732,12 @@ virCgroupDetect(virCgroupPtr group,
 return -1;
 }
 
-VIR_DEBUG("Detected mount/mapping %zu:%s at %s in %s for pid %llu", i,
+VIR_DEBUG("Detected mount/mapping %zu:%s at %s in %s for pid %lld",
+  i,
   virCgroupControllerTypeToString(i),
   group->controllers[i].mountPoint,
   group->controllers[i].placement,
-  (unsigned long long)pid);
+  (long long) pid);
 }
 
 return 0;
-- 
2.8.4

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

Re: [libvirt] RFC: Exposing "ready" bool (of `query-block-jobs`) or QMP BLOCK_JOB_READY event

[Eric Blake]

On 10/06/2016 06:34 AM, Peter Krempa wrote:

>> Currently libvirt block APIs (& consequently higher-level applications
>> like Nova which use these APIs) rely on polling for job completion via
> 
> libvirt is _not_ polling the data. Libvirt relies on the events from
> qemu which are also exposed as libvirt events.

Libvirt is not the one deciding when to issue the pivot command, Nova
is.  Right now, Nova is polling (rather than waiting for events), and
its polling is solely conditional on cur==end rather than on the XML
addition of ready='true'.

> 
> We expose the state of the copy job in the XML and forward the READY
> event from qemu to the users.

I was not aware of that when I was chatting on IRC yesterday; that's
useful to know, because virDomainGetBlockJobInfo() is NOT exposing that
information at the moment.

> The documentation suggests that block jobs should listen to the events
> and act accordingly only after receiving the event.

Yes, but the documentation ALSO states that waiting for cur==end is
SUPPOSED to work.  And it doesn't.

>> ~
>>
>> libvirt finds cur==end AND sends a pivot request, all in the window
>> before QEMU would have sent "ready": true field [emitted as part of the
> 
> This is not true. Libvirt checks that the mirror is actually ready. It's
> done by the commit you've mentioned above.

In other words, Nova sees cur==end, and requests the pivot, but libvirt
is rejecting Nova's request because 'ready' is not true yet; and Nova
then gives up rather than trying again.

> 
>> QMP `query-block-jobs` command's response, indicating that the job has
>> actually completed], however the pivot request fails because it requires
>> "ready": true.
> 
> The problem is that you are polling the block job info which correctly
> reports that all data was properly copied and you are inferring the
> block job state from that data.

But the problem here is that qemu is NOT accurately reporting data - it
is reporting cur==end with the promise that they are only equal if the
job is stable, WHEN THE JOB IS NOT STABLE.

> 
> I'm against deliberately reporting false data in the block info
> structure.

We are NOT falsifying any information, any more than we are falsifying
information by changing cur/end to 0/1 when ready:false and qemu
reported 0/0.  (see commit 988218ca).

> 
> The application should register handlers for the block job events and
> act only if it receives such event. Additionally you may want to check
> that the state is correct in the XML. The current block job information
> structure can't be extended unfortunately.

Yes, changing Nova to use event handlers is a good idea.  But I'm ALSO
in favor of fixing libvirt to work around the qemu bug, by intentionally
munging the output to state curhttp://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 12/18] qemu: assign e1000e network devices to PCIe slots when appropriate

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> The e1000e is an emulated network device based on the Intel 82574,
> present in qemu 2.7.0 and later. Among other differences from the
> e1000, it presents itself as a PCIe device rather than legacy PCI. In
> order to get it assigned to a PCIe controller, this patch updates the
> flags setting for network devices when the model name is "e1000e".
> 
> (Note that for some reason libvirt has never validated the network
> device model names other than to check that there are no dangerous
> characters in them. That should probably change, but is the subject of
> another patch.)
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1343094

[...]
> @@ -468,6 +468,8 @@ 
> qemuDomainDeviceConnectFlagsInternal(virDomainDeviceDefPtr dev,
>  flags = 0;
>  else if (STREQ(net->model, "virtio"))
>  flags = virtioFlags;
> +else if (STREQ(net->model, "e1000e"))
> +flags = pcieFlags;

pcieFlags should no longer be marked as ATTRIBUTE_UNUSED
in the function declaration after this change.

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] qemu_process: add pid of vm in domain log

[Michal Privoznik]

On 28.09.2016 13:43, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> Add pid of VM in domain log.
> We used to show this info in debug log.
> 
> For example:
> If a process send SIGKILL to a qemu process,
> we could find something in audit logs.
> Then the pid of VM in domain log will be helpful.
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  src/qemu/qemu_process.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 27d04a4..8510a89 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5392,11 +5392,14 @@ qemuProcessLaunch(virConnectPtr conn,
> _("Domain %s didn't show up"), vm->def->name);
>  rv = -1;
>  }
> -VIR_DEBUG("QEMU vm=%p name=%s running with pid=%llu",
> -  vm, vm->def->name, (unsigned long long)vm->pid);
> +qemuDomainLogContextWrite(logCtxt,
> +  "QEMU vm=%p name=%s running with 
> pid=%llu\n",
> +  vm,
> +  vm->def->name,
> +  (unsigned long long)vm->pid);
>  } else {
> -VIR_DEBUG("QEMU vm=%p name=%s failed to spawn",
> -  vm, vm->def->name);
> +qemuDomainLogContextWrite(logCtxt, "QEMU vm=%p name=%s failed to 
> spawn",
> +  vm, vm->def->name);
>  }
>  
>  VIR_DEBUG("Writing early domain status to disk");
> 

Can't we have both?

Michal

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

Re: [libvirt] [PATCH] qemu_process: add pid of vm in domain log

[Daniel P. Berrange]

On Thu, Oct 06, 2016 at 04:29:46PM +0200, Michal Privoznik wrote:
> On 28.09.2016 13:43, Chen Hanxiao wrote:
> > From: Chen Hanxiao 
> > 
> > Add pid of VM in domain log.
> > We used to show this info in debug log.
> > 
> > For example:
> > If a process send SIGKILL to a qemu process,
> > we could find something in audit logs.
> > Then the pid of VM in domain log will be helpful.
> > 
> > Signed-off-by: Chen Hanxiao 
> > ---
> >  src/qemu/qemu_process.c | 11 +++
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 27d04a4..8510a89 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -5392,11 +5392,14 @@ qemuProcessLaunch(virConnectPtr conn,
> > _("Domain %s didn't show up"), vm->def->name);
> >  rv = -1;
> >  }
> > -VIR_DEBUG("QEMU vm=%p name=%s running with pid=%llu",
> > -  vm, vm->def->name, (unsigned long long)vm->pid);
> > +qemuDomainLogContextWrite(logCtxt,
> > +  "QEMU vm=%p name=%s running with 
> > pid=%llu\n",
> > +  vm,
> > +  vm->def->name,
> > +  (unsigned long long)vm->pid);
> >  } else {
> > -VIR_DEBUG("QEMU vm=%p name=%s failed to spawn",
> > -  vm, vm->def->name);
> > +qemuDomainLogContextWrite(logCtxt, "QEMU vm=%p name=%s failed to 
> > spawn",
> > +  vm, vm->def->name);
> >  }
> >  
> >  VIR_DEBUG("Writing early domain status to disk");
> > 
> 
> Can't we have both?

IMHO writing to the QEMU log like this after QEMU is running is not
a good idea. Once QEMU is running we only want QEMU writing to this
log. We could be reporting errors QEMU prints in this log shortly and
don't want those mixed up with this message written by libvirt.

These log files really aren't intended as a general audit trail of
everything that's done by libvirt. If you want that info look at the
audit log stream where libvirt is already emitting the PID information
and much more, in a structured format.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH v3 11/18] qemu: assign virtio devices to PCIe slot when appropriate

[Laine Stump]

On 10/06/2016 10:13 AM, Andrea Bolognani wrote:

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:

libvirt previously assigned nearly all devices to a "hotpluggable"
legacy PCI slot even on machines with a PCIe root bus (and even though
most such machines don't even support hotplug on legacy PCI slots!)
Forcing all devices onto legacy PCI slots means that the domain will
need a dmi-to-pci-bridge (to convert from PCIe to legacy PCI) and a
pci-bridge (to provide hotpluggable legacy PCI slots which, again,
usually aren't hotpluggable anyway).
  
To help reduce the need for these legacy controllers, this patch tries

to assign virtio-1.0-capable devices to PCIe slots whenever possible,
by setting appropriate connectFlags in
virDomainDeviceConnectFlagsInternal(). Happily, when that function was
written (just a few commits ago) it was created with a "virtioFlags"
argument, set by both of its callers, which is the proper connectFlags
to set for any virtio-*-pci device - depending on the arch/machinetype
of the domain, and whether or not the qemu binary supports virtio-1.0,
that flag will have either been set to PCI or PCIE. This patch merely
enables the functionality by setting the flags for the device to
whatever is in virtioFlags if the device is a virtio-*-pci device.
  
NB: since the slot must be hotpluggable, and pcie-root (the PCIe root

complex) does *not* support hotplug, this means that suitable
controllers must also be in the config (i.e. either pcie-root-port, or
pcie-downstream-port). For now, libvirt doesn't add those
automatically, so if you put virtio devices in a config for a qemu
that has PCIe-capable virtio devices, you'll need to add extra
pcie-root-ports yourself. That requirement will be eliminated in a
future patch, but for now, it's simple to do this:
  
 

 
 
 ...
  
Partially Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1330024

[...]


+
+  
+  

I was initially baffled by this, because I expected it to
be assigned to one of the available pcie-root-ports just
like all the other virtio devices.

However, according to qemuDomainValidateDevicePCISlotsQ35()
this is intentional, so I guess we're good :)


Actually, you bring up an interesting point in light of the "Should PCIe 
devices ever be placed directly on pcie-root?" debate on qemu-devel (I 
think it was in the thread about the PCI topology document that Marcel 
is writing). We currently always put the primary video device at 00:1 
just because "we always have", and it has the nice side effect of 
eliminating the need for legacy-PCI controllers. But in this one case 
the device is PCIe - to follow Marcel's recommendation of putting only 
legacy devices on pcie-root, we should be putting the virtio video 
device on a root-port.


As I recall, Marcel and Alex were the most vocal on this subject, so I'm 
Cc'ing them, with this bit of context - this patch auto-assigns the 
addresses for virtio devices to be on Express ports rather than legacy 
slots when appropriate, but there is a bit of Q35-specific code that 
overrides any of that and always places the primary video device at 
00:01.0 - should we still do that for the primary video if it's virtio? 
Or should we put it behind a root-port?





One improvement I can recommend to this test case is to add
one more pcie-root-port, so that it becomes quite clear
that virtio-vga has been assigned to pcie-root on purpose
and *not* due to a lack of pcie-root-ports.


Or possibly change the behavior for virtio video even if it's the 
primary (note to self - check if someone has done the patch to allow 
multiple virtio video devices yet)


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

Re: [libvirt] [PATCH v3 11/18] qemu: assign virtio devices to PCIe slot when appropriate

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> libvirt previously assigned nearly all devices to a "hotpluggable"
> legacy PCI slot even on machines with a PCIe root bus (and even though
> most such machines don't even support hotplug on legacy PCI slots!)
> Forcing all devices onto legacy PCI slots means that the domain will
> need a dmi-to-pci-bridge (to convert from PCIe to legacy PCI) and a
> pci-bridge (to provide hotpluggable legacy PCI slots which, again,
> usually aren't hotpluggable anyway).
> 
> To help reduce the need for these legacy controllers, this patch tries
> to assign virtio-1.0-capable devices to PCIe slots whenever possible,
> by setting appropriate connectFlags in
> virDomainDeviceConnectFlagsInternal(). Happily, when that function was
> written (just a few commits ago) it was created with a "virtioFlags"
> argument, set by both of its callers, which is the proper connectFlags
> to set for any virtio-*-pci device - depending on the arch/machinetype
> of the domain, and whether or not the qemu binary supports virtio-1.0,
> that flag will have either been set to PCI or PCIE. This patch merely
> enables the functionality by setting the flags for the device to
> whatever is in virtioFlags if the device is a virtio-*-pci device.
> 
> NB: since the slot must be hotpluggable, and pcie-root (the PCIe root
> complex) does *not* support hotplug, this means that suitable
> controllers must also be in the config (i.e. either pcie-root-port, or
> pcie-downstream-port). For now, libvirt doesn't add those
> automatically, so if you put virtio devices in a config for a qemu
> that has PCIe-capable virtio devices, you'll need to add extra
> pcie-root-ports yourself. That requirement will be eliminated in a
> future patch, but for now, it's simple to do this:
> 
>
>
>
>...
> 
> Partially Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1330024

[...]
> @@ -475,20 +482,26 @@ 
> qemuDomainDeviceConnectFlagsInternal(virDomainDeviceDefPtr dev,
>  break;
>  
>  case VIR_DOMAIN_DEVICE_DISK:
> -if (dev->data.disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
> -flags = 0; /* only virtio disks use PCI */
> +if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
> +flags =  virtioFlags;

Double space.

[...]
> @@ -1692,6 +1692,51 @@ mymain(void)
>  QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1,
>  QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
>  QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
> +/* verify that devices with pcie capability are assigned to a pcie slot 
> */
> +DO_TEST("q35-pcie",
> +QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
> +QEMU_CAPS_DEVICE_VIRTIO_RNG,
> +QEMU_CAPS_OBJECT_RNG_RANDOM,
> +QEMU_CAPS_NETDEV,
> +QEMU_CAPS_DEVICE_VIRTIO_NET,
> +QEMU_CAPS_DEVICE_VIRTIO_GPU,
> +QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL,
> +QEMU_CAPS_VIRTIO_KEYBOARD,
> +QEMU_CAPS_VIRTIO_MOUSE,
> +QEMU_CAPS_VIRTIO_TABLET,
> +QEMU_CAPS_VIRTIO_INPUT_HOST,
> +QEMU_CAPS_VIRTIO_SCSI,
> +QEMU_CAPS_FSDEV,
> +QEMU_CAPS_FSDEV_WRITEOUT,
> +QEMU_CAPS_DEVICE_PCI_BRIDGE,
> +QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> +QEMU_CAPS_DEVICE_IOH3420,
> +QEMU_CAPS_ICH9_AHCI,
> +QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1,
> +QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
> +/* same XML as q35-pcie, but don't set QEMU_CAPS_VIRTIO_PCI_LEGACY,

s/_PCI_LEGACY/_PCI_DISABLE_LEGACY/

Same in qemuxml2xmltest.

[...]
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml 
> b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml
> new file mode 100644
> index 000..60e29b5
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml
> @@ -0,0 +1,149 @@
> +
> +  q35-test
> +  11dbdcdd-4c3b-482b-8903-9bdb8c0a2774
> +  2097152
> +  2097152
> +  2
> +  
> +hvm
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +/usr/libexec/qemu-kvm
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   function='0x0'/>
> +
> +
> +  
> +  
> +   

Re: [libvirt] [PATCH] m4: Drop PKG_PROG_PKG_CONFIG compatibility code

[Michal Privoznik]

On 06.10.2016 13:30, Andrea Bolognani wrote:
> This was needed for RHEL 4 vintage distributions, which we
> haven't supported for a long time now.
> ---
>  m4/virt-pkgconfig-back-compat.m4 | 23 ---
>  1 file changed, 23 deletions(-)
>  delete mode 100644 m4/virt-pkgconfig-back-compat.m4
> 
> diff --git a/m4/virt-pkgconfig-back-compat.m4 
> b/m4/virt-pkgconfig-back-compat.m4
> deleted file mode 100644
> index 7eab84b..000
> --- a/m4/virt-pkgconfig-back-compat.m4
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -dnl
> -dnl To support the old pkg-config from RHEL4 vintage, we need
> -dnl to define the PKG_PROG_PKG_CONFIG macro if its not already
> -dnl present
> -m4_ifndef([PKG_PROG_PKG_CONFIG],
> -  [AC_DEFUN([PKG_PROG_PKG_CONFIG],
> -[m4_pattern_forbid([^_?PKG_[A-Z_]+$])
> -m4_pattern_allow([^PKG_CONFIG(_PATH)?$])
> -AC_ARG_VAR([PKG_CONFIG], [path to pkg-config utility])dnl
> -if test "x$ac_cv_env_PKG_CONFIG_set" != "xset"; then
> -AC_PATH_TOOL([PKG_CONFIG], [pkg-config])
> -fi
> -if test -n "$PKG_CONFIG"; then
> -_pkg_min_version=m4_default([$1], [0.9.0])
> -AC_MSG_CHECKING([pkg-config is at least version $_pkg_min_version])
> -if $PKG_CONFIG --atleast-pkgconfig-version $_pkg_min_version; then
> -AC_MSG_RESULT([yes])
> -else
> -AC_MSG_RESULT([no])
> -PKG_CONFIG=""
> -fi
> -fi[]dnl
> -])])
> 

ACK

Michal

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

Re: [libvirt] Analysis of the effect of adding PCIe root ports

[Laine Stump]

On 10/06/2016 06:58 AM, Andrea Bolognani wrote:

On Wed, 2016-10-05 at 18:36 +0100, Richard W.M. Jones wrote:

(b) It would be nice to turn the whole thing off for people who don't
care about / need hotplugging.
  
I had contemplated having an "availablePCIeSlots" (or something like

that) that was either an attribute of the config, or an option in
qemu.conf or libvirtd.conf. If we had such a setting, it could be
set to "0".

I remember some pushback when this was proposed. Maybe we
should just give up on the idea of providing spare
hotpluggable PCIe slots by default and ask the user to add
them explicitly after all.



We need to have *something* new in the config related to this, otherwise 
it will be impossible to add new "available for hotplug" ports at the 
same time as new unaddressed endpoint devices are added (unless an extra 
port is added for each endpoint device as well, and of course that is 
assuming that the user knows about things like auto-added memory balloon 
devices, and which devices are going to be PCIe and which will be legacy 
PCI, etc). If the "available" ports aren't somehow marked, the endpoint 
devices will end up being placed on them.






Note that changes to libvirt conf files are not usable by libguestfs.
  
The setting would need to go into the XML, and please also make it

possible to determine if $random version of libvirt supports the
setting, either by a version check or something in capabilities.

Note that you can avoid using any PCIe root port at all by
assigning PCI addresses manually. It looks like the overhead
for the small (I'm assuming) number of devices a libguestfs
appliance will use is low enough that you will probably not
want to open that can of worm, though.


I looked in an example domain config Rich pastebin-ed in IRC yesterday. 
These are the PCI devices he has:


*  - currently put on a 
pci-bridge, but will end up on a root-port after my patches


*  x 4 - a set of USB2 controllers. 
This will turn into a single USB3 controller on a root-port after my 
patches. Alternately, since it seems you don't use it, you could 
eliminate it with:


 

*  - this is fixed on 00:1f.2, and there's 
nothing we can do to get rid of it or even change its address


*  - the need for this 
will be eliminated by my patches


*  - also eliminated

*  - currently on the pci-bridge, but 
will move to a root-port after my patches


*  - Rich, do you actually use this device? 
Seems doubtful. If not, then add




 to your config to prevent it.

After getting rid of the controllers that will be eliminated by my 
patches, and the "default" devices that you can get rid of with manual 
intervention (model='none'), you'll be left with the following devices 
on pcie-root-ports which *could* be manually moved to pcie-root (thus 
eliminating one pcie-root-port from the domain):


1) virtio-scsi controller
2) virtio-serial controller

and nothing else. Manually address those two to be on bus 0 (pcie-root), 
and (with my patches) you've reduced your PCI device+controller count 
from the current 10 down to 3 (including the sata controller).


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

[libvirt] [PATCH v2 10/14] qemuBuildVhostuserCommandLine: Reuse qemuBuildChrChardevStr

[Michal Privoznik]

There's no need to reinvent the wheel here. We already have a
function to format virDomainChrSourceDefPtr. It's called
qemuBuildChrChardevStr(). Use that instead of some dummy
virBufferAsprintf().

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 71067ee..246ffe0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7828,7 +7828,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
   virQEMUCapsPtr qemuCaps,
   unsigned int bootindex)
 {
-virBuffer chardev_buf = VIR_BUFFER_INITIALIZER;
+char *chardev = NULL;
 virBuffer netdev_buf = VIR_BUFFER_INITIALIZER;
 unsigned int queues = net->driver.virtio.queues;
 char *nic = NULL;
@@ -7841,9 +7841,10 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
 
 switch ((virDomainChrType) net->data.vhostuser->type) {
 case VIR_DOMAIN_CHR_TYPE_UNIX:
-virBufferAsprintf(_buf, "socket,id=char%s,path=%s%s",
-  net->info.alias, net->data.vhostuser->data.nix.path,
-  net->data.vhostuser->data.nix.listen ? ",server" : 
"");
+if (!(chardev = qemuBuildChrChardevStr(NULL, NULL, NULL, def,
+   net->data.vhostuser,
+   net->info.alias, qemuCaps, 
false)))
+goto error;
 break;
 
 case VIR_DOMAIN_CHR_TYPE_NULL:
@@ -7861,7 +7862,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
 case VIR_DOMAIN_CHR_TYPE_LAST:
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("vhost-user type '%s' not supported"),
-virDomainChrTypeToString(net->data.vhostuser->type));
+   virDomainChrTypeToString(net->data.vhostuser->type));
 goto error;
 }
 
@@ -7879,7 +7880,8 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
 }
 
 virCommandAddArg(cmd, "-chardev");
-virCommandAddArgBuffer(cmd, _buf);
+virCommandAddArg(cmd, chardev);
+VIR_FREE(chardev);
 
 virCommandAddArg(cmd, "-netdev");
 virCommandAddArgBuffer(cmd, _buf);
@@ -7897,7 +7899,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
 return 0;
 
  error:
-virBufferFreeAndReset(_buf);
+VIR_FREE(chardev);
 virBufferFreeAndReset(_buf);
 VIR_FREE(nic);
 
-- 
2.8.4

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

[libvirt] [PATCH v2 13/14] qemuBuildHostNetStr: Support VIR_DOMAIN_NET_TYPE_VHOSTUSER

[Michal Privoznik]

https://bugzilla.redhat.com/show_bug.cgi?id=1366505

So far, this function lacked support for
VIR_DOMAIN_NET_TYPE_VHOSTUSER leaving callers to hack around the
problem by constructing the command line on their own. This is
not ideal as it blocks hot plug support.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c| 38 +-
 .../qemuxml2argv-net-vhostuser-multiq.args |  6 ++--
 .../qemuxml2argv-net-vhostuser.args|  4 +--
 3 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index e8d4d8d..95e40de 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3721,7 +3721,13 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
 return NULL;
 
 case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
-/* Unsupported yet. */
+virBufferAsprintf(, "vhost-user%cchardev=char%s",
+  type_sep,
+  net->info.alias);
+type_sep = ',';
+if (net->driver.virtio.queues > 1)
+virBufferAsprintf(, ",queues=%u",
+  net->driver.virtio.queues);
 break;
 
 case VIR_DOMAIN_NET_TYPE_LAST:
@@ -7832,7 +7838,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
 {
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 char *chardev = NULL;
-virBuffer netdev_buf = VIR_BUFFER_INITIALIZER;
+char *netdev = NULL;
 unsigned int queues = net->driver.virtio.queues;
 char *nic = NULL;
 
@@ -7869,25 +7875,27 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
 goto error;
 }
 
-virBufferAsprintf(_buf, "vhost-user,id=host%s,chardev=char%s",
-  net->info.alias, net->info.alias);
-
-if (queues > 1) {
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("multi-queue is not supported for vhost-user "
- "with this QEMU binary"));
-goto error;
-}
-virBufferAsprintf(_buf, ",queues=%u", queues);
+if (queues > 1 &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("multi-queue is not supported for vhost-user "
+ "with this QEMU binary"));
+goto error;
 }
 
+if (!(netdev = qemuBuildHostNetStr(net, driver,
+   ',', -1,
+   NULL, 0, NULL, 0)))
+goto error;
+
+
 virCommandAddArg(cmd, "-chardev");
 virCommandAddArg(cmd, chardev);
 VIR_FREE(chardev);
 
 virCommandAddArg(cmd, "-netdev");
-virCommandAddArgBuffer(cmd, _buf);
+virCommandAddArg(cmd, netdev);
+VIR_FREE(netdev);
 
 if (!(nic = qemuBuildNicDevStr(def, net, -1, bootindex,
queues, qemuCaps))) {
@@ -7904,8 +7912,8 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
 
  error:
 virObjectUnref(cfg);
+VIR_FREE(netdev);
 VIR_FREE(chardev);
-virBufferFreeAndReset(_buf);
 VIR_FREE(nic);
 
 return -1;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args 
b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args
index 4360e5e..59929c1 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args
@@ -20,17 +20,17 @@ QEMU_AUDIO_DRV=none \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
 -chardev socket,id=charnet0,path=/tmp/vhost0.sock,server \
--netdev vhost-user,id=hostnet0,chardev=charnet0 \
+-netdev vhost-user,chardev=charnet0,id=hostnet0 \
 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:ee:96:6b,bus=pci.0,\
 addr=0x3 \
 -chardev socket,id=charnet1,path=/tmp/vhost1.sock \
--netdev vhost-user,id=hostnet1,chardev=charnet1 \
+-netdev vhost-user,chardev=charnet1,id=hostnet1 \
 -device 
virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:ee:96:6c,bus=pci.0,\
 addr=0x4 \
 -netdev socket,listen=:2015,id=hostnet2 \
 -device rtl8139,netdev=hostnet2,id=net2,mac=52:54:00:95:db:c0,bus=pci.0,\
 addr=0x5 \
 -chardev socket,id=charnet3,path=/tmp/vhost2.sock \
--netdev vhost-user,id=hostnet3,chardev=charnet3,queues=4 \
+-netdev vhost-user,chardev=charnet3,queues=4,id=hostnet3 \
 -device virtio-net-pci,mq=on,vectors=10,netdev=hostnet3,id=net3,\
 mac=52:54:00:ee:96:6d,bus=pci.0,addr=0x6
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args 
b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args
index 47c1d84..e15d735 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args

[libvirt] [PATCH v2 12/14] qemuBuildVhostuserCommandLine: Unify -netdev creation

[Michal Privoznik]

Currently, what we do for vhost-user network is generate the
following part of command line:

-netdev type=vhost-user,id=hostnet0,chardev=charnet0

There's no need for 'type=' it is the default. Drop it.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c   | 2 +-
 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args | 6 +++---
 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args| 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 895bb23..e8d4d8d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7869,7 +7869,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
 goto error;
 }
 
-virBufferAsprintf(_buf, "type=vhost-user,id=host%s,chardev=char%s",
+virBufferAsprintf(_buf, "vhost-user,id=host%s,chardev=char%s",
   net->info.alias, net->info.alias);
 
 if (queues > 1) {
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args 
b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args
index bab15ad..4360e5e 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args
@@ -20,17 +20,17 @@ QEMU_AUDIO_DRV=none \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
 -chardev socket,id=charnet0,path=/tmp/vhost0.sock,server \
--netdev type=vhost-user,id=hostnet0,chardev=charnet0 \
+-netdev vhost-user,id=hostnet0,chardev=charnet0 \
 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:ee:96:6b,bus=pci.0,\
 addr=0x3 \
 -chardev socket,id=charnet1,path=/tmp/vhost1.sock \
--netdev type=vhost-user,id=hostnet1,chardev=charnet1 \
+-netdev vhost-user,id=hostnet1,chardev=charnet1 \
 -device 
virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:ee:96:6c,bus=pci.0,\
 addr=0x4 \
 -netdev socket,listen=:2015,id=hostnet2 \
 -device rtl8139,netdev=hostnet2,id=net2,mac=52:54:00:95:db:c0,bus=pci.0,\
 addr=0x5 \
 -chardev socket,id=charnet3,path=/tmp/vhost2.sock \
--netdev type=vhost-user,id=hostnet3,chardev=charnet3,queues=4 \
+-netdev vhost-user,id=hostnet3,chardev=charnet3,queues=4 \
 -device virtio-net-pci,mq=on,vectors=10,netdev=hostnet3,id=net3,\
 mac=52:54:00:ee:96:6d,bus=pci.0,addr=0x6
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args 
b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args
index ce8d669..47c1d84 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args
@@ -20,11 +20,11 @@ QEMU_AUDIO_DRV=none \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
 -chardev socket,id=charnet0,path=/tmp/vhost0.sock,server \
--netdev type=vhost-user,id=hostnet0,chardev=charnet0 \
+-netdev vhost-user,id=hostnet0,chardev=charnet0 \
 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:ee:96:6b,bus=pci.0,\
 addr=0x3 \
 -chardev socket,id=charnet1,path=/tmp/vhost1.sock \
--netdev type=vhost-user,id=hostnet1,chardev=charnet1 \
+-netdev vhost-user,id=hostnet1,chardev=charnet1 \
 -device 
virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:ee:96:6c,bus=pci.0,\
 addr=0x4 \
 -netdev socket,listen=:2015,id=hostnet2 \
-- 
2.8.4

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

[libvirt] [PATCH v2 05/14] qemuBuildInterfaceCommandLine: Move from if-else forest to switch

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c | 34 +++---
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 5ca82ec..cc3b24f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7956,8 +7956,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 
 cfg = virQEMUDriverGetConfig(driver);
 
-if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
-actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+switch (actualType) {
+case VIR_DOMAIN_NET_TYPE_NETWORK:
+case VIR_DOMAIN_NET_TYPE_BRIDGE:
 tapfdSize = net->driver.virtio.queues;
 if (!tapfdSize)
 tapfdSize = 1;
@@ -7971,7 +7972,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 if (qemuInterfaceBridgeConnect(def, driver, net,
tapfd, ) < 0)
 goto cleanup;
-} else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
+break;
+
+case VIR_DOMAIN_NET_TYPE_DIRECT:
 tapfdSize = net->driver.virtio.queues;
 if (!tapfdSize)
 tapfdSize = 1;
@@ -7985,7 +7988,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 if (qemuInterfaceDirectConnect(def, driver, net,
tapfd, tapfdSize, vmop) < 0)
 goto cleanup;
-} else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
+break;
+
+case VIR_DOMAIN_NET_TYPE_ETHERNET:
 tapfdSize = net->driver.virtio.queues;
 if (!tapfdSize)
 tapfdSize = 1;
@@ -7997,17 +8002,32 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 memset(tapfd, -1, tapfdSize * sizeof(tapfd[0]));
 
 if (qemuInterfaceEthernetConnect(def, driver, net,
-   tapfd, tapfdSize) < 0)
+ tapfd, tapfdSize) < 0)
 goto cleanup;
-} else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+break;
+
+case VIR_DOMAIN_NET_TYPE_HOSTDEV:
 /* NET_TYPE_HOSTDEV devices are really hostdev devices, so
  * their commandlines are constructed with other hostdevs.
  */
 ret = 0;
 goto cleanup;
-} else if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
+break;
+
+case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
 ret = qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, 
bootindex);
 goto cleanup;
+break;
+
+case VIR_DOMAIN_NET_TYPE_USER:
+case VIR_DOMAIN_NET_TYPE_SERVER:
+case VIR_DOMAIN_NET_TYPE_CLIENT:
+case VIR_DOMAIN_NET_TYPE_MCAST:
+case VIR_DOMAIN_NET_TYPE_INTERNAL:
+case VIR_DOMAIN_NET_TYPE_UDP:
+case VIR_DOMAIN_NET_TYPE_LAST:
+/* nada */
+break;
 }
 
 /* For types whose implementations use a netdev on the host, add
-- 
2.8.4

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

[libvirt] [PATCH v2 03/14] qemuBuildInterfaceCommandLine: Move hostdev handling a bit further

[Michal Privoznik]

The idea is to have function that does some checking of the
arguments at its beginning and then have one big switch for all
the interface types it supports. Each one of them generating the
corresponding part of the command line.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c| 13 
 .../qemuxml2argv-net-hostdev-fail.xml  | 39 ++
 tests/qemuxml2argvtest.c   |  4 +++
 3 files changed, 49 insertions(+), 7 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2aebf8a..0da2add 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7922,13 +7922,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)
 return qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, 
bootindex);
 
-if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
-/* NET_TYPE_HOSTDEV devices are really hostdev devices, so
- * their commandlines are constructed with other hostdevs.
- */
-return 0;
-}
-
 /* Currently nothing besides TAP devices supports multiqueue. */
 if (net->driver.virtio.queues > 0 &&
 !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
@@ -8008,6 +8001,12 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 if (qemuInterfaceEthernetConnect(def, driver, net,
tapfd, tapfdSize) < 0)
 goto cleanup;
+} else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+/* NET_TYPE_HOSTDEV devices are really hostdev devices, so
+ * their commandlines are constructed with other hostdevs.
+ */
+ret = 0;
+goto cleanup;
 }
 
 /* For types whose implementations use a netdev on the host, add
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml
new file mode 100644
index 000..7807d79
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml
@@ -0,0 +1,39 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu
+
+  
+  
+  
+
+
+
+
+
+  
+  
+
+  
+  
+  
+  
+  
+
+
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 4b9ecb8..442ab31 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1104,6 +1104,10 @@ mymain(void)
 QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_HOST_PCI_MULTIDOMAIN);
 DO_TEST_FAILURE("net-hostdev-vfio-multidomain",
 QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VFIO_PCI);
+DO_TEST_FAILURE("net-hostdev-fail",
+QEMU_CAPS_NODEFCONFIG,
+QEMU_CAPS_DEVICE_VFIO_PCI);
+
 
 DO_TEST("serial-vc", NONE);
 DO_TEST("serial-pty", NONE);
-- 
2.8.4

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

[libvirt] [PATCH v2 04/14] qemuBuildInterfaceCommandLine: Move vhostuser handling a bit further

[Michal Privoznik]

The idea is to have function that does some checking of the
arguments at its beginning and then have one big switch for all
the interface types it supports. Each one of them generating the
corresponding part of the command line.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c|  9 +++---
 .../qemuxml2argv-net-vhostuser-fail.xml| 36 ++
 tests/qemuxml2argvtest.c   |  3 ++
 3 files changed, 44 insertions(+), 4 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0da2add..5ca82ec 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7919,15 +7919,13 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 if (!bootindex)
 bootindex = net->info.bootIndex;
 
-if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)
-return qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, 
bootindex);
-
 /* Currently nothing besides TAP devices supports multiqueue. */
 if (net->driver.virtio.queues > 0 &&
 !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
   actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
   actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
-  actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
+  actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
+  actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Multiqueue network is not supported for: %s"),
virDomainNetTypeToString(actualType));
@@ -8007,6 +8005,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
  */
 ret = 0;
 goto cleanup;
+} else if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
+ret = qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, 
bootindex);
+goto cleanup;
 }
 
 /* For types whose implementations use a netdev on the host, add
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml
new file mode 100644
index 000..e9fe14f
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml
@@ -0,0 +1,36 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu
+
+  
+  
+  
+  
+
+
+
+
+
+  
+  
+  
+  
+  
+  
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 442ab31..8deb651 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1066,6 +1066,9 @@ mymain(void)
 DO_TEST("net-vhostuser-multiq",
 QEMU_CAPS_NETDEV, QEMU_CAPS_VHOSTUSER_MULTIQUEUE);
 DO_TEST_FAILURE("net-vhostuser-multiq", QEMU_CAPS_NETDEV);
+DO_TEST_FAILURE("net-vhostuser-fail",
+QEMU_CAPS_NETDEV,
+QEMU_CAPS_VHOSTUSER_MULTIQUEUE);
 DO_TEST("net-user", NONE);
 DO_TEST("net-virtio", NONE);
 DO_TEST("net-virtio-device",
-- 
2.8.4

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

[libvirt] [PATCH v2 07/14] qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug

[Michal Privoznik]

Instead of blindly claim support for hot-plugging of every
interface type out there we should copy approach we have for
device types: white listing supported types and explicitly error
out on unsupported ones.
For instance, trying to hotplug vhostuser interface results in
nothing usable from guest currently. vhostuser typed interfaces
require additional work on our side.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_hotplug.c | 33 +++--
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 95bbb35..1b2a075 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -970,8 +970,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 return -1;
 }
 
-if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
-actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
+switch (actualType) {
+case VIR_DOMAIN_NET_TYPE_BRIDGE:
+case VIR_DOMAIN_NET_TYPE_NETWORK:
 tapfdSize = vhostfdSize = net->driver.virtio.queues;
 if (!tapfdSize)
 tapfdSize = vhostfdSize = 1;
@@ -988,7 +989,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 if (qemuInterfaceOpenVhostNet(vm->def, net, priv->qemuCaps,
   vhostfd, ) < 0)
 goto cleanup;
-} else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
+break;
+
+case VIR_DOMAIN_NET_TYPE_DIRECT:
 tapfdSize = vhostfdSize = net->driver.virtio.queues;
 if (!tapfdSize)
 tapfdSize = vhostfdSize = 1;
@@ -1006,7 +1009,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 if (qemuInterfaceOpenVhostNet(vm->def, net, priv->qemuCaps,
   vhostfd, ) < 0)
 goto cleanup;
-} else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
+break;
+
+case VIR_DOMAIN_NET_TYPE_ETHERNET:
 tapfdSize = vhostfdSize = net->driver.virtio.queues;
 if (!tapfdSize)
 tapfdSize = vhostfdSize = 1;
@@ -1017,13 +1022,15 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 goto cleanup;
 memset(vhostfd, -1, sizeof(*vhostfd) * vhostfdSize);
 if (qemuInterfaceEthernetConnect(vm->def, driver, net,
-   tapfd, tapfdSize) < 0)
+ tapfd, tapfdSize) < 0)
 goto cleanup;
 iface_connected = true;
 if (qemuInterfaceOpenVhostNet(vm->def, net, priv->qemuCaps,
   vhostfd, ) < 0)
 goto cleanup;
-} else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+break;
+
+case VIR_DOMAIN_NET_TYPE_HOSTDEV:
 /* This is really a "smart hostdev", so it should be attached
  * as a hostdev (the hostdev code will reach over into the
  * netdev-specific code as appropriate), then also added to
@@ -1035,6 +1042,20 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 ret = qemuDomainAttachHostDevice(NULL, driver, vm,
  virDomainNetGetActualHostdev(net));
 goto cleanup;
+break;
+
+case VIR_DOMAIN_NET_TYPE_USER:
+case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
+case VIR_DOMAIN_NET_TYPE_SERVER:
+case VIR_DOMAIN_NET_TYPE_CLIENT:
+case VIR_DOMAIN_NET_TYPE_MCAST:
+case VIR_DOMAIN_NET_TYPE_INTERNAL:
+case VIR_DOMAIN_NET_TYPE_UDP:
+case VIR_DOMAIN_NET_TYPE_LAST:
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("hotplug of interface type of %s is not implemented 
yet"),
+   virDomainNetTypeToString(actualType));
+goto cleanup;
 }
 
 /* Set device online immediately */
-- 
2.8.4

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

[libvirt] [PATCH v2 09/14] qemuBuildChrChardevStr: Introduce @nowait argument

[Michal Privoznik]

This alone makes not much sense. But the aim is to reuse this
function in qemuBuildVhostuserCommandLine() where 'nowait' is not
supported for vhost-user devices.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c | 35 +++
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4a67d80..71067ee 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4934,7 +4934,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
const virDomainDef *def,
const virDomainChrSourceDef *dev,
const char *alias,
-   virQEMUCapsPtr qemuCaps)
+   virQEMUCapsPtr qemuCaps,
+   bool nowait)
 {
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 bool telnet;
@@ -5007,12 +5008,14 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 case VIR_DOMAIN_CHR_TYPE_TCP:
 telnet = dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET;
 virBufferAsprintf(,
-  "socket,id=char%s,host=%s,port=%s%s%s",
+  "socket,id=char%s,host=%s,port=%s%s",
   alias,
   dev->data.tcp.host,
   dev->data.tcp.service,
-  telnet ? ",telnet" : "",
-  dev->data.tcp.listen ? ",server,nowait" : "");
+  telnet ? ",telnet" : "");
+
+if (dev->data.tcp.listen)
+virBufferAdd(, nowait ? ",server,nowait" : ",server", -1);
 
 if (cfg->chardevTLS) {
 char *objalias = NULL;
@@ -5034,7 +5037,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 virBufferAsprintf(, "socket,id=char%s,path=", alias);
 virQEMUBuildBufferEscapeComma(, dev->data.nix.path);
 if (dev->data.nix.listen)
-virBufferAddLit(, ",server,nowait");
+virBufferAdd(, nowait ? ",server,nowait" : ",server", -1);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
@@ -5358,7 +5361,7 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
 
 if (!(chrdev = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
   monitor_chr, "monitor",
-  qemuCaps)))
+  qemuCaps, true)))
 return -1;
 virCommandAddArg(cmd, "-chardev");
 virCommandAddArg(cmd, chrdev);
@@ -5516,7 +5519,7 @@ qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager,
 case VIR_DOMAIN_RNG_BACKEND_EGD:
 if (!(*chr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
 rng->source.chardev,
-rng->info.alias, qemuCaps)))
+rng->info.alias, qemuCaps, true)))
 return -1;
 }
 
@@ -8375,7 +8378,7 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager,
 if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
   >data.passthru,
   smartcard->info.alias,
-  qemuCaps))) {
+  qemuCaps, true))) {
 virBufferFreeAndReset();
 return -1;
 }
@@ -8464,7 +8467,7 @@ qemuBuildShmemBackendChrStr(virLogManagerPtr logManager,
 
 devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
 >server.chr,
-shmem->info.alias, qemuCaps);
+shmem->info.alias, qemuCaps, true);
 
 return devstr;
 }
@@ -8568,7 +8571,7 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager,
 if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
   >source,
   serial->info.alias,
-  qemuCaps)))
+  qemuCaps, true)))
 return -1;
 virCommandAddArg(cmd, "-chardev");
 virCommandAddArg(cmd, devstr);
@@ -8607,7 +8610,7 @@ qemuBuildParallelsCommandLine(virLogManagerPtr logManager,
 if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
   >source,
   parallel->info.alias,
-  qemuCaps)))
+  qemuCaps, true)))
 return -1;
 virCommandAddArg(cmd, "-chardev");
 virCommandAddArg(cmd, 

[libvirt] [PATCH v2 14/14] qemu_hotplug: Support interface type of vhost-user hotplug

[Michal Privoznik]

https://bugzilla.redhat.com/show_bug.cgi?id=1366108

There are couple of things that needs to be done in order to
allow vhost-user hotplug. Firstly, vhost-user requires a chardev
which is connected to vhost-user bridge and through which qemu
communicates with the bridge (no acutal guest traffic is sent
through there, just some metadata). In order to generate proper
chardev alias, we must assign device alias way sooner.

Then, because we are plugging the chardev first, we need to do
the proper undo if something fails - that is remove netdev too.
We don't want anything to be left over in case attach fails at
some point.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_hotplug.c | 71 +
 1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1b2a075..14af4e1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -932,6 +932,10 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 virDomainCCWAddressSetPtr ccwaddrs = NULL;
 size_t i;
+char *charDevAlias = NULL;
+bool charDevPlugged = false;
+bool netdevPlugged = false;
+bool hostPlugged = false;
 
 /* preallocate new slot for device */
 if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0)
@@ -970,6 +974,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 return -1;
 }
 
+if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0)
+goto cleanup;
+
 switch (actualType) {
 case VIR_DOMAIN_NET_TYPE_BRIDGE:
 case VIR_DOMAIN_NET_TYPE_NETWORK:
@@ -1044,8 +1051,18 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 goto cleanup;
 break;
 
-case VIR_DOMAIN_NET_TYPE_USER:
 case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
+if (!qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("Netdev support unavailable"));
+goto cleanup;
+}
+
+if (virAsprintf(, "char%s", net->info.alias) < 0)
+goto cleanup;
+break;
+
+case VIR_DOMAIN_NET_TYPE_USER:
 case VIR_DOMAIN_NET_TYPE_SERVER:
 case VIR_DOMAIN_NET_TYPE_CLIENT:
 case VIR_DOMAIN_NET_TYPE_MCAST:
@@ -1081,9 +1098,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 goto cleanup;
 }
 
-if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0)
-goto cleanup;
-
 if (qemuDomainMachineIsS390CCW(vm->def) &&
 virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
 net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
@@ -1143,23 +1157,36 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 }
 
 qemuDomainObjEnterMonitor(driver, vm);
+
+if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
+if (qemuMonitorAttachCharDev(priv->mon, charDevAlias, 
net->data.vhostuser) < 0) {
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+virDomainAuditNet(vm, NULL, net, "attach", false);
+goto cleanup;
+}
+charDevPlugged = true;
+}
+
 if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) {
 if (qemuMonitorAddNetdev(priv->mon, netstr,
  tapfd, tapfdName, tapfdSize,
  vhostfd, vhostfdName, vhostfdSize) < 0) {
 ignore_value(qemuDomainObjExitMonitor(driver, vm));
 virDomainAuditNet(vm, NULL, net, "attach", false);
-goto cleanup;
+goto try_remove;
 }
+netdevPlugged = true;
 } else {
 if (qemuMonitorAddHostNetwork(priv->mon, netstr,
   tapfd, tapfdName, tapfdSize,
   vhostfd, vhostfdName, vhostfdSize) < 0) {
 ignore_value(qemuDomainObjExitMonitor(driver, vm));
 virDomainAuditNet(vm, NULL, net, "attach", false);
-goto cleanup;
+goto try_remove;
 }
+hostPlugged = true;
 }
+
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 goto cleanup;
 
@@ -1262,6 +1289,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 }
 VIR_FREE(vhostfd);
 VIR_FREE(vhostfdName);
+VIR_FREE(charDevAlias);
 virObjectUnref(cfg);
 virDomainCCWAddressSetFree(ccwaddrs);
 
@@ -1277,7 +1305,11 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 if (virAsprintf(_name, "host%s", net->info.alias) < 0)
 goto cleanup;
 qemuDomainObjEnterMonitor(driver, vm);
-if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
+if (charDevPlugged &&
+qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
+VIR_WARN("Failed to remove associated chardev %s", 
charDevAlias);
+if (netdevPlugged &&
+ 

[libvirt] [PATCH v2 11/14] qemuBuildInterfaceCommandLine: Pass proper args

[Michal Privoznik]

Instead of various NULL arguments, we can pass proper qemu driver
config, log manager, and virCommand.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c | 31 ++-
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 246ffe0..895bb23 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7822,12 +7822,15 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
 }
 
 static int
-qemuBuildVhostuserCommandLine(virCommandPtr cmd,
+qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
+  virLogManagerPtr logManager,
+  virCommandPtr cmd,
   virDomainDefPtr def,
   virDomainNetDefPtr net,
   virQEMUCapsPtr qemuCaps,
   unsigned int bootindex)
 {
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 char *chardev = NULL;
 virBuffer netdev_buf = VIR_BUFFER_INITIALIZER;
 unsigned int queues = net->driver.virtio.queues;
@@ -7841,7 +7844,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
 
 switch ((virDomainChrType) net->data.vhostuser->type) {
 case VIR_DOMAIN_CHR_TYPE_UNIX:
-if (!(chardev = qemuBuildChrChardevStr(NULL, NULL, NULL, def,
+if (!(chardev = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
net->data.vhostuser,
net->info.alias, qemuCaps, 
false)))
 goto error;
@@ -7895,10 +7898,12 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
 
 virCommandAddArgList(cmd, "-device", nic, NULL);
 VIR_FREE(nic);
+virObjectUnref(cfg);
 
 return 0;
 
  error:
+virObjectUnref(cfg);
 VIR_FREE(chardev);
 virBufferFreeAndReset(_buf);
 VIR_FREE(nic);
@@ -7907,8 +7912,9 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
 }
 
 static int
-qemuBuildInterfaceCommandLine(virCommandPtr cmd,
-  virQEMUDriverPtr driver,
+qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
+  virLogManagerPtr logManager,
+  virCommandPtr cmd,
   virDomainDefPtr def,
   virDomainNetDefPtr net,
   virQEMUCapsPtr qemuCaps,
@@ -7928,7 +7934,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 char **tapfdName = NULL;
 char **vhostfdName = NULL;
 virDomainNetType actualType = virDomainNetGetActualType(net);
-virQEMUDriverConfigPtr cfg = NULL;
 virNetDevBandwidthPtr actualBandwidth;
 size_t i;
 
@@ -7971,8 +7976,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 return -1;
 }
 
-cfg = virQEMUDriverGetConfig(driver);
-
 switch (actualType) {
 case VIR_DOMAIN_NET_TYPE_NETWORK:
 case VIR_DOMAIN_NET_TYPE_BRIDGE:
@@ -8032,7 +8035,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 break;
 
 case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
-ret = qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, 
bootindex);
+ret = qemuBuildVhostuserCommandLine(driver, logManager, cmd, def,
+net, qemuCaps, bootindex);
 goto cleanup;
 break;
 
@@ -8205,7 +8209,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 VIR_FREE(host);
 VIR_FREE(tapfdName);
 VIR_FREE(vhostfdName);
-virObjectUnref(cfg);
 return ret;
 }
 
@@ -8215,8 +8218,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
  *   API domainSetSecurityTapFDLabel that doesn't use the const format.
  */
 static int
-qemuBuildNetCommandLine(virCommandPtr cmd,
-virQEMUDriverPtr driver,
+qemuBuildNetCommandLine(virQEMUDriverPtr driver,
+virLogManagerPtr logManager,
+virCommandPtr cmd,
 virDomainDefPtr def,
 virQEMUCapsPtr qemuCaps,
 virNetDevVPortProfileOp vmop,
@@ -8254,7 +8258,7 @@ qemuBuildNetCommandLine(virCommandPtr cmd,
 else
 vlan = i;
 
-if (qemuBuildInterfaceCommandLine(cmd, driver, def, net,
+if (qemuBuildInterfaceCommandLine(driver, logManager, cmd, def, 
net,
   qemuCaps, vlan, bootNet, vmop,
   standalone, nnicindexes,
   nicindexes) < 0)
@@ -9481,7 +9485,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
 if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0)
 goto error;
 
-if (qemuBuildNetCommandLine(cmd, driver, def, qemuCaps, vmop, standalone,
+if (qemuBuildNetCommandLine(driver, logManager, cmd, def,
+

[libvirt] [PATCH v2 06/14] qemuDomainAttachNetDevice: Move hostdev handling a bit further

[Michal Privoznik]

The idea is to have function that does some checking at its
beginning and then have one big switch for all the interface
types it supports.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_hotplug.c | 26 --
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index bb8ea0d..95bbb35 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -946,20 +946,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 
 actualType = virDomainNetGetActualType(net);
 
-if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
-/* This is really a "smart hostdev", so it should be attached
- * as a hostdev (the hostdev code will reach over into the
- * netdev-specific code as appropriate), then also added to
- * the nets list (see cleanup:) if successful.
- *
- * qemuDomainAttachHostDevice uses a connection to resolve
- * a SCSI hostdev secret, which is not this case, so pass NULL.
- */
-ret = qemuDomainAttachHostDevice(NULL, driver, vm,
- virDomainNetGetActualHostdev(net));
-goto cleanup;
-}
-
 /* Currently only TAP/macvtap devices supports multiqueue. */
 if (net->driver.virtio.queues > 0 &&
 !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
@@ -1037,6 +1023,18 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 if (qemuInterfaceOpenVhostNet(vm->def, net, priv->qemuCaps,
   vhostfd, ) < 0)
 goto cleanup;
+} else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+/* This is really a "smart hostdev", so it should be attached
+ * as a hostdev (the hostdev code will reach over into the
+ * netdev-specific code as appropriate), then also added to
+ * the nets list (see cleanup:) if successful.
+ *
+ * qemuDomainAttachHostDevice uses a connection to resolve
+ * a SCSI hostdev secret, which is not this case, so pass NULL.
+ */
+ret = qemuDomainAttachHostDevice(NULL, driver, vm,
+ virDomainNetGetActualHostdev(net));
+goto cleanup;
 }
 
 /* Set device online immediately */
-- 
2.8.4

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

[libvirt] [PATCH v2 00/14] Couple of vhost-user fixes and cleanups

[Michal Privoznik]

v2 of:

https://www.redhat.com/archives/libvir-list/2016-August/msg00806.html

The first patches are mostly code movement and cleanups.

diff to v1:
- Hopefully all John's review suggestions are worked in now

Michal Privoznik (14):
  virDomainNetDefParseXML: Realign
  virDomainNetGetActualType: Return type is virDomainNetType
  qemuBuildInterfaceCommandLine: Move hostdev handling a bit further
  qemuBuildInterfaceCommandLine: Move vhostuser handling a bit further
  qemuBuildInterfaceCommandLine: Move from if-else forest to switch
  qemuDomainAttachNetDevice: Move hostdev handling a bit further
  qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug
  qemuBuildHostNetStr: Explicitly enumerate net types
  qemuBuildChrChardevStr: Introduce @nowait argument
  qemuBuildVhostuserCommandLine: Reuse qemuBuildChrChardevStr
  qemuBuildInterfaceCommandLine: Pass proper args
  qemuBuildVhostuserCommandLine: Unify -netdev creation
  qemuBuildHostNetStr: Support VIR_DOMAIN_NET_TYPE_VHOSTUSER
  qemu_hotplug: Support interface type of vhost-user hotplug

 src/bhyve/bhyve_command.c  |   2 +-
 src/bhyve/bhyve_process.c  |   2 +-
 src/conf/domain_conf.c |  16 +-
 src/conf/domain_conf.h |   2 +-
 src/libxl/libxl_domain.c   |   2 +-
 src/libxl/libxl_driver.c   |   2 +-
 src/lxc/lxc_driver.c   |   9 +-
 src/qemu/qemu_command.c| 182 +
 src/qemu/qemu_hotplug.c| 130 +++
 src/qemu/qemu_process.c|  13 +-
 .../qemuxml2argv-net-hostdev-fail.xml  |  39 +
 .../qemuxml2argv-net-vhostuser-fail.xml|  36 
 .../qemuxml2argv-net-vhostuser-multiq.args |   6 +-
 .../qemuxml2argv-net-vhostuser.args|   4 +-
 tests/qemuxml2argvtest.c   |   7 +
 15 files changed, 334 insertions(+), 118 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml

-- 
2.8.4

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

[libvirt] [PATCH v2 08/14] qemuBuildHostNetStr: Explicitly enumerate net types

[Michal Privoznik]

We tend to prevent using 'default' in switches. And it is for a
good reason - control may end up in paths we wouldn't want for
new values. In this specific case, if qemuBuildHostNetStr is
called over VIR_DOMAIN_NET_TYPE_VHOSTUSER it would produce
meaningless output. Fortunately, there no such call yet.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index cc3b24f..4a67d80 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3711,9 +3711,21 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
 break;
 
 case VIR_DOMAIN_NET_TYPE_USER:
-default:
+case VIR_DOMAIN_NET_TYPE_INTERNAL:
 virBufferAddLit(, "user");
 break;
+
+case VIR_DOMAIN_NET_TYPE_HOSTDEV:
+/* Should have been handled earlier via PCI/USB hotplug code. */
+virObjectUnref(cfg);
+return NULL;
+
+case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
+/* Unsupported yet. */
+break;
+
+case VIR_DOMAIN_NET_TYPE_LAST:
+break;
 }
 
 if (vlan >= 0) {
-- 
2.8.4

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

[libvirt] [PATCH v2 01/14] virDomainNetDefParseXML: Realign

[Michal Privoznik]

There are couple of formatting issues. No functional change
though.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f562323..8f04e6f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9170,8 +9170,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
 ifname = virXMLPropString(cur, "dev");
 if (ifname &&
 (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
- (STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX) ||
-  (prefix && STRPREFIX(ifname, prefix {
+(STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX) ||
+ (prefix && STRPREFIX(ifname, prefix {
 /* An auto-generated target name, blank it out */
 VIR_FREE(ifname);
 }
@@ -9732,9 +9732,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
 filter = NULL;
 def->filterparams = filterparams;
 filterparams = NULL;
-break;
+break;
 default:
-break;
+break;
 }
 }
 
-- 
2.8.4

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

[libvirt] [PATCH v2 02/14] virDomainNetGetActualType: Return type is virDomainNetType

[Michal Privoznik]

This function for some weird reason returns integer instead of
virDomainNetType type. It is important to return the correct type
so that we know what values we can expect.

Signed-off-by: Michal Privoznik 
---
 src/bhyve/bhyve_command.c |  2 +-
 src/bhyve/bhyve_process.c |  2 +-
 src/conf/domain_conf.c|  8 
 src/conf/domain_conf.h|  2 +-
 src/libxl/libxl_domain.c  |  2 +-
 src/libxl/libxl_driver.c  |  2 +-
 src/lxc/lxc_driver.c  |  9 +++--
 src/qemu/qemu_command.c   |  2 +-
 src/qemu/qemu_hotplug.c   |  4 ++--
 src/qemu/qemu_process.c   | 13 -
 10 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 9ad3f9b..55ad950 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -52,7 +52,7 @@ bhyveBuildNetArgStr(const virDomainDef *def,
 char macaddr[VIR_MAC_STRING_BUFLEN];
 char *realifname = NULL;
 char *brname = NULL;
-int actualType = virDomainNetGetActualType(net);
+virDomainNetType actualType = virDomainNetGetActualType(net);
 
 if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
 if (VIR_STRDUP(brname, virDomainNetGetActualBridgeName(net)) < 0)
diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index 6db070f..9d80976 100644
--- a/src/bhyve/bhyve_process.c
+++ b/src/bhyve/bhyve_process.c
@@ -77,7 +77,7 @@ bhyveNetCleanup(virDomainObjPtr vm)
 
 for (i = 0; i < vm->def->nnets; i++) {
 virDomainNetDefPtr net = vm->def->nets[i];
-int actualType = virDomainNetGetActualType(net);
+virDomainNetType actualType = virDomainNetGetActualType(net);
 
 if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
 if (net->ifname) {
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8f04e6f..9343770 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -20804,7 +20804,7 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf,
 bool inSubelement,
 unsigned int flags)
 {
-int actualType = virDomainNetGetActualType(def);
+virDomainNetType actualType = virDomainNetGetActualType(def);
 
 if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
 if (virDomainHostdevDefFormatSubsys(buf, 
virDomainNetGetActualHostdev(def),
@@ -20884,7 +20884,7 @@ virDomainActualNetDefFormat(virBufferPtr buf,
 virDomainNetDefPtr def,
 unsigned int flags)
 {
-unsigned int type;
+virDomainNetType type;
 const char *typeStr;
 
 if (!def)
@@ -21039,7 +21039,7 @@ virDomainNetDefFormat(virBufferPtr buf,
   char *prefix,
   unsigned int flags)
 {
-unsigned int actualType = virDomainNetGetActualType(def);
+virDomainNetType actualType = virDomainNetGetActualType(def);
 bool publicActual = false;
 int sourceLines = 0;
 const char *typeStr;
@@ -24781,7 +24781,7 @@ virDomainStateReasonFromString(virDomainState state, 
const char *reason)
  * otherwise return the value from the NetDef.
  */
 
-int
+virDomainNetType
 virDomainNetGetActualType(virDomainNetDefPtr iface)
 {
 if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a70bc21..95bcf4f 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2814,7 +2814,7 @@ int 
virDomainGraphicsListenAppendSocket(virDomainGraphicsDefPtr def,
 const char *socket)
 ATTRIBUTE_NONNULL(1);
 
-int virDomainNetGetActualType(virDomainNetDefPtr iface);
+virDomainNetType virDomainNetGetActualType(virDomainNetDefPtr iface);
 const char *virDomainNetGetActualBridgeName(virDomainNetDefPtr iface);
 int virDomainNetGetActualBridgeMACTableManager(virDomainNetDefPtr iface);
 const char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface);
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index db2c1dc..3df04cf 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -937,7 +937,7 @@ libxlNetworkPrepareDevices(virDomainDefPtr def)
 
 for (i = 0; i < def->nnets; i++) {
 virDomainNetDefPtr net = def->nets[i];
-int actualType;
+virDomainNetType actualType;
 
 /* If appropriate, grab a physical device from the configured
  * network's pool of devices, or resolve bridge device name
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index b66cb1f..f87f549 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3328,7 +3328,7 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
virDomainNetDefPtr net)
 {
 libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
-int actualType;
+virDomainNetType actualType;
 libxl_device_nic nic;
 int ret = -1;
 char mac[VIR_MAC_STRING_BUFLEN];
diff --git 

Re: [libvirt] [PATCH v3 10/18] qemu: set/use proper pciConnectFlags during hotplug

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> Before now, all the qemu hotplug functions assumed that all devices to
> be hotplugged were legacy PCI endpoint devices
> (VIR_PCI_CONNECT_TYPE_PCI_DEVICE). This worked out "okay", because all
> devices *are* legacy PCI endpoint devices on x86/440fx machinetypes,
> and hotplug didn't work properly on machinetypes using PCIe anyway
> (hotplugging onto a legacy PCI slot doesn't work, and until commit
> b87703cf any attempt to manually specify a PCIe address for a
> hotplugged device would be erroneously rejected).
> 
> This patch makes all qemu hotplug operations honor the pciConnectFlags
> set by the single all-knowing function
> qemuDomainDeviceConnectFlagsInternal().

[...]
> @@ -485,17 +485,17 @@ 
> virDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr addrs,
>  
>  int
>  virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs,
> -  virDomainDeviceInfoPtr dev)
> +  virDomainDeviceInfoPtr dev,
> +  virDomainPCIConnectFlags flags)
>  {
>  int ret = -1;
>  char *addrStr = NULL;
> -/* Flags should be set according to the particular device,
> - * but only the caller knows the type of device. Currently this
> - * function is only used for hot-plug, though, and hot-plug is
> - * only supported for standard PCI devices, so we can safely use
> - * the setting below */
> -virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
> -  VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
> +
> +/* if pciConnectFlags is 0, the particular model of this device

s/pciConnectFlags/flags/

[...]
> @@ -2032,6 +2032,33 @@ qemuDomainAssignAddresses(virDomainDefPtr def,
>  return 0;
>  }
>  
> +/**
> + * qemuDomainEnsurePCIAddress:
> + *
> + * if @dev should have a PCI address but doesn't, assign
> + * an address on a compatible PCI bus. Validate that any
> + * existing address is on a compatible bus.
> + *
> + * @obj: the virDomainObjPtr for the domain. This will include
> + *   qemuCaps and address cache (if there is one)
> + *
> + * @dev: the device that we need to ensure has a PCI address
> + *
> + * returns 0 on success (and any new address set in dev->...info) -1
> + * on failure.

The new address is set, not returned, so that information
doesn't belong in this section. The description already
mention the fact that an address will be assigned if needed
anyway.

[...]
> @@ -1584,10 +1590,12 @@ qemuDomainChrRemove(virDomainDefPtr vmdef,
>  }
>  
>  static int
> -qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
> +qemuDomainAttachChrDeviceAssignAddr(virDomainObjPtr vm,
>  qemuDomainObjPrivatePtr priv,

Please change the type of the first argument from
virDomainDefPtr to virDomainObjPtr in a separate commit,
and use that chance to remove the qemuDomainObjPrivatePtr
argument altogether - you can retrieve it from @vm the
same way you already retrieve the DomainDef.

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] RFC: Exposing "ready" bool (of `query-block-jobs`) or QMP BLOCK_JOB_READY event

[Kashyap Chamarthy]

On Thu, Oct 06, 2016 at 01:34:59PM +0200, Peter Krempa wrote:
> On Thu, Oct 06, 2016 at 12:26:50 +0200, Kashyap Chamarthy wrote:

[...]

> > And, libvirt was fixed to use the above field in this commit (available
> > in libvirt v1.2.18 or above):
> > 
> > http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=eae5924 -- qemu:
> > Update state of block job to READY only if it actually is ready [1]
> > 
> > RFC
> > ---
> > 
> > Currently libvirt block APIs (& consequently higher-level applications
> > like Nova which use these APIs) rely on polling for job completion via
> 
> libvirt is _not_ polling the data. Libvirt relies on the events from
> qemu which are also exposed as libvirt events.

Err, you're right.  I know you mean the below enum:

enum virConnectDomainEventBlockJobStatus {

VIR_DOMAIN_BLOCK_JOB_COMPLETED  =   0
VIR_DOMAIN_BLOCK_JOB_FAILED =   1
VIR_DOMAIN_BLOCK_JOB_CANCELED   =   2
VIR_DOMAIN_BLOCK_JOB_READY  =   3
VIR_DOMAIN_BLOCK_JOB_LAST   =   4
}

Which are used internally in the event processing code in
qemuBlockJobEventProcess() [libvirt/src/qemu/qemu_blockjob.c] that is
used to update the disk mirroring state based on events from QEMU.
 
> > virDomainGetBlockJobInfo(), which uses QMP `query-block-jobs`, and
> > waits for QEMU to report "offset" == "len", which translates to
> > libvirt "cur" == "end".  Based on this, libvirt can take an action
> > (whether to gracefully abort, or pivot to the copy in case of a COPY
> > job).
> 
> Again false. Internally we honor the ready state

Oops, I do realize that libvirt honors seen the 'ready' boolean -- I
even mentioned the libvirt commit message (eae5924) from you in the
beginning, which handles it.

> and applications like
> virsh have code in place that waits for the async events and act after
> receiving them.

Yep, I remember Eric mentioning the other day that `virsh` is setup to
capture libvirt events.  So I briefly went through the
virshBlockJobWait() in tools/virsh-domain.c where you see that:

[...]
/* Fallback behaviour is only needed if one or both callbacks could not
 * be registered */
if (data->cb_id < 0 || data->cb_id2 < 0) {
/* If the block job vanishes, synthesize a COMPLETED event */
if (result == 0) {
ret = VIR_DOMAIN_BLOCK_JOB_COMPLETED;
break;
}

/* If the block job hits 100%, wait a little while for a possible
 * event from libvirt unless both callbacks could not be registered
 * in order to synthesize our own READY event */
if (info.end == info.cur &&
((data->cb_id < 0 && data->cb_id2 < 0) || --retries == 0)) {
ret = VIR_DOMAIN_BLOCK_JOB_READY;
break;
}
}
[...]

> > Since QEMU reports the "ready": true field (followed by a
> > BLOCK_JOB_READY QMP event).  It would be helpful if libvirt expose this
> > via an API, so upper layers could instead use that, rather than polling.
> 
> We expose the state of the copy job in the XML and forward the READY
> event from qemu to the users.

Yep, I see that in the QMP traffic:

-
2016-10-04 15:54:52.333+: 30550: info : qemuMonitorIOProcess:423 :
QEMU_MONITOR_IO_PROCESS: mon=0x7fa43000ee60 buf={"timestamp":
{"seconds": 1475596492, "microseconds": 333414}, "event":
 "BLOCK_JOB_READY", "data": {"device": "drive-virtio-disk1", "len":
  1073741824, "offset": 1073741824, "speed": 0, "type": "mirror"}}
-
 
> A running copy job exposes itself in the xml as:
> 
> 
>   
>   
>   
>   
> 
> 
>   
>   [...]
> 
> 
> While the ready copy job is exposed as:
> 
> 
>   
>   
>   
>ready='yes'>
> 
> 
>   
>   [...]
> 

Thanks for the XML snippets.

> Additionally we have anyncrhronous events that are emitted once qemu
> notifies us that the block job has reached sync state or finished.
> Libvirt uses the event to switch to the ready state.
> 
> The documentation suggests that block jobs should listen to the events
> and act accordingly only after receiving the event.
> 
> 
> > Problem scenario
> > 
> > 
> > When virDomainBlockRebase() is invoked to start a copy job, then
> > aborting the said copy operation with virDomainBlockJobAbort() + flag
> > VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT can result in a potential race
> > condition (due to the way the virDomainGetBlockJobInfo() reports the job
> > status) where the pivot operation fails.
> 
> Again, this should trigger the block job event and that should be used
> as the marker to perform the pivot operation.
> 
> $ virsh event --all --loop
> event 'block-job-2' for domain hp: Block Copy for hda ready  <- after the 
> sync phase is reached
> event 'block-job-2' for domain hp: Block Copy for hda completed <- after 
> successfully pivoting
> 
> 

Re: [libvirt] PCP libvirt Plugin

[Martin Kletzander]

On Wed, Oct 05, 2016 at 07:28:00PM +0300, Marko Myllynen wrote:

Hi,

FYI, I've written a PCP plugin (PMDA in PCP parlance) to support most
hypervisor / domain information and metrics available over the libvirt
Python API, it's up to date as of libvirt 2.3 (so it already supports
the recently added perf event metrics).

In case you haven't heard about PCP, here's a brief intro:

The Performance Co-Pilot (PCP, http://www.pcp.io/) system is a toolkit
for collecting, archiving, and processing performance metrics. It
supports system and application metrics from local and remote hosts,
archives, and containers. A typical Linux PCP installation offers over
1,000 metrics by default and is easily extensible with its own plugins,
or PMDAs ("Performance Metrics Domain Agents"). In addition to very
complete /proc based statistics, readily available PCP PMDAs provide
support components like 389 Directory Server, Apache, Ceph, GFS2,
Gluster, MySQL, NFS, Oracle, Postfix, PostgreSQL, Samba, and Sendmail,
among others. PCP also runs on many platforms, including Linux, Mac OS
X, FreeBSD, NetBSD, Solaris, and Windows.

And here's Quick Guide to get started with PCP:

http://www.pcp.io/docs/guide.html

(So PCP comes with handy clients like pmrep(1) for custom metrics
reporting, sophisticated logging and inferencing tools, and support for
Grafana based Web UI.)

The libvirt metrics available with PCP libvirt PMDA and their
descriptions are listed below, they include all the recently added perf
event metrics as well as combined and per-device metrics for each VM.

The plugin is available for Fedora in updates-testing and for Ubuntu
from https://bintray.com/pcp/ . As this is a recent addition, the plugin
or some of the later additions like per-device metrics have not
propagated to all distributions yet. For those using PCP already, they
may also manually grab the needed files from the below page without the
need to upgrade any PCP packages:

https://myllynen.fedorapeople.org/pmdalibvirt/

Upstream code lives here:

http://oss.sgi.com/cgi-bin/gitweb.cgi?p=pcp/pcp.git;a=tree;f=src/pmdas/libvirt;hb=HEAD

I wonder could this be mentioned at https://libvirt.org/apps.html ?



Great to hear!  Sure!  Would you mind sending a patch against libvirt's
docs/apps.html.in with what you'd like to have mentioned there?  If you
don't like doing that I can do that for you, just let me know.

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] RFC: Exposing "ready" bool (of `query-block-jobs`) or QMP BLOCK_JOB_READY event

[Peter Krempa]

On Thu, Oct 06, 2016 at 12:26:50 +0200, Kashyap Chamarthy wrote:
> Backround
> -
> 
> For QEMU block device jobs, the "ready" boolean field (part of QMP
> `query-block-jobs`) was introduced in commit ef6dbf1 (available in QEMU
> v2.2.0 or above):
> 
> http://git.qemu.org/?p=qemu.git;a=commitdiff;h=ef6dbf1e4 --
> blockjob: Add "ready" field
> 
> "When a block job signals readiness, this is currently reported only
> through QMP. If qemu wants to use block jobs for internal tasks,
> there needs to be another way to correctly detect when a block job
> may be completed.
> 
> For this reason, introduce a bool "ready" which is set when the
> block job may be completed."
> 
> 
> And, libvirt was fixed to use the above field in this commit (available
> in libvirt v1.2.18 or above):
> 
> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=eae5924 -- qemu:
> Update state of block job to READY only if it actually is ready [1]
> 
> 
> RFC
> ---
> 
> Currently libvirt block APIs (& consequently higher-level applications
> like Nova which use these APIs) rely on polling for job completion via

libvirt is _not_ polling the data. Libvirt relies on the events from
qemu which are also exposed as libvirt events.

> virDomainGetBlockJobInfo(), which uses QMP `query-block-jobs`, and
> waits for QEMU to report "offset" == "len", which translates to libvirt
> "cur" == "end".  Based on this, libvirt can take an action (whether to
> gracefully abort, or pivot to the copy in case of a COPY job).

Again false. Internally we honor the ready state and applications like
virsh have code in place that waits for the async events and act after
receiving them.

> Since QEMU reports the "ready": true field (followed by a
> BLOCK_JOB_READY QMP event).  It would be helpful if libvirt expose this
> via an API, so upper layers could instead use that, rather than polling.

We expose the state of the copy job in the XML and forward the READY
event from qemu to the users.

A running copy job exposes itself in the xml as:


  
  
  
  


  
  [...]


While the ready copy job is exposed as:


  
  
  
  


  
  [...]


Additionally we have anyncrhronous events that are emitted once qemu
notifies us that the block job has reached sync state or finished.
Libvirt uses the event to switch to the ready state.

The documentation suggests that block jobs should listen to the events
and act accordingly only after receiving the event.


> Problem scenario
> 
> 
> When virDomainBlockRebase() is invoked to start a copy job, then
> aborting the said copy operation with virDomainBlockJobAbort() + flag
> VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT can result in a potential race
> condition (due to the way the virDomainGetBlockJobInfo() reports the job
> status) where the pivot operation fails.

Again, this should trigger the block job event and that should be used
as the marker to perform the pivot operation.

$ virsh event --all --loop
event 'block-job-2' for domain hp: Block Copy for hda ready  <- after the sync 
phase is reached
event 'block-job-2' for domain hp: Block Copy for hda completed <- after 
successfully pivoting

Applications should act only after receiving such event.


> Race condition window
> ~
> 
> libvirt finds cur==end AND sends a pivot request, all in the window
> before QEMU would have sent "ready": true field [emitted as part of the

This is not true. Libvirt checks that the mirror is actually ready. It's
done by the commit you've mentioned above.

> QMP `query-block-jobs` command's response, indicating that the job has
> actually completed], however the pivot request fails because it requires
> "ready": true.

The problem is that you are polling the block job info which correctly
reports that all data was properly copied and you are inferring the
block job state from that data.

I'm against deliberately reporting false data in the block info
structure.

The application should register handlers for the block job events and
act only if it receives such event. Additionally you may want to check
that the state is correct in the XML. The current block job information
structure can't be extended unfortunately.

Please look at the virsh handling of the block jobs for inspiration
since we have example code doing the right thing here.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] m4: Drop PKG_PROG_PKG_CONFIG compatibility code

[Andrea Bolognani]

This was needed for RHEL 4 vintage distributions, which we
haven't supported for a long time now.
---
 m4/virt-pkgconfig-back-compat.m4 | 23 ---
 1 file changed, 23 deletions(-)
 delete mode 100644 m4/virt-pkgconfig-back-compat.m4

diff --git a/m4/virt-pkgconfig-back-compat.m4 b/m4/virt-pkgconfig-back-compat.m4
deleted file mode 100644
index 7eab84b..000
--- a/m4/virt-pkgconfig-back-compat.m4
+++ /dev/null
@@ -1,23 +0,0 @@
-dnl
-dnl To support the old pkg-config from RHEL4 vintage, we need
-dnl to define the PKG_PROG_PKG_CONFIG macro if its not already
-dnl present
-m4_ifndef([PKG_PROG_PKG_CONFIG],
-  [AC_DEFUN([PKG_PROG_PKG_CONFIG],
-[m4_pattern_forbid([^_?PKG_[A-Z_]+$])
-m4_pattern_allow([^PKG_CONFIG(_PATH)?$])
-AC_ARG_VAR([PKG_CONFIG], [path to pkg-config utility])dnl
-if test "x$ac_cv_env_PKG_CONFIG_set" != "xset"; then
-AC_PATH_TOOL([PKG_CONFIG], [pkg-config])
-fi
-if test -n "$PKG_CONFIG"; then
-_pkg_min_version=m4_default([$1], [0.9.0])
-AC_MSG_CHECKING([pkg-config is at least version $_pkg_min_version])
-if $PKG_CONFIG --atleast-pkgconfig-version $_pkg_min_version; then
-AC_MSG_RESULT([yes])
-else
-AC_MSG_RESULT([no])
-PKG_CONFIG=""
-fi
-fi[]dnl
-])])
-- 
2.7.4

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

Re: [libvirt] Question about QEMU + hugepages + NUMA memory + migration

[Martin Kletzander]

On Thu, Oct 06, 2016 at 03:58:54PM +1100, Sam Bobroff wrote:

On Tue, Oct 04, 2016 at 11:28:15AM +0200, Martin Kletzander wrote:

On Tue, Oct 04, 2016 at 02:34:57PM +1100, Sam Bobroff wrote:
>On Mon, Oct 03, 2016 at 04:27:25PM +0200, Martin Kletzander wrote:
>>On Mon, Aug 01, 2016 at 02:01:22PM +1000, Sam Bobroff wrote:
>>>Hi libvirt people,
>>>
>>>I've been looking at a (probable) bug and I'm not sure how to progress. The
>>>situation is a bit complicated and involves both QEMU and libvirt (and I 
think
>>>it may have been looked at already) so I would really appreciate some advice 
on
>>>how to approach it. I'm using a pretty recent master version of libvirt from
>>>git and I'm testing on a ppc64le host with a similar guest but this doesn't
>>>seem to be arch-specific.
>>>
>>
>>Sorry I haven't replied earlier and I'm respawning this thread now, I
>>just noticed this thread being marked for replying only after I fixed
>>similar thing to what you describe.
>
>No problem! :-)
>
>>>If I create a QEMU guest (e.g. via virt-install) that requests both hugepage
>>>backing on the host and NUMA memory placement on the host, the NUMA placement
>>>seems to be ignored. If I do:
>>>
>>># echo 0 > /proc/sys/vm/nr_hugepages
>>># echo 512 > 
/sys/devices/system/node/node0/hugepages/hugepages-16384kB/nr_hugepages
>>># virt-install --name tmp --memory=4096 --graphics none --memorybacking 
hugepages=yes --disk none --import --wait 0 --numatune=8
>>>
>>
>>So to be clear, we want to use 16M hugepages, but only allocated from
>>node 8 while the only allocated ones are on node 0.
>
>Yes.
>
>>>... then hugepages are allocated on node 0 and the machine starts 
successfully,
>>>which seems like a bug.
>>>
>>
>>This definitely is a bug.  But I'm afraid it's not in libvirt.  I'll do
>>some explaining first.
>
>OK.
>
>>>I believe it should fail to start due to insufficient memory, and in fact 
that
>>>is what happens if cgroup support isn't detected in the host: there seems to 
be
>>>a fall-back path in libvirt (probably using mbind()) that works as I would
>>>expect.
>>>
>>
>>Yes, we are using multiple things to enforce NUMA binding:
>>
>> 1) cgroups - This restricts all the allocations made on the account of
>>  the process, even when KVM does that.  We cannot use that
>>  until the QEMU process is running because it needs to
>>  allocate some data from the DMA region which is usually
>>  only on one node.
>>
>> 2) numactl's mbind() - it doesn't apply to kernel allocations, so
>>whatever KVM allocates it's not restricted by
>>this.  We use this always on the process before
>>exec()-ing qemu binary (if compiled with
>>support to numactl, of course)
>>
>> 3) memory-backend-file's host-nodes parameter - This is the best
>> option that is used when QEMU
>> supports it, but due to migration
>> compatibility we use it only when
>> requested with 
>> elements.
>
>OK good, that matches what I thought I'd worked out :-)
>
>>>Note: the relevant part of the guest XML seems to be this:
>>>»···
>>>»···»···
>>>»···
>>>»···
>>>»···»···
>>>»···
>>>
>>>It seems fairly clear what is happening: although QEMU is capable of 
allocating
>>>hugepages on specific NUMA nodes (using "memory-backend-file") libvirt is not
>>>passing those options to QEMU in this situation.
>>>
>>
>>I'm guessing you're talking about the host-nodes= parameter.
>
>I am, yes.
>
>>>I investigated this line of reasoning and if I hack libvirt to pass those
>>>options to QEMU it does indeed fix the problem... but it renders the machine
>>>state migration-incompatible with unfixed versions. This seems to have been 
why
>>>this hasn't been fixed already :-(
>>>
>>>So what can we do?
>>>
>>
>>From virt-install POV I would suggest adding some functionality that
>>would probe whether it works with  and use it if possible.  Or
>>dig deep down in kvm or qemu to see why the allocations do not conform
>>to the mbind().
>
>Sorry, I don't think I understand your answer.
>
>You seem to be saying that it doesn't matter that the above XML causes memory
>to be allocated incorrectly, as long as we don't use it (by changing
>virt-install to use something else). But shouldn't the XML work correctly even
>if it was generated by hand? (Sorry if I'm misunderstanding!)
>

It should, but libvirt is doing what it can (see later).  At least what
I thought.  But while composing the mail I must say that's not totally
true.

>And yes, it does work if you specify memnodes but wouldn't adding them change
>the guest definition (in a guest visible way)? (Because memnode requires
>cellid, a guest NUMA node ID, and my test case has no guest NUMA nodes.)
>

I totally 

Re: [libvirt] [PATCH] virt-yajl: Fix detection of yajl requirements

[Andrea Bolognani]

On Thu, 2016-10-06 at 11:32 +0200, Martin Kletzander wrote:
> Running the output of qemu -help doesn't make any sense.  We should be
> looking for libvirt being mentioned in the output.  This worked by
> accident, let's make it work as expected it to.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  m4/virt-yajl.m4 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/m4/virt-yajl.m4 b/m4/virt-yajl.m4
> index adf2819eeb66..8c452adca653 100644
> --- a/m4/virt-yajl.m4
> +++ b/m4/virt-yajl.m4
> @@ -26,7 +26,7 @@ AC_DEFUN([LIBVIRT_CHECK_YAJL],[
>  AC_PATH_PROGS([QEMU], [qemu-kvm qemu kvm qemu-system-x86_64],
>[], [$PATH:/usr/bin:/usr/libexec])
>  if test -x "$QEMU"; then
> -  if `$QEMU -help | grep libvirt` >/dev/null; then
> +  if $QEMU -help 2>/dev/null | grep -q libvirt; then
>  with_yajl=yes
>else
>  [qemu_version_sed='s/.*ersion \([0-9.,]*\).*/\1/']

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] Analysis of the effect of adding PCIe root ports

[Andrea Bolognani]

On Wed, 2016-10-05 at 18:36 +0100, Richard W.M. Jones wrote:
> > > (b) It would be nice to turn the whole thing off for people who don't
> > > care about / need hotplugging.
> > 
> > I had contemplated having an "availablePCIeSlots" (or something like
> > that) that was either an attribute of the config, or an option in
> > qemu.conf or libvirtd.conf. If we had such a setting, it could be
> > set to "0".

I remember some pushback when this was proposed. Maybe we
should just give up on the idea of providing spare
hotpluggable PCIe slots by default and ask the user to add
them explicitly after all.

> Note that changes to libvirt conf files are not usable by libguestfs.
> 
> The setting would need to go into the XML, and please also make it
> possible to determine if $random version of libvirt supports the
> setting, either by a version check or something in capabilities.

Note that you can avoid using any PCIe root port at all by
assigning PCI addresses manually. It looks like the overhead
for the small (I'm assuming) number of devices a libguestfs
appliance will use is low enough that you will probably not
want to open that can of worm, though.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 15/20] daemon: Split output parsing and output defining

[John Ferlan]

On 10/06/2016 06:42 AM, Erik Skultety wrote:
> On 21/09/16 21:14, John Ferlan wrote:
>>
>>
>> On 08/18/2016 07:47 AM, Erik Skultety wrote:
>>> Since virLogParseAndDefineOutputs is going to be stripped from 'output 
>>> defining'
>>> logic, replace all relevant occurrences with virLogSetOutputs call to make 
>>> the
>>> change transparent to all original callers (daemons mostly).
>>
>>
>> I think the commit message needs to be adjusted... What's really
>> happening is that now that you have a "real" virLogParseOutputs and a
>> virLogSetOutputs, the "ParseAndDefine" is being duplicitous so you're
>> replacing them with the Set call.
>>
>> Personally, I think this patch should be combined with Patch 13 so we
>> don't have that duplicity and/or any other "strangeness" as a result of
>> having the ParseAndDefine being called as well as the Parse.
>>
> 
> My original intention was to make it clear in a separate patch that that
> was the specific moment where everything would switch to the new
> versions transparently, I can squash it to 13 I just wanted to make it
> more clear and more easier for the reviewer, that's all.
> 

Understood - the ordering and separation did make review easier.

I think in 20/20 hindsight though - since nothing calls SetOutputs until
now that the duplicity and bisect concern I noted cannot happen.
Similarly for patch 16's comments...

Keep the separation as is - that's fine.

John
> 
>>> diff --git a/tests/testutils.c b/tests/testutils.c
>>> index 8af8707..21687e5 100644
>>> --- a/tests/testutils.c
>>> +++ b/tests/testutils.c
>>> @@ -871,6 +871,9 @@ int virTestMain(int argc,
>>>  #ifdef TEST_OOM
>>>  char *oomstr;
>>>  #endif
>>> +size_t noutputs = 0;
>>> +virLogOutputPtr output = NULL;
>>> +virLogOutputPtr *outputs = NULL;
>>>  
>>>  if (getenv("VIR_TEST_FILE_ACCESS"))
>>>  VIRT_TEST_PRELOAD(TEST_MOCK);
>>> @@ -910,8 +913,11 @@ int virTestMain(int argc,
>>>  
>>>  virLogSetFromEnv();
>>>  if (!getenv("LIBVIRT_DEBUG") && !virLogGetNbOutputs()) {
>>> -if (virLogDefineOutput(virtTestLogOutput, virtTestLogClose, 
>>> ,
>>> -   VIR_LOG_DEBUG, VIR_LOG_TO_STDERR, NULL, 0) 
>>> < 0)
>>> +if (!(output = virLogOutputNew(virtTestLogOutput, virtTestLogClose,
>>> +   , VIR_LOG_DEBUG,
>>> +   VIR_LOG_TO_STDERR, NULL)) ||
>>> +VIR_APPEND_ELEMENT(outputs, noutputs, output) < 0 ||
>>> +virLogDefineOutputs(outputs, noutputs) < 0)
>>
>> I know - just a test, but you could leak output if the APPEND fails.
>> You could use the _COPY macro, then do the Free in both the error and
>> success case...
> 
> Actually, not just the output but outputs as well if define fails. In
> that case APPEND without _COPY still might be a better solution, since
> when APPEND fails, I can free the output, if APPEND succeeds, it clears
> output, so that when define fails afterwards, I can still free both the
> output and outputs (the list), since one of those will always be NULL in
> any case of failure, but thanks anyway.
> 
> 
>>
>> Shouldn't the rest of this go with the "next" patch which does the
>> Filter magic (it's going to have a similar merge with patch comment)
>>
> 
> Oops, dunno how that hunk got there :/ (probably multiple interactive
> rebases as usual...)
> 
> Erik
> 
>> Conditional ACK on moving/merging with patch 13. Beyond that just a
>> small justification for the separation...
>>
>> w/r/t: memory leak on output, that will probably raise the ire of
>> Coverity or running with valgrind.
>>
>> John
>>> @@ -79,9 +80,10 @@ testLogParseFilters(const void *opaque)
>>>  {
>>>  int ret = -1;
>>>  int nfilters;
>>> +virLogFilterPtr *filters = NULL;
>>>  const struct testLogData *data = opaque;
>>>  
>>> -nfilters = virLogParseAndDefineFilters(data->str);
>>> +nfilters = virLogParseFilters(data->str, );
>>>  if (nfilters < 0) {
>>>  if (!data->pass) {
>>>  VIR_TEST_DEBUG("Got expected error: %s\n",
>>> @@ -101,7 +103,7 @@ testLogParseFilters(const void *opaque)
>>>  
>>>  ret = 0;
>>>   cleanup:
>>> -virLogReset();
>>> +virLogFilterListFree(filters, nfilters);
>>>  return ret;
>>>  }
>>>  
>>>
> 
> 
> 

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

Re: [libvirt] [PATCH v2 17/20] virlog: Remove functions that aren't used anywhere anymore

[Erik Skultety]

On 21/09/16 21:39, John Ferlan wrote:
> 
> 
> On 08/18/2016 07:47 AM, Erik Skultety wrote:
>> This is mainly virLogAddOutputTo* which were replaced by virLogNewOutputTo* 
>> and
>> the previously poorly named ones virLogParseAndDefine* functions. All of 
>> these
>> are unnecessary now, since all the original callers were transparently 
>> switched
>> to the new model of separate parsing and defining logic.
>>
>> Signed-off-by: Erik Skultety 
>> ---
>>  src/libvirt_private.syms |   4 -
>>  src/util/virlog.c| 427 
>> ---
>>  src/util/virlog.h|  12 --
>>  3 files changed, 443 deletions(-)
>>
> NOTE:
> 
> My git am -3 failed on this patch:
> 
> Applying: virlog: Remove functions that aren't used anywhere anymore
> fatal: sha1 information is lacking or useless (src/libvirt_private.syms).
> error: could not build fake ancestor
> Patch failed at 0017 virlog: Remove functions that aren't used anywhere
> anymore
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> 
> I'm really not sure why, but I
> 
> NOTE: Existing, but IS_SPACE isn't used either.
> 
> ACK - you could/should remove IS_SPACE separately and use this implied ACK
> 
> John
> 

Thanks, I'll remove it, commit 0b231195 forgot to remove it as part of
the refactor series.

Erik




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 15/20] daemon: Split output parsing and output defining

[Erik Skultety]

On 21/09/16 21:14, John Ferlan wrote:
> 
> 
> On 08/18/2016 07:47 AM, Erik Skultety wrote:
>> Since virLogParseAndDefineOutputs is going to be stripped from 'output 
>> defining'
>> logic, replace all relevant occurrences with virLogSetOutputs call to make 
>> the
>> change transparent to all original callers (daemons mostly).
> 
> 
> I think the commit message needs to be adjusted... What's really
> happening is that now that you have a "real" virLogParseOutputs and a
> virLogSetOutputs, the "ParseAndDefine" is being duplicitous so you're
> replacing them with the Set call.
> 
> Personally, I think this patch should be combined with Patch 13 so we
> don't have that duplicity and/or any other "strangeness" as a result of
> having the ParseAndDefine being called as well as the Parse.
>

My original intention was to make it clear in a separate patch that that
was the specific moment where everything would switch to the new
versions transparently, I can squash it to 13 I just wanted to make it
more clear and more easier for the reviewer, that's all.


>> diff --git a/tests/testutils.c b/tests/testutils.c
>> index 8af8707..21687e5 100644
>> --- a/tests/testutils.c
>> +++ b/tests/testutils.c
>> @@ -871,6 +871,9 @@ int virTestMain(int argc,
>>  #ifdef TEST_OOM
>>  char *oomstr;
>>  #endif
>> +size_t noutputs = 0;
>> +virLogOutputPtr output = NULL;
>> +virLogOutputPtr *outputs = NULL;
>>  
>>  if (getenv("VIR_TEST_FILE_ACCESS"))
>>  VIRT_TEST_PRELOAD(TEST_MOCK);
>> @@ -910,8 +913,11 @@ int virTestMain(int argc,
>>  
>>  virLogSetFromEnv();
>>  if (!getenv("LIBVIRT_DEBUG") && !virLogGetNbOutputs()) {
>> -if (virLogDefineOutput(virtTestLogOutput, virtTestLogClose, 
>> ,
>> -   VIR_LOG_DEBUG, VIR_LOG_TO_STDERR, NULL, 0) < 
>> 0)
>> +if (!(output = virLogOutputNew(virtTestLogOutput, virtTestLogClose,
>> +   , VIR_LOG_DEBUG,
>> +   VIR_LOG_TO_STDERR, NULL)) ||
>> +VIR_APPEND_ELEMENT(outputs, noutputs, output) < 0 ||
>> +virLogDefineOutputs(outputs, noutputs) < 0)
> 
> I know - just a test, but you could leak output if the APPEND fails.
> You could use the _COPY macro, then do the Free in both the error and
> success case...

Actually, not just the output but outputs as well if define fails. In
that case APPEND without _COPY still might be a better solution, since
when APPEND fails, I can free the output, if APPEND succeeds, it clears
output, so that when define fails afterwards, I can still free both the
output and outputs (the list), since one of those will always be NULL in
any case of failure, but thanks anyway.


> 
> Shouldn't the rest of this go with the "next" patch which does the
> Filter magic (it's going to have a similar merge with patch comment)
> 

Oops, dunno how that hunk got there :/ (probably multiple interactive
rebases as usual...)

Erik

> Conditional ACK on moving/merging with patch 13. Beyond that just a
> small justification for the separation...
> 
> w/r/t: memory leak on output, that will probably raise the ire of
> Coverity or running with valgrind.
> 
> John
>> @@ -79,9 +80,10 @@ testLogParseFilters(const void *opaque)
>>  {
>>  int ret = -1;
>>  int nfilters;
>> +virLogFilterPtr *filters = NULL;
>>  const struct testLogData *data = opaque;
>>  
>> -nfilters = virLogParseAndDefineFilters(data->str);
>> +nfilters = virLogParseFilters(data->str, );
>>  if (nfilters < 0) {
>>  if (!data->pass) {
>>  VIR_TEST_DEBUG("Got expected error: %s\n",
>> @@ -101,7 +103,7 @@ testLogParseFilters(const void *opaque)
>>  
>>  ret = 0;
>>   cleanup:
>> -virLogReset();
>> +virLogFilterListFree(filters, nfilters);
>>  return ret;
>>  }
>>  
>>





signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised

[Martin Kletzander]

On Thu, Oct 06, 2016 at 10:59:06AM +0100, Ian Jackson wrote:

Martin Kletzander writes ("Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt 
save/restore when migration not advertised"):

Since offline migration (as in migrating a domain between hosts without
being running) is not that used in the code and talked about, I'm
guessing offline means save restore.  Looking at the history it was
added before the "offline" migration, so it probably means
save/restore.  To avoid confusion, I would suggest we add either
 or rather  (the naming is not important) and document
what it means.  And then you can use it exactly how you'd like.  And
you'll be also sure it means what you need it to mean ;)  The patches
will be straigh-forward, let me know if I can help anyhow.


Except that the point of the exercise is to detect which features are
supported in which versions.  Whatever I do in osstest needs to work
with older libvirt versions, which do not report
 /capabilities/host/migration_features/save
even on x86, where it is supported.  I suppose I could detect
 /capabilities/host/migration_features/live
and assume that save/restore was supported (since it's unlikely that
live migration would be supported but not save/restore).

So for now I think I need to use
 /capabilities/host/migration_features
as a proxy for save/restore ?



Well then, unfortunately you do.

Also, looking at how the code is structured, if you have live migration
but don't have save/restore, you won't have  there
at all.


Ian.


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 11/20] virlog: Introduce virLogParseOutputs

[Erik Skultety]

>> +
>> +VIR_DEBUG("outputs=%s", src);
>> +
>> +if (!(strings = virStringSplit(src, " ", 0)))
> 
> You could use the Count version and then...
> 
>> +goto cleanup;
>> +
>> +for (i = 0; strings[i]; i++) {
> 
> ...rather than strings[i], it's < count

Well, this way we spared one unnecessary variable, but whatever, I can
always add it there to make it more readable I guess.

> 
>> +/* virStringSplit may return empty strings */
>> +if (STREQ(strings[i], ""))
>> +continue;
>> +
>> +if (!(output = virLogParseOutput(strings[i])))
>> +goto cleanup;
>> +
>> +/* let's check if a duplicate output does not already exist in which
>> + * case we replace it with its last occurrence
>> + */
>> +if ((at = virLogFindOutput(list, noutputs, output->dest,
>> +   output->name)) >= 0) {
>> +virLogOutputFree(list[at]);
>> +VIR_DELETE_ELEMENT(list, at, noutputs);
>> +}
>> +
>> +if (VIR_APPEND_ELEMENT(list, noutputs, output) < 0) {
>> +virLogOutputFree(output);
> 
> In this case, the old one is also gone... So we've effectively removed
> it. Is that intentional?  or should the DELETE of 'at' occur after this
> successfully adds a new one?
> 
> IOW:
>at = virLogFindOutput()
>if (VIR_APPEND_ELEMENT() < 0)
> ...
>}
>if (at >= 0) {
>virLogOutputFree(list[at]);
>VIR_DELETE_ELEMENT();
>}
> 

Ouch, perfect catch, thanks, we would definitely lose the original if
the append failed.

>> +goto cleanup;
>> +}
>> +
>> +virLogOutputFree(output);
> 
> Is this right? I don't think it's necessary unless you change to using
> the _COPY append macro

I suppose you're right, since it issues memmove, I'll try some scenarios
to compare the behaviour though.

> 
> 
>> +}
>> +
>> +ret = noutputs;
>> +*outputs = list;
> 
> If you then set "list = NULL"...
> 
>> + cleanup:
>> +if (ret < 0)
> 
> ... the (ret < 0) is not necessary
> 
>> +virLogOutputListFree(list, noutputs);
>> +virStringFreeList(strings);
>> +return ret;
>> +}
>> diff --git a/src/util/virlog.h b/src/util/virlog.h
>> index e7f6b85..ed60c00 100644
>> --- a/src/util/virlog.h
>> +++ b/src/util/virlog.h
>> @@ -247,5 +247,6 @@ virLogOutputPtr virLogNewOutputToSyslog(virLogPriority 
>> priority,
>>  virLogOutputPtr virLogNewOutputToJournald(int priority);
>>  virLogOutputPtr virLogParseOutput(const char *src);
>>  virLogFilterPtr virLogParseFilter(const char *src);
>> +int virLogParseOutputs(const char *src, virLogOutputPtr **outputs);
> 
> s/;/ATTRIBUTE_NONNULL(1);
> 
> Conditional ACK - guess I'm curious how the duplication and error path
> issue falls out...
>

Thanks a lot.
Erik

> John
>>  
>>  #endif
>>





signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] RFC: Exposing "ready" bool (of `query-block-jobs`) or QMP BLOCK_JOB_READY event

[Kashyap Chamarthy]

Backround
-

For QEMU block device jobs, the "ready" boolean field (part of QMP
`query-block-jobs`) was introduced in commit ef6dbf1 (available in QEMU
v2.2.0 or above):

http://git.qemu.org/?p=qemu.git;a=commitdiff;h=ef6dbf1e4 --
blockjob: Add "ready" field

"When a block job signals readiness, this is currently reported only
through QMP. If qemu wants to use block jobs for internal tasks,
there needs to be another way to correctly detect when a block job
may be completed.

For this reason, introduce a bool "ready" which is set when the
block job may be completed."


And, libvirt was fixed to use the above field in this commit (available
in libvirt v1.2.18 or above):

http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=eae5924 -- qemu:
Update state of block job to READY only if it actually is ready


RFC
---

Currently libvirt block APIs (& consequently higher-level applications
like Nova which use these APIs) rely on polling for job completion via
virDomainGetBlockJobInfo(), which uses QMP `query-block-jobs`, and
waits for QEMU to report "offset" == "len", which translates to libvirt
"cur" == "end".  Based on this, libvirt can take an action (whether to
gracefully abort, or pivot to the copy in case of a COPY job).

Since QEMU reports the "ready": true field (followed by a
BLOCK_JOB_READY QMP event).  It would be helpful if libvirt expose this
via an API, so upper layers could instead use that, rather than polling.


Problem scenario


When virDomainBlockRebase() is invoked to start a copy job, then
aborting the said copy operation with virDomainBlockJobAbort() + flag
VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT can result in a potential race
condition (due to the way the virDomainGetBlockJobInfo() reports the job
status) where the pivot operation fails.

Race condition window
~

libvirt finds cur==end AND sends a pivot request, all in the window
before QEMU would have sent "ready": true field [emitted as part of the
QMP `query-block-jobs` command's response, indicating that the job has
actually completed], however the pivot request fails because it requires
"ready": true.

So Eric Blake suggests:

QEMU 2.0 or 1.x probably had a synchronous setup where you could
never observer cur==end on a non-ready job. But I don't remember
enough history to point to when QEMU switched jobs to be a bit more
asynchronous.  Maybe there was no qemu regression - maybe it was
BECAUSE of other block-job additions in 2.2 that offset==len was no
longer reliable.  I don't know that for sure.

But what it DOES sound like is that IF qemu reports "ready": false,
offset==len is not reliable, and libvirt should be taught to fudge
that.

And hopefully, QEMU too old to report "ready:" at all is reliable
with regards to offset==len, because that's all we have to go by.


For now, I filed this upstream libvirt bug:

https://bugzilla.redhat.com/show_bug.cgi?id=1382165 --
virDomainGetBlockJobInfo: Adjust job reporting based on QEMU stats &
the "ready" field of `query-block-jobs`

However, exposing the "ready" boolean from QMP `query-block-jobs` might
be worth considering.

-- 
/kashyap

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

Re: [libvirt] [OSSTEST PATCH 1/2] libvirt: Check migration capabilities using proper XML parser

[Ian Jackson]

Julien Grall writes ("Re: [OSSTEST PATCH 1/2] libvirt: Check migration 
capabilities using proper XML parser"):
> On 04/10/2016 10:05, Ian Jackson wrote:
> > Missing _.  I didn't test this again before sending it.  I'd still
> > like a review from libvirt (and Xen/ARM) folks.
> 
> I am not sure what kind of input you would need from Xen/ARM. This patch 
> set looks very libvirt specific.

It is.  Sorry to spam you with the whole thread.  The only thing I
need from Xen/ARM people is a sanity check that my understanding of
the current and likely future supported features is correct.

Thanks,
Ian.

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

Re: [libvirt] [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised

[Ian Jackson]

Martin Kletzander writes ("Re: [OSSTEST PATCH 2/2] libvirt: Do not attempt 
save/restore when migration not advertised"):
> Since offline migration (as in migrating a domain between hosts without
> being running) is not that used in the code and talked about, I'm
> guessing offline means save restore.  Looking at the history it was
> added before the "offline" migration, so it probably means
> save/restore.  To avoid confusion, I would suggest we add either
>  or rather  (the naming is not important) and document
> what it means.  And then you can use it exactly how you'd like.  And
> you'll be also sure it means what you need it to mean ;)  The patches
> will be straigh-forward, let me know if I can help anyhow.

Except that the point of the exercise is to detect which features are
supported in which versions.  Whatever I do in osstest needs to work
with older libvirt versions, which do not report
  /capabilities/host/migration_features/save
even on x86, where it is supported.  I suppose I could detect
  /capabilities/host/migration_features/live
and assume that save/restore was supported (since it's unlikely that
live migration would be supported but not save/restore).

So for now I think I need to use
  /capabilities/host/migration_features
as a proxy for save/restore ?

Ian.

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

Re: [libvirt] [OSSTEST PATCH 2/2] libvirt: Do not attempt save/restore when migration not advertised

[Martin Kletzander]

On Wed, Oct 05, 2016 at 06:06:29PM -0600, Jim Fehlig wrote:

On 10/04/2016 11:02 AM, Ian Jackson wrote:

Currently, osstest wrongly thinks that ARM can do save/restore,
because `virsh help' does mention the save command (on all
architectures).

Additionally, check the virth capabilities xpath
   /capabilities/host/migration_features
to try to see whether this host supports migration.

I am not sure if this is the right path to check.  Perhaps
   /capabilities/host/migration_features/live
is more correct, but this may be wrong if Xen comes to support save/restore
on ARM, but not live migration (but perhaps libvirt cannot express this
distinction in which case perhaps it's right after all).


Looking at the capabilities generation code again, I see that
virCapabilitiesNew() takes 'offlineMigrate' and 'liveMigrate' parameters. I
assume offline in this context means save, copy, restore. Martin, is that
assumption correct?



The thing is that it's not documented.  I can't even say "enough", it's
more like "at all".  You can have a look at it:

 https://libvirt.org/formatcaps.html

It doesn't even talk about , just , but
I guess that's the same thing.

Since offline migration (as in migrating a domain between hosts without
being running) is not that used in the code and talked about, I'm
guessing offline means save restore.  Looking at the history it was
added before the "offline" migration, so it probably means
save/restore.  To avoid confusion, I would suggest we add either
 or rather  (the naming is not important) and document
what it means.  And then you can use it exactly how you'd like.  And
you'll be also sure it means what you need it to mean ;)  The patches
will be straigh-forward, let me know if I can help anyhow.


If so, I think the saverestore_check() below can look for
/capabilities/host/migration_features. The migration check in 1/2 can look for
/capabilities/host/migration_features/live. Is it fair to assume save/restore is
available when live migration is supported?



With that you could straight check for  and  ;)

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] virt-yajl: Fix detection of yajl requirements

[Martin Kletzander]

Running the output of qemu -help doesn't make any sense.  We should be
looking for libvirt being mentioned in the output.  This worked by
accident, let's make it work as expected it to.

Signed-off-by: Martin Kletzander 
---
 m4/virt-yajl.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/m4/virt-yajl.m4 b/m4/virt-yajl.m4
index adf2819eeb66..8c452adca653 100644
--- a/m4/virt-yajl.m4
+++ b/m4/virt-yajl.m4
@@ -26,7 +26,7 @@ AC_DEFUN([LIBVIRT_CHECK_YAJL],[
 AC_PATH_PROGS([QEMU], [qemu-kvm qemu kvm qemu-system-x86_64],
   [], [$PATH:/usr/bin:/usr/libexec])
 if test -x "$QEMU"; then
-  if `$QEMU -help | grep libvirt` >/dev/null; then
+  if $QEMU -help 2>/dev/null | grep -q libvirt; then
 with_yajl=yes
   else
 [qemu_version_sed='s/.*ersion \([0-9.,]*\).*/\1/']
-- 
2.10.1

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

Re: [libvirt] [PATCH] vsh: Fix warnings in command line completer

[Erik Skultety]

On 06/10/16 09:57, Michal Privoznik wrote:
> On 05.10.2016 09:26, Jiri Denemark wrote:
>> GCC complained that
>>
>> vsh.c: In function 'vshReadlineOptionsGenerator':
>> vsh.c:2622:29: warning: unused variable 'opt' [-Wunused-variable]
>>  const vshCmdOptDef *opt = >opts[list_index];
>>  ^
>> vsh.c: In function 'vshReadlineParse':
>> vsh.c:2830:44: warning: 'opt' may be used uninitialized in this function
>> [-Wmaybe-uninitialized]
>>  completed_list = opt->completer(autoCompleteOpaque,
>>
>> Signed-off-by: Jiri Denemark 
>> ---
>>
>> Notes:
>> I'm not very fond of the second hunk, but the completer is such
>> a horrible piece of code I couldn't believe it was pushed. I just
>> made the easiest fix and ran away from the code screaming.
> 
> Well, this area has always been a mess. And I think it is mostly because
> of how readline API works. The completer is called multiple times and we
> have to do a lot in it in order to provide all the completions.
> Having said that, I'm not sure what can be done here to improve the code
> apart from rewriting it completely from scratch (or switching to a
> different library, e.g. libedit).
> 

Well, depending on which parts exactly you meant need rewriting (if of
course you didn't refer to the whole virsh code then all of the
following is just ambiguous) because even if you tried to rewrite just
parts of it, there's imho very little chance you'd end up with a less
mess that it is now and that is because there are parts of the code that
would prevent you from doing so like the fact that we allow multiple
commands delimited by ';' in the interactive shell. I've looked into
that at least 3 times already and still think that the biggest mess is
the parser which we should start from. It is very unfortunate that we
allow multiple commands simultaneously, since that is preventing us from
using an external library to do the parsing for us in a transparent way.
The code would look sooo much cooler then...sigh...

Erik

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




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 20/20] virlog: Split parsing and setting priority

[Erik Skultety]

On 21/09/16 22:01, John Ferlan wrote:
> 
> 
> On 08/18/2016 07:47 AM, Erik Skultety wrote:
>> Handling of outputs and filters has been changed in a way that splits
>> parsing and defining. Do the same thing for logging priority as well, this
>> however, doesn't need much of a preparation.
>> ---
>>  src/util/virlog.c | 21 +
>>  tests/eventtest.c |  3 ++-
>>  2 files changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/util/virlog.c b/src/util/virlog.c
>> index 713cd0c..683cf6b 100644
>> --- a/src/util/virlog.c
>> +++ b/src/util/virlog.c
>> @@ -220,7 +220,9 @@ int
>>  virLogSetDefaultPriority(virLogPriority priority)
>>  {
>>  if ((priority < VIR_LOG_DEBUG) || (priority > VIR_LOG_ERROR)) {
>> -VIR_WARN("Ignoring invalid log level setting.");
>> +virReportError(VIR_ERR_INVALID_ARG,
>> +   _("Failed to set logging priority, argument '%u' is "
>> + "invalid"), priority);
> 
> By this point I was too lazy to check, but we're not going to start
> seeing strange failures from some rogue/incorrect setting that
> previously essentially ignored this.
> 

No, we won't. The only thing that changed is that now we'll see issues
logged as errors rather than warnings, but so far, each of the original
callers ignores the outcome of the function, so no unexpected
crashes/behaviour related to incorrect setting shouldn't take place, if
the new setting is incorrect, the original setting will stay intact.

Erik

>>  return -1;
>>  }
>>  if (virLogInitialize() < 0)
>> @@ -1158,20 +1160,15 @@ virLogGetNbOutputs(void)
> 
> 
> You'll need to update the comments to this function about treturn values...
> 
> Conditional ACK w/ adjustment and assurance about the above usage of
> virReportError vs. VIR_WARN
> 
> 
> John




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] qemu: report block job errors from qemu to the user

[Nikolay Shirokovskiy]

On 06.10.2016 11:01, Jiri Denemark wrote:
> On Wed, Oct 05, 2016 at 16:52:10 +0300, Nikolay Shirokovskiy wrote:
>> ---
>>  src/qemu/qemu_blockjob.c | 13 +++--
>>  src/qemu/qemu_blockjob.h |  3 ++-
>>  src/qemu/qemu_domain.h   |  1 +
>>  src/qemu/qemu_driver.c   |  4 ++--
>>  src/qemu/qemu_migration.c| 34 ++
>>  src/qemu/qemu_monitor.c  |  5 +++--
>>  src/qemu/qemu_monitor.h  |  4 +++-
>>  src/qemu/qemu_monitor_json.c |  2 +-
>>  src/qemu/qemu_process.c  |  3 +++
>>  9 files changed, 48 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
>> index 83a5a3f..937d931 100644
>> --- a/src/qemu/qemu_blockjob.c
>> +++ b/src/qemu/qemu_blockjob.c
>> @@ -33,6 +33,7 @@
>>  #include "virstoragefile.h"
>>  #include "virthread.h"
>>  #include "virtime.h"
>> +#include "viralloc.h"
>>  
>>  #define VIR_FROM_THIS VIR_FROM_QEMU
>>  
>> @@ -53,16 +54,24 @@ VIR_LOG_INIT("qemu.qemu_blockjob");
>>  int
>>  qemuBlockJobUpdate(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> -   virDomainDiskDefPtr disk)
>> +   virDomainDiskDefPtr disk,
>> +   char **error)
>>  {
>>  qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>>  int status = diskPriv->blockJobStatus;
>>  
>> +if (error)
>> +*error = NULL;
>> +
>>  if (status != -1) {
>>  qemuBlockJobEventProcess(driver, vm, disk,
>>   diskPriv->blockJobType,
>>   diskPriv->blockJobStatus);
>>  diskPriv->blockJobStatus = -1;
>> +if (error)
>> +VIR_STEAL_PTR(*error, diskPriv->blockJobHint);
>> +else
>> +VIR_FREE(diskPriv->blockJobHint);
> 
> What if qemuBlockJobUpdate is never called? Shouldn't
> qemuBlockJobEventProcess be the place to free the error?

blockJobHint is used only in block job "sync" mode, thus
user will call qemuBlockJobSyncEnd and it will take care.

> 
>>  }
>>  
>>  return status;
>> @@ -241,6 +250,6 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver,
>>  virDomainDiskDefPtr disk)
>>  {
>>  VIR_DEBUG("disk=%s", disk->dst);
>> -qemuBlockJobUpdate(driver, vm, disk);
>> +qemuBlockJobUpdate(driver, vm, disk, NULL);
>>  QEMU_DOMAIN_DISK_PRIVATE(disk)->blockJobSync = false;
>>  }
>> diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
>> index 775ce95..c452edc 100644
>> --- a/src/qemu/qemu_blockjob.h
>> +++ b/src/qemu/qemu_blockjob.h
>> @@ -27,7 +27,8 @@
>>  
>>  int qemuBlockJobUpdate(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> -   virDomainDiskDefPtr disk);
>> +   virDomainDiskDefPtr disk,
>> +   char **error);
>>  void qemuBlockJobEventProcess(virQEMUDriverPtr driver,
>>virDomainObjPtr vm,
>>virDomainDiskDefPtr disk,
>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>> index 521531b..79d88fa 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
>> @@ -290,6 +290,7 @@ struct _qemuDomainDiskPrivate {
>>  /* for some synchronous block jobs, we need to notify the owner */
>>  int blockJobType;   /* type of the block job from the event */
>>  int blockJobStatus; /* status of the finished block job */
>> +char *blockJobHint; /* hint from block job event (like error message) */
> 
> So why is this called blockJobHint if it's used for storing the errors?
> I think blockJobError would be a better name...

Different qemu blockjob events use the same interface on the notification path.
Ready, cancelled, failed, complete. I doubt 'error' can be applied
to any of them except "failed", so I decided choose a more neutral name.
It's like void* in namespace of variable names )) Anyway it is not that
principal, I just wanted to explain my POV

Nikolay

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

Re: [libvirt] [libvirt-glib 03/20] gconfig: Introduce GVirConfigDomainGraphicsListenAddress

[Daniel P. Berrange]

On Wed, Oct 05, 2016 at 09:37:52AM +0200, Christophe Fergeau wrote:
> hey,
> 
> On Tue, Oct 04, 2016 at 04:30:14PM +0100, Daniel P. Berrange wrote:
> > > +
> > > +/**
> > > + * gvir_config_domain_graphics_listen_address_get_inet_address:
> > > + *
> > > + * Returns the #GInetAddress associated with the 
> > > #GVirConfigDomainGraphicsListenAddress.
> > > + *
> > > + * Returns: (transfer full): a #GInetAddress.
> > > + *
> > > + */
> > > +GInetAddress *
> > > +gvir_config_domain_graphics_listen_address_get_inet_address(GVirConfigDomainGraphicsListenAddress
> > >  *listen)
> > > +{
> > > +
> > > g_return_val_if_fail(GVIR_CONFIG_IS_DOMAIN_GRAPHICS_LISTEN_ADDRESS(listen),
> > >  NULL);
> > > +
> > > +const gchar *address = 
> > > gvir_config_object_get_attribute(GVIR_CONFIG_OBJECT(listen),
> > > +NULL,
> > > +"address");
> > > +return g_inet_address_new_from_string(address);
> > > +}
> > 
> > IIUC GInetAddress only supports numeric IP addresses, where as libvirt
> > also allows hostnames anywhere that an IP address is used. So apps
> > using get_inet_address are liable to get NULL returned if we use
> > GInetAddress.
> > 
> > So IMHO we should not use GInetAddress as it'd lead to apps which
> > blindly use this API not realising they'll break if someone put a
> > hostname in the XML until it is too late.
> 
> Good point. Should we keep _set_inet_address() which would limit what we
> can set, but would not lead to unexpected results depending on the XML
> content, or should we just drop the GInetAddress altogether, and only
> keep the string based API (which is what Visarion initially proposed).

I think I'd just drop GInetAddress completely to avoid confusion.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH 2/3] qemu: use blockjob completed event's error field to detect errors

[Nikolay Shirokovskiy]

On 06.10.2016 11:02, Peter Krempa wrote:
> On Wed, Oct 05, 2016 at 16:52:09 +0300, Nikolay Shirokovskiy wrote:
>> BLOCK_JOB_COMPLETED has error field set on error from day one (12bd451f)
>> thus there is no need to guess for error. Is it true that when
>> len == offset then can be no error?
> 
> That is the question you should be able to answer when sending a patch,
> not a content for the commit message.
> 
> AFAIK a copy job can fail even after all data was copied while syncing
> the buffers so this change makes sense.
> 
> Additionally qemu also supports the BLOCK_JOB_ERROR event which is
> currently not handled by qemu. I'll need to have a look into it.
> 
>> ---
>>  src/qemu/qemu_monitor_json.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index 6c2884d..8218e12 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -801,6 +801,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
>>int event)
>>  {
>>  const char *device;
>> +const char *error = NULL;
>>  const char *type_str;
>>  int type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
>>  unsigned long long offset, len;
>> @@ -834,8 +835,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
>>  
>>  switch ((virConnectDomainEventBlockJobStatus) event) {
>>  case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
>> -/* Make sure the whole device has been processed */
>> -if (offset != len)
>> +if ((error = virJSONValueObjectGetString(data, "error")))
> 
> I'd prefer if you'd keep the original check as long as adding the check
> for the error field.

Like for safety reasons? Ok.

> 
> The 'error' variable is not used besides setting it twice, so you
> apparently don't need it at all.

This is intentional. Or next patch will touch this place one more time.
If it does't matter then ok.

> 
>>  event = VIR_DOMAIN_BLOCK_JOB_FAILED;
>>  break;
>>  case VIR_DOMAIN_BLOCK_JOB_CANCELED:

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

Re: [libvirt] [PATCH 2/3] qemu: use blockjob completed event's error field to detect errors

[Peter Krempa]

On Wed, Oct 05, 2016 at 16:52:09 +0300, Nikolay Shirokovskiy wrote:
> BLOCK_JOB_COMPLETED has error field set on error from day one (12bd451f)
> thus there is no need to guess for error. Is it true that when
> len == offset then can be no error?

That is the question you should be able to answer when sending a patch,
not a content for the commit message.

AFAIK a copy job can fail even after all data was copied while syncing
the buffers so this change makes sense.

Additionally qemu also supports the BLOCK_JOB_ERROR event which is
currently not handled by qemu. I'll need to have a look into it.

> ---
>  src/qemu/qemu_monitor_json.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 6c2884d..8218e12 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -801,6 +801,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
>int event)
>  {
>  const char *device;
> +const char *error = NULL;
>  const char *type_str;
>  int type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
>  unsigned long long offset, len;
> @@ -834,8 +835,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
>  
>  switch ((virConnectDomainEventBlockJobStatus) event) {
>  case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
> -/* Make sure the whole device has been processed */
> -if (offset != len)
> +if ((error = virJSONValueObjectGetString(data, "error")))

I'd prefer if you'd keep the original check as long as adding the check
for the error field.

The 'error' variable is not used besides setting it twice, so you
apparently don't need it at all.

>  event = VIR_DOMAIN_BLOCK_JOB_FAILED;
>  break;
>  case VIR_DOMAIN_BLOCK_JOB_CANCELED:


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] qemu: report block job errors from qemu to the user

[Jiri Denemark]

On Wed, Oct 05, 2016 at 16:52:10 +0300, Nikolay Shirokovskiy wrote:
> ---
>  src/qemu/qemu_blockjob.c | 13 +++--
>  src/qemu/qemu_blockjob.h |  3 ++-
>  src/qemu/qemu_domain.h   |  1 +
>  src/qemu/qemu_driver.c   |  4 ++--
>  src/qemu/qemu_migration.c| 34 ++
>  src/qemu/qemu_monitor.c  |  5 +++--
>  src/qemu/qemu_monitor.h  |  4 +++-
>  src/qemu/qemu_monitor_json.c |  2 +-
>  src/qemu/qemu_process.c  |  3 +++
>  9 files changed, 48 insertions(+), 21 deletions(-)
> 
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index 83a5a3f..937d931 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -33,6 +33,7 @@
>  #include "virstoragefile.h"
>  #include "virthread.h"
>  #include "virtime.h"
> +#include "viralloc.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> @@ -53,16 +54,24 @@ VIR_LOG_INIT("qemu.qemu_blockjob");
>  int
>  qemuBlockJobUpdate(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> -   virDomainDiskDefPtr disk)
> +   virDomainDiskDefPtr disk,
> +   char **error)
>  {
>  qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>  int status = diskPriv->blockJobStatus;
>  
> +if (error)
> +*error = NULL;
> +
>  if (status != -1) {
>  qemuBlockJobEventProcess(driver, vm, disk,
>   diskPriv->blockJobType,
>   diskPriv->blockJobStatus);
>  diskPriv->blockJobStatus = -1;
> +if (error)
> +VIR_STEAL_PTR(*error, diskPriv->blockJobHint);
> +else
> +VIR_FREE(diskPriv->blockJobHint);

What if qemuBlockJobUpdate is never called? Shouldn't
qemuBlockJobEventProcess be the place to free the error?

>  }
>  
>  return status;
> @@ -241,6 +250,6 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver,
>  virDomainDiskDefPtr disk)
>  {
>  VIR_DEBUG("disk=%s", disk->dst);
> -qemuBlockJobUpdate(driver, vm, disk);
> +qemuBlockJobUpdate(driver, vm, disk, NULL);
>  QEMU_DOMAIN_DISK_PRIVATE(disk)->blockJobSync = false;
>  }
> diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
> index 775ce95..c452edc 100644
> --- a/src/qemu/qemu_blockjob.h
> +++ b/src/qemu/qemu_blockjob.h
> @@ -27,7 +27,8 @@
>  
>  int qemuBlockJobUpdate(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> -   virDomainDiskDefPtr disk);
> +   virDomainDiskDefPtr disk,
> +   char **error);
>  void qemuBlockJobEventProcess(virQEMUDriverPtr driver,
>virDomainObjPtr vm,
>virDomainDiskDefPtr disk,
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 521531b..79d88fa 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -290,6 +290,7 @@ struct _qemuDomainDiskPrivate {
>  /* for some synchronous block jobs, we need to notify the owner */
>  int blockJobType;   /* type of the block job from the event */
>  int blockJobStatus; /* status of the finished block job */
> +char *blockJobHint; /* hint from block job event (like error message) */

So why is this called blockJobHint if it's used for storing the errors?
I think blockJobError would be a better name...

And you forgot to free it in qemuDomainDiskPrivateDispose.

>  bool blockJobSync; /* the block job needs synchronized termination */
>  
>  bool migrating; /* the disk is being migrated */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ee16cb5..ac204c3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16280,13 +16280,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
>   VIR_DOMAIN_BLOCK_JOB_CANCELED);
>  } else {
>  qemuDomainDiskPrivatePtr diskPriv = 
> QEMU_DOMAIN_DISK_PRIVATE(disk);
> -qemuBlockJobUpdate(driver, vm, disk);
> +qemuBlockJobUpdate(driver, vm, disk, NULL);
>  while (diskPriv->blockjob) {
>  if (virDomainObjWait(vm) < 0) {
>  ret = -1;
>  goto endjob;
>  }
> -qemuBlockJobUpdate(driver, vm, disk);
> +qemuBlockJobUpdate(driver, vm, disk, NULL);
>  }
>  }
>  }
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 8a220d9..0671e23 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1853,17 +1853,20 @@ qemuMigrationDriveMirrorReady(virQEMUDriverPtr driver,
>  for (i = 0; i < vm->def->ndisks; i++) {
>  virDomainDiskDefPtr disk = vm->def->disks[i];
>  qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +char *error;
>  
>  if (!diskPriv->migrating)
>   

Re: [libvirt] [PATCH] vsh: Fix warnings in command line completer

[Michal Privoznik]

On 05.10.2016 09:26, Jiri Denemark wrote:
> GCC complained that
> 
> vsh.c: In function 'vshReadlineOptionsGenerator':
> vsh.c:2622:29: warning: unused variable 'opt' [-Wunused-variable]
>  const vshCmdOptDef *opt = >opts[list_index];
>  ^
> vsh.c: In function 'vshReadlineParse':
> vsh.c:2830:44: warning: 'opt' may be used uninitialized in this function
> [-Wmaybe-uninitialized]
>  completed_list = opt->completer(autoCompleteOpaque,
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> I'm not very fond of the second hunk, but the completer is such
> a horrible piece of code I couldn't believe it was pushed. I just
> made the easiest fix and ran away from the code screaming.

Well, this area has always been a mess. And I think it is mostly because
of how readline API works. The completer is called multiple times and we
have to do a lot in it in order to provide all the completions.
Having said that, I'm not sure what can be done here to improve the code
apart from rewriting it completely from scratch (or switching to a
different library, e.g. libedit).

Michal

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

Re: [libvirt] [[PATCH v2] 0/4] try harder to get dest qemu errors on migation

[Nikolay Shirokovskiy]

On 06.10.2016 10:29, Peter Krempa wrote:
> On Thu, Oct 06, 2016 at 10:23:05 +0300, Nikolay Shirokovskiy wrote:
>>
>>
>> On 05.10.2016 18:13, Peter Krempa wrote:
>>> On Mon, Sep 12, 2016 at 17:34:39 +0300, Nikolay Shirokovskiy wrote:
 Hi, all.

   In case migration fails due to destination qemu exits unexpectedly user
 recevies the qemu log in the error message. Unfortunately log is truncated 
 and
 the most interesting part is missed (below is the example of such a log 
 [1]).

 Actually for the most cases the first patch will be enough to fix the 
 issue.
 Originally I thought the problem is qemu logging and reading the log are 
 not in
 sync (which is true) so I tried to fix it as well in the next patches.

 * diff from v1:

 1. split changes to libvirtd and virtlogd to different patches
 2. split virtlogd patch further
 3. simplify handling eofs and hangups in draining function

 [1] log example:

 CPU Reset (CPU 0)
 EAX= EBX= ECX= EDX=
 ESI= EDI= EBP= ESP=
 EIP= EFL= [---] CPL=0 II=0 A20=0 SMM=0 HLT=0
 ES =   
 CS =   
 SS =   
 DS =   
 FS =   
 GS =   
 LDT=   
 TR =   
 GDT=  
 IDT=  
 CR0= CR2= CR3= CR4=
 DR0= DR1= DR2= 
 DR3= 
 DR6= DR7=
 CCS= CCD= CCO=DYNAMIC 
 EFER=
 FCW= FSW= [ST=0] FTW=ff MXCSR=
 FPR0=  FPR1= 
 FPR2=  FPR3= 
 FPR4=  FPR5= 
 FPR6=  FPR7= 
 XMM00= 
 XMM01=
 XMM02= 
 XMM03=
 XMM04= 
 XMM05=
 XMM06= 
 XMM07=
 CPU Reset (CPU 1)
 EAX= EBX= ECX= EDX=000206a1
 ESI= EDI= EBP= ESP=
 EIP=fff0 EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
 ES =   9300
 CS =f000   9b00
 SS =   9300
 DS =   9300
 FS =   9300
 GS =   9300
 LDT=   8200
 TR =   8b00
 GDT=  
 IDT=  
 CR0=6010 CR2= CR3= CR4=
 DR0= DR1= DR2= 
 DR3= 
 DR6=0ff0 DR7=0400
 CCS= CCD= CCO=DYNAMIC 
 EFER=
 FCW=037f FSW= [ST=0] FTW=00 MXCSR=1f80
 FPR0=  FPR1= 
 FPR2=  FPR3= 
 FPR4=  FPR5= 
 FPR6=  FPR7= 
 XMM00= 
 XMM01=
 XMM02= 
 XMM03=
 XMM04= 
 XMM05=
 XMM06= XMM07=000
 qemu: terminating on signal 15 from pid 168133
>>>
>>> I don't think that reporting all of the above is a good idea. We should
>>> perhaps report at most two last lines.
>>>
>>
>> We already report about half of this, this patch just removes random 
>> truncation.
>> As to most two lines, AFAIU one can not say what part of this log will be
>> useful for crash investigation.
> 
> This is not about the log but about the error message. The error message
> containing ALL of the above stuff is useless for any user. For crash
> investigation you can always get the full log from the actual log file.

Isn't leaving 1-2 lines is random? Sometimes it will help, sometimes not.
If before die qemu writes 10 lines (besides registers dump) the first line
will probably be the most interesting. I think we'd better just print
something like "qemu died" ("go and see it's log if you want to").

> 
> When I've implemented this I did not see such error message. I'd
> otherwise truncate it to the end 

Re: [libvirt] [[PATCH v2] 0/4] try harder to get dest qemu errors on migation

[Jiri Denemark]

On Thu, Oct 06, 2016 at 10:23:05 +0300, Nikolay Shirokovskiy wrote:
> 
> 
> On 05.10.2016 18:13, Peter Krempa wrote:
> > On Mon, Sep 12, 2016 at 17:34:39 +0300, Nikolay Shirokovskiy wrote:
> >> Hi, all.
> >>
> >>   In case migration fails due to destination qemu exits unexpectedly user
> >> recevies the qemu log in the error message. Unfortunately log is truncated 
> >> and
> >> the most interesting part is missed (below is the example of such a log 
> >> [1]).
> >>
> >> Actually for the most cases the first patch will be enough to fix the 
> >> issue.
> >> Originally I thought the problem is qemu logging and reading the log are 
> >> not in
> >> sync (which is true) so I tried to fix it as well in the next patches.
> >>
> >> * diff from v1:
> >>
> >> 1. split changes to libvirtd and virtlogd to different patches
> >> 2. split virtlogd patch further
> >> 3. simplify handling eofs and hangups in draining function
> >>
> >> [1] log example:
> >>
> >> CPU Reset (CPU 0)
> >> EAX= EBX= ECX= EDX=
> >> ESI= EDI= EBP= ESP=
> >> EIP= EFL= [---] CPL=0 II=0 A20=0 SMM=0 HLT=0
> >> ES =   
> >> CS =   
> >> SS =   
> >> DS =   
> >> FS =   
> >> GS =   
> >> LDT=   
> >> TR =   
> >> GDT=  
> >> IDT=  
> >> CR0= CR2= CR3= CR4=
> >> DR0= DR1= DR2= 
> >> DR3= 
> >> DR6= DR7=
> >> CCS= CCD= CCO=DYNAMIC 
> >> EFER=
> >> FCW= FSW= [ST=0] FTW=ff MXCSR=
> >> FPR0=  FPR1= 
> >> FPR2=  FPR3= 
> >> FPR4=  FPR5= 
> >> FPR6=  FPR7= 
> >> XMM00= 
> >> XMM01=
> >> XMM02= 
> >> XMM03=
> >> XMM04= 
> >> XMM05=
> >> XMM06= 
> >> XMM07=
> >> CPU Reset (CPU 1)
> >> EAX= EBX= ECX= EDX=000206a1
> >> ESI= EDI= EBP= ESP=
> >> EIP=fff0 EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
> >> ES =   9300
> >> CS =f000   9b00
> >> SS =   9300
> >> DS =   9300
> >> FS =   9300
> >> GS =   9300
> >> LDT=   8200
> >> TR =   8b00
> >> GDT=  
> >> IDT=  
> >> CR0=6010 CR2= CR3= CR4=
> >> DR0= DR1= DR2= 
> >> DR3= 
> >> DR6=0ff0 DR7=0400
> >> CCS= CCD= CCO=DYNAMIC 
> >> EFER=
> >> FCW=037f FSW= [ST=0] FTW=00 MXCSR=1f80
> >> FPR0=  FPR1= 
> >> FPR2=  FPR3= 
> >> FPR4=  FPR5= 
> >> FPR6=  FPR7= 
> >> XMM00= 
> >> XMM01=
> >> XMM02= 
> >> XMM03=
> >> XMM04= 
> >> XMM05=
> >> XMM06= XMM07=000
> >> qemu: terminating on signal 15 from pid 168133
> > 
> > I don't think that reporting all of the above is a good idea. We should
> > perhaps report at most two last lines.
> > 
> 
> We already report about half of this, this patch just removes random 
> truncation.
> As to most two lines, AFAIU one can not say what part of this log will be
> useful for crash investigation.

Well, we're talking about error messages. And I doubt anyone would like
to see a 50 lines long error message. We have logs for investigating
crashes, the user should just get a reasonable message about why QEMU
died, i.e., showing something like the last two lines from the log looks
like a reasonable thing to do.

Jirka

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

Re: [libvirt] [[PATCH v2] 0/4] try harder to get dest qemu errors on migation

[Peter Krempa]

On Thu, Oct 06, 2016 at 10:23:05 +0300, Nikolay Shirokovskiy wrote:
> 
> 
> On 05.10.2016 18:13, Peter Krempa wrote:
> > On Mon, Sep 12, 2016 at 17:34:39 +0300, Nikolay Shirokovskiy wrote:
> >> Hi, all.
> >>
> >>   In case migration fails due to destination qemu exits unexpectedly user
> >> recevies the qemu log in the error message. Unfortunately log is truncated 
> >> and
> >> the most interesting part is missed (below is the example of such a log 
> >> [1]).
> >>
> >> Actually for the most cases the first patch will be enough to fix the 
> >> issue.
> >> Originally I thought the problem is qemu logging and reading the log are 
> >> not in
> >> sync (which is true) so I tried to fix it as well in the next patches.
> >>
> >> * diff from v1:
> >>
> >> 1. split changes to libvirtd and virtlogd to different patches
> >> 2. split virtlogd patch further
> >> 3. simplify handling eofs and hangups in draining function
> >>
> >> [1] log example:
> >>
> >> CPU Reset (CPU 0)
> >> EAX= EBX= ECX= EDX=
> >> ESI= EDI= EBP= ESP=
> >> EIP= EFL= [---] CPL=0 II=0 A20=0 SMM=0 HLT=0
> >> ES =   
> >> CS =   
> >> SS =   
> >> DS =   
> >> FS =   
> >> GS =   
> >> LDT=   
> >> TR =   
> >> GDT=  
> >> IDT=  
> >> CR0= CR2= CR3= CR4=
> >> DR0= DR1= DR2= 
> >> DR3= 
> >> DR6= DR7=
> >> CCS= CCD= CCO=DYNAMIC 
> >> EFER=
> >> FCW= FSW= [ST=0] FTW=ff MXCSR=
> >> FPR0=  FPR1= 
> >> FPR2=  FPR3= 
> >> FPR4=  FPR5= 
> >> FPR6=  FPR7= 
> >> XMM00= 
> >> XMM01=
> >> XMM02= 
> >> XMM03=
> >> XMM04= 
> >> XMM05=
> >> XMM06= 
> >> XMM07=
> >> CPU Reset (CPU 1)
> >> EAX= EBX= ECX= EDX=000206a1
> >> ESI= EDI= EBP= ESP=
> >> EIP=fff0 EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
> >> ES =   9300
> >> CS =f000   9b00
> >> SS =   9300
> >> DS =   9300
> >> FS =   9300
> >> GS =   9300
> >> LDT=   8200
> >> TR =   8b00
> >> GDT=  
> >> IDT=  
> >> CR0=6010 CR2= CR3= CR4=
> >> DR0= DR1= DR2= 
> >> DR3= 
> >> DR6=0ff0 DR7=0400
> >> CCS= CCD= CCO=DYNAMIC 
> >> EFER=
> >> FCW=037f FSW= [ST=0] FTW=00 MXCSR=1f80
> >> FPR0=  FPR1= 
> >> FPR2=  FPR3= 
> >> FPR4=  FPR5= 
> >> FPR6=  FPR7= 
> >> XMM00= 
> >> XMM01=
> >> XMM02= 
> >> XMM03=
> >> XMM04= 
> >> XMM05=
> >> XMM06= XMM07=000
> >> qemu: terminating on signal 15 from pid 168133
> > 
> > I don't think that reporting all of the above is a good idea. We should
> > perhaps report at most two last lines.
> > 
> 
> We already report about half of this, this patch just removes random 
> truncation.
> As to most two lines, AFAIU one can not say what part of this log will be
> useful for crash investigation.

This is not about the log but about the error message. The error message
containing ALL of the above stuff is useless for any user. For crash
investigation you can always get the full log from the actual log file.

When I've implemented this I did not see such error message. I'd
otherwise truncate it to the end since all the above in a error message
is clearly ridiculous.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] qemu: simplify switch case for blockjob events

[Nikolay Shirokovskiy]

On 05.10.2016 17:36, Jiri Denemark wrote:
> On Wed, Oct 05, 2016 at 16:52:08 +0300, Nikolay Shirokovskiy wrote:
>> ---
>>  src/qemu/qemu_monitor_json.c | 6 ++
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index e1494df..6c2884d 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -841,10 +841,8 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
>>  case VIR_DOMAIN_BLOCK_JOB_CANCELED:
>>  case VIR_DOMAIN_BLOCK_JOB_READY:
>>  break;
>> -case VIR_DOMAIN_BLOCK_JOB_FAILED:
>> -case VIR_DOMAIN_BLOCK_JOB_LAST:
>> -VIR_DEBUG("should not get here");
>> -break;
>> +default:
>> +VIR_WARN("unexpected block job type: %d", event);
>>  }
> 
> NACK, we intentionally don't use default branches. Default switch
> branch disables GCC warnings in case any enum item is not handled.
> 
> Runtime warnings are invisible while compile time warnings are very easy
> to spot.
> 

Ok, thanks for the explanation. Let's drop it.

Nikolay

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

Re: [libvirt] [[PATCH v2] 0/4] try harder to get dest qemu errors on migation

[Nikolay Shirokovskiy]

On 05.10.2016 18:13, Peter Krempa wrote:
> On Mon, Sep 12, 2016 at 17:34:39 +0300, Nikolay Shirokovskiy wrote:
>> Hi, all.
>>
>>   In case migration fails due to destination qemu exits unexpectedly user
>> recevies the qemu log in the error message. Unfortunately log is truncated 
>> and
>> the most interesting part is missed (below is the example of such a log [1]).
>>
>> Actually for the most cases the first patch will be enough to fix the issue.
>> Originally I thought the problem is qemu logging and reading the log are not 
>> in
>> sync (which is true) so I tried to fix it as well in the next patches.
>>
>> * diff from v1:
>>
>> 1. split changes to libvirtd and virtlogd to different patches
>> 2. split virtlogd patch further
>> 3. simplify handling eofs and hangups in draining function
>>
>> [1] log example:
>>
>> CPU Reset (CPU 0)
>> EAX= EBX= ECX= EDX=
>> ESI= EDI= EBP= ESP=
>> EIP= EFL= [---] CPL=0 II=0 A20=0 SMM=0 HLT=0
>> ES =   
>> CS =   
>> SS =   
>> DS =   
>> FS =   
>> GS =   
>> LDT=   
>> TR =   
>> GDT=  
>> IDT=  
>> CR0= CR2= CR3= CR4=
>> DR0= DR1= DR2= 
>> DR3= 
>> DR6= DR7=
>> CCS= CCD= CCO=DYNAMIC 
>> EFER=
>> FCW= FSW= [ST=0] FTW=ff MXCSR=
>> FPR0=  FPR1= 
>> FPR2=  FPR3= 
>> FPR4=  FPR5= 
>> FPR6=  FPR7= 
>> XMM00= XMM01=
>> XMM02= XMM03=
>> XMM04= XMM05=
>> XMM06= XMM07=
>> CPU Reset (CPU 1)
>> EAX= EBX= ECX= EDX=000206a1
>> ESI= EDI= EBP= ESP=
>> EIP=fff0 EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
>> ES =   9300
>> CS =f000   9b00
>> SS =   9300
>> DS =   9300
>> FS =   9300
>> GS =   9300
>> LDT=   8200
>> TR =   8b00
>> GDT=  
>> IDT=  
>> CR0=6010 CR2= CR3= CR4=
>> DR0= DR1= DR2= 
>> DR3= 
>> DR6=0ff0 DR7=0400
>> CCS= CCD= CCO=DYNAMIC 
>> EFER=
>> FCW=037f FSW= [ST=0] FTW=00 MXCSR=1f80
>> FPR0=  FPR1= 
>> FPR2=  FPR3= 
>> FPR4=  FPR5= 
>> FPR6=  FPR7= 
>> XMM00= XMM01=
>> XMM02= XMM03=
>> XMM04= XMM05=
>> XMM06= XMM07=000
>> qemu: terminating on signal 15 from pid 168133
> 
> I don't think that reporting all of the above is a good idea. We should
> perhaps report at most two last lines.
> 

We already report about half of this, this patch just removes random truncation.
As to most two lines, AFAIU one can not say what part of this log will be
useful for crash investigation.

Nikolay

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

Re: [libvirt] [[PATCH v2] 3/4] virtlogd: add flag to wait for log end on read

[Nikolay Shirokovskiy]

On 05.10.2016 18:08, Peter Krempa wrote:
> On Mon, Sep 12, 2016 at 17:34:42 +0300, Nikolay Shirokovskiy wrote:
> 
> This is a pretty big change but you did not write anything to describe
> or justify it.
> 
>> ---
>>  src/logging/log_handler.c  | 38 --
>>  src/logging/log_protocol.x |  5 +
>>  2 files changed, 41 insertions(+), 2 deletions(-)
> 
> [...]
> 
>> @@ -492,10 +517,19 @@ virLogHandlerDomainReadLogFile(virLogHandlerPtr 
>> handler,
>>  char *data = NULL;
>>  ssize_t got;
>>  
>> -virCheckFlags(0, NULL);
>> +virCheckFlags(VIR_LOG_MANAGER_PROTOCOL_DOMAIN_READ_LOG_FILE_WAIT, NULL);
>>  
>>  virObjectLock(handler);
>>  
>> +if (flags & VIR_LOG_MANAGER_PROTOCOL_DOMAIN_READ_LOG_FILE_WAIT) {
>> +while (virLogHandlerGetLogFileFromPath(handler, path)) {
>> +if (virCondWait(>condition, >parent.lock) < 
>> 0) {
>> +virReportSystemError(errno, "%s", _("failed to wait for 
>> EOF"));
>> +goto error;
>> +}
>> +}
> 
> E.g why do you need this? The qemu process is dead at the point when
> libvirt attempts to read the log file so I don't see a reason to wait
> here.
> 
> Peter
> 

When we read log it can be written partially at that moment. Qemu is dead but
there is a chance the pipe that connects the process and the log daemon is
not drained. Thus if we want read everything the qemu process has written
we need to wait for EOF. Log file handler for path of interest
will disappear in case of EOF.

By the way patch can be simplified. No need to store path in 
_virLogHandlerLogFile
because it's child store this path already.

Should I send v2 of the patch?

Nikolay

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

Re: [libvirt] [PATCH V2 4/4] qemu: Ensure reported VCPU state is current in driver API

[Viktor Mihajlovski]

On 29.09.2016 16:35, John Ferlan wrote:
[...]
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -1478,13 +1478,17 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
>>  virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i);
>>  pid_t vcpupid = qemuDomainGetVcpuPid(vm, i);
>>  virVcpuInfoPtr vcpuinfo = info + ncpuinfo;
>> +bool vcpuhalted = qemuDomainGetVcpuHalted(vm, i);
>>  
>>  if (!vcpu->online)
>>  continue;
>>  
>>  if (info) {
>>  vcpuinfo->number = i;
>> -vcpuinfo->state = VIR_VCPU_RUNNING;
>> +if (vcpuhalted)
>> +vcpuinfo->state = VIR_VCPU_HALTED;
> 
> And this causes the client to see "halted" even though the vcpu may be
> running, but just not busy.
> 
> Also if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU), then we'll always
> be halted since qemuDomainRefreshVcpuHalted will avoid the refetch of data.
> 
> 
agree: as discussed in 3/4, wrong default for vcpuhalted
>> +else
>> +vcpuinfo->state = VIR_VCPU_RUNNING;
>>  
>>  if (qemuGetProcessInfo(>cpuTime,
>> >cpu, NULL,
>> @@ -5370,6 +5374,7 @@ qemuDomainGetVcpus(virDomainPtr dom,
>> unsigned char *cpumaps,
>> int maplen)
>>  {
> 
> The opposite end of virDomainGetVcpus a/k/a 'virsh vcpuinfo'
> 
>> +virQEMUDriverPtr driver = dom->conn->privateData;
>>  virDomainObjPtr vm;
>>  int ret = -1;
>>  
>> @@ -5385,6 +5390,13 @@ qemuDomainGetVcpus(virDomainPtr dom,
>>  goto cleanup;
>>  }
>>  
>> +if (qemuDomainRefreshVcpuHalted(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   "%s",
>> +   _("could not refresh CPU states"));
> 
> This overwrites what message qemuDomainRefreshVcpuHalted should have
> generated.  Besides the "%s", could be on the previous line...
> 
yeah, rebase damage (similar to rc = 2)
[...]
now, the comments are rather easy to incorporate in a V3 of this series,
but the main question for me is how to address your concerns about
exposing the idleness of x86 vcpus?

-- 

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

Re: [libvirt] [PATCH 4/6] tests: qemumonitorjsontest: Do some actual testing in qemuMonitorJSONTestAttachChardev

[Peter Krempa]

On Wed, Oct 05, 2016 at 09:38:16 -0400, John Ferlan wrote:
> On 09/27/2016 12:39 PM, Peter Krempa wrote:
> > Until now the test was rather useless since it didn't check the
> > arguments formatted and didn't use properly configured chardev objects.
> > 
> > Add the expected arguments and instrument the test to validate them.
> > Modify some test cases to actually add valid data.
> > 
> > Note that the UDP test data is currently wrong due to a bug.
> > ---
> >  tests/qemumonitorjsontest.c | 93 
> > +
> >  1 file changed, 68 insertions(+), 25 deletions(-)
> > 
> > diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> > index 23877f8..61344b7 100644
> > --- a/tests/qemumonitorjsontest.c
> > +++ b/tests/qemumonitorjsontest.c
> > @@ -747,6 +747,7 @@ static int
> >  qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt,
> >  const char *label,
> >  virDomainChrSourceDefPtr chr,
> > +const char *expectargs,
> >  const char *reply,
> >  const char *expectPty,
> >  bool fail)
> > @@ -772,7 +773,8 @@ 
> > qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt,
> >  if (!(data.test = qemuMonitorTestNewSimple(true, xmlopt)))
> >  goto cleanup;
> > 
> > -if (qemuMonitorTestAddItem(data.test, "chardev-add", jsonreply) < 0)
> > +if (qemuMonitorTestAddItemExpect(data.test, "chardev-add",
> > + expectargs, true, jsonreply) < 0)
> 
> FWIW: The "knowledge" of whether the expectargs has an apostrophe would
> seem to lie in the caller. If in fact you still keep the apostrophe

That acutally means that the string uses apostrophes instead of quotes
for easier embedding into C.

In this exact case, all the strings tested use apostrophes instead of
quotes so this will always be true. I don't see a benefit of adding it
as an argument for every single test case in this test.

> check in patch 2, then I would think the caller would be able to supply
> true/false, but that's a minor thing.

This code is always using the shortcut, since I'm lazy to escape every
single quote.

> Also, something that just occurred to me... 'expectargs' had better not
> be NULL; otherwise, patch 2 will core at the while (*tmp != '\0')

There are other monitor testing APIs that you can use if you don't care
about the arguments.

Additionally the worker function that is verifying the data is not able
to expect a NULL string as these APIs are made to test the actual
arguments. For commands which don't have arguments you'll get an earlier
error. The expected string should thus never be NULL, since it would be
a programmer mistake.

And if it will be by accident, the testsuite will crash which is correct
in this regard IMO.

> ACK in principle - your call on the apostrophe thing - although perhaps
> a check in patch 2 should be added for (apostrophe && data->expectArgs)
> to protect from future mishaps...
> 
> John


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2 3/4] qemu: Add domain support for VCPU halted state

[Viktor Mihajlovski]

On 29.09.2016 16:34, John Ferlan wrote:
[...]
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -5956,6 +5956,75 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>>  return ret;
>>  }
>>  
>> +/**
>> + * qemuDomainGetVcpuHalted:
>> + * @vm: domain object
>> + * @vcpu: cpu id
>> + *
>> + * Returns the vCPU halted state.
>> +  */
>> +bool
>> +qemuDomainGetVcpuHalted(virDomainObjPtr vm,
>> +unsigned int vcpuid)
>> +{
>> +virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid);
>> +return QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted;
>> +}
>> +
>> +/**
>> + * qemuDomainRefreshVcpuHalted:
>> + * @driver: qemu driver data
>> + * @vm: domain object
>> + * @asyncJob: current asynchronous job type
>> + *
>> + * Updates vCPU halted state in the private data of @vm.
>> + *
>> + * Returns number of detected vCPUs on success, -1 on error and reports
>> + * an appropriate error, -2 if the domain doesn't exist any more.
> 
> Neither of the callers checks -1 or -2, just < 0, so is this really
> necessary?
> 
yes, this is nonsense, IIRC this was needed in the first version of the
series to differentiate the two cases
>> + */
>> +int
>> +qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
>> +virDomainObjPtr vm,
>> +int asyncJob)
>> +{
>> +virDomainVcpuDefPtr vcpu;
>> +qemuMonitorCPUInfoPtr info = NULL;
>> +size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
>> +size_t i;
>> +bool hotplug;
>> +int rc;
>> +int ret = -1;
>> +
>> +/* Not supported currently for TCG, see qemuDomainRefreshVcpuInfo */
>> +if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
>> +return 0;
> 
> Since the "default" is halted = true could we run into a situation where
> "halted" (or "running (inactive)") is always set for TCG...
> 
good point, "halted" as the new kid in town should actually be false by
default, as you have observed the issue is introduced in patch 2/4
(monitors).
[...]

-- 

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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