Re: [libvirt] [PATCH v3] Add support for Veritas HyperScale (VxHS) block device protocol

[ashish mittal]

Thanks for the information. I'll work with this and get back if I get
stuck somewhere.

My immediate objective is to figure out how to pass the TLS x509
certificate information to the vxhs block device on the qemu command
line. I guess I expected some other block device (i.e. NBD) to call
the qemuBuildTLSx509CommandLine(), but got confused when I did not
find that...

On Tue, Apr 11, 2017 at 3:47 PM, John Ferlan  wrote:
>
>
> On 04/10/2017 07:32 PM, ashish mittal wrote:
>> Hi,
>>
>> I'm trying to figure out what changes are needed in the libvirt vxhs
>> patch to support passing TLS X509 arguments to qemu, similar to the
>> following -
>>
>> Sample QEMU command line passing TLS credentials to the VxHS block
>> device (run in secure mode):
>> ./qemu-io --object
>> tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client -c 'read
>> -v 66000 2.5k' 'json:{"server.host": "127.0.0.1", "server.port": "",
>> "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}'
>>
>> I was hoping to find some NBD code related to this, but not able to
>> locate it. Any pointers will be appreciated.
>
> Well you have a couple of things to deal with... There's the creation of
> the TLS object and there's altering the parameters used for the qemu
> command based on your needs/model.
>
> First off you'll need to figure out where/how you're going to define
> where the TLS creds exist. For that, I suspect you'll have code similar
> to how chardevTLS support was added.  Essentially some way to either use
> an existing TLS environment or a way to allow someone to define a vxhs
> specific environment (hint, see src/qemu/qemu_conf.c, src/qemu/qemu.conf
> - I've made changes recently there too).
>
> For the TLS object creation on the command line, see
> qemuBuildTLSx509CommandLine to see how the code builds the
> "tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client" portion
> of your command line.
>
> I forget if hot plug was in your plan, but see qemuDomainGetTLSObjects,
> qemuDomainAddTLSObjects, and qemuDomainDelTLSObjects for that.
>
> The rest of the command line is going to be a bit tricky since using the
> "newer" driver syntax for libvirt is "sparse". Traditionally libvirt has
> used "-drive file=[$uri:]$path,format=$driver,..."  (use grep "\-drive
> file" tests/*/*.args from a libvirt git directory - you can grep that
> output for gluster or rbd to see the uri format).
>
> IIUC the qemu changes correctly though, you cannot use that "file="
> syntax, instead you'll need to format the command line similar to how
> things were done for gluster to add multiple host support where the
> syntax is "-drive 'file.driver=gluster,file.volume=..." (use grep
> "\-drive file.driver" tests/*/*.args to see how this is done for gluster).
>
> That code/support was added in a series starting at commit id '22ad4a7c'
> and working your way forward through about 18 patches. Using a visual
> tool like gitk helps a lot...
>
> I think what will be easiest is to start at that commit and look "up"
> for gluster specific changes.  Be careful not to fully cut-n-paste
> because there have been patches since that time to fix some issues with
> the initial implementation. I point it out only as a way for you to see
> which modules and where "similar" code exists.
>
> You'll also note there is an nbd patch in that series of patches - not
> sure how much that helps, but it perhaps gives you some amount of
> guidelines. Although I don't believe nbd was added to the command line -
> it was just a way of syntax generation/testing.
>
>
> John
>
>>
>> Thanks,
>> Ashish
>>
>> On Wed, Feb 1, 2017 at 8:36 AM, John Ferlan  wrote:
>>> [...]
>>> Pressed send too soon, sigh.
>>>
>>>
>>
>> #1. Based on Peter's v2 comments, we don't want to support the
>> older/legacy syntax for VxHS, so it's something that should be removed -
>> although we should check for it being present and fail if found.
>>
>
> I am testing with changed code to return error if legacy syntax is
> found for VxHS. Also added a test case to check for failure on legacy
> syntax and it seems to pass (test #41 below).
>
> Then I added a pass test case to check conversion from new native
> syntax to XML (test #40 below). That test fails with error
> 'qemuParseCommandLineDisk:901 : internal error: missing file parameter
> in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b...'

 The qemu_parse_command.c changes while nice to have weren't even updated
 when multiple gluster servers were added (e.g. commit id '' or '7b7da9e28')
 Check the changes to add the new s

 IOW: This code knows how to parse something like:

 -drive
 'file=gluster+unix:///Volume2/Image?socket=/path/to/sock,format=raw,if=none,id=drive-virtio-disk1'

 but it's clueless for:

 -drive file.driver=gluster,file.volume=Volume3,file.path=/Image.qcow2,\
 file.server.0.type=tcp,file.server.0.host=example

Re: [libvirt] [PATCH v3] Add support for Veritas HyperScale (VxHS) block device protocol

[John Ferlan]

On 04/10/2017 07:32 PM, ashish mittal wrote:
> Hi,
> 
> I'm trying to figure out what changes are needed in the libvirt vxhs
> patch to support passing TLS X509 arguments to qemu, similar to the
> following -
> 
> Sample QEMU command line passing TLS credentials to the VxHS block
> device (run in secure mode):
> ./qemu-io --object
> tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client -c 'read
> -v 66000 2.5k' 'json:{"server.host": "127.0.0.1", "server.port": "",
> "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}'
> 
> I was hoping to find some NBD code related to this, but not able to
> locate it. Any pointers will be appreciated.

Well you have a couple of things to deal with... There's the creation of
the TLS object and there's altering the parameters used for the qemu
command based on your needs/model.

First off you'll need to figure out where/how you're going to define
where the TLS creds exist. For that, I suspect you'll have code similar
to how chardevTLS support was added.  Essentially some way to either use
an existing TLS environment or a way to allow someone to define a vxhs
specific environment (hint, see src/qemu/qemu_conf.c, src/qemu/qemu.conf
- I've made changes recently there too).

For the TLS object creation on the command line, see
qemuBuildTLSx509CommandLine to see how the code builds the
"tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client" portion
of your command line.

I forget if hot plug was in your plan, but see qemuDomainGetTLSObjects,
qemuDomainAddTLSObjects, and qemuDomainDelTLSObjects for that.

The rest of the command line is going to be a bit tricky since using the
"newer" driver syntax for libvirt is "sparse". Traditionally libvirt has
used "-drive file=[$uri:]$path,format=$driver,..."  (use grep "\-drive
file" tests/*/*.args from a libvirt git directory - you can grep that
output for gluster or rbd to see the uri format).

IIUC the qemu changes correctly though, you cannot use that "file="
syntax, instead you'll need to format the command line similar to how
things were done for gluster to add multiple host support where the
syntax is "-drive 'file.driver=gluster,file.volume=..." (use grep
"\-drive file.driver" tests/*/*.args to see how this is done for gluster).

That code/support was added in a series starting at commit id '22ad4a7c'
and working your way forward through about 18 patches. Using a visual
tool like gitk helps a lot...

I think what will be easiest is to start at that commit and look "up"
for gluster specific changes.  Be careful not to fully cut-n-paste
because there have been patches since that time to fix some issues with
the initial implementation. I point it out only as a way for you to see
which modules and where "similar" code exists.

You'll also note there is an nbd patch in that series of patches - not
sure how much that helps, but it perhaps gives you some amount of
guidelines. Although I don't believe nbd was added to the command line -
it was just a way of syntax generation/testing.


John

> 
> Thanks,
> Ashish
> 
> On Wed, Feb 1, 2017 at 8:36 AM, John Ferlan  wrote:
>> [...]
>> Pressed send too soon, sigh.
>>
>>
>
> #1. Based on Peter's v2 comments, we don't want to support the
> older/legacy syntax for VxHS, so it's something that should be removed -
> although we should check for it being present and fail if found.
>

 I am testing with changed code to return error if legacy syntax is
 found for VxHS. Also added a test case to check for failure on legacy
 syntax and it seems to pass (test #41 below).

 Then I added a pass test case to check conversion from new native
 syntax to XML (test #40 below). That test fails with error
 'qemuParseCommandLineDisk:901 : internal error: missing file parameter
 in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b...'
>>>
>>> The qemu_parse_command.c changes while nice to have weren't even updated
>>> when multiple gluster servers were added (e.g. commit id '' or '7b7da9e28')
>>> Check the changes to add the new s
>>>
>>> IOW: This code knows how to parse something like:
>>>
>>> -drive
>>> 'file=gluster+unix:///Volume2/Image?socket=/path/to/sock,format=raw,if=none,id=drive-virtio-disk1'
>>>
>>> but it's clueless for:
>>>
>>> -drive file.driver=gluster,file.volume=Volume3,file.path=/Image.qcow2,\
>>> file.server.0.type=tcp,file.server.0.host=example.org,file.server.0.port=6000,\
>>> file.server.1.type=tcp,file.server.1.host=example.org,file.server.1.port=24007,\
>>> file.server.2.type=unix,file.server.2.socket=/path/to/sock,format=qcow2,\
>>> if=none,id=drive-virtio-disk2 \
>>> -device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk2,\
>>> id=virtio-disk2
>>>
>>> See

 Looks like none of the existing tests in qemuargv2xmltest test for the
 parsing of new syntax, and qemuParseCommandLineDisk() expects to find
 'file=' for a drive or it errors out. If this is true, will it be a

[libvirt] [PATCH v2] hyperv: recognize array property as distinct type.

[Dawid Zamirski]

When hyperv code generator for WMI classes identifies common
properties, it needs to take into account array type as a distinct
type, i.e string != string[]. This is the case where v1 of the
Msvm_VirtualSystemSettingData has Notes property as string whereas v2
uses Notes[], therefore they have to be treated as different fields and
cannot be placed in the "common" struct.
---

changes in v2:
 * move notes variable assignment outside of the loop so that it's not
   reset at each iteration.

 src/hyperv/hyperv_driver.c | 25 +++--
 src/hyperv/hyperv_wmi_generator.py |  2 +-
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 090ea24..350f123 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -894,8 +894,29 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int 
flags)
 if (VIR_STRDUP(def->name, computerSystem->data.common->ElementName) < 0)
 goto cleanup;
 
-if (VIR_STRDUP(def->description, 
virtualSystemSettingData->data.common->Notes) < 0)
-goto cleanup;
+if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
+if (VIR_STRDUP(def->description,
+   virtualSystemSettingData->data.v1->Notes) < 0)
+goto cleanup;
+} else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2 &&
+   virtualSystemSettingData->data.v2->Notes.data != NULL) {
+char **notes = (char **) virtualSystemSettingData->data.v2->Notes.data;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+size_t i = 0;
+
+/* in practice Notes has 1 element */
+for (i = 0; i < virtualSystemSettingData->data.v2->Notes.count; i++) {
+/* but if there's more than 1, separate by double new line */
+if (virBufferUse(&buf) > 0)
+virBufferAddLit(&buf, "\n\n");
+
+virBufferAdd(&buf, *notes, -1);
+notes++;
+}
+
+def->description = virBufferContentAndReset(&buf);
+}
+
 
 virDomainDefSetMemoryTotal(def, memorySettingData->data.common->Limit * 
1024); /* megabyte to kilobyte */
 def->mem.cur_balloon = memorySettingData->data.common->VirtualQuantity * 
1024; /* megabyte to kilobyte */
diff --git a/src/hyperv/hyperv_wmi_generator.py 
b/src/hyperv/hyperv_wmi_generator.py
index c15d97a..9aee0b9 100755
--- a/src/hyperv/hyperv_wmi_generator.py
+++ b/src/hyperv/hyperv_wmi_generator.py
@@ -251,7 +251,7 @@ class WmiClass:
 for cls in self.versions:
 for prop in cls.properties:
 # consdered same if matches by name AND type
-key = "%s_%s" % (prop.name, prop.type)
+key = "%s_%s_%s" % (prop.name, prop.type, prop.is_array)
 
 if key in property_info:
 property_info[key][1] += 1
-- 
2.9.3

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

Re: [libvirt] [PATCH] hyperv: recognize array property as distinct type.

[Matthias Bolte]

2017-04-11 17:02 GMT+02:00 Dawid Zamirski :
> When hyperv code generator for WMI classes identifies common
> properties, it needs to take into account array type as a distinct
> type, i.e string != string[]. This is the case where v1 of the
> Msvm_VirtualSystemSettingData has Notes property as string whereas v2
> uses Notes[], therefore they have to be treated as different fields and
> cannot be placed in the "common" struct.
> ---
>  src/hyperv/hyperv_driver.c | 26 --
>  src/hyperv/hyperv_wmi_generator.py |  2 +-
>  2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 090ea24..5a27908 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -894,8 +894,30 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int 
> flags)
>  if (VIR_STRDUP(def->name, computerSystem->data.common->ElementName) < 0)
>  goto cleanup;
>
> -if (VIR_STRDUP(def->description, 
> virtualSystemSettingData->data.common->Notes) < 0)
> -goto cleanup;
> +if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
> +if (VIR_STRDUP(def->description,
> +   virtualSystemSettingData->data.v1->Notes) < 0)
> +goto cleanup;
> +} else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2 &&
> +   virtualSystemSettingData->data.v2->Notes.data != NULL) {
> +char **notes = NULL;
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +size_t i = 0;
> +
> +/* in practice Notes has 1 element */
> +for (i = 0; i < virtualSystemSettingData->data.v2->Notes.count; i++) 
> {
> +/* but if there's more than 1, separate by double new line */
> +if (virBufferUse(&buf) > 0)
> +virBufferAddLit(&buf, "\n\n");
> +
> +notes = (char **) virtualSystemSettingData->data.v2->Notes.data;
> +virBufferAdd(&buf, *notes, -1);
> +notes++;
> +}

This reset notes in each iteration of the for loop to the start of the
array, then adds the first item to the buffer and then advances the
pointer by one item. This will add the first item of the notes array
count times to the buffer instead of adding each item to the buffer
once.

I think you need to move the "notes = (char **)
virtualSystemSettingData->data.v2->Notes.data;" line before the for
loop to make this work correctly.

-- 
Matthias Bolte
http://photron.blogspot.com

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

[libvirt] virsh manpage bug: wrong argument order for virsh vol-resize

[Feiko Nanninga]

According to the manpage the arguments for virsh vol-resize are:

vol-resize [--pool pool-or-uuid] vol-name-or-path pool-or-uuid capacity […]

But actually only
vol-resize --pool pool-or-uuid vol-name-or-path capacity

or
vol-resize  vol-name-or-path capacity pool-or-uuid

are accepted. So  and  are swapped.


Tested on Debian 9 (stretch):
$ virsh --version
3.0.0


Best regards,
Feiko Nanninga

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

[libvirt] [PATCH 9/9] virsh-domain: Refactor cmdTTYConsole

[Peter Krempa]

Use the new XML helpers and use virXPathString rather than hand-rolling
the code.
---
 tools/virsh-domain.c | 24 +---
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index d64a2dca0..d6f33b1ee 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11183,37 +11183,23 @@ static bool
 cmdTTYConsole(vshControl *ctl, const vshCmd *cmd)
 {
 xmlDocPtr xml = NULL;
-xmlXPathObjectPtr obj = NULL;
 xmlXPathContextPtr ctxt = NULL;
-virDomainPtr dom;
 bool ret = false;
-char *doc;
+char *tty = NULL;

-if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+if (virshDomainGetXML(ctl, cmd, 0, &xml, &ctxt) < 0)
 return false;

-doc = virDomainGetXMLDesc(dom, 0);
-if (!doc)
+if (!(tty = virXPathString("string(/domain/devices/console/@tty)", ctxt)))
 goto cleanup;

-xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt);
-VIR_FREE(doc);
-if (!xml)
-goto cleanup;
-
-obj = xmlXPathEval(BAD_CAST "string(/domain/devices/console/@tty)", ctxt);
-if (obj == NULL || obj->type != XPATH_STRING ||
-obj->stringval == NULL || obj->stringval[0] == 0) {
-goto cleanup;
-}
-vshPrint(ctl, "%s\n", (const char *)obj->stringval);
+vshPrint(ctl, "%s\n", tty);
 ret = true;

  cleanup:
-xmlXPathFreeObject(obj);
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(xml);
-virshDomainFree(dom);
+VIR_FREE(tty);
 return ret;
 }

-- 
2.12.2

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

[libvirt] [PATCH 5/9] vsh: Add helper for safe remembering of libvirt errors

[Peter Krempa]

Avoid the annoying issue where the public object freeing APIs overwrite
the error set by helper functions, since they don't invoke the callback.

The new helper remembers the error only if no previous error was set.
---
 tools/virsh-util.c |  2 ++
 tools/vsh.c| 15 +++
 tools/vsh.h|  1 +
 3 files changed, 18 insertions(+)

diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index e225d3332..79a38bb23 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -158,6 +158,7 @@ virshDomainFree(virDomainPtr dom)
 if (!dom)
 return;

+vshSaveLibvirtHelperError();
 virDomainFree(dom); /* sc_prohibit_obj_free_apis_in_virsh */
 }

@@ -168,5 +169,6 @@ virshDomainSnapshotFree(virDomainSnapshotPtr snap)
 if (!snap)
 return;

+vshSaveLibvirtHelperError();
 virDomainSnapshotFree(snap); /* sc_prohibit_obj_free_apis_in_virsh */
 }
diff --git a/tools/vsh.c b/tools/vsh.c
index d2024be91..10a65c39f 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -258,6 +258,21 @@ vshSaveLibvirtError(void)
 last_error = virSaveLastError();
 }

+
+/* Store libvirt error from helper API but don't overwrite existing errors */
+void
+vshSaveLibvirtHelperError(void)
+{
+if (last_error)
+return;
+
+if (!virGetLastError())
+return;
+
+vshSaveLibvirtError();
+}
+
+
 /*
  * Reset libvirt error on graceful fallback paths
  */
diff --git a/tools/vsh.h b/tools/vsh.h
index 8f5d1a69f..2f686eba6 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -341,6 +341,7 @@ void vshErrorHandler(void *opaque, virErrorPtr error);
 void vshReportError(vshControl *ctl);
 void vshResetLibvirtError(void);
 void vshSaveLibvirtError(void);
+void vshSaveLibvirtHelperError(void);

 /* file handling */
 char *vshEditWriteToTempFile(vshControl *ctl, const char *doc);
-- 
2.12.2

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

[libvirt] [PATCH 8/9] virsh-domain: Use the virsh wrappers for getting XML to simplify code

[Peter Krempa]

Reuse virshDomainGetXML and virshDomainGetXMLFromDom.
---
 tools/virsh-domain.c | 62 ++--
 1 file changed, 12 insertions(+), 50 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index ba179c89c..d64a2dca0 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2983,13 +2983,13 @@ cmdDomIfSetLink(vshControl *ctl, const vshCmd *cmd)
 const char *iface;
 const char *state;
 char *value;
-char *desc;
 virMacAddr macaddr;
 const char *element;
 const char *attr;
 bool config;
 bool ret = false;
 unsigned int flags = 0;
+unsigned int xmlflags = 0;
 size_t i;
 xmlDocPtr xml = NULL;
 xmlXPathContextPtr ctxt = NULL;
@@ -3011,28 +3011,18 @@ cmdDomIfSetLink(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }

-/* get persistent or live description of network device */
-desc = virDomainGetXMLDesc(dom, config ? VIR_DOMAIN_XML_INACTIVE : 0);
-if (desc == NULL) {
-vshError(ctl, _("Failed to get domain description xml"));
-goto cleanup;
-}
-
-if (config)
+if (config) {
 flags = VIR_DOMAIN_AFFECT_CONFIG;
-else
+xmlflags |= VIR_DOMAIN_XML_INACTIVE;
+} else {
 flags = VIR_DOMAIN_AFFECT_LIVE;
+}

 if (virDomainIsActive(dom) == 0)
 flags = VIR_DOMAIN_AFFECT_CONFIG;

-/* extract current network device description */
-xml = virXMLParseStringCtxt(desc, _("(domain_definition)"), &ctxt);
-VIR_FREE(desc);
-if (!xml) {
-vshError(ctl, _("Failed to parse domain description xml"));
+if (virshDomainGetXMLFromDom(ctl, dom, xmlflags, &xml, &ctxt) < 0)
 goto cleanup;
-}

 obj = xmlXPathEval(BAD_CAST "/domain/devices/interface", ctxt);
 if (obj == NULL || obj->type != XPATH_NODESET ||
@@ -3575,7 +3565,6 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
 int nvol_list = 0;
 virshUndefineVolume *vols = NULL; /* info about the volumes to delete*/
 size_t nvols = 0;
-char *def = NULL;   /* domain def */
 xmlDocPtr doc = NULL;
 xmlXPathContextPtr ctxt = NULL;
 xmlNodePtr *vol_nodes = NULL;   /* XML nodes of volumes of the guest */
@@ -3685,14 +3674,8 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }

-if (!(def = virDomainGetXMLDesc(dom, 0))) {
-vshError(ctl, _("Could not retrieve domain XML description"));
+if (virshDomainGetXMLFromDom(ctl, dom, 0, &doc, &ctxt) < 0)
 goto cleanup;
-}
-
-if (!(doc = virXMLParseStringCtxt(def, _("(domain_definition)"),
-  &ctxt)))
-goto error;

 /* tokenize the string from user and save its parts into an array */
 if (vol_string &&
@@ -3897,7 +3880,6 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
 VIR_FREE(vol_list[i]);
 VIR_FREE(vol_list);

-VIR_FREE(def);
 VIR_FREE(vol_nodes);
 xmlFreeDoc(doc);
 xmlXPathFreeContext(ctxt);
@@ -6029,7 +6011,6 @@ virshCPUCountCollect(vshControl *ctl,
 int ret = -2;
 virDomainInfo info;
 int count;
-char *def = NULL;
 xmlDocPtr xml = NULL;
 xmlXPathContextPtr ctxt = NULL;

@@ -6071,10 +6052,8 @@ virshCPUCountCollect(vshControl *ctl,
count = info.nrVirtCpu;
 }
 } else {
-if (!(def = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE)))
-goto cleanup;
-
-if (!(xml = virXMLParseStringCtxt(def, _("(domain_definition)"), 
&ctxt)))
+if (virshDomainGetXMLFromDom(ctl, dom, VIR_DOMAIN_XML_INACTIVE,
+ &xml, &ctxt) < 0)
 goto cleanup;

 if (flags & VIR_DOMAIN_VCPU_MAXIMUM) {
@@ -6092,7 +6071,6 @@ virshCPUCountCollect(vshControl *ctl,

 ret = count;
  cleanup:
-VIR_FREE(def);
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(xml);

@@ -6237,7 +6215,6 @@ virshDomainGetVcpuBitmap(vshControl *ctl,
  bool inactive)
 {
 unsigned int flags = 0;
-char *def = NULL;
 virBitmapPtr ret = NULL;
 xmlDocPtr xml = NULL;
 xmlXPathContextPtr ctxt = NULL;
@@ -6253,10 +6230,7 @@ virshDomainGetVcpuBitmap(vshControl *ctl,
 if (inactive)
 flags |= VIR_DOMAIN_XML_INACTIVE;

-if (!(def = virDomainGetXMLDesc(dom, flags)))
-goto cleanup;
-
-if (!(xml = virXMLParseStringCtxt(def, _("(domain_definition)"), &ctxt)))
+if (virshDomainGetXMLFromDom(ctl, dom, flags, &xml, &ctxt) < 0)
 goto cleanup;

 if (virXPathUInt("string(/domain/vcpu)", ctxt, &maxvcpus) < 0) {
@@ -6308,7 +6282,6 @@ virshDomainGetVcpuBitmap(vshControl *ctl,
 VIR_FREE(nodes);
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(xml);
-VIR_FREE(def);
 return ret;
 }

@@ -10901,7 +10874,6 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
 virDomainPtr dom;
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 boo

[libvirt] [PATCH 4/9] virsh: Add wrapper for virDomainSnapshotFree

[Peter Krempa]

Similarly to virshDomainFree add a wrapper for the snapshot object
freeing function.
---
 tools/virsh-snapshot.c | 48 ++--
 tools/virsh-util.c | 10 ++
 tools/virsh-util.h |  3 +++
 3 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 5af9a7d7a..24cd4abd9 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -106,8 +106,7 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, 
const char *buffer,
  cleanup:
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(xml);
-if (snapshot)
-virDomainSnapshotFree(snapshot);
+virshDomainSnapshotFree(snapshot);
 VIR_FREE(doc);
 return ret;
 }
@@ -601,10 +600,8 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd)
  cleanup:
 if (!ret && name)
 vshError(ctl, _("Failed to update %s"), name);
-if (edited)
-virDomainSnapshotFree(edited);
-if (snapshot)
-virDomainSnapshotFree(snapshot);
+virshDomainSnapshotFree(edited);
+virshDomainSnapshotFree(snapshot);
 virshDomainFree(dom);
 return ret;
 }
@@ -681,7 +678,7 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd)
 if (!(snapshot2 = virDomainSnapshotCreateXML(dom, xml, flags)))
 goto cleanup;

-virDomainSnapshotFree(snapshot2);
+virshDomainSnapshotFree(snapshot2);
 vshPrintExtra(ctl, _("Snapshot %s set as current"), snapshotname);
 ret = true;
 goto cleanup;
@@ -717,8 +714,7 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd)
 if (!ret)
 vshReportError(ctl);
 VIR_FREE(xml);
-if (snapshot)
-virDomainSnapshotFree(snapshot);
+virshDomainSnapshotFree(snapshot);
 virshDomainFree(dom);

 return ret;
@@ -777,8 +773,7 @@ virshGetSnapshotParent(vshControl *ctl, 
virDomainSnapshotPtr snapshot,
 } else {
 vshResetLibvirtError();
 }
-if (parent)
-virDomainSnapshotFree(parent);
+virshDomainSnapshotFree(parent);
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(xmldoc);
 VIR_FREE(xml);
@@ -906,7 +901,7 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd)
 if (other) {
 if (STREQ(name, virDomainSnapshotGetName(other)))
 current = 1;
-virDomainSnapshotFree(other);
+virshDomainSnapshotFree(other);
 }
 }
 vshPrint(ctl, "%-15s %s\n", _("Current:"),
@@ -1005,8 +1000,7 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd)
 xmlFreeDoc(xmldoc);
 VIR_FREE(doc);
 VIR_FREE(parent);
-if (snapshot)
-virDomainSnapshotFree(snapshot);
+virshDomainSnapshotFree(snapshot);
 virshDomainFree(dom);
 return ret;
 }
@@ -1031,8 +1025,7 @@ virshSnapshotListFree(virshSnapshotListPtr snaplist)
 return;
 if (snaplist->snaps) {
 for (i = 0; i < snaplist->nsnaps; i++) {
-if (snaplist->snaps[i].snap)
-virDomainSnapshotFree(snaplist->snaps[i].snap);
+virshDomainSnapshotFree(snaplist->snaps[i].snap);
 VIR_FREE(snaplist->snaps[i].parent);
 }
 VIR_FREE(snaplist->snaps);
@@ -1270,7 +1263,7 @@ virshSnapshotListCollect(vshControl *ctl, virDomainPtr 
dom,
STRNEQ_NULLABLE(fromname,
snaplist->snaps[i].parent ||
 (roots && snaplist->snaps[i].parent)) {
-virDomainSnapshotFree(snaplist->snaps[i].snap);
+virshDomainSnapshotFree(snaplist->snaps[i].snap);
 snaplist->snaps[i].snap = NULL;
 VIR_FREE(snaplist->snaps[i].parent);
 deleted++;
@@ -1298,7 +1291,7 @@ virshSnapshotListCollect(vshControl *ctl, virDomainPtr 
dom,
 for (i = 0; i < count; i++) {
 if (i == start_index || !snaplist->snaps[i].parent) {
 VIR_FREE(names[i]);
-virDomainSnapshotFree(snaplist->snaps[i].snap);
+virshDomainSnapshotFree(snaplist->snaps[i].snap);
 snaplist->snaps[i].snap = NULL;
 VIR_FREE(snaplist->snaps[i].parent);
 deleted++;
@@ -1336,7 +1329,7 @@ virshSnapshotListCollect(vshControl *ctl, virDomainPtr 
dom,
 if (!found_parent) {
 changed = true;
 VIR_FREE(names[i]);
-virDomainSnapshotFree(snaplist->snaps[i].snap);
+virshDomainSnapshotFree(snaplist->snaps[i].snap);
 snaplist->snaps[i].snap = NULL;
 VIR_FREE(snaplist->snaps[i].parent);
 deleted++;
@@ -1359,7 +1352,7 @@ virshSnapshotListCollect(vshControl *ctl, virDomainPtr 
dom,
 case 1:
 break;
 case 0:
-virDomainSnapshotFree(snaplist->snaps[i].snap);
+virshDomainSnapshotFree(snaplist->snaps[i].snap);
   

[libvirt] [PATCH 6/9] virsh: add helpers for getting domain XML for XPath purposes

[Peter Krempa]

In virsh we quite often get the domain XML just to initialize the XPath
parser so that we can extract information.

Add helpers which will simplify this by wrapping the getting of the XML
and parsing it along with error reporting.

Additionally a second helper also gets the domain object from the
parameters and releases it so that functions which need the XML as only
source of data can be simplified further.
---
 tools/virsh-util.c | 48 
 tools/virsh-util.h | 20 
 2 files changed, 68 insertions(+)

diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index 79a38bb23..4b86e29cb 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -22,6 +22,7 @@

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

 static virDomainPtr
 virshLookupDomainInternal(vshControl *ctl,
@@ -172,3 +173,50 @@ virshDomainSnapshotFree(virDomainSnapshotPtr snap)
 vshSaveLibvirtHelperError();
 virDomainSnapshotFree(snap); /* sc_prohibit_obj_free_apis_in_virsh */
 }
+
+
+int
+virshDomainGetXMLFromDom(vshControl *ctl,
+ virDomainPtr dom,
+ unsigned int flags,
+ xmlDocPtr *xml,
+ xmlXPathContextPtr *ctxt)
+{
+char *desc = NULL;
+
+if (!(desc = virDomainGetXMLDesc(dom, flags))) {
+vshError(ctl, _("Failed to get domain description xml"));
+return -1;
+}
+
+*xml = virXMLParseStringCtxt(desc, _("(domain_definition)"), ctxt);
+VIR_FREE(desc);
+
+if (!(*xml)) {
+vshError(ctl, _("Failed to parse domain description xml"));
+return -1;
+}
+
+return 0;
+}
+
+
+int
+virshDomainGetXML(vshControl *ctl,
+  const vshCmd *cmd,
+  unsigned int flags,
+  xmlDocPtr *xml,
+  xmlXPathContextPtr *ctxt)
+{
+virDomainPtr dom;
+int ret;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return -1;
+
+ret = virshDomainGetXMLFromDom(ctl, dom, flags, xml, ctxt);
+
+virshDomainFree(dom);
+
+return ret;
+}
diff --git a/tools/virsh-util.h b/tools/virsh-util.h
index 2b5aedfba..64cef23c0 100644
--- a/tools/virsh-util.h
+++ b/tools/virsh-util.h
@@ -21,6 +21,8 @@

 # include "virsh.h"

+# include 
+# include 

 virDomainPtr
 virshLookupDomainBy(vshControl *ctl,
@@ -55,4 +57,22 @@ virshStreamSink(virStreamPtr st,
 size_t nbytes,
 void *opaque);

+int
+virshDomainGetXMLFromDom(vshControl *ctl,
+ virDomainPtr dom,
+ unsigned int flags,
+ xmlDocPtr *xml,
+ xmlXPathContextPtr *ctxt)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4)
+ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK;
+
+int
+virshDomainGetXML(vshControl *ctl,
+  const vshCmd *cmd,
+  unsigned int flags,
+  xmlDocPtr *xml,
+  xmlXPathContextPtr *ctxt)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4)
+ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK;
+
 #endif /* VIRSH_UTIL_H */
-- 
2.12.2

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

[libvirt] [PATCH 7/9] virsh-domain-monitor: Use the virsh wrappers for getting XML to simplify code

[Peter Krempa]

Reuse virshDomainGetXML and virshDomainGetXMLFromDom.
---
 tools/virsh-domain-monitor.c | 57 
 1 file changed, 5 insertions(+), 52 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 92995ad7d..3db4795d2 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -63,7 +63,6 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, 
bool title,
   unsigned int flags)
 {
 char *desc = NULL;
-char *domxml = NULL;
 virErrorPtr err = NULL;
 xmlDocPtr doc = NULL;
 xmlXPathContextPtr ctxt = NULL;
@@ -90,16 +89,9 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, 
bool title,
 }

 /* fall back to xml */
-/* get domain's xml description and extract the title/description */
-if (!(domxml = virDomainGetXMLDesc(dom, flags))) {
-vshError(ctl, "%s", _("Failed to retrieve domain XML"));
+if (virshDomainGetXMLFromDom(ctl, dom, flags, &doc, &ctxt) < 0)
 goto cleanup;
-}
-doc = virXMLParseStringCtxt(domxml, _("(domain_definition)"), &ctxt);
-if (!doc) {
-vshError(ctl, "%s", _("Couldn't parse domain XML"));
-goto cleanup;
-}
+
 if (title)
 desc = virXPathString("string(./title[1])", ctxt);
 else
@@ -109,7 +101,6 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr 
dom, bool title,
 desc = vshStrdup(ctl, "");

  cleanup:
-VIR_FREE(domxml);
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(doc);

@@ -465,10 +456,8 @@ static const vshCmdOptDef opts_domblklist[] = {
 static bool
 cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
 {
-virDomainPtr dom;
 bool ret = false;
 unsigned int flags = 0;
-char *xml = NULL;
 xmlDocPtr xmldoc = NULL;
 xmlXPathContextPtr ctxt = NULL;
 int ndisks;
@@ -485,15 +474,7 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)

 details = vshCommandOptBool(cmd, "details");

-if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
-return false;
-
-xml = virDomainGetXMLDesc(dom, flags);
-if (!xml)
-goto cleanup;
-
-xmldoc = virXMLParseStringCtxt(xml, _("(domain_definition)"), &ctxt);
-if (!xmldoc)
+if (virshDomainGetXML(ctl, cmd, flags, &xmldoc, &ctxt) < 0)
 goto cleanup;

 ndisks = virXPathNodeSet("./devices/disk", ctxt, &disks);
@@ -553,8 +534,6 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
 VIR_FREE(disks);
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(xmldoc);
-VIR_FREE(xml);
-virshDomainFree(dom);
 return ret;
 }

@@ -579,10 +558,8 @@ static const vshCmdOptDef opts_domiflist[] = {
 static bool
 cmdDomiflist(vshControl *ctl, const vshCmd *cmd)
 {
-virDomainPtr dom;
 bool ret = false;
 unsigned int flags = 0;
-char *xml = NULL;
 xmlDocPtr xmldoc = NULL;
 xmlXPathContextPtr ctxt = NULL;
 int ninterfaces;
@@ -592,15 +569,7 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptBool(cmd, "inactive"))
 flags |= VIR_DOMAIN_XML_INACTIVE;

-if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
-return false;
-
-xml = virDomainGetXMLDesc(dom, flags);
-if (!xml)
-goto cleanup;
-
-xmldoc = virXMLParseStringCtxt(xml, _("(domain_definition)"), &ctxt);
-if (!xmldoc)
+if (virshDomainGetXML(ctl, cmd, flags, &xmldoc, &ctxt) < 0)
 goto cleanup;

 ninterfaces = virXPathNodeSet("./devices/interface", ctxt, &interfaces);
@@ -648,8 +617,6 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd)

  cleanup:
 VIR_FREE(interfaces);
-virshDomainFree(dom);
-VIR_FREE(xml);
 xmlFreeDoc(xmldoc);
 xmlXPathFreeContext(ctxt);
 return ret;
@@ -686,13 +653,11 @@ static const vshCmdOptDef opts_domif_getlink[] = {
 static bool
 cmdDomIfGetLink(vshControl *ctl, const vshCmd *cmd)
 {
-virDomainPtr dom;
 const char *iface = NULL;
 char *state = NULL;
 char *xpath = NULL;
 virMacAddr macaddr;
 char macstr[VIR_MAC_STRING_BUFLEN] = "";
-char *desc = NULL;
 xmlDocPtr xml = NULL;
 xmlXPathContextPtr ctxt = NULL;
 xmlNodePtr *interfaces = NULL;
@@ -703,21 +668,11 @@ cmdDomIfGetLink(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptStringReq(ctl, cmd, "interface", &iface) < 0)
 return false;

-if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
-return false;
-
 if (vshCommandOptBool(cmd, "config"))
 flags = VIR_DOMAIN_XML_INACTIVE;

-if (!(desc = virDomainGetXMLDesc(dom, flags))) {
-vshError(ctl, _("Failed to get domain description xml"));
+if (virshDomainGetXML(ctl, cmd, flags, &xml, &ctxt) < 0)
 goto cleanup;
-}
-
-if (!(xml = virXMLParseStringCtxt(desc, _("(domain_definition)"), &ctxt))) 
{
-vshError(ctl, _("Failed to parse domain description xml"));
-goto cleanup;
-}

 /* normalize the mac addr */
 if (virMacAddrPa

[libvirt] [PATCH 3/9] virsh-util: Add wrapper for virDomainFree

[Peter Krempa]

virDomainFree has it's quirks (does not like NULL pointers, resets
libvirt errors). Replace it by a virsh helper which will allow us to
centrally fix issues with it.

The syntax-check rule will prohibit new uses of virDomainFree.
---
 cfg.mk   |   8 ++
 tools/virsh-domain-monitor.c |  36 
 tools/virsh-domain.c | 216 ---
 tools/virsh-snapshot.c   |  24 ++---
 tools/virsh-util.c   |  10 ++
 tools/virsh-util.h   |   3 +
 6 files changed, 149 insertions(+), 148 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index cc174a3cc..1ff87f445 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1014,6 +1014,14 @@ sc_gettext_init:
halt='the above files do not call virGettextInitialize' \
  $(_sc_search_regexp)

+#Ensure that virsh uses the wrappers on top of the object freeing APIs
+sc_prohibit_obj_free_apis_in_virsh:
+   @prohibit='\virDomainFree\b' \
+   in_vc_files='\virsh.*\.[ch]$$'  \
+   exclude='sc_prohibit_obj_free_apis_in_virsh' \
+   halt='avoid using virDomainFree in virsh, use virsh-prefixed wrappers 
instead' \
+ $(_sc_search_regexp)
+
 # We don't use this feature of maint.mk.
 prev_version_file = /dev/null

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 5215ac6f9..92995ad7d 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -381,7 +381,7 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd)

 ret = true;
  cleanup:
-virDomainFree(dom);
+virshDomainFree(dom);
 return ret;
 }

@@ -432,7 +432,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 ret = true;

  cleanup:
-virDomainFree(dom);
+virshDomainFree(dom);
 return ret;
 }

@@ -554,7 +554,7 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(xmldoc);
 VIR_FREE(xml);
-virDomainFree(dom);
+virshDomainFree(dom);
 return ret;
 }

@@ -648,7 +648,7 @@ cmdDomiflist(vshControl *ctl, const vshCmd *cmd)

  cleanup:
 VIR_FREE(interfaces);
-virDomainFree(dom);
+virshDomainFree(dom);
 VIR_FREE(xml);
 xmlFreeDoc(xmldoc);
 xmlXPathFreeContext(ctxt);
@@ -758,7 +758,7 @@ cmdDomIfGetLink(vshControl *ctl, const vshCmd *cmd)
 VIR_FREE(xpath);
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(xml);
-virDomainFree(dom);
+virshDomainFree(dom);

 return ret;
 }
@@ -811,7 +811,7 @@ cmdDomControl(vshControl *ctl, const vshCmd *cmd)
 }

  cleanup:
-virDomainFree(dom);
+virshDomainFree(dom);
 return ret;
 }

@@ -997,7 +997,7 @@ cmdDomblkstat(vshControl *ctl, const vshCmd *cmd)

  cleanup:
 VIR_FREE(params);
-virDomainFree(dom);
+virshDomainFree(dom);
 return ret;
 }
 #undef DOMBLKSTAT_LEGACY_PRINT
@@ -1071,7 +1071,7 @@ cmdDomIfstat(vshControl *ctl, const vshCmd *cmd)
 ret = true;

  cleanup:
-virDomainFree(dom);
+virshDomainFree(dom);
 return ret;
 }

@@ -1132,7 +1132,7 @@ cmdDomBlkError(vshControl *ctl, const vshCmd *cmd)

  cleanup:
 VIR_FREE(disks);
-virDomainFree(dom);
+virshDomainFree(dom);
 return ret;
 }

@@ -1241,7 +1241,7 @@ cmdDominfo(vshControl *ctl, const vshCmd *cmd)
 memset(&secmodel, 0, sizeof(secmodel));
 if (virNodeGetSecurityModel(priv->conn, &secmodel) == -1) {
 if (last_error->code != VIR_ERR_NO_SUPPORT) {
-virDomainFree(dom);
+virshDomainFree(dom);
 return false;
 } else {
 vshResetLibvirtError();
@@ -1254,12 +1254,12 @@ cmdDominfo(vshControl *ctl, const vshCmd *cmd)

 /* Security labels are only valid for active domains */
 if (VIR_ALLOC(seclabel) < 0) {
-virDomainFree(dom);
+virshDomainFree(dom);
 return false;
 }

 if (virDomainGetSecurityLabel(dom, seclabel) == -1) {
-virDomainFree(dom);
+virshDomainFree(dom);
 VIR_FREE(seclabel);
 return false;
 } else {
@@ -1271,7 +1271,7 @@ cmdDominfo(vshControl *ctl, const vshCmd *cmd)
 VIR_FREE(seclabel);
 }
 }
-virDomainFree(dom);
+virshDomainFree(dom);
 return ret;
 }

@@ -1323,7 +1323,7 @@ cmdDomstate(vshControl *ctl, const vshCmd *cmd)
 }

  cleanup:
-virDomainFree(dom);
+virshDomainFree(dom);
 return ret;
 }

@@ -1427,7 +1427,7 @@ cmdDomTime(vshControl *ctl, const vshCmd *cmd)

 ret = true;
  cleanup:
-virDomainFree(dom);
+virshDomainFree(dom);
 return ret;
 }

@@ -1493,7 +1493,7 @@ virshDomainListFree(virshDomainListPtr domlist)
 if (domlist && domlist->domains) {
 for (i = 0; i < domlist->ndomains; i++) {
 if (domlist->domains[i])
-virDomainFree(domlist->domains[i]);
+virshDomainFree(domlist->domains[i]);
 }
 VIR_FREE(domlist->dom

[libvirt] [PATCH 2/9] virsh-util: Move domain lookup helpers into virsh-util

[Peter Krempa]

Move virshLookupDomainBy, virshCommandOptDomainBy and
virshCommandOptDomainBy to the helper file. Additionally turn the
virshCommandOptDomainBy macro into a function.
---
 po/POTFILES.in   |  1 +
 tools/virsh-domain-monitor.c |  1 -
 tools/virsh-domain.c | 71 
 tools/virsh-domain.h | 12 ---
 tools/virsh-host.c   |  1 -
 tools/virsh-snapshot.c   |  2 +-
 tools/virsh-util.c   | 86 
 tools/virsh-util.h   | 17 +
 8 files changed, 105 insertions(+), 86 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 064abd5bb..ccef6f3cf 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -310,6 +310,7 @@ tools/virsh-nwfilter.c
 tools/virsh-pool.c
 tools/virsh-secret.c
 tools/virsh-snapshot.c
+tools/virsh-util.c
 tools/virsh-volume.c
 tools/virsh.c
 tools/virt-admin.c
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 4ade5651c..5215ac6f9 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -37,7 +37,6 @@
 #include "intprops.h"
 #include "viralloc.h"
 #include "virmacaddr.h"
-#include "virsh-domain.h"
 #include "virxml.h"
 #include "virstring.h"

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 7db74fecf..4e4df5b4f 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -83,77 +83,6 @@
 #define VIRSH_COMMON_OPT_DOMAIN_CURRENT   \
 VIRSH_COMMON_OPT_CURRENT(N_("affect current domain")) \

-static virDomainPtr
-virshLookupDomainInternal(vshControl *ctl,
-  const char *cmdname,
-  const char *name,
-  unsigned int flags)
-{
-virDomainPtr dom = NULL;
-int id;
-virCheckFlags(VIRSH_BYID | VIRSH_BYUUID | VIRSH_BYNAME, NULL);
-virshControlPtr priv = ctl->privData;
-
-/* try it by ID */
-if (flags & VIRSH_BYID) {
-if (virStrToLong_i(name, NULL, 10, &id) == 0 && id >= 0) {
-vshDebug(ctl, VSH_ERR_DEBUG, "%s:  looks like ID\n",
- cmdname);
-dom = virDomainLookupByID(priv->conn, id);
-}
-}
-
-/* try it by UUID */
-if (!dom && (flags & VIRSH_BYUUID) &&
-strlen(name) == VIR_UUID_STRING_BUFLEN-1) {
-vshDebug(ctl, VSH_ERR_DEBUG, "%s:  trying as domain UUID\n",
- cmdname);
-dom = virDomainLookupByUUIDString(priv->conn, name);
-}
-
-/* try it by NAME */
-if (!dom && (flags & VIRSH_BYNAME)) {
-vshDebug(ctl, VSH_ERR_DEBUG, "%s:  trying as domain NAME\n",
- cmdname);
-dom = virDomainLookupByName(priv->conn, name);
-}
-
-vshResetLibvirtError();
-
-if (!dom)
-vshError(ctl, _("failed to get domain '%s'"), name);
-
-return dom;
-}
-
-
-virDomainPtr
-virshLookupDomainBy(vshControl *ctl,
-const char *name,
-unsigned int flags)
-{
-return virshLookupDomainInternal(ctl, "unknown", name, flags);
-}
-
-
-virDomainPtr
-virshCommandOptDomainBy(vshControl *ctl, const vshCmd *cmd,
-const char **name, unsigned int flags)
-{
-const char *n = NULL;
-const char *optname = "domain";
-
-if (vshCommandOptStringReq(ctl, cmd, optname, &n) < 0)
-return NULL;
-
-vshDebug(ctl, VSH_ERR_INFO, "%s: found option <%s>: %s\n",
- cmd->def->name, optname, n);
-
-if (name)
-*name = n;
-
-return virshLookupDomainInternal(ctl, cmd->def->name, n, flags);
-}

 static virDomainPtr
 virshDomainDefine(virConnectPtr conn, const char *xml, unsigned int flags)
diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h
index 462f560f9..3f9d12a5f 100644
--- a/tools/virsh-domain.h
+++ b/tools/virsh-domain.h
@@ -28,18 +28,6 @@

 # include "virsh.h"

-virDomainPtr virshLookupDomainBy(vshControl *ctl,
- const char *name,
- unsigned int flags);
-
-virDomainPtr virshCommandOptDomainBy(vshControl *ctl, const vshCmd *cmd,
- const char **name, unsigned int flags);
-
-/* default is lookup by Id, Name and UUID */
-# define virshCommandOptDomain(_ctl, _cmd, _name)  \
-virshCommandOptDomainBy(_ctl, _cmd, _name, \
-VIRSH_BYID | VIRSH_BYUUID | VIRSH_BYNAME)
-
 extern const vshCmdDef domManagementCmds[];

 #endif /* VIRSH_DOMAIN_H */
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 3b86c75fa..5509065fd 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -35,7 +35,6 @@
 #include "virbitmap.h"
 #include "virbuffer.h"
 #include "viralloc.h"
-#include "virsh-domain.h"
 #include "virxml.h"
 #include "virtypedparam.h"
 #include "virstring.h"
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 5c844a5ea..46e2cbb24 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snap

[libvirt] [PATCH 0/9] virsh: don't overwrite errors in virDomain(Snapshot)Free functions and other refactors

[Peter Krempa]

Avoid the annoying problem that virDomainFree and virDomainSnapshotFree reset
libvirt errors and thus errors from helper functions don't get reported.

This seriesl also contains other refactors which I noticed along.

Peter Krempa (9):
  virsh: Add new file for utility functions and move a few
  virsh-util: Move domain lookup helpers into virsh-util
  virsh-util: Add wrapper for virDomainFree
  virsh: Add wrapper for virDomainSnapshotFree
  vsh: Add helper for safe remembering of libvirt errors
  virsh: add helpers for getting domain XML for XPath purposes
  virsh-domain-monitor: Use the virsh wrappers for getting XML to
simplify code
  virsh-domain: Use the virsh wrappers for getting XML to simplify code
  virsh-domain: Refactor cmdTTYConsole

 cfg.mk   |   8 +
 po/POTFILES.in   |   1 +
 tools/Makefile.am|   1 +
 tools/virsh-domain-monitor.c |  89 +++
 tools/virsh-domain.c | 370 ++-
 tools/virsh-domain.h |  12 --
 tools/virsh-host.c   |   1 -
 tools/virsh-snapshot.c   |  74 -
 tools/virsh-util.c   | 222 ++
 tools/virsh-util.h   |  78 +
 tools/virsh-volume.c |   1 +
 tools/virsh.c|  41 -
 tools/virsh.h|   4 -
 tools/vsh.c  |  15 ++
 tools/vsh.h  |   1 +
 15 files changed, 493 insertions(+), 425 deletions(-)
 create mode 100644 tools/virsh-util.c
 create mode 100644 tools/virsh-util.h

-- 
2.12.2

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

[libvirt] [PATCH 1/9] virsh: Add new file for utility functions and move a few

[Peter Krempa]

Don't accumulate helpers in virsh.c
---
 tools/Makefile.am|  1 +
 tools/virsh-domain-monitor.c |  1 +
 tools/virsh-domain.c |  1 +
 tools/virsh-util.c   | 66 
 tools/virsh-util.h   | 35 +++
 tools/virsh-volume.c |  1 +
 tools/virsh.c| 41 ---
 tools/virsh.h|  4 ---
 8 files changed, 105 insertions(+), 45 deletions(-)
 create mode 100644 tools/virsh-util.c
 create mode 100644 tools/virsh-util.h

diff --git a/tools/Makefile.am b/tools/Makefile.am
index bfacaf214..56691c289 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -226,6 +226,7 @@ virsh_SOURCES = 
\
virsh-pool.c virsh-pool.h   \
virsh-secret.c virsh-secret.h   \
virsh-snapshot.c virsh-snapshot.h   \
+   virsh-util.c virsh-util.h   \
virsh-volume.c virsh-volume.h   \
$(NULL)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 901a6ebef..4ade5651c 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -25,6 +25,7 @@

 #include 
 #include "virsh-domain-monitor.h"
+#include "virsh-util.h"

 #include 
 #include 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 4b6c13ce4..7db74fecf 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -25,6 +25,7 @@

 #include 
 #include "virsh-domain.h"
+#include "virsh-util.h"

 #include 
 #include 
diff --git a/tools/virsh-util.c b/tools/virsh-util.c
new file mode 100644
index 0..98f16ff1a
--- /dev/null
+++ b/tools/virsh-util.c
@@ -0,0 +1,66 @@
+/*
+ * virsh-util.c: helpers for virsh
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include "virsh-util.h"
+
+#include "virfile.h"
+
+int
+virshDomainState(vshControl *ctl,
+ virDomainPtr dom,
+ int *reason)
+{
+virDomainInfo info;
+virshControlPtr priv = ctl->privData;
+
+if (reason)
+*reason = -1;
+
+if (!priv->useGetInfo) {
+int state;
+if (virDomainGetState(dom, &state, reason, 0) < 0) {
+virErrorPtr err = virGetLastError();
+if (err && err->code == VIR_ERR_NO_SUPPORT)
+priv->useGetInfo = true;
+else
+return -1;
+} else {
+return state;
+}
+}
+
+/* fall back to virDomainGetInfo if virDomainGetState is not supported */
+if (virDomainGetInfo(dom, &info) < 0)
+return -1;
+else
+return info.state;
+}
+
+
+int
+virshStreamSink(virStreamPtr st ATTRIBUTE_UNUSED,
+const char *bytes,
+size_t nbytes,
+void *opaque)
+{
+int *fd = opaque;
+
+return safewrite(*fd, bytes, nbytes);
+}
diff --git a/tools/virsh-util.h b/tools/virsh-util.h
new file mode 100644
index 0..207d57859
--- /dev/null
+++ b/tools/virsh-util.h
@@ -0,0 +1,35 @@
+/*
+ * virsh-util.h: helpers for virsh
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#ifndef VIRSH_UTIL_H
+# define VIRSH_UTIL_H
+
+# include "virsh.h"
+
+int
+virshDomainState(vshControl *ctl,
+ virDomainPtr dom,
+ int *reason);
+
+int
+virshStreamSink(virStreamPtr st,
+const char *bytes,
+size_t nbytes,
+void *opaque);
+
+#endif /* VIRSH_UTIL_H */
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 32ffb8149..ddd41d229 100644
--- a/tools/virsh-vo

Re: [libvirt] [PATCH v4 09/14] hostdev: Maintain a driver list of active mediated devices

[Daniel P. Berrange]

On Wed, Mar 22, 2017 at 04:27:37PM +0100, Erik Skultety wrote:
> Keep track of the assigned mediated devices the same way we do it for
> the rest of hostdevs. Methods like 'Prepare', 'Update', and 'ReAttach'
> are introduced by this patch.
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/libvirt_private.syms |   3 +
>  src/qemu/qemu_hostdev.c  |  56 
>  src/qemu/qemu_hostdev.h  |  10 +++
>  src/util/virhostdev.c| 165 
> ++-
>  src/util/virhostdev.h|  23 +++
>  5 files changed, 256 insertions(+), 1 deletion(-)


> @@ -305,6 +324,24 @@ qemuHostdevPrepareSCSIVHostDevices(virQEMUDriverPtr 
> driver,
>  }
>  
>  int
> +qemuHostdevPrepareMediatedDevices(virQEMUDriverPtr driver,
> +  const char *name,
> +  virDomainHostdevDefPtr *hostdevs,
> +  int nhostdevs)
> +{
> +virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> +
> +if (!qemuHostdevHostSupportsPassthroughVFIO()) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("host doesn't support VFIO PCI interface"));
> +return -1;
> +}

This is unconditionally breaking *all* use of host devices on libvirt
if the system lacks VFIO, as it is not actually checking if any of
the 'hostdevs' are actually mediated devices, or indeed whether they
are even PCI devices. ie i can no longer boot a guest that uses USB
host device passthrough.

> +
> +return virHostdevPrepareMediatedDevices(hostdev_mgr, QEMU_DRIVER_NAME,
> +name, hostdevs, nhostdevs);
> +}
> +
> +int
>  qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver,
>  virDomainDefPtr def,
>  virQEMUCapsPtr qemuCaps,
> @@ -330,6 +367,10 @@ qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver,
> def->hostdevs, def->nhostdevs) < 
> 0)
>  return -1;
>  
> +if (qemuHostdevPrepareMediatedDevices(driver, def->name,
> +  def->hostdevs, def->nhostdevs) < 0)
> +return -1;



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

[libvirt] [PATCH] hyperv: recognize array property as distinct type.

[Dawid Zamirski]

When hyperv code generator for WMI classes identifies common
properties, it needs to take into account array type as a distinct
type, i.e string != string[]. This is the case where v1 of the
Msvm_VirtualSystemSettingData has Notes property as string whereas v2
uses Notes[], therefore they have to be treated as different fields and
cannot be placed in the "common" struct.
---
 src/hyperv/hyperv_driver.c | 26 --
 src/hyperv/hyperv_wmi_generator.py |  2 +-
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 090ea24..5a27908 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -894,8 +894,30 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int 
flags)
 if (VIR_STRDUP(def->name, computerSystem->data.common->ElementName) < 0)
 goto cleanup;
 
-if (VIR_STRDUP(def->description, 
virtualSystemSettingData->data.common->Notes) < 0)
-goto cleanup;
+if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
+if (VIR_STRDUP(def->description,
+   virtualSystemSettingData->data.v1->Notes) < 0)
+goto cleanup;
+} else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2 &&
+   virtualSystemSettingData->data.v2->Notes.data != NULL) {
+char **notes = NULL;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+size_t i = 0;
+
+/* in practice Notes has 1 element */
+for (i = 0; i < virtualSystemSettingData->data.v2->Notes.count; i++) {
+/* but if there's more than 1, separate by double new line */
+if (virBufferUse(&buf) > 0)
+virBufferAddLit(&buf, "\n\n");
+
+notes = (char **) virtualSystemSettingData->data.v2->Notes.data;
+virBufferAdd(&buf, *notes, -1);
+notes++;
+}
+
+def->description = virBufferContentAndReset(&buf);
+}
+
 
 virDomainDefSetMemoryTotal(def, memorySettingData->data.common->Limit * 
1024); /* megabyte to kilobyte */
 def->mem.cur_balloon = memorySettingData->data.common->VirtualQuantity * 
1024; /* megabyte to kilobyte */
diff --git a/src/hyperv/hyperv_wmi_generator.py 
b/src/hyperv/hyperv_wmi_generator.py
index c15d97a..9aee0b9 100755
--- a/src/hyperv/hyperv_wmi_generator.py
+++ b/src/hyperv/hyperv_wmi_generator.py
@@ -251,7 +251,7 @@ class WmiClass:
 for cls in self.versions:
 for prop in cls.properties:
 # consdered same if matches by name AND type
-key = "%s_%s" % (prop.name, prop.type)
+key = "%s_%s_%s" % (prop.name, prop.type, prop.is_array)
 
 if key in property_info:
 property_info[key][1] += 1
-- 
2.9.3

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

Re: [libvirt] Question about virsh domifaddr

[Daniel P. Berrange]

On Tue, Apr 11, 2017 at 06:48:05AM -0700, Vinod Chegu wrote:
> Hi,
> 
> In one of our test env. the virsh domifaddr  --full doesn't
> provide any info about the interfaces of a live VM when the interfaces were
> configured on linux bridges (DHCP is being used to IPs).  This command does
> return information when interfaces were using NAT.

domifaddr has two ways to get the answer. First it can ask the libvirt
managed dnsmasq instance what it assigned for DHCP. Second it can ask
the QEMU guest agent to report what the guest OS has.

The former won't work for you if you're relying on DHCP server on the
LAN, instead of libvirt's local NAT based network. The latter obviously
requires you to install the guest agent.



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

[libvirt] Question about virsh domifaddr

[Vinod Chegu]

Hi,

In one of our test env. the virsh domifaddr  --full doesn't
provide any info about the interfaces of a live VM when the interfaces were
configured on linux bridges (DHCP is being used to IPs).  This command does
return information when interfaces were using NAT.

Is this expected behavior ?

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

Re: [libvirt] [PATCH] tests: link virpcimock with utils

[Martin Kletzander]

On Mon, Apr 10, 2017 at 12:26:40PM +0100, Daniel P. Berrange wrote:

On Mon, Apr 10, 2017 at 01:18:05PM +0200, Michal Privoznik wrote:

On 04/10/2017 01:04 PM, Daniel P. Berrange wrote:
> On Mon, Apr 10, 2017 at 01:00:58PM +0200, Michal Privoznik wrote:
>> On 04/10/2017 09:52 AM, Pavel Hrdina wrote:
>>> In virpcimock we use some functions from utils so we should link
>>> utils library to provide required symbols.
>>>
>>> Signed-off-by: Pavel Hrdina 
>>> ---
>>>  tests/Makefile.am | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>>> index 279e9b7da8..8b9bba31ce 100644
>>> --- a/tests/Makefile.am
>>> +++ b/tests/Makefile.am
>>> @@ -1131,7 +1131,8 @@ virpcimock_la_SOURCES = \
>>>virpcimock.c
>>>  virpcimock_la_CFLAGS = $(AM_CFLAGS)
>>>  virpcimock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
>>> -virpcimock_la_LIBADD = $(MOCKLIBS_LIBS)
>>> +virpcimock_la_LIBADD = $(MOCKLIBS_LIBS) \
>>> +  ../src/libvirt_util.la
>>
>> You forgot to include $(PROBES_O). But even with that added in, this will
>> not fly. While you make it work under libvirt it fails without it because
>> 'stat' is undefined but called. Look into virtestmock.c for explanation.
>>
>> I'm not sure how to address this issue. I know about it for quite some time
>> now but all my previous attempts to fix it (including this one) failed. I've
>> tried to mimic what virtestmock.c does, but without any luck.
>
> What is the actual build problem with the virpcimock ? It seems to work
> fine for me right now.

It does build fine. You want to run it under valgrind:

libvirt.git/tests $ ../run valgrind --trace-children=yes ./virpcitest
==30672== Memcheck, a memory error detector
==30672== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==30672== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==30672== Command: libvirt.git/tests/.libs/virpcitest
==30672==
valgrind: symbol lookup error: libvirt.git/tests/.libs/virpcimock.so: undefined 
symbol: virFree



>
> FWIW, the original intenion was that the mock impls should not use any
> libvirt APIs, just pure POSIX / Linux APIs, so as to avoid dependancies
> which could end up being circular.

Yeah, I was just about to suggests this. However, in general would
it be a problem if a mock library would link with say libvirt_utils
statically? That way we shouldn't have any dependency problems,
should we?


Yep, static linking makes you immune from LD_PRELOADs.

The one caveat is about global data & any 3rd party library initialization
though. eg You'd now have 2 copies of any global variables, and if you
trigger code that initializes 3rd party libraries that'd get run twice.

Probably ok if we're only using very simple things like virfile & virstring
but if we get into the more advanced modules it could cause trouble.



Can't we instead make all mocked functions just weak aliases to our real
functions?  That way mocks wouldn't need to be preloaded and it could
'just work'.  At least that what I think why virfilemock.c works.
Despite its name it's not actually a mock.  See vircaps2xmltest.c and
Makefile in patch 04/12:

 https://www.redhat.com/archives/libvir-list/2017-April/msg00233.html

And usage in 05/12:

 https://www.redhat.com/archives/libvir-list/2017-April/msg00234.html

Martin


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

Re: [libvirt] [PATCH 0/8] qemu tests improvements and cleanups

[Pavel Hrdina]

On Tue, Apr 11, 2017 at 01:39:48PM +0200, Ján Tomko wrote:
> On Fri, Apr 07, 2017 at 03:44:15PM +0200, Pavel Hrdina wrote:
> >Pavel Hrdina (8):
> >  tests/qemuxml2argvtest: remove unnecessary machine canonicalization
> >  tests/qemuxml2xmltest: remove NOP call of virQEMUCapsSetList
> >  tests: use global virQEMUDriver
> >  tests/testutilsqemu: extract guest creation into separate functions
> >  tests/testutilsqemu: introduce QEMUBinList with all qemu binaries for
> >tests
> >  tests: don't use different QEMU binary paths for different virt types
> >  tests: unify qemu binary paths for all qemu related tests
> >  tests/testutilsqemu: properly initialize qemu caps for tests
> >
> 
> ACK series
> 
> (including the error->cleanup change in testInfoSet if you split it out
> of patch 8/8)

Thanks, I've decided to drop the error->cleanup change as it actually
doesn't improve the code at all.

Pushed now.

Pavel


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

Re: [libvirt] [PATCH 8/8] tests/testutilsqemu: properly initialize qemu caps for tests

[Pavel Hrdina]

On Tue, Apr 11, 2017 at 01:39:08PM +0200, Ján Tomko wrote:
> On Fri, Apr 07, 2017 at 03:44:23PM +0200, Pavel Hrdina wrote:
> >This removes the hacky extern global variable and modifies the
> >test code to properly create QEMU capabilities cache for QEMU
> >binaries used in our tests.
> >
> >Signed-off-by: Pavel Hrdina 
> >---
> > src/qemu/qemu_capabilities.c |  6 --
> > tests/qemuhotplugtest.c  | 11 ---
> > tests/qemuxml2argvtest.c |  7 +++
> > tests/qemuxml2xmltest.c  | 30 --
> > tests/testutilsqemu.c| 36 +++-
> > tests/testutilsqemu.h|  5 +
> > 6 files changed, 43 insertions(+), 52 deletions(-)
> >
> 
> >diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> >index 18ff5ad147..525aa67e02 100644
> >--- a/tests/qemuxml2argvtest.c
> >+++ b/tests/qemuxml2argvtest.c
> >@@ -434,16 +434,15 @@ testCompareXMLToArgv(const void *data)
> > if (virQEMUCapsGet(info->qemuCaps, QEMU_CAPS_ENABLE_FIPS))
> > flags |= FLAG_FIPS;
> >
> >-if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->name,
> >-info->qemuCaps) < 0)
> >-goto cleanup;
> >-
> > if (virAsprintf(&xml, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml",
> > abs_srcdir, info->name) < 0 ||
> > virAsprintf(&args, "%s/qemuxml2argvdata/qemuxml2argv-%s.args",
> > abs_srcdir, info->name) < 0)
> > goto cleanup;
> >
> >+if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps) < 0)
> >+goto cleanup;
> >+
> 
> Is there a reason for exchaging these two conditions?

No, this is a leftover from early version where I was parsing the binary
from the *xml*.  I'll drop the movement.

> > if (info->migrateFrom &&
> > !(migrateURI = qemuMigrationIncomingURI(info->migrateFrom,
> > info->migrateFd)))
> 
> >diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> >index 579328912a..e1ef9e5b86 100644
> >--- a/tests/qemuxml2xmltest.c
> >+++ b/tests/qemuxml2xmltest.c
> >@@ -293,15 +294,16 @@ testInfoSet(struct testInfo *info,
> > if (virAsprintf(&info->outActiveName,
> > "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s.xml",
> > abs_srcdir, name) < 0)
> >-goto error;
> >+goto cleanup;
> > }
> > }
> >
> >-return 0;
> >+ret = 0;
> >
> >- error:
> >-testInfoFree(info);
> >-return -1;
> >+ cleanup:
> >+if (ret < 0)
> >+testInfoFree(info);
> >+return ret;
> > }
> 
> The error -> cleanup change also does not belong in this patch.

This one is also a leftover, but it makes sense so I'll move it to
separate patch.

Pavel


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

Re: [libvirt] [PATCH 0/8] qemu tests improvements and cleanups

[Ján Tomko]

On Fri, Apr 07, 2017 at 03:44:15PM +0200, Pavel Hrdina wrote:

Pavel Hrdina (8):
 tests/qemuxml2argvtest: remove unnecessary machine canonicalization
 tests/qemuxml2xmltest: remove NOP call of virQEMUCapsSetList
 tests: use global virQEMUDriver
 tests/testutilsqemu: extract guest creation into separate functions
 tests/testutilsqemu: introduce QEMUBinList with all qemu binaries for
   tests
 tests: don't use different QEMU binary paths for different virt types
 tests: unify qemu binary paths for all qemu related tests
 tests/testutilsqemu: properly initialize qemu caps for tests



ACK series

(including the error->cleanup change in testInfoSet if you split it out
of patch 8/8)

Jan


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

Re: [libvirt] [PATCH 8/8] tests/testutilsqemu: properly initialize qemu caps for tests

[Ján Tomko]

On Fri, Apr 07, 2017 at 03:44:23PM +0200, Pavel Hrdina wrote:

This removes the hacky extern global variable and modifies the
test code to properly create QEMU capabilities cache for QEMU
binaries used in our tests.

Signed-off-by: Pavel Hrdina 
---
src/qemu/qemu_capabilities.c |  6 --
tests/qemuhotplugtest.c  | 11 ---
tests/qemuxml2argvtest.c |  7 +++
tests/qemuxml2xmltest.c  | 30 --
tests/testutilsqemu.c| 36 +++-
tests/testutilsqemu.h|  5 +
6 files changed, 43 insertions(+), 52 deletions(-)




diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 18ff5ad147..525aa67e02 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -434,16 +434,15 @@ testCompareXMLToArgv(const void *data)
if (virQEMUCapsGet(info->qemuCaps, QEMU_CAPS_ENABLE_FIPS))
flags |= FLAG_FIPS;

-if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->name,
-info->qemuCaps) < 0)
-goto cleanup;
-
if (virAsprintf(&xml, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml",
abs_srcdir, info->name) < 0 ||
virAsprintf(&args, "%s/qemuxml2argvdata/qemuxml2argv-%s.args",
abs_srcdir, info->name) < 0)
goto cleanup;

+if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps) < 0)
+goto cleanup;
+


Is there a reason for exchaging these two conditions?


if (info->migrateFrom &&
!(migrateURI = qemuMigrationIncomingURI(info->migrateFrom,
info->migrateFd)))



diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 579328912a..e1ef9e5b86 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -293,15 +294,16 @@ testInfoSet(struct testInfo *info,
if (virAsprintf(&info->outActiveName,
"%s/qemuxml2xmloutdata/qemuxml2xmlout-%s.xml",
abs_srcdir, name) < 0)
-goto error;
+goto cleanup;
}
}

-return 0;
+ret = 0;

- error:
-testInfoFree(info);
-return -1;
+ cleanup:
+if (ret < 0)
+testInfoFree(info);
+return ret;
}


The error -> cleanup change also does not belong in this patch.

Jan


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

Re: [libvirt] [PATCH 4/8] tests/testutilsqemu: extract guest creation into separate functions

[Ján Tomko]

On Fri, Apr 07, 2017 at 03:44:19PM +0200, Pavel Hrdina wrote:

All other architectures have separate functions to prepare guest
capabilities, do the same for i686 and x86_64 as well.

Signed-off-by: Pavel Hrdina 
---
tests/testutilsqemu.c | 182 +++---
1 file changed, 114 insertions(+), 68 deletions(-)

diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 4cc482dfb0..d82cebd578 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -141,6 +141,118 @@ static virCapsGuestMachinePtr 
*testQemuAllocNewerMachines(int *nmachines)
}


+static int
+testQemuAddI686Guest(virCapsPtr caps)
+{
+int nmachines = 0;
+virCapsGuestMachinePtr *machines = NULL;
+virCapsGuestPtr guest;
+
+if (!(machines = testQemuAllocMachines(&nmachines)))
+goto error;
+
+if (!(guest = virCapabilitiesAddGuest(caps,
+  VIR_DOMAIN_OSTYPE_HVM,
+  VIR_ARCH_I686,
+  "/usr/bin/qemu",
+  NULL,
+  nmachines,
+  machines)))
+goto error;
+
+if (!virCapabilitiesAddGuestFeature(guest, "cpuselection", true, false))
+goto error;
+
+machines = NULL;


This is correct from the code movement point of view, but 
virCapabilitiesAddGuest
transfers the ownership of 'machines' to 'caps', so this assignment
should be above virCapabilitiesAddGuestFeature. Preferably in a
follow-up patch.


+
+if (!virCapabilitiesAddGuestDomain(guest,
+   VIR_DOMAIN_VIRT_QEMU,


[...]


+
+static int
+testQemuAddX86_64Guest(virCapsPtr caps)
+{
+int nmachines = 0;
+virCapsGuestMachinePtr *machines = NULL;
+virCapsGuestPtr guest;
+
+if (!(machines = testQemuAllocNewerMachines(&nmachines)))
+goto error;
+
+if (!(guest = virCapabilitiesAddGuest(caps,
+  VIR_DOMAIN_OSTYPE_HVM,
+  VIR_ARCH_X86_64,
+  "/usr/bin/qemu-system-x86_64",
+  NULL,
+  nmachines,
+  machines)))
+goto error;
+
+if (!virCapabilitiesAddGuestFeature(guest, "cpuselection", true, false))
+goto error;
+
+machines = NULL;


Same here.

Jan


+if (!virCapabilitiesAddGuestDomain(guest,
+   VIR_DOMAIN_VIRT_QEMU,
+   NULL,


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

Re: [libvirt] [PATCH 0/4] fix some resource leaks

[Pavel Hrdina]

On Tue, Apr 11, 2017 at 12:51:58PM +0200, Ján Tomko wrote:
> On Mon, Apr 10, 2017 at 09:53:56AM +0200, Pavel Hrdina wrote:
> >Pavel Hrdina (4):
> >  conf/domain_capabilities: fix resource leak
> >  src: fix multiple resource leaks in loops
> >  rpc: fix resource leak
> >  tests: fix some resource leaks
> >
> 
> ACK series, terms and conditions apply.

Thanks, terms and conditions accepted and pushed.

Pavel


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

Re: [libvirt] [PATCH 0/4] fix some resource leaks

[Ján Tomko]

On Mon, Apr 10, 2017 at 09:53:56AM +0200, Pavel Hrdina wrote:

Pavel Hrdina (4):
 conf/domain_capabilities: fix resource leak
 src: fix multiple resource leaks in loops
 rpc: fix resource leak
 tests: fix some resource leaks



ACK series, terms and conditions apply.

Jan


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

Re: [libvirt] [PATCH 0/2] Fixes for CI virt-manager test failures

[Michal Privoznik]

On 04/10/2017 07:56 PM, John Ferlan wrote:

Test began failing after commit 'd06d1a32' and 'be3c2dfd':

https://ci.centos.org/view/libvirt/job/virt-manager-master-check/systems=libvirt-fedora-rawhide/426/console

Needed to fix the aclfilter check in both NumOfDevices and GetNames


John Ferlan (2):
  conf: Fix virNodeDeviceObjGetNames nnames increment
  nodedev: Fix aclfilter check

 src/conf/virnodedeviceobj.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)



ACK

Michal

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

Re: [libvirt] [PATCH 4/4] tests: fix some resource leaks

[Ján Tomko]

On Mon, Apr 10, 2017 at 09:54:00AM +0200, Pavel Hrdina wrote:

Found by running valgrind for these tests.

Signed-off-by: Pavel Hrdina 
---
tests/commandtest.c| 3 +++
tests/domaincapstest.c | 2 ++
tests/fchosttest.c | 2 ++
tests/vircgroupmock.c  | 2 ++
4 files changed, 9 insertions(+)

diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index f871197c25..b76332ce94 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -563,6 +563,8 @@ mymain(void)
DO_TEST_BHYVE("fbuf", "/usr/sbin/bhyve", &bhyve_caps, 
VIR_DOMAIN_VIRT_BHYVE);
#endif /* WITH_BHYVE */

+virObjectUnref(cfg);
+
return ret;
}



The cfg variable is defined inside an #if WITH_QEMU block, so the unref
should be inside one too.

Jan


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

Re: [libvirt] [PATCH 2/4] src: fix multiple resource leaks in loops

[Ján Tomko]

On Mon, Apr 10, 2017 at 09:53:58AM +0200, Pavel Hrdina wrote:

All of the variables are filled inside a loop and therefore
needs to be also freed in every cycle.

Signed-off-by: Pavel Hrdina 
---
src/conf/node_device_conf.c | 10 +-
src/conf/storage_conf.c |  1 +
src/util/virsysinfo.c   |  2 ++
3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 7d0baa9d1a..e21a98d51f 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -1582,7 +1582,6 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
for (i = 0, m = 0; i < n; i++) {
xmlNodePtr node = nodes[i];
char *tmp = virXMLPropString(node, "type");
-virNodeDevDevnodeType type;
int val;

if (!tmp) {
@@ -1591,15 +1590,16 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
goto error;
}

-if ((val = virNodeDevDevnodeTypeFromString(tmp)) < 0) {
+val = virNodeDevDevnodeTypeFromString(tmp);
+VIR_FREE(tmp);
+
+if (val < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
   _("unknown devnode type '%s'"), tmp);


This accesses the 'tmp' variable after it has been freed.

I suggest preserving the error message, so there will be two VIR_FREE's
needed - one for the success path and one for the error path.

Jan


-VIR_FREE(tmp);
goto error;
}


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

[libvirt] [RFC] RFC: Reimplement cache allocation for a VM

[Eli Qiao]

This is a RFC patch for the reimplement of `support cache tune(CAT) in
libvirt`[1].

There are already patch sets[2] to address it, and functional
works, but people doesn't like it cause it has global variable, and
missing unit test case for new added capabilites, etc.

Martin has proposed a test infra to do vircaps2xmltest, and I extened it
on top of it to extend resctrl control[3], this is kinds of new desiged
apart from [2], so I propose this RFC patch to do some rework on it.

[] https://www.redhat.com/archives/libvir-list/2017-January/msg00683.html
[] https://www.redhat.com/archives/libvir-list/2017-March/msg00181.html
[] https://www.redhat.com/archives/libvir-list/2017-April/msg00516.html
---
 src/util/virresctrl.c | 81 
 src/util/virresctrl.h | 86 +++
 2 files changed, 167 insertions(+)
 create mode 100644 src/util/virresctrl.c
 create mode 100644 src/util/virresctrl.h

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
new file mode 100644
index 000..f555eb8
--- /dev/null
+++ b/src/util/virresctrl.c
@@ -0,0 +1,81 @@
+/*
+ * virresctrl.c: methods for managing resource control
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Authors:
+ *  Eli Qiao 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "virresctrl.h"
+#include "viralloc.h"
+#include "virerror.h"
+#include "virfile.h"
+#include "virhostcpu.h"
+#include "virlog.h"
+#include "virstring.h"
+#include "virarch.h"
+
+VIR_LOG_INIT("util.resctrl");
+
+#define VIR_FROM_THIS VIR_FROM_RESCTRL
+
+VIR_ENUM_IMPL(virResctrl, VIR_RESCTRL_TYPE_LAST,
+  "L3",
+  "L3CODE",
+  "L3DATA",
+  "L2")
+
+/**
+ * a virResctrlDomain represents a resource control group, it's a directory
+ * under /sys/fs/resctrl.
+ * eg: /sys/fs/resctrl/CG1
+ * |-- cpus
+ * |-- schemata
+ * `-- tasks
+ * # cat schemata
+ * L3DATA:0=f;1=f
+ * L3CODE:0=f;1=f
+ *
+ * Besides, it can also represent the default resource control group of the
+ * host.
+ */
+
+typedef struct _virResctrlGroup virResctrlGroup;
+typedef virResctrlGroup *virResctrlGroupPtr;
+struct _virResctrlGroup {
+char *name; /* resource group name, eg: CG1. If it represent host's
+   default resource group name, should be a NULL pointer */
+size_t n_tasks; /* number of task assigned to the resource group */
+char **tasks; /* task list which contains task id eg: 77454 */
+
+size_t n_schematas; /* number of schemata the resource group contains,
+ eg: 2 */
+virResctrlSchemataPtr *schematas; /* scheamta list */
+};
+
+/* All resource control groups on this host, including default resource group 
*/
+typedef struct _virResCtrlDomain virResCtrlDomain;
+typedef virResCtrlDomain *virResCtrlDomainPtr;
+struct _virResCtrlDomain {
+size_t n_groups; /* number of resource control group */
+virResctrlGroupPtr groups; /* list of resource control group */
+};
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
new file mode 100644
index 000..da89542
--- /dev/null
+++ b/src/util/virresctrl.h
@@ -0,0 +1,86 @@
+/*
+ * virresctrl.h: header for managing resctrl control
+ *
+ * Copyright (C) 2016 Intel, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Authors:
+ * Eli Qiao 
+ */
+
+#ifndef __VIR_RESCTRL_H__
+# define __VIR_RESCTRL_H__
+
+typedef enum {
+VIR_RESCTRL_TYPE_L3,
+VIR_RESCTRL_TYPE_L3_CODE,
+VIR_RESCTRL_TYPE_L3_DATA,
+VIR_RESCTRL_TYPE_L2,
+
+VIR_RESCTRL_TYPE_LAST
+} virResctrlType;
+
+VIR_ENUM_DECL(virResctrl);
+
+/*
+ * a virResctrlSchemataItem represents one of schemat

[libvirt] [PATCH V5] Expose resource control capabilites on cache bank

[Eli Qiao]

This patch is based on Martin's cache branch.

* This patch amends the cache bank capability as follow:


  

  
  

  


For CDP enabled on x86 arch, we will have:

  


  
...

* Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
case.

* Along with vircaps2xmltest case updated.

P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "code" "data"}, I
keep it capital upper as I need to use a macro to convert from enum to
string easily.

Signed-off-by: Eli Qiao 
---
 docs/schemas/capability.rng|  20 +++
 src/conf/capabilities.c| 139 -
 src/conf/capabilities.h|  20 +++
 .../vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus |   1 +
 .../linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask |   1 +
 .../resctrl/info/L3CODE/min_cbm_bits   |   1 +
 .../resctrl/info/L3CODE/num_closids|   1 +
 .../linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask |   1 +
 .../resctrl/info/L3DATA/min_cbm_bits   |   1 +
 .../resctrl/info/L3DATA/num_closids|   1 +
 .../linux-resctrl-cdp/resctrl/manualres/cpus   |   1 +
 .../linux-resctrl-cdp/resctrl/manualres/schemata   |   2 +
 .../linux-resctrl-cdp/resctrl/manualres/tasks  |   0
 .../linux-resctrl-cdp/resctrl/schemata |   2 +
 .../linux-resctrl-cdp/resctrl/tasks|   0
 tests/vircaps2xmldata/linux-resctrl-cdp/system |   1 +
 .../vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml |  55 
 tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml   |   8 +-
 tests/vircaps2xmltest.c|   8 ++
 19 files changed, 259 insertions(+), 4 deletions(-)
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/min_cbm_bits
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/num_closids
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/min_cbm_bits
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/num_closids
 create mode 100644 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/cpus
 create mode 100644 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/schemata
 create mode 100644 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/tasks
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/schemata
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/tasks
 create mode 12 tests/vircaps2xmldata/linux-resctrl-cdp/system
 create mode 100644 tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml

diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
index 2080953..f371a1c 100644
--- a/docs/schemas/capability.rng
+++ b/docs/schemas/capability.rng
@@ -277,6 +277,26 @@
   
 
   
+  
+
+  
+
+  
+  
+
+  
+  
+
+  both
+  code
+  data
+
+  
+  
+
+  
+
+  
 
   
 
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 416dd1a..c7a2e73 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -52,6 +52,7 @@
 #define VIR_FROM_THIS VIR_FROM_CAPABILITIES
 
 #define SYSFS_SYSTEM_PATH "/sys/devices/system/"
+#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"
 
 VIR_LOG_INIT("conf.capabilities")
 
@@ -873,6 +874,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
 virCapsHostCacheBankPtr *caches)
 {
 size_t i = 0;
+size_t j = 0;
+int indent = virBufferGetIndent(buf, false);
+virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
 
 if (!ncaches)
 return 0;
@@ -894,13 +898,35 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
  */
 virBufferAsprintf(buf,
   "\n",
+  "size='%llu' unit='%s' cpus='%s'",
   bank->id, bank->level,
   virCacheTypeToString(bank->type),
   bank->size >> (kilos * 10),
   kilos ? "KiB" : "B",
   cpus_str);
 
+virBufferAdjustIndent(&controlBuf, indent + 4);
+for (j = 0; j < bank->ncontrols; j++) {
+bool min_kilos = !(bank->controls[j]->min % 1024);
+virBufferAsprintf(&controlBuf,
+  "\n",
+  bank->controls[j]->min >> (min_kilos * 10),
+  min_kilos ? "Ki

Re: [libvirt] Determining domain job kind from job stats?

[Daniel P. Berrange]

On Tue, Apr 11, 2017 at 09:49:42AM +0200, Jiri Denemark wrote:
> On Mon, Apr 10, 2017 at 12:01:43 +0100, Daniel P. Berrange wrote:
> > On Mon, Apr 10, 2017 at 12:58:09PM +0200, Milan Zamazal wrote:
> > > I looked how the change could be implemented.  Could you please help me
> > > clarify some things?
> > > 
> > > - I think a new member should be added to _virDomainJobInfo for the
> > >   purpose.  What would be a good name for it?  Maybe "operation"?
> > > - Do I need to care about backends other than QEMU?
> > > - Jobs are classified by qemuDomainAsyncJob, which is a QEMU specific
> > >   type.  Is it OK to use such structures in virsh-domain.c or is there
> > >   any additional abstraction needed?
> > 
> > I don't much like the idea of exposing the QEMU job operation names
> > in the public API. 
> > 
> > Perhaps we instead need to have the method which starts the job, return
> > an integer "job id" that is then reported against the job, so apps can
> > match them up.
> 
> The problem with "job id" is that only the process which started the job
> would know what it means. Not to mention it would require a lot of API
> changes.
> 
> I think we should just introduce a new virDomainJobSomething enum as
> 
> VIR_DOMAIN_JOB_SOMETHING_INCOMING_MIGRATION,
> VIR_DOMAIN_JOB_SOMETHING_OUTGOING_MIGRATION,
> VIR_DOMAIN_JOB_SOMETHING_SAVE,
> VIR_DOMAIN_JOB_SOMETHING_RESTORE,
> ...
> 
> and report it in virDomainGetJobStat (definitely not in
> _virDomainJobInfo as it would break ABI).
> 
> I'm not sure what the best name for "Something" would be. "Operation",
> "Action", or something else?

IMHO It would be an "Operation", since a single logical operation can involve
running multiple actions.

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 V4] Expose resource control capabilites on cache bank

[Eli Qiao]

On Tuesday, 11 April 2017 at 4:10 PM, Martin Kletzander wrote:

> This should just be:
> 
> + if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
> + abs_srcdir, data->filename) < 0)
> 
yep, stupid me. I get it now. --
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 03/16] qemu: introduce QEMU_DOMAIN_JOB_STATUS_POSTCOPY

[Nikolay Shirokovskiy]

Current code consults job.current->stats.status to check for postcopy
state. First it is more correct to check for both job.current->status
and job.current->stats.status.code because on some paths on failures
we change only the former. Second if qemu supports migration events
then stats can change unexpectedly.

Let's introduce QEMU_DOMAIN_JOB_STATUS_POSTCOPY state for job.current->status.

This patch removes all state checking usage of stats except for
qemuDomainGetJobStatsInternal. This place will be handled separately.
---
 src/qemu/qemu_domain.c|  1 +
 src/qemu/qemu_domain.h|  1 +
 src/qemu/qemu_driver.c|  5 +++--
 src/qemu/qemu_migration.c | 18 +++---
 src/qemu/qemu_process.c   |  4 ++--
 5 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0ae53ef..ba331dc 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -410,6 +410,7 @@ qemuDomainJobStatusToType(qemuDomainJobStatus status)
 break;
 
 case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
+case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
 return VIR_DOMAIN_JOB_UNBOUNDED;
 
 case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index a5791bf..58715c2 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -102,6 +102,7 @@ VIR_ENUM_DECL(qemuDomainAsyncJob)
 typedef enum {
 QEMU_DOMAIN_JOB_STATUS_NONE = 0,
 QEMU_DOMAIN_JOB_STATUS_ACTIVE,
+QEMU_DOMAIN_JOB_STATUS_POSTCOPY,
 QEMU_DOMAIN_JOB_STATUS_COMPLETED,
 QEMU_DOMAIN_JOB_STATUS_FAILED,
 QEMU_DOMAIN_JOB_STATUS_CANCELED,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c5cca07..48029be 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12787,7 +12787,8 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 }
 *jobInfo = *info;
 
-if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE) {
+if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
+jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
 if (fetch)
 ret = qemuMigrationFetchJobStatus(driver, vm, QEMU_ASYNC_JOB_NONE,
   jobInfo);
@@ -12921,7 +12922,7 @@ static int qemuDomainAbortJob(virDomainPtr dom)
 }
 
 if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT &&
-(priv->job.current->stats.status == 
QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY ||
+(priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY ||
  (virDomainObjGetState(vm, &reason) == VIR_DOMAIN_PAUSED &&
   reason == VIR_DOMAIN_PAUSED_POSTCOPY))) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index e9a3fd0..b4fc46c 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1314,6 +1314,10 @@ static void
 qemuMigrationUpdateJobType(qemuDomainJobInfoPtr jobInfo)
 {
 switch ((qemuMonitorMigrationStatus) jobInfo->stats.status) {
+case QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY:
+jobInfo->status = QEMU_DOMAIN_JOB_STATUS_POSTCOPY;
+break;
+
 case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED:
 jobInfo->status = QEMU_DOMAIN_JOB_STATUS_COMPLETED;
 break;
@@ -1332,7 +1336,6 @@ qemuMigrationUpdateJobType(qemuDomainJobInfoPtr jobInfo)
 
 case QEMU_MONITOR_MIGRATION_STATUS_SETUP:
 case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE:
-case QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY:
 case QEMU_MONITOR_MIGRATION_STATUS_CANCELLING:
 case QEMU_MONITOR_MIGRATION_STATUS_LAST:
 break;
@@ -1438,6 +1441,7 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 break;
 
 case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
+case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
 break;
 }
 return 0;
@@ -1495,8 +1499,7 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
  * will continue waiting until the migrate state changes to completed.
  */
 if (flags & QEMU_MIGRATION_COMPLETED_POSTCOPY &&
-jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE &&
-jobInfo->stats.status == QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY) {
+jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
 VIR_DEBUG("Migration switched to post-copy");
 if (updateStats &&
 qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0)
@@ -1510,7 +1513,8 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
 return 0;
 
  error:
-if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE) {
+if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
+jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
 /* The migration was aborted by us rather than QEMU itself. */
 jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
 return -2;
@@ -3799,7 +3803,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 else if (rc == -1)
 goto cleanup;
 
-if (priv->job.current->stat

[libvirt] [PATCH v3 10/16] qemu: introduce migrating job status

[Nikolay Shirokovskiy]

Instead of checking stat.status let's set status to migrating
as soon as migrate command is send (waiting for completion
is a good place too).
---
 src/qemu/qemu_domain.c| 1 +
 src/qemu/qemu_domain.h| 1 +
 src/qemu/qemu_driver.c| 4 +++-
 src/qemu/qemu_migration.c | 9 +++--
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2a14499..c023e0e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -410,6 +410,7 @@ qemuDomainJobStatusToType(qemuDomainJobStatus status)
 break;
 
 case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
+case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
 case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
 return VIR_DOMAIN_JOB_UNBOUNDED;
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 58715c2..564da49 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -102,6 +102,7 @@ VIR_ENUM_DECL(qemuDomainAsyncJob)
 typedef enum {
 QEMU_DOMAIN_JOB_STATUS_NONE = 0,
 QEMU_DOMAIN_JOB_STATUS_ACTIVE,
+QEMU_DOMAIN_JOB_STATUS_MIGRATING,
 QEMU_DOMAIN_JOB_STATUS_POSTCOPY,
 QEMU_DOMAIN_JOB_STATUS_COMPLETED,
 QEMU_DOMAIN_JOB_STATUS_FAILED,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3854a41..b592abe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12762,7 +12762,8 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 }
 
 /* Do not ask QEMU if migration is not even running yet  */
-if (!priv->job.current || !priv->job.current->stats.status)
+if (!priv->job.current ||
+priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE)
 fetch = false;
 
 if (fetch && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
@@ -12782,6 +12783,7 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 *jobInfo = *priv->job.current;
 
 if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
+jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
 if (fetch &&
 qemuMigrationFetchMigrationStats(driver, vm, QEMU_ASYNC_JOB_NONE,
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 388f770..35c86bb 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1421,6 +1421,7 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 
 case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
 case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
+case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
 case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
 break;
 }
@@ -1489,7 +1490,8 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
 return 0;
 
  error:
-if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
+/* state can not be active at this point */
+if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
 /* The migration was aborted by us rather than QEMU itself. */
 jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
@@ -1518,6 +1520,8 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
 int rv;
 
+jobInfo->status = QEMU_DOMAIN_JOB_STATUS_MIGRATING;
+
 while ((rv = qemuMigrationCompleted(driver, vm, asyncJob,
 dconn, flags)) != 1) {
 if (rv < 0)
@@ -3833,7 +3837,8 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 ignore_value(virTimeMillisNow(&priv->job.completed->sent));
 }
 
-if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE)
+if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
+priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING)
 priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
 
 cookieFlags |= QEMU_MIGRATION_COOKIE_NETWORK |
-- 
1.8.3.1

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

[libvirt] [PATCH v3 11/16] qemu: always get job condition on getting job stats

[Nikolay Shirokovskiy]

Looks like it is more simple to drop this optimization as we are
going to add getting disks stats during migration via quering qemu
process and checking if we have to acquire job condition becomes
more complicate.
---
 src/qemu/qemu_driver.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b592abe..bb7a64d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12741,7 +12741,7 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
   qemuDomainJobInfoPtr jobInfo)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
-bool fetch = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
+bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
 int ret = -1;
 
 if (completed) {
@@ -12761,12 +12761,7 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 return -1;
 }
 
-/* Do not ask QEMU if migration is not even running yet  */
-if (!priv->job.current ||
-priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE)
-fetch = false;
-
-if (fetch && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
 return -1;
 
 if (!virDomainObjIsActive(vm)) {
@@ -12785,7 +12780,8 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
-if (fetch &&
+
+if (events && jobInfo->status != QEMU_DOMAIN_JOB_STATUS_ACTIVE &&
 qemuMigrationFetchMigrationStats(driver, vm, QEMU_ASYNC_JOB_NONE,
  &jobInfo->stats, false) < 0)
 goto cleanup;
@@ -12797,8 +12793,7 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 ret = 0;
 
  cleanup:
-if (fetch)
-qemuDomainObjEndJob(driver, vm);
+qemuDomainObjEndJob(driver, vm);
 return ret;
 }
 
-- 
1.8.3.1

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

[libvirt] [PATCH v3 14/16] qemu: migation: resolve race on getting job info and stopping block jobs

[Nikolay Shirokovskiy]

During stopping mirror block jobs vm lock is droped on awating block
job events, thus next scenario is possible:

1. stop mirror block job is sent
2. migration routine awaits for block job event
3. mirror job stopped and event send
4. getting migration job info routine asks for block job info and as
qemu block job stopped and disapperaed we get no block job info.

As a result block job info for the disk will be missed in the result.

To handle this case let's ask qemu for block job info and store it
just before stopping mirror jobs. Next in getting migration job info
routine we detect this race condition and take stored block job info
instead. I guess info will be just slightly outdated and at least
will not go backwards.
---
 src/qemu/qemu_migration.c | 33 -
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 43d87a0..76c8554 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1557,6 +1557,7 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 ignore_value(qemuMigrationFetchMigrationStats(driver, vm, asyncJob,
   &jobInfo->stats, true));
 
+qemuMigrationFetchMirrorStats(driver, vm, asyncJob, NULL);
 qemuDomainJobInfoUpdateTime(jobInfo);
 qemuDomainJobInfoUpdateDowntime(jobInfo);
 VIR_FREE(priv->job.completed);
@@ -5884,6 +5885,9 @@ qemuMigrationReset(virQEMUDriverPtr driver,
 }
 
 
+/* Query qemu for block job stats. If @stats is not NULL then sum them up
+ * in @stats, otherwise store them in disks private area for each disk.
+ */
 int
 qemuMigrationFetchMirrorStats(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
@@ -5918,14 +5922,33 @@ qemuMigrationFetchMirrorStats(virQEMUDriverPtr driver,
 virDomainDiskDefPtr disk = vm->def->disks[i];
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
 qemuMonitorBlockJobInfoPtr data;
+unsigned long long transferred;
+unsigned long long total;
 
-if (!diskPriv->migrating ||
-!(data = virHashLookup(blockinfo, disk->info.alias)))
+if (!diskPriv->migrating)
 continue;
 
-stats->disk_transferred += data->cur;
-stats->disk_total += data->end;
-stats->disk_remaining += data->end - data->cur;
+data = virHashLookup(blockinfo, disk->info.alias);
+
+if (stats) {
+if (data) {
+transferred = data->cur;
+total = data->end;
+} else {
+/* Race condition. There is no blockjob for disk already
+ * but is has been migrating via nbd. Thanx we store job
+ * len value just before stopping mirror jobs which can be
+ * good approximation at this point.
+ */
+total = transferred = diskPriv->blockJobLength;
+}
+
+stats->disk_transferred += transferred;
+stats->disk_total += total;
+stats->disk_remaining += total - transferred;
+} else if (data) {
+diskPriv->blockJobLength = data->end;
+}
 }
 
 virHashFree(blockinfo);
-- 
1.8.3.1

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

[libvirt] [PATCH v3 13/16] qemu: support getting disks stats during stopping block jobs

[Nikolay Shirokovskiy]

Let's store disks stats for completed mirror jobs in current
job info. So on getting migration job stats thru API
we take records for completed jobs from current job info
and records for still active jobs by querying qemu process.

As we need to keep disks stats for completed mirror jobs
in current job let's zero out migration stats conditionally
in fetching function.
---
 src/qemu/qemu_blockjob.c |  1 +
 src/qemu/qemu_domain.h   |  1 +
 src/qemu/qemu_driver.c   |  3 +++
 src/qemu/qemu_migration.c| 11 +++
 src/qemu/qemu_monitor.c  |  5 +++--
 src/qemu/qemu_monitor.h  |  4 +++-
 src/qemu/qemu_monitor_json.c |  4 +---
 src/qemu/qemu_process.c  |  3 +++
 tests/qemumonitorjsontest.c  |  1 +
 9 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 0601e68..f517999 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -232,6 +232,7 @@ qemuBlockJobSyncBegin(virDomainDiskDefPtr disk)
 VIR_DEBUG("disk=%s", disk->dst);
 diskPriv->blockJobSync = true;
 diskPriv->blockJobStatus = -1;
+diskPriv->blockJobLength = 0;
 }
 
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 564da49..b002184 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -323,6 +323,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 */
+unsigned long long blockJobLength; /* length of the finished block job */
 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 1539e15..9a706d5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12781,6 +12781,9 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
 
+/* Disks stats accounting presumes that fetching migration
+ * stats will not touch disk stats records if disks are migrated via 
nbd.
+ */
 if (events && jobInfo->status != QEMU_DOMAIN_JOB_STATUS_ACTIVE &&
 qemuMigrationFetchMigrationStats(driver, vm, QEMU_ASYNC_JOB_NONE,
  &jobInfo->stats, false) < 0)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 16f1265..43d87a0 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -654,6 +654,7 @@ qemuMigrationDriveMirrorCancelled(virQEMUDriverPtr driver,
 size_t active = 0;
 int status;
 bool failed = false;
+qemuDomainObjPrivatePtr priv = vm->privateData;
 
 for (i = 0; i < vm->def->ndisks; i++) {
 virDomainDiskDefPtr disk = vm->def->disks[i];
@@ -681,6 +682,13 @@ qemuMigrationDriveMirrorCancelled(virQEMUDriverPtr driver,
 default:
 active++;
 }
+
+if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) {
+qemuMonitorMigrationStatsPtr stats = &priv->job.current->stats;
+
+stats->disk_transferred += diskPriv->blockJobLength;
+stats->disk_total += diskPriv->blockJobLength;
+}
 }
 
 if (failed) {
@@ -1357,6 +1365,9 @@ qemuMigrationFetchMigrationStats(virQEMUDriverPtr driver,
 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
 return -1;
 
+if (copy)
+memset(&statsCopy, 0, sizeof(statsCopy));
+
 rv = qemuMonitorGetMigrationStats(priv->mon, copy ? &statsCopy : stats);
 
 if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 1d40d52..06a5cf8 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1504,13 +1504,14 @@ int
 qemuMonitorEmitBlockJob(qemuMonitorPtr mon,
 const char *diskAlias,
 int type,
-int status)
+int status,
+unsigned long long len)
 {
 int ret = -1;
 VIR_DEBUG("mon=%p", mon);
 
 QEMU_MONITOR_CALLBACK(mon, ret, domainBlockJob, mon->vm,
-  diskAlias, type, status);
+  diskAlias, type, status, len);
 return ret;
 }
 
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 12f98be..a71c344 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -175,6 +175,7 @@ typedef int 
(*qemuMonitorDomainBlockJobCallback)(qemuMonitorPtr mon,
  const char *diskAlias,
  int type,
  int status,
+ unsigned long long len,
   

[libvirt] [PATCH v3 16/16] qemu: migration: don't expose incomplete job as complete

[Nikolay Shirokovskiy]

In case of real migration (not migrating to file on save, dump etc)
migration info is not complete at time qemu finishes migration
in normal (non postcopy) mode. We need to update disks stats,
downtime info etc. Thus let's not expose this job status as
completed.

To archive this let's set status to 'qemu completed' after
qemu reports migration is finished. It is not visible as complete
job to clients. Cookie code on confirm phase will finally turn
job into completed. As we don't need more things to do when
migrating to file status is set to 'completed' as before
in this case.
---
 src/qemu/qemu_domain.c|  1 +
 src/qemu/qemu_domain.h|  1 +
 src/qemu/qemu_driver.c|  1 +
 src/qemu/qemu_migration.c | 13 +
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c023e0e..a61ee81 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -411,6 +411,7 @@ qemuDomainJobStatusToType(qemuDomainJobStatus status)
 
 case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
 case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
+case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
 case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
 return VIR_DOMAIN_JOB_UNBOUNDED;
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index b002184..983ca4e 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -103,6 +103,7 @@ typedef enum {
 QEMU_DOMAIN_JOB_STATUS_NONE = 0,
 QEMU_DOMAIN_JOB_STATUS_ACTIVE,
 QEMU_DOMAIN_JOB_STATUS_MIGRATING,
+QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED,
 QEMU_DOMAIN_JOB_STATUS_POSTCOPY,
 QEMU_DOMAIN_JOB_STATUS_COMPLETED,
 QEMU_DOMAIN_JOB_STATUS_FAILED,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9a706d5..e7a0687 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12779,6 +12779,7 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 
 if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
+jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED ||
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
 
 /* Disks stats accounting presumes that fetching migration
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 1703274..dfecc96 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1327,7 +1327,7 @@ qemuMigrationUpdateJobType(qemuDomainJobInfoPtr jobInfo)
 break;
 
 case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED:
-jobInfo->status = QEMU_DOMAIN_JOB_STATUS_COMPLETED;
+jobInfo->status = QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED;
 break;
 
 case QEMU_MONITOR_MIGRATION_STATUS_INACTIVE:
@@ -1433,6 +1433,7 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
 case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
 case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
+case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
 case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
 break;
 }
@@ -1495,19 +1496,19 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
 return 1;
 }
 
-if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_COMPLETED)
+if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED)
 return 1;
 else
 return 0;
 
  error:
-/* state can not be active at this point */
+/* state can not be active or completed at this point */
 if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
 /* The migration was aborted by us rather than QEMU itself. */
 jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
 return -2;
-} else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_COMPLETED) {
+} else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
 jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
 return -1;
 } else {
@@ -1564,6 +1565,10 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 if (VIR_ALLOC(priv->job.completed) == 0)
 *priv->job.completed = *jobInfo;
 
+if (asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT &&
+jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED)
+jobInfo->status = QEMU_DOMAIN_JOB_STATUS_COMPLETED;
+
 return 0;
 }
 
-- 
1.8.3.1

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

[libvirt] [PATCH v3 07/16] qemu: simplify getting completed job stats

[Nikolay Shirokovskiy]

---
 src/qemu/qemu_driver.c | 26 --
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f23503a..da4d8e9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12744,12 +12744,18 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
   qemuDomainJobInfoPtr jobInfo)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
-qemuDomainJobInfoPtr info;
 bool fetch = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
 int ret = -1;
 
-if (completed)
-fetch = false;
+if (completed) {
+if (priv->job.current || !priv->job.completed) {
+jobInfo->status = QEMU_DOMAIN_JOB_STATUS_NONE;
+return 0;
+}
+
+*jobInfo = *priv->job.completed;
+return 0;
+}
 
 /* Do not ask QEMU if migration is not even running yet  */
 if (!priv->job.current || !priv->job.current->stats.status)
@@ -12766,26 +12772,18 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 return -1;
 }
 
-if (!completed &&
-!virDomainObjIsActive(vm)) {
+if (!virDomainObjIsActive(vm)) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("domain is not running"));
 goto cleanup;
 }
 
-if (completed && priv->job.current)
-info = NULL;
-else if (completed)
-info = priv->job.completed;
-else
-info = priv->job.current;
-
-if (!info) {
+if (!priv->job.current) {
 jobInfo->status = QEMU_DOMAIN_JOB_STATUS_NONE;
 ret = 0;
 goto cleanup;
 }
-*jobInfo = *info;
+*jobInfo = *priv->job.current;
 
 if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
-- 
1.8.3.1

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

[libvirt] [PATCH v3 00/16] qemu: migration: show disks stats for nbd migration

[Nikolay Shirokovskiy]

diff from v2:


1. Fix style issues.
2. Rework patch for fetching job info 
   (save logic to use temporary variable when drop vm lock)
3. Update disk stats when block jobs are canceled.
4. Adress a few more corner cases.

This patch series add disks stats to domain job info(stats) as
well as to migration completed event in case nbd scheme is used.

There is little nuisance with qcow2 disks (which is the main scenario
I guess) tied to the way qemu reports stats for this type of disks.
For example if we have 64G disk filled only to 1G then stats
start from 63G and will grow up to 64G on completion. The same way disk stats
will be reported by this patch.

I guess the better way to express the situation is to say we have 64G 'total',
and have 'processed' field grow from 0G to 1G, like in case of memory
stats. [1] is the example of completed memory stats of empty guest
domain, which show difference between processed and total.

There can be a workaround like getting initial blockjob offset position
as a zero but is is rather ugly and racy and like uses undocumented
behaviour.

[1] memory migration stats example
Memory processed: 3.307 MiB
Memory remaining: 0.000 B
Memory total: 1.032 GiB

The above is applied to qemu 2.6 at least.

Patches that were explicitly ACKed in previous review
(up to style issues) marked with A.

Nikolay Shirokovskiy (16):
  qemu: drop code for VIR_DOMAIN_JOB_BOUNDED and timeRemaining
A qemu: introduce qemu domain job status
A qemu: introduce QEMU_DOMAIN_JOB_STATUS_POSTCOPY
A qemu: drop QEMU_MIGRATION_COMPLETED_UPDATE_STATS
A qemu: drop excessive zero-out in qemuMigrationFetchJobStatus
  qemu: refactor fetching migration stats
A qemu: simplify getting completed job stats
  qemu: fail querying destination migration statistics always
  qemu: start all async job with job status active
  qemu: introduce migrating job status
  qemu: always get job condition on getting job stats
  qemu: migrate: show disks stats on job info requests
  qemu: support getting disks stats during stopping block jobs
  qemu: migation: resolve race on getting job info and stopping block jobs
  qemu: migrate: copy disks stats to completed job
  qemu: migration: don't expose incomplete job as complete

 src/qemu/qemu_blockjob.c |   1 +
 src/qemu/qemu_domain.c   |  38 +--
 src/qemu/qemu_domain.h   |  16 ++-
 src/qemu/qemu_driver.c   |  95 
 src/qemu/qemu_migration.c| 236 ++-
 src/qemu/qemu_migration.h|  15 ++-
 src/qemu/qemu_migration_cookie.c |   7 +-
 src/qemu/qemu_monitor.c  |   5 +-
 src/qemu/qemu_monitor.h  |   4 +-
 src/qemu/qemu_monitor_json.c |   4 +-
 src/qemu/qemu_process.c  |  10 +-
 tests/qemumonitorjsontest.c  |   1 +
 12 files changed, 273 insertions(+), 159 deletions(-)

-- 
1.8.3.1

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

[libvirt] [PATCH v3 02/16] qemu: introduce qemu domain job status

[Nikolay Shirokovskiy]

This patch simply switches code from using VIR_DOMAIN_JOB_* to
introduced QEMU_DOMAIN_JOB_STATUS_*. Later this gives us freedom
to introduce states for postcopy and mirroring phases.
---
 src/qemu/qemu_domain.c   | 27 --
 src/qemu/qemu_domain.h   | 11 -
 src/qemu/qemu_driver.c   | 11 -
 src/qemu/qemu_migration.c| 50 +++-
 src/qemu/qemu_migration_cookie.c |  2 +-
 src/qemu/qemu_process.c  |  2 +-
 6 files changed, 66 insertions(+), 37 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 54d9425..0ae53ef 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -402,11 +402,34 @@ qemuDomainJobInfoUpdateDowntime(qemuDomainJobInfoPtr 
jobInfo)
 return 0;
 }
 
+static virDomainJobType
+qemuDomainJobStatusToType(qemuDomainJobStatus status)
+{
+switch (status) {
+case QEMU_DOMAIN_JOB_STATUS_NONE:
+break;
+
+case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
+return VIR_DOMAIN_JOB_UNBOUNDED;
+
+case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
+return VIR_DOMAIN_JOB_COMPLETED;
+
+case QEMU_DOMAIN_JOB_STATUS_FAILED:
+return VIR_DOMAIN_JOB_FAILED;
+
+case QEMU_DOMAIN_JOB_STATUS_CANCELED:
+return VIR_DOMAIN_JOB_CANCELLED;
+}
+
+return VIR_DOMAIN_JOB_NONE;
+}
+
 int
 qemuDomainJobInfoToInfo(qemuDomainJobInfoPtr jobInfo,
 virDomainJobInfoPtr info)
 {
-info->type = jobInfo->type;
+info->type = qemuDomainJobStatusToType(jobInfo->status);
 info->timeElapsed = jobInfo->timeElapsed;
 
 info->memTotal = jobInfo->stats.ram_total;
@@ -561,7 +584,7 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
  stats->cpu_throttle_percentage) < 0)
 goto error;
 
-*type = jobInfo->type;
+*type = qemuDomainJobStatusToType(jobInfo->status);
 *params = par;
 *nparams = npar;
 return 0;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index caf1ebe..a5791bf 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -99,10 +99,19 @@ typedef enum {
 } qemuDomainAsyncJob;
 VIR_ENUM_DECL(qemuDomainAsyncJob)
 
+typedef enum {
+QEMU_DOMAIN_JOB_STATUS_NONE = 0,
+QEMU_DOMAIN_JOB_STATUS_ACTIVE,
+QEMU_DOMAIN_JOB_STATUS_COMPLETED,
+QEMU_DOMAIN_JOB_STATUS_FAILED,
+QEMU_DOMAIN_JOB_STATUS_CANCELED,
+} qemuDomainJobStatus;
+
 typedef struct _qemuDomainJobInfo qemuDomainJobInfo;
 typedef qemuDomainJobInfo *qemuDomainJobInfoPtr;
 struct _qemuDomainJobInfo {
-virDomainJobType type;
+qemuDomainJobStatus status;
+
 unsigned long long started; /* When the async job started */
 unsigned long long stopped; /* When the domain's CPUs were stopped */
 unsigned long long sent; /* When the source sent status info to the
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 388af4f..c5cca07 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3156,7 +3156,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, 
virDomainPtr dom,
 goto endjob;
 }
 
-priv->job.current->type = VIR_DOMAIN_JOB_UNBOUNDED;
+priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE;
 
 /* Pause */
 if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
@@ -12781,14 +12781,13 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 info = priv->job.current;
 
 if (!info) {
-jobInfo->type = VIR_DOMAIN_JOB_NONE;
+jobInfo->status = QEMU_DOMAIN_JOB_STATUS_NONE;
 ret = 0;
 goto cleanup;
 }
 *jobInfo = *info;
 
-if (jobInfo->type == VIR_DOMAIN_JOB_BOUNDED ||
-jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
+if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE) {
 if (fetch)
 ret = qemuMigrationFetchJobStatus(driver, vm, QEMU_ASYNC_JOB_NONE,
   jobInfo);
@@ -12823,7 +12822,7 @@ qemuDomainGetJobInfo(virDomainPtr dom,
 if (qemuDomainGetJobStatsInternal(driver, vm, false, &jobInfo) < 0)
 goto cleanup;
 
-if (jobInfo.type == VIR_DOMAIN_JOB_NONE) {
+if (jobInfo.status == QEMU_DOMAIN_JOB_STATUS_NONE) {
 memset(info, 0, sizeof(*info));
 info->type = VIR_DOMAIN_JOB_NONE;
 ret = 0;
@@ -12864,7 +12863,7 @@ qemuDomainGetJobStats(virDomainPtr dom,
 if (qemuDomainGetJobStatsInternal(driver, vm, completed, &jobInfo) < 0)
 goto cleanup;
 
-if (jobInfo.type == VIR_DOMAIN_JOB_NONE) {
+if (jobInfo.status == QEMU_DOMAIN_JOB_STATUS_NONE) {
 *type = VIR_DOMAIN_JOB_NONE;
 *params = NULL;
 *nparams = 0;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index d8222fe..e9a3fd0 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -946,7 +946,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 goto cleanup;
 
 if (priv->job.abortJob) {
-  

[libvirt] [PATCH v3 12/16] qemu: migrate: show disks stats on job info requests

[Nikolay Shirokovskiy]

This patch shows incorrect info when client request comes
when migration routine is stopping mirror jobs or mirror
jobs already stopped. This issue will be addressed in next
patch.
---
 src/qemu/qemu_driver.c|  4 
 src/qemu/qemu_migration.c | 49 +++
 src/qemu/qemu_migration.h |  6 ++
 3 files changed, 59 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bb7a64d..1539e15 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12786,6 +12786,10 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
  &jobInfo->stats, false) < 0)
 goto cleanup;
 
+if (qemuMigrationFetchMirrorStats(driver, vm, QEMU_ASYNC_JOB_NONE,
+  &jobInfo->stats) < 0)
+goto cleanup;
+
 if (qemuDomainJobInfoUpdateTime(jobInfo) < 0)
 goto cleanup;
 }
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 35c86bb..16f1265 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5871,3 +5871,52 @@ qemuMigrationReset(virQEMUDriverPtr driver,
 virFreeError(err);
 }
 }
+
+
+int
+qemuMigrationFetchMirrorStats(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  qemuDomainAsyncJob asyncJob,
+  qemuMonitorMigrationStatsPtr stats)
+{
+size_t i;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+bool nbd = false;
+virHashTablePtr blockinfo = NULL;
+
+for (i = 0; i < vm->def->ndisks; i++) {
+virDomainDiskDefPtr disk = vm->def->disks[i];
+if (QEMU_DOMAIN_DISK_PRIVATE(disk)->migrating) {
+nbd = true;
+break;
+}
+}
+
+if (!nbd)
+return 0;
+
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+return -1;
+
+blockinfo = qemuMonitorGetAllBlockJobInfo(priv->mon);
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || !blockinfo)
+return -1;
+
+for (i = 0; i < vm->def->ndisks; i++) {
+virDomainDiskDefPtr disk = vm->def->disks[i];
+qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+qemuMonitorBlockJobInfoPtr data;
+
+if (!diskPriv->migrating ||
+!(data = virHashLookup(blockinfo, disk->info.alias)))
+continue;
+
+stats->disk_transferred += data->cur;
+stats->disk_total += data->end;
+stats->disk_remaining += data->end - data->cur;
+}
+
+virHashFree(blockinfo);
+return 0;
+}
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index 1f6ddba..13cfe47 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -320,4 +320,10 @@ qemuMigrationReset(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuDomainAsyncJob job);
 
+int
+qemuMigrationFetchMirrorStats(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  qemuDomainAsyncJob asyncJob,
+  qemuMonitorMigrationStatsPtr stats);
+
 #endif /* __QEMU_MIGRATION_H__ */
-- 
1.8.3.1

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

[libvirt] [PATCH v3 09/16] qemu: start all async job with job status active

[Nikolay Shirokovskiy]

Setting status to none has little value - getting job status
will not return even elapsed time.

After this patch getting job stats stays correct in a sence
it will not fetch migration stats because it consults
stats.status before doing the fetch.
---
 src/qemu/qemu_domain.c| 1 +
 src/qemu/qemu_driver.c| 3 ---
 src/qemu/qemu_migration.c | 5 -
 src/qemu/qemu_process.c   | 3 ---
 4 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ba331dc..2a14499 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3622,6 +3622,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
 qemuDomainObjResetAsyncJob(priv);
 if (VIR_ALLOC(priv->job.current) < 0)
 goto cleanup;
+priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE;
 priv->job.asyncJob = asyncJob;
 priv->job.asyncOwner = virThreadSelfID();
 priv->job.asyncOwnerAPI = virThreadJobGet();
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e65448f..3854a41 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3138,7 +3138,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, 
virDomainPtr dom,
 bool was_running = false;
 int ret = -1;
 virObjectEventPtr event = NULL;
-qemuDomainObjPrivatePtr priv = vm->privateData;
 virCapsPtr caps;
 
 if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
@@ -3156,8 +3155,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, 
virDomainPtr dom,
 goto endjob;
 }
 
-priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE;
-
 /* Pause */
 if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
 was_running = true;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 1760908..388f770 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1518,7 +1518,6 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
 int rv;
 
-jobInfo->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE;
 while ((rv = qemuMigrationCompleted(driver, vm, asyncJob,
 dconn, flags)) != 1) {
 if (rv < 0)
@@ -5575,8 +5574,6 @@ qemuMigrationJobStart(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   qemuDomainAsyncJob job)
 {
-qemuDomainObjPrivatePtr priv = vm->privateData;
-
 if (qemuDomainObjBeginAsyncJob(driver, vm, job) < 0)
 return -1;
 
@@ -5588,8 +5585,6 @@ qemuMigrationJobStart(virQEMUDriverPtr driver,
   JOB_MASK(QEMU_JOB_MIGRATION_OP)));
 }
 
-priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE;
-
 return 0;
 }
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 42cb0d4..f5cd00b 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4145,13 +4145,10 @@ int
 qemuProcessBeginJob(virQEMUDriverPtr driver,
 virDomainObjPtr vm)
 {
-qemuDomainObjPrivatePtr priv = vm->privateData;
-
 if (qemuDomainObjBeginAsyncJob(driver, vm, QEMU_ASYNC_JOB_START) < 0)
 return -1;
 
 qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE);
-priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE;
 
 return 0;
 }
-- 
1.8.3.1

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

Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank

[Eli Qiao]

On Tuesday, 11 April 2017 at 3:48 PM, Peter Krempa wrote:

> On Tue, Apr 11, 2017 at 13:44:26 +0800, Eli Qiao wrote:
> > This patch is based on Martin's cache branch.
> >  
> > * This patch amends the cache bank capability as follow:
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  
> > For CDP enabled on x86 arch, we will have:
> > 
> > 
> > 
> > 
> > 
> > ...
> >  
> > * Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
> > case.
> >  
> > * Along with vircaps2xmltest case updated.
> >  
> > P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I
> > keep it capital upper as I need to use a macro to convert from enum to
> > string easily.
> >  
>  
>  
> We dont need to do that. The attributes should be lower case. The code
> can convert it to anything it needs.
>  
>  

what I did is that expose what the host’s sys/fs/resctrl/info looks like, if 
people
think we should use lower case, I am okay to change.  
>  
> >  
> > Signed-off-by: Eli Qiao  > (mailto:liyong.q...@intel.com)>
> > ---
> >  
>  
>  
> [...]
>  
> > diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> > index 2080953..bfed4f8 100644
> > --- a/docs/schemas/capability.rng
> > +++ b/docs/schemas/capability.rng
> > @@ -277,6 +277,26 @@
> > 
> > 
> > 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + BOTH
> > + CODE
> > + DATA
> >  
>  
>  
> Why are the values all caps? We prefer lowercase attributes in the XML.
>  
Answer in previous reply.  
>  
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > 
> > 
> > 
> > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> > index 416dd1a..c6641d1 100644
> > --- a/src/conf/capabilities.c
> > +++ b/src/conf/capabilities.c
> > @@ -52,6 +52,7 @@
> > #define VIR_FROM_THIS VIR_FROM_CAPABILITIES
> >  
> > #define SYSFS_SYSTEM_PATH "/sys/devices/system/"
> > +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"
> >  
> > VIR_LOG_INIT("conf.capabilities")
> >  
> > @@ -873,6 +874,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> > virCapsHostCacheBankPtr *caches)
> > {
> > size_t i = 0;
> > + size_t j = 0;
> > + int indent = virBufferGetIndent(buf, false);
> > + virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
> >  
> > if (!ncaches)
> > return 0;
> > @@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
> > */
> > virBufferAsprintf(buf,
> > " > - "size='%llu' unit='%s' cpus='%s'/>\n",
> > + "size='%llu' unit='%s' cpus='%s'",
> > bank->id, bank->level,
> > virCacheTypeToString(bank->type),
> > bank->size >> (kilos * 10),
> > kilos ? "KiB" : "B",
> > cpus_str);
> >  
> > + /* Format control */
> > + virBufferAdjustIndent(&controlBuf, indent + 4);
> >  
>  
>  
> This looks wrong. You should increase/decrease the indent by some
> number. The old value should not be used.
>  
I have test case covered, please check 
tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml  
>  
> > + for (j = 0; j < bank->ncontrols; j++) {
> > + bool min_kilos = !(bank->controls[j]->min % 1024);
> > + virBufferAsprintf(&controlBuf,
> > + " > + "scope='%s' max_allocation='%u'/>\n",
> > + bank->controls[j]->min >> (min_kilos * 10),
> > + min_kilos ? "KiB" : "B",
> > + virResctrlCacheTypeToString(bank->controls[j]->scope),
> > + bank->controls[j]->max_allocation);
> > + }
> > +
> > + /* Put it all together */
> >  
>  
>  
> You don't need to point out obvious things.

Just copy from other place, I am okay to remove it.  
> > + if (virBufferUse(&controlBuf)) {
>  
>  
> This logic looks weird and opaque. Can you decide by any other means
> than the filling of the buffer?
>  
>  

It’s same meaning with  bank->ncontrols > 0
this testfile will help you to easy understand what’s doing here.
tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml  

I am okay to change if you feel it’s hard to get understand.
>  
> > + virBufferAddLit(buf, ">\n");
> > + virBufferAddBuffer(buf, &controlBuf);
> > + virBufferAddLit(buf, "\n");
> > +
> > + } else {
> > + virBufferAddLit(buf, "/>\n");
> > + }
> > +
> > +
> > + virBufferFreeAndReset(&controlBuf);
> > VIR_FREE(cpus_str);
> > }
> >  
> > @@ -1513,13 +1542,101 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr 
> > a,
> > void
> > virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
> > {
> > + size_t i;
> > +
> > if (!ptr)
> > return;
> >  
> > virBitmapFree(ptr->cpus);
> > + for (i = 0; i < ptr->ncontrols; i++)
> > + VIR_FREE(ptr->controls[i]);
> > + VIR_FREE(ptr->controls);
> > VIR_FREE(ptr);
> > }
> >  
> > +VIR_ENUM_IMPL(virResctrlCache, VIR_RESCTRL_CACHE_TYPE_LAST,
> > + "BOTH",
> > + "CODE",
> > + "DATA")
> > +
> > +/* test which TYPE of cache control supported
> > + * -1: don't support
> > + * 0: cat
> > + * 1: cdp
> > + */
> > +static int
> > +virCapabilitiesGetCacheControlType(virCapsHostCacheBankPtr bank)
> > +{
> > + int ret = -1;
> > + char *path = NULL;
> > + if (virAsprintf(&path,
> > + SYSFS_RESCTRL_PATH "info/L%u",
> > + bank->level) < 0)
> > + return -1;
> > +
> > + if (

Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank

[Martin Kletzander]

On Tue, Apr 11, 2017 at 09:48:54AM +0200, Peter Krempa wrote:

On Tue, Apr 11, 2017 at 13:44:26 +0800, Eli Qiao wrote:

This patch is based on Martin's cache branch.

* This patch amends the cache bank capability as follow:


  

  
  

  


For CDP enabled on x86 arch, we will have:

  


  
...

* Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
case.

* Along with vircaps2xmltest case updated.

P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I
keep it capital upper as I need to use a macro to convert from enum to
string easily.


We dont need to do that. The attributes should be lower case. The code
can convert it to anything it needs.



Signed-off-by: Eli Qiao 
---


[...]


diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 416dd1a..c6641d1 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c


[...]


@@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
  */
 virBufferAsprintf(buf,
   "\n",
+  "size='%llu' unit='%s' cpus='%s'",
   bank->id, bank->level,
   virCacheTypeToString(bank->type),
   bank->size >> (kilos * 10),
   kilos ? "KiB" : "B",
   cpus_str);

+/* Format control */
+virBufferAdjustIndent(&controlBuf, indent + 4);


This looks wrong. You should increase/decrease the indent by some
number. The old value should not be used.



This is used everywhere in the code with childrenBuf, it's pretty widely
used and I think readable as well.  I agree with the rest of the review,
though.


+for (j = 0; j < bank->ncontrols; j++) {
+bool min_kilos = !(bank->controls[j]->min % 1024);
+virBufferAsprintf(&controlBuf,
+  "\n",
+  bank->controls[j]->min >> (min_kilos * 10),
+  min_kilos ? "KiB" : "B",
+  
virResctrlCacheTypeToString(bank->controls[j]->scope),
+  bank->controls[j]->max_allocation);
+}
+
+/* Put it all together */


You don't need to point out obvious things.


+if (virBufferUse(&controlBuf)) {


This logic looks weird and opaque. Can you decide by any other means
than the filling of the buffer?



Same here.


+virBufferAddLit(buf, ">\n");
+virBufferAddBuffer(buf, &controlBuf);
+virBufferAddLit(buf, "\n");
+
+} else {
+virBufferAddLit(buf, "/>\n");
+}
+
+
+virBufferFreeAndReset(&controlBuf);
 VIR_FREE(cpus_str);
 }



[...]


diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
index f590249..db7de4c 100644
--- a/tests/vircaps2xmltest.c
+++ b/tests/vircaps2xmltest.c
@@ -58,6 +59,13 @@ test_virCapabilities(const void *opaque)
 data->resctrl ? "/system" : "") < 0)
 goto cleanup;

+if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
+abs_srcdir,
+data->resctrl ? data->filename : "foo") < 0)


"foo"? Some testing code leftover?



This should just be:

+if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl",
+abs_srcdir, data->filename) < 0)

I think I said that in v3


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

Re: [libvirt] Determining domain job kind from job stats?

[Jiri Denemark]

On Mon, Apr 10, 2017 at 12:01:43 +0100, Daniel P. Berrange wrote:
> On Mon, Apr 10, 2017 at 12:58:09PM +0200, Milan Zamazal wrote:
> > I looked how the change could be implemented.  Could you please help me
> > clarify some things?
> > 
> > - I think a new member should be added to _virDomainJobInfo for the
> >   purpose.  What would be a good name for it?  Maybe "operation"?
> > - Do I need to care about backends other than QEMU?
> > - Jobs are classified by qemuDomainAsyncJob, which is a QEMU specific
> >   type.  Is it OK to use such structures in virsh-domain.c or is there
> >   any additional abstraction needed?
> 
> I don't much like the idea of exposing the QEMU job operation names
> in the public API. 
> 
> Perhaps we instead need to have the method which starts the job, return
> an integer "job id" that is then reported against the job, so apps can
> match them up.

The problem with "job id" is that only the process which started the job
would know what it means. Not to mention it would require a lot of API
changes.

I think we should just introduce a new virDomainJobSomething enum as

VIR_DOMAIN_JOB_SOMETHING_INCOMING_MIGRATION,
VIR_DOMAIN_JOB_SOMETHING_OUTGOING_MIGRATION,
VIR_DOMAIN_JOB_SOMETHING_SAVE,
VIR_DOMAIN_JOB_SOMETHING_RESTORE,
...

and report it in virDomainGetJobStat (definitely not in
_virDomainJobInfo as it would break ABI).

I'm not sure what the best name for "Something" would be. "Operation",
"Action", or something else?

Jirka

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

Re: [libvirt] [PATCH V4] Expose resource control capabilites on cache bank

[Peter Krempa]

On Tue, Apr 11, 2017 at 13:44:26 +0800, Eli Qiao wrote:
> This patch is based on Martin's cache branch.
> 
> * This patch amends the cache bank capability as follow:
> 
> 
>   
> 
>   
>   
> 
>   
> 
> 
> For CDP enabled on x86 arch, we will have:
> 
>   
> 
> 
>   
> ...
> 
> * Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
> case.
> 
> * Along with vircaps2xmltest case updated.
> 
> P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I
> keep it capital upper as I need to use a macro to convert from enum to
> string easily.

We dont need to do that. The attributes should be lower case. The code
can convert it to anything it needs.

> 
> Signed-off-by: Eli Qiao 
> ---

[...]

> diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> index 2080953..bfed4f8 100644
> --- a/docs/schemas/capability.rng
> +++ b/docs/schemas/capability.rng
> @@ -277,6 +277,26 @@
>
>  
>
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  BOTH
> +  CODE
> +  DATA

Why are the values all caps? We prefer lowercase attributes in the XML.

> +
> +  
> +  
> +
> +  
> +
> +  
>  
>
>  
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 416dd1a..c6641d1 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -52,6 +52,7 @@
>  #define VIR_FROM_THIS VIR_FROM_CAPABILITIES
>  
>  #define SYSFS_SYSTEM_PATH "/sys/devices/system/"
> +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/"
>  
>  VIR_LOG_INIT("conf.capabilities")
>  
> @@ -873,6 +874,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>  virCapsHostCacheBankPtr *caches)
>  {
>  size_t i = 0;
> +size_t j = 0;
> +int indent = virBufferGetIndent(buf, false);
> +virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
>  
>  if (!ncaches)
>  return 0;
> @@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
>   */
>  virBufferAsprintf(buf,
>" -  "size='%llu' unit='%s' cpus='%s'/>\n",
> +  "size='%llu' unit='%s' cpus='%s'",
>bank->id, bank->level,
>virCacheTypeToString(bank->type),
>bank->size >> (kilos * 10),
>kilos ? "KiB" : "B",
>cpus_str);
>  
> +/* Format control */
> +virBufferAdjustIndent(&controlBuf, indent + 4);

This looks wrong. You should increase/decrease the indent by some
number. The old value should not be used.

> +for (j = 0; j < bank->ncontrols; j++) {
> +bool min_kilos = !(bank->controls[j]->min % 1024);
> +virBufferAsprintf(&controlBuf,
> +  " +  "scope='%s' max_allocation='%u'/>\n",
> +  bank->controls[j]->min >> (min_kilos * 10),
> +  min_kilos ? "KiB" : "B",
> +  
> virResctrlCacheTypeToString(bank->controls[j]->scope),
> +  bank->controls[j]->max_allocation);
> +}
> +
> +/* Put it all together */

You don't need to point out obvious things.

> +if (virBufferUse(&controlBuf)) {

This logic looks weird and opaque. Can you decide by any other means
than the filling of the buffer?

> +virBufferAddLit(buf, ">\n");
> +virBufferAddBuffer(buf, &controlBuf);
> +virBufferAddLit(buf, "\n");
> +
> +} else {
> +virBufferAddLit(buf, "/>\n");
> +}
> +
> +
> +virBufferFreeAndReset(&controlBuf);
>  VIR_FREE(cpus_str);
>  }
>  
> @@ -1513,13 +1542,101 @@ virCapsHostCacheBankEquals(virCapsHostCacheBankPtr a,
>  void
>  virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr)
>  {
> +size_t i;
> +
>  if (!ptr)
>  return;
>  
>  virBitmapFree(ptr->cpus);
> +for (i = 0; i < ptr->ncontrols; i++)
> +VIR_FREE(ptr->controls[i]);
> +VIR_FREE(ptr->controls);
>  VIR_FREE(ptr);
>  }
>  
> +VIR_ENUM_IMPL(virResctrlCache, VIR_RESCTRL_CACHE_TYPE_LAST,
> +  "BOTH",
> +  "CODE",
> +  "DATA")
> +
> +/* test which TYPE of cache control supported
> + * -1: don't support
> + *  0: cat
> + *  1: cdp
> + */
> +static int
> +virCapabilitiesGetCacheControlType(virCapsHostCacheBankPtr bank)
> +{
> +int ret = -1;
> +char *path = NULL;
> +if (virAsprintf(&path,
> +SYSFS_RESCTRL_PATH "info/L%u",
> +bank->level) < 0)
> +return -1;
> +
> +if (

[libvirt] [PATCH v3 04/16] qemu: drop QEMU_MIGRATION_COMPLETED_UPDATE_STATS

[Nikolay Shirokovskiy]

This way we get stats only in one place. The former code waits for
complete/postcopy status basically and don't need to mess with stats.

The patch drops raising an error on stats updates failure. This
does not make much sense anyway.
---
 src/qemu/qemu_migration.c | 24 +++-
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b4fc46c..f766ca6 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1404,8 +1404,7 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
 static int
 qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
-qemuDomainAsyncJob asyncJob,
-bool updateJobStats)
+qemuDomainAsyncJob asyncJob)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 qemuDomainJobInfoPtr jobInfo = priv->job.current;
@@ -1434,12 +1433,6 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 return -1;
 
 case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
-/* Fetch statistics of a completed migration */
-if (events && updateJobStats &&
-qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0)
-return -1;
-break;
-
 case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
 case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
 break;
@@ -1451,10 +1444,10 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 enum qemuMigrationCompletedFlags {
 QEMU_MIGRATION_COMPLETED_ABORT_ON_ERROR = (1 << 0),
 QEMU_MIGRATION_COMPLETED_CHECK_STORAGE  = (1 << 1),
-QEMU_MIGRATION_COMPLETED_UPDATE_STATS   = (1 << 2),
-QEMU_MIGRATION_COMPLETED_POSTCOPY   = (1 << 3),
+QEMU_MIGRATION_COMPLETED_POSTCOPY   = (1 << 2),
 };
 
+
 /**
  * Returns 1 if migration completed successfully,
  * 0 if the domain is still being migrated,
@@ -1471,9 +1464,8 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
 qemuDomainObjPrivatePtr priv = vm->privateData;
 qemuDomainJobInfoPtr jobInfo = priv->job.current;
 int pauseReason;
-bool updateStats = !!(flags & QEMU_MIGRATION_COMPLETED_UPDATE_STATS);
 
-if (qemuMigrationCheckJobStatus(driver, vm, asyncJob, updateStats) < 0)
+if (qemuMigrationCheckJobStatus(driver, vm, asyncJob) < 0)
 goto error;
 
 if (flags & QEMU_MIGRATION_COMPLETED_CHECK_STORAGE &&
@@ -1501,9 +1493,6 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
 if (flags & QEMU_MIGRATION_COMPLETED_POSTCOPY &&
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
 VIR_DEBUG("Migration switched to post-copy");
-if (updateStats &&
-qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0)
-goto error;
 return 1;
 }
 
@@ -1542,8 +1531,6 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
 int rv;
 
-flags |= QEMU_MIGRATION_COMPLETED_UPDATE_STATS;
-
 jobInfo->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE;
 while ((rv = qemuMigrationCompleted(driver, vm, asyncJob,
 dconn, flags)) != 1) {
@@ -1565,6 +1552,9 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 }
 }
 
+if (events)
+ignore_value(qemuMigrationUpdateJobStatus(driver, vm, asyncJob));
+
 qemuDomainJobInfoUpdateDowntime(jobInfo);
 VIR_FREE(priv->job.completed);
 if (VIR_ALLOC(priv->job.completed) == 0)
-- 
1.8.3.1

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

[libvirt] [PATCH v3 06/16] qemu: refactor fetching migration stats

[Nikolay Shirokovskiy]

qemuMigrationFetchJobStatus is rather inconvinient. Some of its
callers don't need status to be updated, some don't need to update
elapsed time right away. So let's update status or elapsed time
in callers instead.

In qemuMigrationConfirmPhase we should fetch stats with copy
flag set as stats variable refers to domain object not the stack.

This patch drops updating job status on getting job stats by
client. This way we will not provide status 'completed' while
it is not yet updated by migration routine.
---
 src/qemu/qemu_driver.c| 16 ---
 src/qemu/qemu_migration.c | 52 +++
 src/qemu/qemu_migration.h |  9 
 3 files changed, 35 insertions(+), 42 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 48029be..f23503a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12789,15 +12789,17 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 
 if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
 jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
-if (fetch)
-ret = qemuMigrationFetchJobStatus(driver, vm, QEMU_ASYNC_JOB_NONE,
-  jobInfo);
-else
-ret = qemuDomainJobInfoUpdateTime(jobInfo);
-} else {
-ret = 0;
+if (fetch &&
+qemuMigrationFetchMigrationStats(driver, vm, QEMU_ASYNC_JOB_NONE,
+ &jobInfo->stats, false) < 0)
+goto cleanup;
+
+if (qemuDomainJobInfoUpdateTime(jobInfo) < 0)
+goto cleanup;
 }
 
+ret = 0;
+
  cleanup:
 if (fetch)
 qemuDomainObjEndJob(driver, vm);
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 022fad0..1760908 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1344,24 +1344,28 @@ qemuMigrationUpdateJobType(qemuDomainJobInfoPtr jobInfo)
 
 
 int
-qemuMigrationFetchJobStatus(virQEMUDriverPtr driver,
-virDomainObjPtr vm,
-qemuDomainAsyncJob asyncJob,
-qemuDomainJobInfoPtr jobInfo)
+qemuMigrationFetchMigrationStats(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuDomainAsyncJob asyncJob,
+ qemuMonitorMigrationStatsPtr stats,
+ bool copy)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
+qemuMonitorMigrationStats statsCopy;
 int rv;
 
 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
 return -1;
 
-rv = qemuMonitorGetMigrationStats(priv->mon, &jobInfo->stats);
+rv = qemuMonitorGetMigrationStats(priv->mon, copy ? &statsCopy : stats);
 
 if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0)
 return -1;
 
-qemuMigrationUpdateJobType(jobInfo);
-return qemuDomainJobInfoUpdateTime(jobInfo);
+if (copy)
+*stats = statsCopy;
+
+return 0;
 }
 
 
@@ -1384,23 +1388,6 @@ qemuMigrationJobName(virDomainObjPtr vm)
 
 
 static int
-qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- qemuDomainAsyncJob asyncJob)
-{
-qemuDomainObjPrivatePtr priv = vm->privateData;
-qemuDomainJobInfoPtr jobInfo = priv->job.current;
-qemuDomainJobInfo newInfo = *jobInfo;
-
-if (qemuMigrationFetchJobStatus(driver, vm, asyncJob, &newInfo) < 0)
-return -1;
-
-*jobInfo = newInfo;
-return 0;
-}
-
-
-static int
 qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 qemuDomainAsyncJob asyncJob)
@@ -1410,11 +1397,12 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
 
 bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
 
-if (events)
-qemuMigrationUpdateJobType(jobInfo);
-else if (qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0)
+if (!events && qemuMigrationFetchMigrationStats(driver, vm, asyncJob,
+&jobInfo->stats, true) < 0)
 return -1;
 
+qemuMigrationUpdateJobType(jobInfo);
+
 switch (jobInfo->status) {
 case QEMU_DOMAIN_JOB_STATUS_NONE:
 virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"),
@@ -1552,8 +1540,10 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
 }
 
 if (events)
-ignore_value(qemuMigrationUpdateJobStatus(driver, vm, asyncJob));
+ignore_value(qemuMigrationFetchMigrationStats(driver, vm, asyncJob,
+  &jobInfo->stats, true));
 
+qemuDomainJobInfoUpdateTime(jobInfo);
 qemuDomainJobInfoUpdateDowntime(jobInfo);
 VIR_FREE(priv->job.completed);
 if (VIR_ALLOC(priv->job.completed) == 0)
@@ -3140,9 +3130,9 @@ qemuMigration

[libvirt] [PATCH v3 05/16] qemu: drop excessive zero-out in qemuMigrationFetchJobStatus

[Nikolay Shirokovskiy]

qemuMonitorGetMigrationStats will do it for us anyway.
---
 src/qemu/qemu_migration.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f766ca6..022fad0 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1355,7 +1355,6 @@ qemuMigrationFetchJobStatus(virQEMUDriverPtr driver,
 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
 return -1;
 
-memset(&jobInfo->stats, 0, sizeof(jobInfo->stats));
 rv = qemuMonitorGetMigrationStats(priv->mon, &jobInfo->stats);
 
 if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0)
-- 
1.8.3.1

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

[libvirt] [PATCH v3 01/16] qemu: drop code for VIR_DOMAIN_JOB_BOUNDED and timeRemaining

[Nikolay Shirokovskiy]

qemu driver does not have VIR_DOMAIN_JOB_BOUNDED jobs and
timeRemaining is always 0.
---
 src/qemu/qemu_domain.c   | 7 ---
 src/qemu/qemu_domain.h   | 1 -
 src/qemu/qemu_migration_cookie.c | 5 -
 3 files changed, 13 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 39bc8c7..54d9425 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -408,7 +408,6 @@ qemuDomainJobInfoToInfo(qemuDomainJobInfoPtr jobInfo,
 {
 info->type = jobInfo->type;
 info->timeElapsed = jobInfo->timeElapsed;
-info->timeRemaining = jobInfo->timeRemaining;
 
 info->memTotal = jobInfo->stats.ram_total;
 info->memRemaining = jobInfo->stats.ram_remaining;
@@ -448,12 +447,6 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
 jobInfo->timeElapsed - jobInfo->timeDelta) < 0)
 goto error;
 
-if (jobInfo->type == VIR_DOMAIN_JOB_BOUNDED &&
-virTypedParamsAddULLong(&par, &npar, &maxpar,
-VIR_DOMAIN_JOB_TIME_REMAINING,
-jobInfo->timeRemaining) < 0)
-goto error;
-
 if (stats->downtime_set &&
 virTypedParamsAddULLong(&par, &npar, &maxpar,
 VIR_DOMAIN_JOB_DOWNTIME,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index caac5d5..caf1ebe 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -111,7 +111,6 @@ struct _qemuDomainJobInfo {
 info from the source (migrations only). */
 /* Computed values */
 unsigned long long timeElapsed;
-unsigned long long timeRemaining;
 long long timeDelta; /* delta = received - sent, i.e., the difference
 between the source and the destination time plus
 the time between the end of Perform phase on the
diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c
index bd12f11..ae56569 100644
--- a/src/qemu/qemu_migration_cookie.c
+++ b/src/qemu/qemu_migration_cookie.c
@@ -594,9 +594,6 @@ qemuMigrationCookieStatisticsXMLFormat(virBufferPtr buf,
 virBufferAsprintf(buf, "<%1$s>%2$llu\n",
   VIR_DOMAIN_JOB_TIME_ELAPSED,
   jobInfo->timeElapsed);
-virBufferAsprintf(buf, "<%1$s>%2$llu\n",
-  VIR_DOMAIN_JOB_TIME_REMAINING,
-  jobInfo->timeRemaining);
 if (stats->downtime_set)
 virBufferAsprintf(buf, "<%1$s>%2$llu\n",
   VIR_DOMAIN_JOB_DOWNTIME,
@@ -966,8 +963,6 @@ qemuMigrationCookieStatisticsXMLParse(xmlXPathContextPtr 
ctxt)
 
 virXPathULongLong("string(./" VIR_DOMAIN_JOB_TIME_ELAPSED "[1])",
   ctxt, &jobInfo->timeElapsed);
-virXPathULongLong("string(./" VIR_DOMAIN_JOB_TIME_REMAINING "[1])",
-  ctxt, &jobInfo->timeRemaining);
 
 if (virXPathULongLong("string(./" VIR_DOMAIN_JOB_DOWNTIME "[1])",
   ctxt, &stats->downtime) == 0)
-- 
1.8.3.1

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

[libvirt] [PATCH v3 08/16] qemu: fail querying destination migration statistics always

[Nikolay Shirokovskiy]

Querying destination migration statistics may result in getting
a failure or getting a elapsed time value depending on stats.status
value which is odd. Instead let's always fail. Clients should
be ready to handle this as currently getting failure period
can be considerable.
---
 src/qemu/qemu_driver.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index da4d8e9..e65448f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12757,20 +12757,19 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
 return 0;
 }
 
+if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_IN) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("migration statistics are available only on "
+ "the source host"));
+return -1;
+}
+
 /* Do not ask QEMU if migration is not even running yet  */
 if (!priv->job.current || !priv->job.current->stats.status)
 fetch = false;
 
-if (fetch) {
-if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_IN) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-   _("migration statistics are available only on "
- "the source host"));
-return -1;
-}
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
-return -1;
-}
+if (fetch && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+return -1;
 
 if (!virDomainObjIsActive(vm)) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-- 
1.8.3.1

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

[libvirt] [PATCH v3 15/16] qemu: migrate: copy disks stats to completed job

[Nikolay Shirokovskiy]

---
 src/qemu/qemu_migration.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 76c8554..1703274 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3831,6 +3831,13 @@ qemuMigrationRun(virQEMUDriverPtr driver,
QEMU_ASYNC_JOB_MIGRATION_OUT,
dconn) < 0)
 ret = -1;
+
+if (ret == 0 && priv->job.completed) {
+priv->job.completed->stats.disk_transferred =
+priv->job.current->stats.disk_transferred;
+priv->job.completed->stats.disk_total =
+priv->job.current->stats.disk_total;
+}
 }
 
 VIR_FREE(tlsAlias);
-- 
1.8.3.1

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