[libvirt] [RFC] duplicate suspend/resume lifecycle events with QEMU
Hello,
I'm using the libvirt event mechanism and noticed, that several events
are reported twice:
> $ python examples/event-test.py qemu:///session
> Using uri:qemu:///session
> myDomainEventCallback1 EVENT: Domain installer(2) Resumed Unpaused
> myDomainEventCallback2 EVENT: Domain installer(2) Resumed Unpaused
> myDomainEventCallback1 EVENT: Domain installer(2) Resumed Unpaused
> myDomainEventCallback2 EVENT: Domain installer(2) Resumed Unpaused
> myDomainEventCallback1 EVENT: Domain installer(2) Suspended Paused
> myDomainEventCallback2 EVENT: Domain installer(2) Suspended Paused
> myDomainEventCallback1 EVENT: Domain installer(2) Suspended Paused
> myDomainEventCallback2 EVENT: Domain installer(2) Suspended Paused
> myDomainEventCallback1 EVENT: Domain installer(-1) Stopped Destroyed
> myDomainEventCallback2 EVENT: Domain installer(-1) Stopped Destroyed
(the Python example program registers 2 handlers, so each event is
printed twice, but each handler gets the suspend/resume event twice itself.)
Interestingly enough it only "suspend" and "resume", but not "created"
or "destroyed".
But it isn't Python specific, virsh shows the same behaviour:
> $ virsh event --loop --event lifecycle
> event 'lifecycle' for domain installer: Started Booted
> event 'lifecycle' for domain installer: Suspended Paused
> event 'lifecycle' for domain installer: Resumed Unpaused
> event 'lifecycle' for domain installer: Resumed Unpaused
> event 'lifecycle' for domain installer: Suspended Paused
> event 'lifecycle' for domain installer: Suspended Paused
> event 'lifecycle' for domain installer: Stopped Destroyed
After shutting down libvirtd I used `socat stdio unix-connect:...` to
connect the the qemu UNIX socket and used
{"execute": "stop"}
and
{"execute": "cont"}
to verify, that QEMU only send one event. So to me it looks like libvirt
is duplicating the event.
Enabling DEBUG for »log_filters="1:qemu_monitor_json"« shows noting
wrong with QEMU:
> qemuMonitorJSONCommandWithFd:286 : Send command
> '{"execute":"cont","id":"libvirt-9"}' for write with FD -1
> qemuMonitorSend:972 : QEMU_MONITOR_SEND_MSG: mon=0x7f194910
> msg={"execute":"cont","id":"libvirt-9"}
> fd=-1
> qemuMonitorIOWrite:503 : QEMU_MONITOR_IO_WRITE: mon=0x7f194910
> buf={"execute":"cont","id":"libvirt-9"}
> len=37 ret=37 errno=11
> qemuMonitorIOProcess:399 : QEMU_MONITOR_IO_PROCESS: mon=0x7f194910
> buf={"timestamp": {"seconds": 1497326279, "microseconds": 688619}, "event":
> "RESUME"}
> {"return": {}, "id": "libvirt-9"}
> len=118
> qemuMonitorJSONIOProcessLine:179 : Line [{"timestamp": {"seconds":
> 1497326279, "microseconds": 688619}, "event": "RESUME"}]
> qemuMonitorJSONIOProcessLine:194 : QEMU_MONITOR_RECV_EVENT:
> mon=0x7f194910 event={"timestamp": {"seconds": 1497326279,
> "microseconds": 688619}, "event": "RESUME"}
> qemuMonitorJSONIOProcessEvent:138 : mon=0x7f194910 obj=0x5636fa1f1f00
> qemuMonitorEmitEvent:1186 : mon=0x7f194910 event=RESUME
> qemuMonitorJSONIOProcessEvent:165 : handle RESUME handler=0x7f19535903d0
> data=(nil)
> qemuMonitorEmitResume:1237 : mon=0x7f194910
Looking at src/qemu/qemu_monitor_json.c:qemuMonitorJSONIOProcessEvent:
> 141 qemuMonitorJSONIOProcessEvent(qemuMonitorPtr mon,
> 142 virJSONValuePtr obj)
> 143 {
...
> 170 qemuMonitorEmitEvent(mon, type, seconds, micros, details);
This seems to send the first event
> 171 VIR_FREE(details);
> 172
> 173 handler = bsearch(type, eventHandlers,
> ARRAY_CARDINALITY(eventHandlers),
> 174 sizeof(eventHandlers[0]), qemuMonitorEventCompare);
> 175 if (handler) {
> 176 VIR_DEBUG("handle %s handler=%p data=%p", type,
> 177 handler->handler, data);
> 178 (handler->handler)(mon, data);
and this the second instance.
> 179 }
> 180 return 0;
> 181 }
Looking deeper into the GIT source code I see this:
> 98 static qemuEventHandler eventHandlers[] = {
...
> 113 { "RESUME", qemuMonitorJSONHandleResume, },
> 544 static void qemuMonitorJSONHandleResume(qemuMonitorPtr mon,
> virJSONValuePtr data ATTRIBUTE_UNUSED)
> 546 qemuMonitorEmitResume(mon);
...
> 1267 qemuMonitorEmitEvent(qemuMonitorPtr mon, const char *event,
>
>
> 1268 long long seconds, unsigned int micros,
> 1269 const char *details)
...
> 1274 QEMU_MONITOR_CALLBACK(mon, ret, domainEvent, mon->vm, event, seconds,
> 1275 micros, details);
...
> 1325 qemuMonitorEmitResume(qemuMonitorPtr mon)
> 1330 QEMU_MONITOR_CALLBACK(mon, ret, domainResume, mon->vm);
but this doesn't yet completely explain, why only some events are
reported twice.
Is there some way to get rid of the duplication (in Python) or at least
to distinguish th
Re: [libvirt] [PATCH 09/26] conf: Move index number checking to drivers
On 06/12/2017 10:08 PM, Laine Stump wrote:
> On 06/02/2017 12:07 PM, Andrea Bolognani wrote:
>> pSeries guests will soon be allowed to have multiple
>> PHBs (pci-root controllers), which of course means that
>> all but one of them will have a non-zero index; hence,
>> we'll need to relax the current check.
>>
>> However, right now the check is performed in the conf
>> module, which is generic rather than tied to the QEMU
>> driver, and where we don't have information such as the
>> guest machine type available.
>>
>> To make this change of behavior possible down the line,
>> we need to move the check from the XML parser to the
>> driver. Unfortunately, this means duplicating code :(\
>
>
> Maybe instead we should just eliminate this check (since the pSeries
> case shows that it's an invalid assumption). And since the index is
> really just something used internally by libvirt, we really don't even
> need to verify that one of the pci controllers has index=0. All we
> *really* care about is that there is at least one "root" PCI bus, and
> that no device includes a bus# in its PCI address that isn't represented
> by the index in one of the PCI controllers.
>
> (But for the purposes of this patch, I think we can just remove the
> validation in conf and not worry about adding it elsewhere).\\
After looking at the next patch, I may have changed my mind. If we
remove the validation here, then we would still have to add in
validation when the commandline is generated. But I suppose it's better
to give an error at domain definition time rather than domain runtime,
so this makes sense. I don't think all of those drivers actually use PCI
controllers though, do they? (Look for which ones actually call the
functions to assign PCI addresses).
>
>> ---
>> src/bhyve/bhyve_domain.c | 15 +++
>> src/conf/domain_conf.c | 6 --
>> src/libxl/libxl_domain.c | 14 ++
>> src/lxc/lxc_domain.c | 14 ++
>> src/openvz/openvz_driver.c | 14 ++
>> src/qemu/qemu_domain.c | 9 +
>> src/uml/uml_driver.c | 14 ++
>> src/vz/vz_driver.c | 14 ++
>> src/xen/xen_driver.c | 14 ++
>> 9 files changed, 108 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
>> index 76b4fac..05c1508 100644
>> --- a/src/bhyve/bhyve_domain.c
>> +++ b/src/bhyve/bhyve_domain.c
>> @@ -122,6 +122,21 @@ bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>> bhyveDomainDiskDefAssignAddress(driver, disk, def) < 0)
>> return -1;
>> }
>> +
>> +if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
>> +virDomainControllerDefPtr cont = dev->data.controller;
>> +
>> +if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +(cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
>> + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
>> +cont->idx != 0) {
>> +virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("pci-root and pcie-root controllers "
>> + "should have index 0"));
>> +return -1;
>> +}
>> +}
>> +
>> return 0;
>> }
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index c7e20b8..d5dff8e 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -9059,12 +9059,6 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>> "have an address"));
>> goto error;
>> }
>> -if (def->idx > 0) {
>> -virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("pci-root and pcie-root controllers "
>> - "should have index 0"));
>> -goto error;
>> -}
>> if ((rc = virDomainParseScaledValue("./pcihole64", NULL,
>> ctxt, &bytes, 1024,
>> 1024ULL * ULONG_MAX,
>> false)) < 0)
>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>> index 256cf1d..59ef2fb 100644
>> --- a/src/libxl/libxl_domain.c
>> +++ b/src/libxl/libxl_domain.c
>> @@ -379,6 +379,20 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>> virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
>> }
>>
>> +if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
>> +virDomainControllerDefPtr cont = dev->data.controller;
>> +
>> +if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +(cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
>> + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
>> +cont->idx != 0) {
>> +virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("pci-root and pcie-root controllers "
>> +
Re: [libvirt] [PATCH 09/26] conf: Move index number checking to drivers
On 06/02/2017 12:07 PM, Andrea Bolognani wrote:
> pSeries guests will soon be allowed to have multiple
> PHBs (pci-root controllers), which of course means that
> all but one of them will have a non-zero index; hence,
> we'll need to relax the current check.
>
> However, right now the check is performed in the conf
> module, which is generic rather than tied to the QEMU
> driver, and where we don't have information such as the
> guest machine type available.
>
> To make this change of behavior possible down the line,
> we need to move the check from the XML parser to the
> driver. Unfortunately, this means duplicating code :(\
Maybe instead we should just eliminate this check (since the pSeries
case shows that it's an invalid assumption). And since the index is
really just something used internally by libvirt, we really don't even
need to verify that one of the pci controllers has index=0. All we
*really* care about is that there is at least one "root" PCI bus, and
that no device includes a bus# in its PCI address that isn't represented
by the index in one of the PCI controllers.
(But for the purposes of this patch, I think we can just remove the
validation in conf and not worry about adding it elsewhere).
> ---
> src/bhyve/bhyve_domain.c | 15 +++
> src/conf/domain_conf.c | 6 --
> src/libxl/libxl_domain.c | 14 ++
> src/lxc/lxc_domain.c | 14 ++
> src/openvz/openvz_driver.c | 14 ++
> src/qemu/qemu_domain.c | 9 +
> src/uml/uml_driver.c | 14 ++
> src/vz/vz_driver.c | 14 ++
> src/xen/xen_driver.c | 14 ++
> 9 files changed, 108 insertions(+), 6 deletions(-)
>
> diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
> index 76b4fac..05c1508 100644
> --- a/src/bhyve/bhyve_domain.c
> +++ b/src/bhyve/bhyve_domain.c
> @@ -122,6 +122,21 @@ bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> bhyveDomainDiskDefAssignAddress(driver, disk, def) < 0)
> return -1;
> }
> +
> +if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
> +virDomainControllerDefPtr cont = dev->data.controller;
> +
> +if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> +(cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
> + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
> +cont->idx != 0) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("pci-root and pcie-root controllers "
> + "should have index 0"));
> +return -1;
> +}
> +}
> +
> return 0;
> }
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c7e20b8..d5dff8e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9059,12 +9059,6 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> "have an address"));
> goto error;
> }
> -if (def->idx > 0) {
> -virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("pci-root and pcie-root controllers "
> - "should have index 0"));
> -goto error;
> -}
> if ((rc = virDomainParseScaledValue("./pcihole64", NULL,
> ctxt, &bytes, 1024,
> 1024ULL * ULONG_MAX, false))
> < 0)
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 256cf1d..59ef2fb 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -379,6 +379,20 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
> }
>
> +if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
> +virDomainControllerDefPtr cont = dev->data.controller;
> +
> +if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> +(cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
> + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) &&
> +cont->idx != 0) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("pci-root and pcie-root controllers "
> + "should have index 0"));
> +return -1;
> +}
> +}
> +
> return 0;
> }
>
> diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
> index 3a7404f..a50f6fe 100644
> --- a/src/lxc/lxc_domain.c
> +++ b/src/lxc/lxc_domain.c
> @@ -389,6 +389,20 @@ virLXCDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE)
> dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC;
>
> +if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
> +virDomainControllerDefPtr cont
Re: [libvirt] [PATCH 08/26] qemu: Tweak index number checking
On 06/02/2017 12:07 PM, Andrea Bolognani wrote:
> Moving the check and rewriting it this way doesn't alter
> the current behavior, but will allow us to special-case
> pci-root down the line.
But this function should never be called for a controller with
model='pci-root'?
> ---
> src/qemu/qemu_command.c | 26 --
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a0403bf..082ffa9 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2828,6 +2828,26 @@ qemuBuildControllerDevStr(const virDomainDef
> *domainDef,
> case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> switch ((virDomainControllerModelPCI) def->model) {
> case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> +if (def->idx == 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("index for pci controllers of model '%s'
> must be > 0"),
> +
> virDomainControllerModelPCITypeToString(def->model));
> +goto error;
> +}
> +break;
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> +break;
> +}
> +switch ((virDomainControllerModelPCI) def->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> if (def->opts.pciopts.modelName
> == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE ||
> def->opts.pciopts.chassisNr == -1) {
> @@ -3086,12 +3106,6 @@ qemuBuildControllerDevStr(const virDomainDef
> *domainDef,
> _("wrong function called"));
> goto error;
> }
> -if (def->idx == 0) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("index for pci controllers of model '%s' must
> be > 0"),
> -
> virDomainControllerModelPCITypeToString(def->model));
> -goto error;
> -}
> break;
>
> case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] Use ATTRIBUTE_FALLTHROUGH
On 06/07/2017 04:46 AM, Marc Hartmayer wrote: > Use ATTRIBUTE_FALLTHROUGH, introduced by commit > 5d84f5961b8e28e802f600bb2d2c6903e219092e, instead of comments to > indicate that the fall through is an intentional behavior. > > Signed-off-by: Marc Hartmayer > Reviewed-by: Boris Fiuczynski > Reviewed-by: Bjoern Walk > --- > src/conf/domain_conf.c | 2 +- > src/conf/nwfilter_conf.c | 14 +++--- > src/cpu/cpu_ppc64.c | 2 +- > src/libvirt-domain.c | 2 +- > src/libxl/libxl_conf.c | 2 +- > src/network/leaseshelper.c | 4 ++-- > src/qemu/qemu_command.c | 2 +- > src/qemu/qemu_domain.c | 4 ++-- > src/qemu/qemu_driver.c | 4 ++-- > src/qemu/qemu_hotplug.c | 4 ++-- > src/qemu/qemu_migration.c| 2 +- > src/remote/remote_driver.c | 2 +- > src/rpc/virnetservermdns.c | 2 +- > src/storage/storage_driver.c | 2 +- > src/util/virconf.c | 2 +- > src/util/virhashcode.c | 6 +++--- > src/util/virnetdevbridge.c | 1 + > src/util/virutil.c | 10 +- > tools/virsh-domain.c | 4 ++-- > tools/virsh.c| 2 +- > tools/virt-admin.c | 2 +- > 21 files changed, 38 insertions(+), 37 deletions(-) > Reviewed-by: John Ferlan John I've pushed the series... For some reason the brain cells never remember who has push access and who doesn't nor how to check ;-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemu: add a comment for mon->watch
On 06/07/2017 04:46 AM, Marc Hartmayer wrote: > Add a comment for mon->watch to make clear what's the purpose of this > value. > > Signed-off-by: Marc Hartmayer > Reviewed-by: Bjoern Walk > --- > src/qemu/qemu_monitor.c | 6 ++ > 1 file changed, 6 insertions(+) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] rpc: first allocate the memory and then set the count
On 06/07/2017 04:46 AM, Marc Hartmayer wrote: > Signed-off-by: Marc Hartmayer > Reviewed-by: Boris Fiuczynski > Reviewed-by: Bjoern Walk > --- > src/rpc/virnetclientprogram.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 6/6] virStream*All: Report error if a callback fails
On 06/05/2017 04:23 AM, Michal Privoznik wrote:
> All of these four functions (virStreamRecvAll, virStreamSendAll,
> virStreamSparseRecvAll, virStreamSparseSendAll) take one or more
> callback that handle various aspects of streams. However, if any
s/callback/callback functions/
> of them fails no error is reported therefore caller does not know
> what went wrong.
>
> At the same time, we silently presumed callbacks to set errno on
> failure. With this change we should document it explicitly as the
s/should//
> error is not properly reported.
>
> Signed-off-by: Michal Privoznik
> ---
> include/libvirt/libvirt-stream.h | 17 +-
> src/libvirt-stream.c | 50
> +---
> 2 files changed, 58 insertions(+), 9 deletions(-)
>
> diff --git a/include/libvirt/libvirt-stream.h
> b/include/libvirt/libvirt-stream.h
> index d18d43140..86f96b158 100644
> --- a/include/libvirt/libvirt-stream.h
> +++ b/include/libvirt/libvirt-stream.h
> @@ -82,7 +82,10 @@ int virStreamRecvHole(virStreamPtr,
> * of bytes. The callback will continue to be
> * invoked until it indicates the end of the source
> * has been reached by returning 0. A return value
> - * of -1 at any time will abort the send operation
> + * of -1 at any time will abort the send operation.
> + *
> + * Please note that for more accurate error reporting the
> + * callback should set appropriate errno on failure.
> *
> * Returns the number of bytes filled, 0 upon end
> * of file, or -1 upon error
> @@ -119,6 +122,9 @@ int virStreamSendAll(virStreamPtr st,
> * This function should not adjust the current position within
> * the file.
> *
> + * Please note that for more accurate error reporting the
> + * callback should set appropriate errno on failure.
> + *
> * Returns 0 on success,
> *-1 upon error
> */
> @@ -142,6 +148,9 @@ typedef int (*virStreamSourceHoleFunc)(virStreamPtr st,
> * processing the hole in the stream source and then return.
> * A return value of -1 at any time will abort the send operation.
> *
> + * Please note that for more accurate error reporting the
> + * callback should set appropriate errno on failure.
> + *
> * Returns 0 on success,
> *-1 upon error.
> */
> @@ -176,6 +185,9 @@ int virStreamSparseSendAll(virStreamPtr st,
> * has been reached. A return value of -1 at any time
> * will abort the receive operation
> *
> + * Please note that for more accurate error reporting the
> + * callback should set appropriate errno on failure.
> + *
> * Returns the number of bytes consumed or -1 upon
> * error
> */
> @@ -203,6 +215,9 @@ int virStreamRecvAll(virStreamPtr st,
> * hole in the stream target and then return. A return value of
> * -1 at any time will abort the receive operation.
> *
> + * Please note that for more accurate error reporting the
> + * callback should set appropriate errno on failure.
> + *
> * Returns 0 on success,
> *-1 upon error
> */
> diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
> index 1594ed212..cf1b2293a 100644
> --- a/src/libvirt-stream.c
> +++ b/src/libvirt-stream.c
> @@ -567,7 +567,7 @@ virStreamInData(virStreamPtr stream,
> *
> * Returns -1 upon any error, with virStreamAbort() already
> * having been called, so the caller need only call
> - * virStreamFree()
> + * virStreamFree().
Noted, spurious, IDC ;-)
> */
> int
> virStreamSendAll(virStreamPtr stream,
> @@ -595,9 +595,15 @@ virStreamSendAll(virStreamPtr stream,
>
> for (;;) {
> int got, offset = 0;
> +
> +errno = 0;
> got = (handler)(stream, bytes, want, opaque);
> -if (got < 0)
> +if (got < 0) {
> +if (errno == 0)
> +errno = EIO;
> +virReportSystemError(errno, "%s", _("send handler failed"));
Since errno and vir*LastError are not necessarily linked - do we really
want to write an error message in the event that a different message was
already in the pipeline?
That is should all of the new virReportSystemError calls be prefixed by:
if (!virGetLastError())
or perhaps use virGetLastErrorMessage ?
John
> goto cleanup;
> +}
> if (got == 0)
> break;
> while (offset < got) {
> @@ -732,17 +738,24 @@ int virStreamSparseSendAll(virStreamPtr stream,
> size_t want = bufLen;
> const unsigned int skipFlags = 0;
>
> +errno = 0;
> if (!dataLen) {
> -if (holeHandler(stream, &inData, §ionLen, opaque) < 0)
> +if (holeHandler(stream, &inData, §ionLen, opaque) < 0) {
> +if (errno == 0)
> +errno = EIO;
> +virReportSystemError(errno, "%s", _("send holeHandler
> failed"));
> goto cleanup;
> +}
>
> if (!inData && sectionLen) {
> if (virStreamSendHole(stream, sectionLen, skipFlags) < 0)
>
Re: [libvirt] [PATCH v3 5/6] virFileInData: preserve errno in cleanup path
On 06/05/2017 04:23 AM, Michal Privoznik wrote:
> Callers might be interested in the original value of errno. Let's
> not overwrite it with lseek() done in cleanup path.
>
> Signed-off-by: Michal Privoznik
> ---
> src/util/virfile.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
One could argue that if the original lseek fails, rather than the goto
cleanup, the return -1 could be done. That way, the "if" condition in
cleanup: isn't necessary...
Additionally, if the second if the second lseek fails to reposition back
to where we started and we don't tell the user that, then we're off in
never never land possibly, right?
Of course IIRC the argument about this was that lseek is highly unlikely
to fail in that second scenario.
Up to this point we don't say/guarantee anything about the errno return
value generally, but I see the subsequent patch adds some verbiage -
whether it can be accurately held over any libvirt call perhaps remains
to be seen.
Still the premise of this is, don't mess with an errno that was set to
something with a second one that could change the original intention.
Before the ACK/R-B applies, is there any odd special casing that needs
to be done for that ENXIO case? That is setting errno = 0 if we decide
to not goto cleanup as I don't think in that case we're considering an
errno value to be problematic, instead it'd be expected.
As long as the errno != ENXIO case is handled... Since we learned
nothing and are moving on happily.
Reviewed-by: John Ferlan
John
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index d444b32f8..2be64f1db 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.cf
> @@ -3900,8 +3900,11 @@ virFileInData(int fd,
> ret = 0;
> cleanup:
> /* At any rate, reposition back to where we started. */
> -if (cur != (off_t) -1)
> +if (cur != (off_t) -1) {
> +int save_errno = errno;
> ignore_value(lseek(fd, cur, SEEK_SET));
> +errno = save_errno;
> +}
> return ret;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 4/6] virStream*All: Preserve reported error
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
> If one these four functions fail (virStreamRecvAll,
> virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll)
> the stream is aborted by calling virStreamAbort(). This is,
> however, an public API - therefore the first thing it does is
s/This is, however, an/This is a/
s/ - therefore/; therefore,/
> error reset. At that point any error that caused us to abort
> stream in the first place is gone.
>
> Signed-off-by: Michal Privoznik
> ---
> src/libvirt-stream.c | 20
> 1 file changed, 20 insertions(+)
>
Almost makes me wonder why we don't have a local/static helper that will
do the save, abort, restore rather than the repetition.
Reviewed-by: John Ferlan
John
> diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
> index bff0a0571..1594ed212 100644
> --- a/src/libvirt-stream.c
> +++ b/src/libvirt-stream.c
> @@ -614,7 +614,12 @@ virStreamSendAll(virStreamPtr stream,
> VIR_FREE(bytes);
>
> if (ret != 0) {
> +virErrorPtr orig_err = virSaveLastError();
> virStreamAbort(stream);
> +if (orig_err) {
> +virSetError(orig_err);
> +virFreeError(orig_err);
> +}
> virDispatchError(stream->conn);
> }
>
> @@ -769,7 +774,12 @@ int virStreamSparseSendAll(virStreamPtr stream,
> VIR_FREE(bytes);
>
> if (ret != 0) {
> +virErrorPtr orig_err = virSaveLastError();
> virStreamAbort(stream);
> +if (orig_err) {
> +virSetError(orig_err);
> +virFreeError(orig_err);
> +}
> virDispatchError(stream->conn);
> }
>
> @@ -863,7 +873,12 @@ virStreamRecvAll(virStreamPtr stream,
> VIR_FREE(bytes);
>
> if (ret != 0) {
> +virErrorPtr orig_err = virSaveLastError();
> virStreamAbort(stream);
> +if (orig_err) {
> +virSetError(orig_err);
> +virFreeError(orig_err);
> +}
> virDispatchError(stream->conn);
> }
>
> @@ -983,7 +998,12 @@ virStreamSparseRecvAll(virStreamPtr stream,
> VIR_FREE(bytes);
>
> if (ret != 0) {
> +virErrorPtr orig_err = virSaveLastError();
> virStreamAbort(stream);
> +if (orig_err) {
> +virSetError(orig_err);
> +virFreeError(orig_err);
> +}
> virDispatchError(stream->conn);
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 3/6] virStream*All: Call virStreamAbort() more frequently
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
> Our documentation to all four virStreamRecvAll, virStreamSendAll,
s/all four/the/
> virStreamSparseRecvAll, virStreamSparseSendAll says that if these
s/RecvAll, virStream/RecvAll, and virStream/
s/says that if these functions fail,/functions indicates that on failure/
> functions fail, virStreamAbort() is called. But that is not
> necessarily true. For instance all of these functions allocate a
> buffer to work with. If the allocation fails, no virStreamAbort()
> is called despite -1 being returned. It's the same story with
> argument sanity checks and a lot of other checks.
>
> Signed-off-by: Michal Privoznik
> ---
> src/libvirt-stream.c | 49 -
> 1 file changed, 20 insertions(+), 29 deletions(-)
>
This patch particularly scares me because there are so many more paths
that could now call virStreamAbort. Yes, I know that's the point, but
nonetheless it's large leap of faith to only calling Abort as a result
of some Send/Recv callback failure to calling Abort for the ancillary
stuff surrounding or supporting those calls as well.
Would there be a chance of multiple callers to virStreamAbort?
It'd be nice if there were any other opinions on this. I'd lean towards
saying looks good, but I'm also interested if there's any other opposing
opinions or concerns.
John
> diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
> index d7a8f5816..bff0a0571 100644
> --- a/src/libvirt-stream.c
> +++ b/src/libvirt-stream.c
> @@ -596,10 +596,8 @@ virStreamSendAll(virStreamPtr stream,
> for (;;) {
> int got, offset = 0;
> got = (handler)(stream, bytes, want, opaque);
> -if (got < 0) {
> -virStreamAbort(stream);
> +if (got < 0)
> goto cleanup;
> -}
> if (got == 0)
> break;
> while (offset < got) {
> @@ -615,8 +613,10 @@ virStreamSendAll(virStreamPtr stream,
> cleanup:
> VIR_FREE(bytes);
>
> -if (ret != 0)
> +if (ret != 0) {
> +virStreamAbort(stream);
> virDispatchError(stream->conn);
> +}
>
> return ret;
> }
> @@ -728,21 +728,16 @@ int virStreamSparseSendAll(virStreamPtr stream,
> const unsigned int skipFlags = 0;
>
> if (!dataLen) {
> -if (holeHandler(stream, &inData, §ionLen, opaque) < 0) {
> -virStreamAbort(stream);
> +if (holeHandler(stream, &inData, §ionLen, opaque) < 0)
> goto cleanup;
> -}
>
> if (!inData && sectionLen) {
> -if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) {
> -virStreamAbort(stream);
> +if (virStreamSendHole(stream, sectionLen, skipFlags) < 0)
> goto cleanup;
> -}
>
> if (skipHandler(stream, sectionLen, opaque) < 0) {
> virReportSystemError(errno, "%s",
> _("unable to skip hole"));
> -virStreamAbort(stream);
> goto cleanup;
> }
> continue;
> @@ -755,10 +750,8 @@ int virStreamSparseSendAll(virStreamPtr stream,
> want = dataLen;
>
> got = (handler)(stream, bytes, want, opaque);
> -if (got < 0) {
> -virStreamAbort(stream);
> +if (got < 0)
> goto cleanup;
> -}
> if (got == 0)
> break;
> while (offset < got) {
> @@ -775,8 +768,10 @@ int virStreamSparseSendAll(virStreamPtr stream,
> cleanup:
> VIR_FREE(bytes);
>
> -if (ret != 0)
> +if (ret != 0) {
> +virStreamAbort(stream);
> virDispatchError(stream->conn);
> +}
>
> return ret;
> }
> @@ -857,10 +852,8 @@ virStreamRecvAll(virStreamPtr stream,
> while (offset < got) {
> int done;
> done = (handler)(stream, bytes + offset, got - offset, opaque);
> -if (done < 0) {
> -virStreamAbort(stream);
> +if (done < 0)
> goto cleanup;
> -}
> offset += done;
> }
> }
> @@ -869,8 +862,10 @@ virStreamRecvAll(virStreamPtr stream,
> cleanup:
> VIR_FREE(bytes);
>
> -if (ret != 0)
> +if (ret != 0) {
> +virStreamAbort(stream);
> virDispatchError(stream->conn);
> +}
>
> return ret;
> }
> @@ -963,15 +958,11 @@ virStreamSparseRecvAll(virStreamPtr stream,
>
> got = virStreamRecvFlags(stream, bytes, want, flags);
> if (got == -3) {
> -if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) {
> -virStreamAbort(stream);
> +if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0)
> goto cleanup;
> -}
>
> -if (holeHandler(stream, holeLen, opaque) < 0) {
> -
Re: [libvirt] [PATCH v3 2/6] fdstream: Report error from the I/O thread
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
> Problem with our error reporting is that the error object is a
> thread local variable. That means if there's an error reported
> within the I/O thread it gets logged and everything, but later
> when the event loop aborts the stream it doesn't see the original
> error. So we are left with some generic error. We can do better
> if we copy the error message between the threads.
>
> Signed-off-by: Michal Privoznik
> ---
> daemon/stream.c| 18 --
> src/util/virfdstream.c | 9 ++---
> 2 files changed, 18 insertions(+), 9 deletions(-)
>
Perhaps I'm lost in weeds of this code, but I thought the magic of error
message details for the threads was handled through rerr (and
virNetMessageSaveError).
Still, based on later patches which seem to care about the value of
errno, it interesting this one removes an errno setting and replaces it
with an error message.
Maybe the answer is rather than replacing threadErr it should be
'augmented' with a 'threadErrMsg'.
> diff --git a/daemon/stream.c b/daemon/stream.c
> index 1d5b50ad7..5077ac8b0 100644
> --- a/daemon/stream.c
> +++ b/daemon/stream.c
> @@ -231,17 +231,23 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
> int ret;
> virNetMessagePtr msg;
> virNetMessageError rerr;
> +virErrorPtr origErr = virSaveLastError();
>
> memset(&rerr, 0, sizeof(rerr));
> stream->closed = true;
> virStreamEventRemoveCallback(stream->st);
> virStreamAbort(stream->st);
> -if (events & VIR_STREAM_EVENT_HANGUP)
> -virReportError(VIR_ERR_RPC,
> - "%s", _("stream had unexpected termination"));
> -else
> -virReportError(VIR_ERR_RPC,
> - "%s", _("stream had I/O failure"));
> +if (origErr && origErr->code != VIR_ERR_OK) {
> +virSetError(origErr);
> +virFreeError(origErr);
> +} else {
> +if (events & VIR_STREAM_EVENT_HANGUP)
> +virReportError(VIR_ERR_RPC,
> + "%s", _("stream had unexpected termination"));
> +else
> +virReportError(VIR_ERR_RPC,
> + "%s", _("stream had I/O failure"));
> +}
This seems fine - display the error that we got from some lower spot if
it exists; otherwise,
Might be important to note the saving of the error has a lot to do with
virStreamAbort's clearing.
At the very least this is an error in our processing vs. something
within the stream, correct? Or is that the thicket of weeds I'm lost in.
>
> msg = virNetMessageNew(false);
> if (!msg) {
> diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
> index cd24757e6..5b59765fa 100644
> --- a/src/util/virfdstream.c
> +++ b/src/util/virfdstream.c
> @@ -106,7 +106,7 @@ struct virFDStreamData {
> /* Thread data */
> virThreadPtr thread;
> virCond threadCond;
> -int threadErr;
> +virErrorPtr threadErr;
> bool threadQuit;
> bool threadAbort;
> bool threadDoRead;
> @@ -123,6 +123,7 @@ virFDStreamDataDispose(void *obj)
> virFDStreamDataPtr fdst = obj;
>
> VIR_DEBUG("obj=%p", fdst);
> +virFreeError(fdst->threadErr);
> virFDStreamMsgQueueFree(&fdst->msg);
> }
>
> @@ -312,8 +313,10 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED,
> return;
> }
>
> -if (fdst->threadErr)
> +if (fdst->threadErr) {
> events |= VIR_STREAM_EVENT_ERROR;
> +virSetError(fdst->threadErr);> +}
>
> cb = fdst->cb;
> cbopaque = fdst->opaque;
> @@ -637,7 +640,7 @@ virFDStreamThread(void *opaque)
> return;
>
> error:
> -fdst->threadErr = errno;
> +fdst->threadErr = virSaveLastError();
"Typically" the processing is:
orig_err = virSaveLastError();
{ do stuff };
if (orig_err) {
virSetError(orig_err);
virFreeError(orig_err);
}
So, how do we know threadErr got something in return and if it didn't
we've messed up "other" algorithms. In particular, the lines in the
previous hunk won't set 'events' any more.
I'm not worried about being called twice (we know that won't happen
;-)), but spreading out the error message processing through 3 functions
causes me wonder whether that virFreeError should be done right after
the virSetError rather than waiting for virFDStreamDataDispose.
John
> goto cleanup;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/6] virfdstream: Check for thread error more frequently
On 06/05/2017 04:22 AM, Michal Privoznik wrote:
> When the I/O thread quits (e.g. due to an I/O error, lseek()
> error, whatever), any subsequent virFDStream API should return
> error too. Moreover, when invoking stream event callback, we must
> set the VIR_STREAM_EVENT_ERROR flag so that the callback knows
> something bad happened.
>
> Signed-off-by: Michal Privoznik
> ---
> src/util/virfdstream.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
> index 7ee58be13..cd24757e6 100644
> --- a/src/util/virfdstream.c
> +++ b/src/util/virfdstream.c
> @@ -312,6 +312,9 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED,
> return;
> }
>
> +if (fdst->threadErr)
> +events |= VIR_STREAM_EVENT_ERROR;
> +
This hunk almost feels separable... That is, it's updating the events
(VIR_STREAM_EVENT_ERROR) in the callback function feels different than
checking for threadErr more often, but I'll leave it up to you to decide
to split more or not.
> cb = fdst->cb;
> cbopaque = fdst->opaque;
> ff = fdst->ff;
> @@ -787,10 +790,10 @@ static int virFDStreamWrite(virStreamPtr st, const char
> *bytes, size_t nbytes)
> if (fdst->thread) {
> char *buf;
>
> -if (fdst->threadQuit) {
> +if (fdst->threadQuit || fdst->threadErr) {
> virReportSystemError(EBADF, "%s",
> _("cannot write to stream"));
Would this (and the subsequent similar check) possibly overwrite
something from a threadErr with this error message?
> -return -1;
> +goto cleanup;
ouch, and this is separable too technically
> }
>
> if (VIR_ALLOC(msg) < 0 ||
> @@ -866,7 +869,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes,
> size_t nbytes)
> virFDStreamMsgPtr msg = NULL;
>
> while (!(msg = fdst->msg)) {
> -if (fdst->threadQuit) {
> +if (fdst->threadQuit || fdst->threadErr) {
> if (nbytes) {
> virReportSystemError(EBADF, "%s",
> _("stream is not open"));
> @@ -959,6 +962,9 @@ virFDStreamSendHole(virStreamPtr st,
> fdst->offset += length;
> }
>
> +if (fdst->threadErr)
> +goto cleanup;
> +
So it's interesting in these paths the threadErr check is done outside
the fdst->thread check which is different than the virFDStreamRead and
Write API's.
> if (fdst->thread) {
> /* Things are a bit complicated here. If FDStream is in a
> * read mode, then if the message at the queue head is
> @@ -1018,6 +1024,9 @@ virFDStreamInData(virStreamPtr st,
>
> virObjectLock(fdst);
>
> +if (fdst->threadErr)
> +goto cleanup;
> +
This one in particular because threadQuit is checked in the subsequent
code similar to virFDStreamRead made me ponder a bit more, but I'm not
sure I could come to a conclusion...
I guess I'd want to see some consistency over when threadErr is checked
between the 4 functions or I'd wonder why 2 do things one way and 2 the
other. If they need to be different for a specific reason, then I
suppose more separation could be done or at least the commit message
indicate why they're different.
John
> if (fdst->thread) {
> virFDStreamMsgPtr msg;
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu/doc: Fix function name for handling events
Insert missing "IO" into function name. --- src/qemu/EVENTHANDLERS.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/EVENTHANDLERS.txt b/src/qemu/EVENTHANDLERS.txt index 79c1505caa..c7798d600b 100644 --- a/src/qemu/EVENTHANDLERS.txt +++ b/src/qemu/EVENTHANDLERS.txt @@ -17,7 +17,7 @@ other qemu events in the future. Any event emitted by qemu is received by -qemu_monitor_json.c:qemuMonitorJSONProcessEvent(). It looks up the +qemu_monitor_json.c:qemuMonitorJSONIOProcessEvent(). It looks up the event by name in the table eventHandlers (in the same file), which should have an entry like this for each event that libvirt understands: -- 2.11.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v5 1/5] hyperv: Functions to work with invocation parameters.
This commit introduces functionality for creating and working with
invoke parameters. This commit does not include any code for serializing
and actually performing the method invocations; it merely defines the
functions and API for using invocation parameters in driver code.
HYPERV_DEFAULT_PARAM_COUNT was chosen because almost no method
invocations have more than 4 parameters.
Functions added:
* hypervInitInvokeParamsList
* hypervFreeInvokeParams
* hypervAddSimpleParam
* hypervAddEprParam
* hypervCreateEmbeddedParam
* hypervSetEmbeddedProperty
* hypervAddEmbeddedParam
* hypervFreeEmbeddedParam
---
src/hyperv/hyperv_wmi.c | 262
src/hyperv/hyperv_wmi.h | 79 ++-
2 files changed, 340 insertions(+), 1 deletion(-)
diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
index a3c7dc0..2732db3 100644
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -2,6 +2,7 @@
* hyperv_wmi.c: general WMI over WSMAN related functions and structures for
* managing Microsoft Hyper-V hosts
*
+ * Copyright (C) 2017 Datto Inc
* Copyright (C) 2014 Red Hat, Inc.
* Copyright (C) 2011 Matthias Bolte
* Copyright (C) 2009 Michael Sievers
@@ -142,6 +143,267 @@ hypervVerifyResponse(WsManClient *client, WsXmlDocH
response,
}
+/*
+ * Methods to work with method invocation parameters
+ */
+
+/*
+ * hypervCreateInvokeParamsList:
+ * @priv: hypervPrivate object associated with the connection.
+ * @method: The name of the method you are calling
+ * @selector: The selector for the object you are invoking the method on
+ * @obj: The WmiInfo of the object class you are invoking the method on.
+ *
+ * Create a new InvokeParamsList object for the method call.
+ *
+ * Returns a pointer to the newly instantiated object on success, which should
+ * be freed by hypervInvokeMethod. Otherwise returns NULL.
+ */
+hypervInvokeParamsListPtr
+hypervCreateInvokeParamsList(hypervPrivate *priv, const char *method,
+const char *selector, hypervWmiClassInfoListPtr obj)
+{
+hypervInvokeParamsListPtr params = NULL;
+hypervWmiClassInfoPtr info = NULL;
+
+if (hypervGetWmiClassInfo(priv, obj, &info) < 0)
+goto cleanup;
+
+if (VIR_ALLOC(params) < 0)
+goto cleanup;
+
+if (VIR_ALLOC_N(params->params,
+HYPERV_DEFAULT_PARAM_COUNT) < 0) {
+VIR_FREE(params);
+goto cleanup;
+}
+
+params->method = method;
+params->ns = info->rootUri;
+params->resourceUri = info->resourceUri;
+params->selector = selector;
+params->nbParams = 0;
+params->nbAvailParams = HYPERV_DEFAULT_PARAM_COUNT;
+
+ cleanup:
+return params;
+}
+
+/*
+ * hypervFreeInvokeParams:
+ * @params: Params object to be freed
+ *
+ */
+void
+hypervFreeInvokeParams(hypervInvokeParamsListPtr params)
+{
+hypervParamPtr p = NULL;
+size_t i = 0;
+
+if (params == NULL)
+return;
+
+for (i = 0; i < params->nbParams; i++) {
+p = &(params->params[i]);
+
+switch (p->type) {
+case HYPERV_SIMPLE_PARAM:
+break;
+case HYPERV_EPR_PARAM:
+virBufferFreeAndReset(p->epr.query);
+break;
+case HYPERV_EMBEDDED_PARAM:
+hypervFreeEmbeddedParam(p->embedded.table);
+break;
+default:
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+_("Invalid parameter type passed to free"));
+}
+}
+
+VIR_DISPOSE_N(params->params, params->nbAvailParams);
+VIR_FREE(params);
+}
+
+static inline int
+hypervCheckParams(hypervInvokeParamsListPtr params)
+{
+if (params->nbParams + 1 > params->nbAvailParams) {
+if (VIR_EXPAND_N(params->params, params->nbAvailParams, 5) < 0)
+return -1;
+}
+
+return 0;
+}
+
+/*
+ * hypervAddSimpleParam:
+ * @params: Params object to add to
+ * @name: Name of the parameter
+ * @value: Value of the parameter
+ *
+ * Add a param of type HYPERV_SIMPLE_PARAM, which is essentially a serialized
+ * key/value pair.
+ *
+ * Returns -1 on failure, 0 on success.
+ */
+int
+hypervAddSimpleParam(hypervInvokeParamsListPtr params, const char *name,
+const char *value)
+{
+int result = -1;
+hypervParamPtr p = NULL;
+
+if (hypervCheckParams(params) < 0)
+goto cleanup;
+
+p = ¶ms->params[params->nbParams];
+p->type = HYPERV_SIMPLE_PARAM;
+
+p->simple.name = name;
+p->simple.value = value;
+
+params->nbParams++;
+
+result = 0;
+
+ cleanup:
+return result;
+}
+
+/*
+ * hypervAddEprParam:
+ * @params: Params object to add to
+ * @name: Parameter name
+ * @priv: hypervPrivate object associated with the connection
+ * @query: WQL filter
+ * @eprInfo: WmiInfo of the object being filtered
+ *
+ * Adds an EPR param to the params list. Returns -1 on failure, 0 on success.
+ */
+int
+hypervAddEprParam(hypervInvokeParamsListPtr params, con
[libvirt] [PATCH v5 3/5] hyperv: add hypervInvokeMethod
This commit adds support for invoking methods on remote objects
via hypervInvokeMethod.
---
src/hyperv/hyperv_wmi.c | 591
src/hyperv/hyperv_wmi.h | 8 +-
src/hyperv/openwsman.h | 4 +
3 files changed, 601 insertions(+), 2 deletions(-)
diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
index 2732db3..3b65f60 100644
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -24,6 +24,7 @@
*/
#include
+#include
#include "internal.h"
#include "virerror.h"
@@ -34,11 +35,16 @@
#include "hyperv_private.h"
#include "hyperv_wmi.h"
#include "virstring.h"
+#include "openwsman.h"
+#include "virlog.h"
#define WS_SERIALIZER_FREE_MEM_WORKS 0
#define VIR_FROM_THIS VIR_FROM_HYPERV
+#define HYPERV_JOB_TIMEOUT_MS 5000
+
+VIR_LOG_INIT("hyperv.hyperv_wmi");
static int
hypervGetWmiClassInfo(hypervPrivate *priv, hypervWmiClassInfoListPtr list,
@@ -334,6 +340,7 @@ hypervCreateEmbeddedParam(hypervPrivate *priv,
hypervWmiClassInfoListPtr info)
for (i = 0; typeinfo[i].name != NULL; i++) {}
count = i;
+count = i + 1;
table = virHashCreate(count, NULL);
if (table == NULL)
goto error;
@@ -405,6 +412,590 @@ hypervFreeEmbeddedParam(virHashTablePtr p)
virHashFree(p);
}
+/*
+ * Serializing parameters to XML and invoking methods
+ */
+
+static int
+hypervGetCimTypeInfo(hypervCimTypePtr typemap, const char *name,
+hypervCimTypePtr *property)
+{
+size_t i = 0;
+while (typemap[i].name[0] != '\0') {
+if (STREQ(typemap[i].name, name)) {
+*property = &typemap[i];
+return 0;
+}
+i++;
+}
+
+return -1;
+}
+
+
+static int
+hypervCreateInvokeXmlDoc(hypervInvokeParamsListPtr params, WsXmlDocH *docRoot)
+{
+int result = -1;
+char *method = NULL;
+WsXmlNodeH xmlNodeMethod = NULL;
+
+if (virAsprintf(&method, "%s_INPUT", params->method) < 0)
+goto cleanup;
+
+*docRoot = ws_xml_create_doc(NULL, method);
+if (*docRoot == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+_("Could not instantiate XML document"));
+goto cleanup;
+}
+
+xmlNodeMethod = xml_parser_get_root(*docRoot);
+if (xmlNodeMethod == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+_("Could not get root node of XML document"));
+goto cleanup;
+}
+
+/* add resource URI as namespace */
+ws_xml_set_ns(xmlNodeMethod, params->resourceUri, "p");
+
+result = 0;
+
+ cleanup:
+if (result < 0 && *docRoot != NULL) {
+ws_xml_destroy_doc(*docRoot);
+*docRoot = NULL;
+}
+VIR_FREE(method);
+return result;
+}
+
+static int
+hypervSerializeSimpleParam(hypervParamPtr p, const char *resourceUri,
+WsXmlNodeH *methodNode)
+{
+WsXmlNodeH xmlNodeParam = NULL;
+
+xmlNodeParam = ws_xml_add_child(*methodNode, resourceUri,
+p->simple.name, p->simple.value);
+if (xmlNodeParam == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+_("Could not create simple param"));
+return -1;
+}
+
+return 0;
+}
+
+static int
+hypervSerializeEprParam(hypervParamPtr p, hypervPrivate *priv,
+const char *resourceUri, WsXmlDocH doc, WsXmlNodeH *methodNode)
+{
+int result = -1;
+WsXmlNodeH xmlNodeParam = NULL,
+ xmlNodeTemp = NULL,
+ xmlNodeAddr = NULL,
+ xmlNodeRef = NULL;
+xmlNodePtr xmlNodeAddrPtr = NULL,
+ xmlNodeRefPtr = NULL;
+WsXmlDocH xmlDocResponse = NULL;
+xmlDocPtr docPtr = (xmlDocPtr) doc->parserDoc;
+WsXmlNsH ns = NULL;
+client_opt_t *options = NULL;
+filter_t *filter = NULL;
+char *enumContext = NULL;
+char *query_string = NULL;
+
+/* init and set up options */
+options = wsmc_options_init();
+if (!options) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not init
options"));
+goto cleanup;
+}
+wsmc_set_action_option(options, FLAG_ENUMERATION_ENUM_EPR);
+
+/* Get query and create filter based on it */
+if (virBufferCheckError(p->epr.query) < 0) {
+virBufferFreeAndReset(p->epr.query);
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid query"));
+goto cleanup;
+}
+query_string = virBufferContentAndReset(p->epr.query);
+
+filter = filter_create_simple(WSM_WQL_FILTER_DIALECT, query_string);
+if (!filter) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not create WQL
filter"));
+goto cleanup;
+}
+
+/* enumerate based on the filter from this query */
+xmlDocResponse = wsmc_action_enumerate(priv->client, p->epr.info->rootUri,
+options, filter);
+if (hypervVerifyResponse(priv->client, xmlDocResponse, "enumeration") < 0)
+goto cleanup;
+
+/* Get context */
+enumContext = wsmc_get_enum_context(xmlDocResponse);
+ws_xm
[libvirt] [PATCH v5 4/5] hyperv: support virDomainSendKey
This commit adds support for virDomainSendKey. It also serves as an
example of how to use the new method invocation APIs with a single
"simple" type parameter.
---
src/hyperv/hyperv_driver.c| 123 ++
src/hyperv/hyperv_wmi.c | 7 ++
src/hyperv/hyperv_wmi.h | 3 +-
src/hyperv/hyperv_wmi_generator.input | 86
4 files changed, 218 insertions(+), 1 deletion(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 0ca5971..3f5b94e 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -35,6 +35,8 @@
#include "hyperv_wmi.h"
#include "openwsman.h"
#include "virstring.h"
+#include "virkeycode.h"
+#include "intprops.h"
#define VIR_FROM_THIS VIR_FROM_HYPERV
@@ -1373,6 +1375,126 @@ hypervConnectListAllDomains(virConnectPtr conn,
#undef MATCH
+static int
+hypervDomainSendKey(virDomainPtr domain, unsigned int codeset,
+unsigned int holdtime, unsigned int *keycodes, int nkeycodes,
+unsigned int flags)
+{
+int result = -1;
+size_t i = 0;
+int keycode = 0;
+int *translatedKeycodes = NULL;
+hypervPrivate *priv = domain->conn->privateData;
+char uuid_string[VIR_UUID_STRING_BUFLEN];
+char *selector = NULL;
+Msvm_ComputerSystem *computerSystem = NULL;
+Msvm_Keyboard *keyboard = NULL;
+virBuffer query = VIR_BUFFER_INITIALIZER;
+hypervInvokeParamsListPtr params = NULL;
+char keycodeStr[INT_BUFSIZE_BOUND(int)];
+
+virCheckFlags(0, -1);
+
+virUUIDFormat(domain->uuid, uuid_string);
+
+if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0)
+goto cleanup;
+
+virBufferAsprintf(&query,
+"associators of "
+"{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\","
+"Name=\"%s\"} "
+"where ResultClass = Msvm_Keyboard",
+uuid_string);
+
+if (hypervGetMsvmKeyboardList(priv, &query, &keyboard) < 0)
+goto cleanup;
+
+if (VIR_ALLOC_N(translatedKeycodes, nkeycodes) < 0)
+goto cleanup;
+
+/* translate keycodes to win32 and generate keyup scancodes. */
+for (i = 0; i < nkeycodes; i++) {
+if (codeset != VIR_KEYCODE_SET_WIN32) {
+keycode = virKeycodeValueTranslate(codeset, VIR_KEYCODE_SET_WIN32,
+keycodes[i]);
+
+if (keycode < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+_("Could not translate keycode"));
+goto cleanup;
+}
+translatedKeycodes[i] = keycode;
+}
+}
+
+if (virAsprintf(&selector,
+"CreationClassName=Msvm_Keyboard&DeviceID=%s&"
+"SystemCreationClassName=Msvm_ComputerSystem&"
+"SystemName=%s", keyboard->data.common->DeviceID, uuid_string)
< 0)
+goto cleanup;
+
+/* press the keys */
+for (i = 0; i < nkeycodes; i++) {
+params = NULL;
+
+snprintf(keycodeStr, sizeof(keycodeStr), "%d", translatedKeycodes[i]);
+
+params = hypervCreateInvokeParamsList(priv, "PressKey", selector,
+Msvm_Keyboard_WmiInfo);
+
+if (!params) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not create
param"));
+goto cleanup;
+}
+
+if (hypervAddSimpleParam(params, "keyCode", keycodeStr) < 0)
+goto cleanup;
+
+if (hypervInvokeMethod(priv, params, NULL) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not press key %d"),
+ translatedKeycodes[i]);
+goto cleanup;
+}
+}
+
+/* simulate holdtime by sleeping */
+if (holdtime > 0)
+usleep(holdtime * 1000);
+
+/* release the keys */
+for (i = 0; i < nkeycodes; i++) {
+params = NULL;
+
+snprintf(keycodeStr, sizeof(keycodeStr), "%d", translatedKeycodes[i]);
+params = hypervCreateInvokeParamsList(priv, "ReleaseKey", selector,
+Msvm_Keyboard_WmiInfo);
+
+if (!params) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not create
param"));
+goto cleanup;
+}
+
+if (hypervAddSimpleParam(params, "keyCode", keycodeStr) < 0)
+goto cleanup;
+
+if (hypervInvokeMethod(priv, params, NULL) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not release key
%s"),
+keycodeStr);
+goto cleanup;
+}
+}
+
+result = 0;
+
+ cleanup:
+VIR_FREE(translatedKeycodes);
+VIR_FREE(selector);
+hypervFreeObject(priv, (hypervObject *) keyboard);
+hypervFreeObject(priv, (hypervObject *) computerSystem);
+virBufferFreeAndReset(&query);
+return result;
+}
static virHypervisorDriver hypervHypervisorDriver = {
@@ -1408,6 +1530,7 @@ static virHypervisorDriver hypervHypervisorDriver = {
[libvirt] [PATCH v5 5/5] hyperv: Add support for virDomainSetMemory
Introduces support for virDomainSetMemory. This also serves an an
example for how to use the new method invocation API with a more
complicated method, this time including an EPR and embedded param.
---
src/hyperv/hyperv_driver.c| 105 ++
src/hyperv/hyperv_wmi.c | 51 +
src/hyperv/hyperv_wmi.h | 11
src/hyperv/hyperv_wmi_generator.input | 30 ++
4 files changed, 197 insertions(+)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 3f5b94e..f557408 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -1497,6 +1497,109 @@ hypervDomainSendKey(virDomainPtr domain, unsigned int
codeset,
}
+static int
+hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory,
+unsigned int flags)
+{
+int result = -1;
+char uuid_string[VIR_UUID_STRING_BUFLEN];
+hypervPrivate *priv = domain->conn->privateData;
+char *memory_str = NULL;
+hypervInvokeParamsListPtr params = NULL;
+unsigned long memory_mb = VIR_ROUND_UP(VIR_DIV_UP(memory, 1024), 2);
+Msvm_VirtualSystemSettingData *vssd = NULL;
+Msvm_MemorySettingData *memsd = NULL;
+virBuffer eprQuery = VIR_BUFFER_INITIALIZER;
+virHashTablePtr memResource = NULL;
+
+virCheckFlags(0, -1);
+
+if (virAsprintf(&memory_str, "%lu", memory_mb) < 0)
+goto cleanup;
+
+virUUIDFormat(domain->uuid, uuid_string);
+
+if (hypervGetMsvmVirtualSystemSettingDataFromUUID(priv, uuid_string,
&vssd) < 0)
+goto cleanup;
+
+if (hypervGetMsvmMemorySettingDataFromVSSD(priv,
vssd->data.common->InstanceID,
+&memsd) < 0)
+goto cleanup;
+
+if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
+params = hypervCreateInvokeParamsList(priv,
"ModifyVirtualSystemResources",
+MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR,
+Msvm_VirtualSystemManagementService_WmiInfo);
+
+if (!params) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not create
params"));
+goto cleanup;
+}
+
+virBufferAddLit(&eprQuery, MSVM_COMPUTERSYSTEM_WQL_SELECT);
+virBufferAsprintf(&eprQuery, "where Name = \"%s\"", uuid_string);
+
+if (hypervAddEprParam(params, "ComputerSystem", priv, &eprQuery,
+Msvm_ComputerSystem_WmiInfo) < 0)
+goto cleanup;
+} else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) {
+params = hypervCreateInvokeParamsList(priv, "ModifyResourceSettings",
+MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR,
+Msvm_VirtualSystemManagementService_WmiInfo);
+
+if (!params) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not create
params"));
+goto cleanup;
+}
+}
+
+memResource = hypervCreateEmbeddedParam(priv,
Msvm_MemorySettingData_WmiInfo);
+if (!memResource)
+goto cleanup;
+
+if (hypervSetEmbeddedProperty(memResource, "VirtualQuantity", memory_str)
< 0)
+goto cleanup;
+
+if (hypervSetEmbeddedProperty(memResource, "InstanceID",
+memsd->data.common->InstanceID) < 0)
+goto cleanup;
+
+if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
+if (hypervAddEmbeddedParam(params, priv, "ResourceSettingData",
+memResource, Msvm_MemorySettingData_WmiInfo) < 0)
+goto cleanup;
+
+} else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) {
+if (hypervAddEmbeddedParam(params, priv, "ResourceSettings",
+memResource, Msvm_MemorySettingData_WmiInfo) < 0)
+goto cleanup;
+}
+
+if (hypervInvokeMethod(priv, params, NULL) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not set
memory"));
+goto cleanup;
+}
+
+/* set embedded param to NULL on success to avoid double-free in cleanup */
+memResource = NULL;
+
+result = 0;
+ cleanup:
+VIR_FREE(memory_str);
+hypervFreeEmbeddedParam(memResource);
+hypervFreeObject(priv, (hypervObject *) vssd);
+hypervFreeObject(priv, (hypervObject *) memsd);
+return result;
+}
+
+
+static int
+hypervDomainSetMemory(virDomainPtr domain, unsigned long memory)
+{
+return hypervDomainSetMemoryFlags(domain, memory, 0);
+}
+
+
static virHypervisorDriver hypervHypervisorDriver = {
.name = "Hyper-V",
.connectOpen = hypervConnectOpen, /* 0.9.5 */
@@ -1531,6 +1634,8 @@ static virHypervisorDriver hypervHypervisorDriver = {
.domainHasManagedSaveImage = hypervDomainHasManagedSaveImage, /* 0.9.5 */
.domainManagedSaveRemove = hypervDomainManagedSaveRemove, /* 0.9.5 */
.domainSendKey = hypervDomainSendKey, /* TODO: version */
+.domainSetMemory = hypervDomainSetMemory, /* TODO: version */
+.domainSetMemoryFlags = hypervDomainSetMemoryFlags, /* TODO: version */
.connectIsAlive = hypervConnectIsAli
[libvirt] [PATCH v5 0/5] Hyper-V method invocation
Changes from v4: * Changes from review * Added hypervFreeEmbeddedParam Sri Ramanujam (5): hyperv: Functions to work with invocation parameters. hyperv: Generate object property type information. hyperv: add hypervInvokeMethod hyperv: support virDomainSendKey hyperv: Add support for virDomainSetMemory src/hyperv/hyperv_driver.c| 228 + src/hyperv/hyperv_wmi.c | 911 ++ src/hyperv/hyperv_wmi.h | 95 +++- src/hyperv/hyperv_wmi_classes.h | 19 + src/hyperv/hyperv_wmi_generator.input | 116 + src/hyperv/hyperv_wmi_generator.py| 15 +- src/hyperv/openwsman.h| 4 + 7 files changed, 1386 insertions(+), 2 deletions(-) -- 2.9.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v5 2/5] hyperv: Generate object property type information.
Update the generator to generate basic property type information for
each CIM object representation. Right now, it generates arrays of
hypervCimType structs:
struct _hypervCimType {
const char *name;
const char *type;
bool isArray;
};
---
src/hyperv/hyperv_wmi_classes.h| 19 +++
src/hyperv/hyperv_wmi_generator.py | 15 ++-
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/src/hyperv/hyperv_wmi_classes.h b/src/hyperv/hyperv_wmi_classes.h
index f7d596f..ce4643e 100644
--- a/src/hyperv/hyperv_wmi_classes.h
+++ b/src/hyperv/hyperv_wmi_classes.h
@@ -1,6 +1,7 @@
/*
* hyperv_wmi_classes.h: WMI classes for managing Microsoft Hyper-V hosts
*
+ * Copyright (C) 2017 Datto Inc
* Copyright (C) 2011 Matthias Bolte
* Copyright (C) 2009 Michael Sievers
*
@@ -23,6 +24,7 @@
#ifndef __HYPERV_WMI_CLASSES_H__
# define __HYPERV_WMI_CLASSES_H__
+# include "internal.h"
# include "openwsman.h"
# include "hyperv_wmi_classes.generated.typedef"
@@ -96,6 +98,21 @@ enum _Msvm_ConcreteJob_JobState {
};
+/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
+ * WMI
+ */
+
+typedef struct _hypervCimType hypervCimType;
+typedef hypervCimType *hypervCimTypePtr;
+struct _hypervCimType {
+/* Parameter name */
+const char *name;
+/* Parameter type */
+const char *type;
+/* whether parameter is an array type */
+bool isArray;
+};
+
typedef struct _hypervWmiClassInfo hypervWmiClassInfo;
typedef hypervWmiClassInfo *hypervWmiClassInfoPtr;
struct _hypervWmiClassInfo {
@@ -109,6 +126,8 @@ struct _hypervWmiClassInfo {
const char *resourceUri;
/* The wsman serializer info - one of the *_TypeInfo structs */
XmlSerializerInfo *serializerInfo;
+/* Property type information */
+hypervCimTypePtr propertyInfo;
};
diff --git a/src/hyperv/hyperv_wmi_generator.py
b/src/hyperv/hyperv_wmi_generator.py
index 9aee0b9..9c0acce 100755
--- a/src/hyperv/hyperv_wmi_generator.py
+++ b/src/hyperv/hyperv_wmi_generator.py
@@ -122,6 +122,14 @@ class WmiClass:
source += "SER_END_ITEMS(%s_Data);\n\n" % cls.name
+# also generate typemap data while we're here
+source += "hypervCimType %s_Typemap[] = {\n" % cls.name
+
+for property in cls.properties:
+source += property.generate_typemap()
+source += '{ "", "", 0 },\n' # null terminated
+source += '};\n\n'
+
source += self._define_WmiInfo_struct()
source += "\n\n"
@@ -222,7 +230,8 @@ class WmiClass:
source += ".version = NULL,\n"
source += ".rootUri = %s,\n" % cls.uri_info.rootUri
source += ".resourceUri = %s_RESOURCE_URI,\n" %
cls.name.upper()
-source += ".serializerInfo = %s_Data_TypeInfo\n" %
cls.name
+source += ".serializerInfo = %s_Data_TypeInfo,\n" %
cls.name
+source += ".propertyInfo = %s_Typemap\n" % cls.name
source += "},\n"
source += "}\n"
@@ -374,6 +383,10 @@ class Property:
% (Property.typemap[self.type], class_name.upper(),
self.name)
+def generate_typemap(self):
+return '{ "%s", "%s", %s },\n' % (self.name, self.type.lower(),
str(self.is_array).lower())
+
+
def open_and_print(filename):
if filename.startswith("./"):
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC PATCH v1 2/4] libxl: vnuma support
From: Wim ten Have
This patch generates a NUMA distance-aware libxl description from the
information extracted from a NUMA distance-aware libvirt XML file.
By default, if no NUMA node distance information is supplied in the
libvirt XML file, this patch uses the distances 10 for local and 21 for
remote nodes/sockets."
Signed-off-by: Wim ten Have
---
src/libxl/libxl_conf.c | 128 +
1 file changed, 128 insertions(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 04d9dd1..3e1a759 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -617,6 +617,129 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
return 0;
}
+#ifdef LIBXL_HAVE_VNUMA
+static int
+libxlMakeVnumaList(
+virDomainDefPtr def,
+libxl_ctx *ctx,
+libxl_domain_config *d_config)
+{
+int ret = -1;
+char *vcpus = NULL;
+size_t i, j;
+size_t nr_nodes;
+size_t num_vnuma;
+virDomainNumaPtr numa = def->numa;
+libxl_domain_build_info *b_info = &d_config->b_info;
+libxl_physinfo physinfo;
+libxl_vnode_info *vnuma_nodes = NULL;
+
+if (!numa)
+return 0;
+
+num_vnuma = virDomainNumaGetNodeCount(numa);
+if (!num_vnuma)
+return 0;
+
+libxl_physinfo_init(&physinfo);
+if (libxl_get_physinfo(ctx, &physinfo) < 0) {
+libxl_physinfo_dispose(&physinfo);
+VIR_WARN("libxl_get_physinfo failed");
+goto cleanup;
+}
+nr_nodes = physinfo.nr_nodes;
+libxl_physinfo_dispose(&physinfo);
+
+if (num_vnuma > nr_nodes) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Number of configured numa cells %zu exceeds the
physical available nodes %zu"),
+ num_vnuma, nr_nodes);
+goto cleanup;
+}
+
+/*
+ * allocate the vnuma_nodes for assignment under b_info.
+ */
+if (VIR_ALLOC_N(vnuma_nodes, num_vnuma) < 0)
+goto cleanup;
+
+/*
+ * parse the vnuma vnodes data.
+ */
+for (i = 0; i < num_vnuma; i++) {
+int cpu;
+virBitmapPtr bitmap = NULL;
+libxl_bitmap vcpu_bitmap;
+libxl_vnode_info *p = &vnuma_nodes[i];
+
+libxl_vnode_info_init(p);
+
+/* pnode */
+p->pnode = i;
+
+/* memory size */
+p->memkb = virDomainNumaGetNodeMemorySize(numa, i);
+
+/* vcpus */
+vcpus = virBitmapFormat(virDomainNumaGetNodeCpumask(numa, i));
+if (vcpus == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("vnuma sibling %zu missing vcpus set"), i);
+goto cleanup;
+}
+
+if (virBitmapParse(vcpus, &bitmap, VIR_DOMAIN_CPUMASK_LEN) < 0)
+goto cleanup;
+
+if ((cpu = virBitmapNextSetBit(bitmap, -1)) < 0) {
+VIR_FREE(bitmap);
+goto cleanup;
+}
+
+libxl_bitmap_init(&vcpu_bitmap);
+if (libxl_cpu_bitmap_alloc(ctx, &vcpu_bitmap, b_info->max_vcpus)) {
+virReportOOMError();
+goto cleanup;
+}
+
+do {
+libxl_bitmap_set(&vcpu_bitmap, cpu);
+} while ((cpu = virBitmapNextSetBit(bitmap, cpu)) >= 0);
+
+libxl_bitmap_copy_alloc(ctx, &p->vcpus, &vcpu_bitmap);
+libxl_bitmap_dispose(&vcpu_bitmap);
+
+VIR_FREE(bitmap);
+VIR_FREE(vcpus);
+
+/* vdistances */
+if (VIR_ALLOC_N(p->distances, num_vnuma) < 0)
+goto cleanup;
+p->num_distances = num_vnuma;
+
+/*
+ * Apply the configured distance value. If not
+ * available set 10 for local or 21 for remote nodes.
+ */
+for (j = 0; j < num_vnuma; j++)
+p->distances[j] = virDomainNumaGetNodeDistance(numa, i, j) ?:
+(i == j ? 10 : 21);
+}
+
+b_info->vnuma_nodes = vnuma_nodes;
+b_info->num_vnuma_nodes = num_vnuma;
+
+ret = 0;
+
+ cleanup:
+if (ret)
+VIR_FREE(vnuma_nodes);
+VIR_FREE(vcpus);
+
+return ret;
+}
+#endif
+
static int
libxlDiskSetDiscard(libxl_device_disk *x_disk, int discard)
{
@@ -2207,6 +2330,11 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
if (libxlMakeDomBuildInfo(def, ctx, caps, d_config) < 0)
return -1;
+#ifdef LIBXL_HAVE_VNUMA
+if (libxlMakeVnumaList(def, ctx, d_config) < 0)
+return -1;
+#endif
+
if (libxlMakeDiskList(def, d_config) < 0)
return -1;
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC PATCH v1 0/4] numa: describe sibling nodes distances
From: Wim ten Have
This patch extents guest domain administration adding support to advertise
node sibling distances when configuring HVM numa guests.
NUMA (non-uniform memory access), a method of configuring a cluster of nodes
within a single multiprocessing system such that it shares processor
local memory amongst others improving performance and the ability of the
system to be expanded.
A NUMA system could be illustrated as shown below. Within this 4-node
system, every socket is equipped with its own distinct memory. The whole
typically resembles a SMP (symmetric multiprocessing) system being a
"tightly-coupled," "share everything" system in which multiple processors
are working under a single operating system and can access each others'
memory over multiple "Bus Interconnect" paths.
+-+-+-+ +-+-+-+
| M | CPU | CPU | | CPU | CPU | M |
| E | | | | | | E |
| M +- Socket0 -+ +- Socket3 -+ M |
| O | | | | | | O |
| R | CPU | CPU <-> CPU | CPU | R |
| Y | | | | | | Y |
+-+--^--+-+ +-+--^--+-+
| |
| Bus Interconnect |
| |
+-+--v--+-+ +-+--v--+-+
| M | | | | | | M |
| E | CPU | CPU <-> CPU | CPU | E |
| M | | | | | | M |
| O +- Socket1 -+ +- Socket2 -+ O |
| R | | | | | | R |
| Y | CPU | CPU | | CPU | CPU | Y |
+-+-+-+ +-+-+-+
In contrast there is the limitation of a flat SMP system, not illustrated.
Here, as sockets are added, the bus (data and address path), under high
activity, gets overloaded and easily becomes a performance bottleneck.
NUMA adds an intermediate level of memory shared amongst a few cores per
socket as illustrated above, so that data accesses do not have to travel
over a single bus.
Unfortunately the way NUMA does this adds its own limitations. This,
as visualized in the illustration above, happens when data is stored in
memory associated with Socket2 and is accessed by a CPU (core) in Socket0.
The processors use the "Bus Interconnect" to create gateways between the
sockets (nodes) enabling inter-socket access to memory. These "Bus
Interconnect" hops add data access delays when a CPU (core) accesses
memory associated with a remote socket (node).
For terminology we refer to sockets as "nodes" where access to each
others' distinct resources such as memory make them "siblings" with a
designated "distance" between them. A specific design is described under
the ACPI (Advanced Configuration and Power Interface Specification)
within the chapter explaining the system's SLIT (System Locality Distance
Information Table).
These patches extend core libvirt's XML description of a virtual machine's
hardware to include NUMA distance information for sibling nodes, which
is then passed to Xen guests via libxl. Recently qemu landed support for
constructing the SLIT since commit 0f203430dd ("numa: Allow setting NUMA
distance for different NUMA nodes"), hence these core libvirt extensions
can also help other drivers in supporting this feature.
The XML changes made allow to describe the (or node/sockets)
amongst node identifiers and propagate these towards the numa
domain functionality finally adding support to libxl.
[below is an example illustrating a 4 node/socket setup]
By default on libxl, if no are given to describe the SLIT data
between different s, this patch will default to a scheme using 10
for local and 21 for any remote node/socket, which is the assumption of
guest OS when no SLIT is specified. While SLIT is optional, libxl requires
that distances are set nonetheless.
On Linux systems the SLIT detail can be listed with help of the 'numactl -H'
command. An above HVM guest as described would on such prompt with below output.
[root@f25 ~]# numactl -H
available: 4 nodes (0-3)
node 0 cpus: 0 4 5 6 7
node 0 size: 1988 MB
node 0 free: 1743 MB
node 1 cpus: 1 8 9 10 12 13 14 15
node 1 size: 1946 MB
node 1 free: 1885 MB
node 2 cpus: 2 11
node 2 size: 2011 MB
node 2 free: 1912 MB
node 3 cpus: 3
node 3 size: 2010 MB
node 3 free: 1980 MB
node distances:
node 0
[libvirt] [RFC PATCH v1 3/4] xenconfig: add domxml conversions for xen-xl
From: Wim ten Have
This patch converts NUMA configurations between the Xen libxl
configuration file format and libvirt's XML format.
XML HVM domain configuration:
Xen xl.cfg domain configuration:
vnuma = [["pnode=0","size=2048","vcpus=0-1","vdistances=10,21,31,41"],
["pnode=1","size=2048","vcpus=2-3","vdistances=21,10,21,31"],
["pnode=2","size=2048","vcpus=4-5","vdistances=31,21,10,21"],
["pnode=3","size=2048","vcpus=6-7","vdistances=41,31,21,10"]]
If there is no XML description amongst the data the
conversion schema from xml to native will generate 10 for local and 21
for all remote instances.
Signed-off-by: Wim ten Have
---
src/xenconfig/xen_xl.c | 300 +
1 file changed, 300 insertions(+)
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index cac440c..0c7dd23 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -309,6 +309,168 @@ xenParseXLSpice(virConfPtr conf, virDomainDefPtr def)
return -1;
}
+#ifdef LIBXL_HAVE_VNUMA
+static int
+xenParseXLVnuma(virConfPtr conf, virDomainDefPtr def)
+{
+int ret = -1;
+char *tmp = NULL;
+char **token = NULL;
+size_t vcpus = 0;
+
+virConfValuePtr list;
+virDomainNumaPtr numa;
+
+numa = def->numa;
+if (numa == NULL)
+return -1;
+
+list = virConfGetValue(conf, "vnuma");
+if (list && list->type == VIR_CONF_LIST) {
+size_t nr_nodes = 0, vnodeCnt = 0;
+virConfValuePtr vnode = list->list;
+virCPUDefPtr cpu;
+
+vnode = list->list;
+while (vnode) {
+vnode = vnode->next;
+nr_nodes++;
+}
+
+if (!virDomainNumaSetNodeCount(numa, nr_nodes))
+goto cleanup;
+
+list = list->list;
+
+if (VIR_ALLOC(cpu) < 0)
+goto cleanup;
+
+while (list) {
+
+/* Is there a sublist (vnode)? */
+if (list && list->type == VIR_CONF_LIST) {
+vnode = list->list;
+
+while (vnode && vnode->type == VIR_CONF_STRING) {
+const char *data;
+const char *str = vnode->str;
+
+if (!str ||
+ !(data = strrchr(str, '='))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("vnuma vnode invalid format '%s'"),
+ str);
+goto skipvnode;
+}
+data++;
+
+if (*data) {
+size_t len;
+char vtoken[64];
+
+if (STRPREFIX(str, "pnode")) {
+unsigned int cellid;
+
+len = strlen(data);
+if (!virStrncpy(vtoken, data,
+len, sizeof(vtoken))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("vnuma vnode %zu pnode '%s'
too long for destination"),
+ vnodeCnt, data);
+goto cleanup;
+}
+
+if ((virStrToLong_ui(vtoken, NULL, 10, &cellid) <
0) ||
+(cellid >= nr_nodes)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("vnuma vnode %zu invalid
value pnode '%s'"),
+ vnodeCnt, data);
+goto cleanup;
+}
+} else if (STRPREFIX(str, "size")) {
+unsigned long long kbsize;
+
+len = strlen(data);
+if (!virStrncpy(vtoken, data,
+len, sizeof(vtoken))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("vnuma vnode %zu size '%s'
too long for destination"),
+ vnodeCnt, data);
+goto cleanup;
+}
+
+if (virStrToLong_ull(vtoken, NULL, 10, &kbsize) <
0)
+goto cleanup;
+
+virDomainNumaSetNodeMemorySize(numa, vnodeCnt,
(kbsize * 1024));
+} else if (STRPREFIX(str, "vcpus")) {
+
[libvirt] [RFC PATCH v1 1/4] numa: describe siblings distances within cells
From: Wim ten Have
Add libvirtd NUMA cell domain administration functionality to
describe underlying cell id sibling distances in full fashion
when configuring HVM guests.
[below is an example of a 4 node setup]
Changes under this commit concern all those that require adding
the valid data structures, virDomainNuma* functional routines and the
XML doc schema enhancements to enforce appropriate administration.
These changes alter the docs/schemas/cputypes.rng enforcing
domain administration to follow the syntax below per numa cell id.
These changes also alter docs/schemas/basictypes.rng to add
"numaDistanceValue" which is an "unsignedInt" with a minimum value
of 10 as 0-9 are reserved values and can not be used as System
Locality Distance Information Table data.
Signed-off-by: Wim ten Have
---
docs/schemas/basictypes.rng | 8 ++
docs/schemas/cputypes.rng | 18 +++
src/conf/cpu_conf.c | 2 +-
src/conf/numa_conf.c| 260 +++-
src/conf/numa_conf.h| 25 -
src/libvirt_private.syms| 6 +
6 files changed, 313 insertions(+), 6 deletions(-)
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 1ea667c..a335b5d 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -77,6 +77,14 @@
+
+
+
+10
+
+
+
+
diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng
index 3eef16a..c45b6df 100644
--- a/docs/schemas/cputypes.rng
+++ b/docs/schemas/cputypes.rng
@@ -129,6 +129,24 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index da40e9b..5d8f7be3 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -643,7 +643,7 @@ virCPUDefFormatBufFull(virBufferPtr buf,
if (virCPUDefFormatBuf(&childrenBuf, def, updateCPU) < 0)
goto cleanup;
-if (virDomainNumaDefCPUFormat(&childrenBuf, numa) < 0)
+if (virDomainNumaDefCPUFormatXML(&childrenBuf, numa) < 0)
goto cleanup;
/* Put it all together */
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index bfd3703..1914810 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -48,6 +48,8 @@ VIR_ENUM_IMPL(virDomainMemoryAccess,
VIR_DOMAIN_MEMORY_ACCESS_LAST,
"shared",
"private")
+typedef struct _virDomainNumaDistance virDomainNumaDistance;
+typedef virDomainNumaDistance *virDomainNumaDistancePtr;
typedef struct _virDomainNumaNode virDomainNumaNode;
typedef virDomainNumaNode *virDomainNumaNodePtr;
@@ -66,6 +68,12 @@ struct _virDomainNuma {
virBitmapPtr nodeset; /* host memory nodes where this guest node
resides */
virDomainNumatuneMemMode mode; /* memory mode selection */
virDomainMemoryAccess memAccess; /* shared memory access configuration
*/
+
+struct _virDomainNumaDistance {
+ unsigned int value;/* locality value for node i*j */
+ unsigned int cellid;
+} *distances; /* remote node distances */
+size_t ndistances;
} *mem_nodes; /* guest node configuration */
size_t nmem_nodes;
@@ -686,6 +694,95 @@ virDomainNumatuneNodesetIsAvailable(virDomainNumaPtr
numatune,
}
+static int
+virDomainNumaDefNodeDistanceParseXML(virDomainNumaPtr def,
+ xmlXPathContextPtr ctxt,
+ unsigned int cur_cell)
+{
+int ret = -1;
+char *tmp = NULL;
+size_t i;
+xmlNodePtr *nodes = NULL;
+int ndistances;
+virDomainNumaDistancePtr distances = NULL;
+
+
+if (!virXPathNode("./distances[1]/sibling", ctxt))
+return 0;
+
+if ((ndistances = virXPathNodeSet("./distances[1]/sibling", ctxt, &nodes))
<= 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("NUMA distances defined without siblings"));
+goto cleanup;
+}
+
+if (ndistances < def->nmem_nodes) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("NUMA distances defined with fewer siblings than
nodes for cell id: '%d'"),
+ cur_cell);
+goto cleanup;
+}
+
+if (VIR_ALLOC_N(distances, ndistances) < 0)
+goto cleanup;
+
+for (i = 0; i < ndistances; i++) {
+unsigned int sibling_id = i, sibling_value;
+
+/* siblings are in order of parsing or explicitly numbered */
+if ((tmp = virXMLPropString(nodes[i], "id"))) {
+
[libvirt] [RFC PATCH v1 4/4] xlconfigtest: add tests for numa cell sibling distances
From: Wim ten Have
Test a bidirectional xen-xl domxml to and from native for numa
support administration as brought under this patch series.
Signed-off-by: Wim ten Have
---
.../test-fullvirt-vnuma-nodistances.cfg| 26 +++
.../test-fullvirt-vnuma-nodistances.xml| 54 +++
tests/xlconfigdata/test-fullvirt-vnuma.cfg | 26 +++
tests/xlconfigdata/test-fullvirt-vnuma.xml | 81 ++
tests/xlconfigtest.c | 4 ++
5 files changed, 191 insertions(+)
create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml
create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma.cfg
create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma.xml
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
new file mode 100644
index 000..9871f21
--- /dev/null
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
@@ -0,0 +1,26 @@
+name = "XenGuest2"
+uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
+maxmem = 8192
+memory = 8192
+vcpus = 8
+pae = 1
+acpi = 1
+apic = 1
+viridian = 0
+rtc_timeoffset = 0
+localtime = 0
+on_poweroff = "destroy"
+on_reboot = "restart"
+on_crash = "restart"
+device_model = "/usr/lib/xen/bin/qemu-system-i386"
+sdl = 0
+vnc = 1
+vncunused = 1
+vnclisten = "127.0.0.1"
+vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
+parallel = "none"
+serial = "none"
+builder = "hvm"
+boot = "d"
+vnuma = [ [ "pnode=0", "size=2048", "vcpus=0-1", "vdistances=10,21,21,21" ], [
"pnode=1", "size=2048", "vcpus=2-3", "vdistances=21,10,21,21" ], [ "pnode=2",
"size=2048", "vcpus=4-5", "vdistances=21,21,10,21" ], [ "pnode=3", "size=2048",
"vcpus=6-7", "vdistances=21,21,21,10" ] ]
+disk = [
"format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2" ]
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml
b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml
new file mode 100644
index 000..a576881
--- /dev/null
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml
@@ -0,0 +1,54 @@
+
+ XenGuest2
+ c7a5fdb2-cdaf-9455-926a-d65c16db1809
+ 8388608
+ 8388608
+ 8
+
+hvm
+/usr/lib/xen/boot/hvmloader
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+ destroy
+ restart
+ restart
+
+/usr/lib/xen/bin/qemu-system-i386
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma.cfg
b/tests/xlconfigdata/test-fullvirt-vnuma.cfg
new file mode 100644
index 000..91e233a
--- /dev/null
+++ b/tests/xlconfigdata/test-fullvirt-vnuma.cfg
@@ -0,0 +1,26 @@
+name = "XenGuest2"
+uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
+maxmem = 8192
+memory = 8192
+vcpus = 8
+pae = 1
+acpi = 1
+apic = 1
+viridian = 0
+rtc_timeoffset = 0
+localtime = 0
+on_poweroff = "destroy"
+on_reboot = "restart"
+on_crash = "restart"
+device_model = "/usr/lib/xen/bin/qemu-system-i386"
+sdl = 0
+vnc = 1
+vncunused = 1
+vnclisten = "127.0.0.1"
+vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
+parallel = "none"
+serial = "none"
+builder = "hvm"
+boot = "d"
+vnuma = [ [ "pnode=0", "size=2048", "vcpus=0-1", "vdistances=10,21,31,41" ], [
"pnode=1", "size=2048", "vcpus=2-3", "vdistances=21,10,21,31" ], [ "pnode=2",
"size=2048", "vcpus=4-5", "vdistances=31,21,10,21" ], [ "pnode=3", "size=2048",
"vcpus=6-7", "vdistances=41,31,21,10" ] ]
+disk = [
"format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2" ]
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma.xml
b/tests/xlconfigdata/test-fullvirt-vnuma.xml
new file mode 100644
index 000..5368b0d
--- /dev/null
+++ b/tests/xlconfigdata/test-fullvirt-vnuma.xml
@@ -0,0 +1,81 @@
+
+ XenGuest2
+ c7a5fdb2-cdaf-9455-926a-d65c16db1809
+ 8388608
+ 8388608
+ 8
+
+hvm
+/usr/lib/xen/boot/hvmloader
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+ destroy
+ restart
+ restart
+
+/usr/lib/xen/bin/qemu-system-i386
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c
index 3fe4298..b5c6891 100644
--- a/tests/xlconfigtest.c
+++ b/tests/xlconfigtest.c
@@ -270,6 +270,10 @@ mymain(void)
DO_TEST("fullvirt-multi-timer");
[libvirt] [PATCH 3/3] qemuDomainGetPreservedMounts: Fix suffixes for corner cases
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
Imagine a FS mounted on /dev/blah/blah2. Our process of creating
suffix for temporary location where all the mounted filesystems
are moved is very simplistic. We want:
/var/run/libvirt/qemu/$domName.$suffix\
were $suffix is just the mount point path stripped of the "/dev/"
preffix. For instance:
/var/run/libvirt/qemu/fedora.mqueue for /dev/mqueue
/var/run/libvirt/qemu/fedora.pts for /dev/pts
and so on. Now if we plug /dev/blah/blah2 into the example we see
some misbehaviour:
/var/run/libvirt/qemu/fedora.blah/blah2
Well, misbehaviour if /dev/blah/blah2 is a file, because in that
case we call virFileTouch() instead of virFileMakePath().
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_domain.c | 16
1 file changed, 16 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index accf05a6f..547c9fbfb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7570,7 +7570,9 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
goto error;
for (i = 0; i < nmounts; i++) {
+char *tmp;
const char *suffix = mounts[i] + strlen(DEVPREFIX);
+size_t off;
if (STREQ(mounts[i], "/dev"))
suffix = "dev";
@@ -7578,6 +7580,20 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
if (virAsprintf(&paths[i], "%s/%s.%s",
cfg->stateDir, vm->def->name, suffix) < 0)
goto error;
+
+/* Now consider that mounts[i] is "/dev/blah/blah2".
+ * @suffix then points to "blah/blah2". However, caller
+ * expects all the @paths to be the same depth. The
+ * caller doesn't always do `mkdir -p` but sometimes bare
+ * `touch`. Therefore fix all the suffixes. */
+off = strlen(paths[i]) - strlen(suffix);
+
+tmp = paths[i] + off;
+while (*tmp) {
+if (*tmp == '/')
+*tmp = '.';
+tmp++;
+}
}
if (devPath)
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] qemuDomainGetPreservedMounts: Prune nested mount points
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
There can be nested mount points. For instance /dev/shm/blah can
be a mount point and /dev/shm too. It doesn't make much sense to
return the former path because callers preserve the latter (and
with that the former too). Therefore prune nested mount points.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_domain.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 23b92606e..accf05a6f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7533,7 +7533,7 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
size_t *ndevPath)
{
char **paths = NULL, **mounts = NULL;
-size_t i, nmounts;
+size_t i, j, nmounts;
if (virFileGetMountSubtree(PROC_MOUNTS, "/dev",
&mounts, &nmounts) < 0)
@@ -7545,6 +7545,27 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
return 0;
}
+/* There can be nested mount points. For instance
+ * /dev/shm/blah can be a mount point and /dev/shm too. It
+ * doesn't make much sense to return the former path because
+ * caller preserves the latter (and with that the former
+ * too). Therefore prune nested mount points.
+ * NB mounts[0] is "/dev". Should we start the outer loop
+ * from the beginning of the array all we'd be left with is
+ * just the first element. Think about it.
+ */
+for (i = 1; i < nmounts; i++) {
+for (j = i + 1; j < nmounts;) {
+if (STRPREFIX(mounts[j], mounts[i])) {
+VIR_DEBUG("Dropping path %s because of %s", mounts[j],
mounts[i]);
+VIR_DELETE_ELEMENT(mounts, j, nmounts);
+} else {
+j++;
+}
+}
+}
+
+
if (VIR_ALLOC_N(paths, nmounts) < 0)
goto error;
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] Couple of qemu NS fixes
Yet again, some corner cases, nothing critical. But it is certainly nice to fix them regardless. Michal Privoznik (3): qemuDomainBuildNamespace: Clean up temp files qemuDomainGetPreservedMounts: Prune nested mount points qemuDomainGetPreservedMounts: Fix suffixes for corner cases src/qemu/qemu_domain.c | 44 ++-- 1 file changed, 42 insertions(+), 2 deletions(-) -- 2.13.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
https://bugzilla.redhat.com/show_bug.cgi?id=1431112
After 290a00e41d we know how to deal with file mount points.
However, when cleaning up the temporary location for preserved
mount points we are still calling rmdir(). This won't fly for
files. We need to call unlink(). Now, since we don't really care
if the cleanup succeeded or not (it's the best effort anyway), we
can call both rmdir() and unlink() without need for
differentiation between files and directories.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_domain.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 36fa450e8..23b92606e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8311,8 +8311,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
ret = 0;
cleanup:
-for (i = 0; i < ndevMountsPath; i++)
+for (i = 0; i < ndevMountsPath; i++) {
+/* The path can be either a regular file or a dir. */
rmdir(devMountsSavePath[i]);
+unlink(devMountsSavePath[i]);
+}
virStringListFreeCount(devMountsPath, ndevMountsPath);
virStringListFreeCount(devMountsSavePath, ndevMountsPath);
return ret;
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/26] qemu: Allow qemuBuildControllerDevStr() to return NULL
On 06/02/2017 12:07 PM, Andrea Bolognani wrote:
> We will soon need to be able to return a NULL pointer
> without the caller considering that an error: to make
> it possible, change the return type to int and use
> an out parameter for the string instead.
>
> Add some documentation for the function as well.
> ---
> src/qemu/qemu_command.c | 53
> -
> src/qemu/qemu_command.h | 9 +
> src/qemu/qemu_hotplug.c | 5 -
> 3 files changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 015af10..a0403bf 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2664,10 +2664,31 @@ qemuCheckSCSIControllerIOThreads(const virDomainDef
> *domainDef,
> }
>
>
> -char *
> +/**
> + * qemuBuildControllerDevStr:
> + * @domainDef: domain definition
> + * @def: controller definition
> + * @qemuCaps: QEMU binary capabilities
> + * @devstr: device string
> + * @nusbcontroller: number of USB controllers
> + *
> + * Turn @def into a description of the controller that QEMU will understand,
> + * to be used either on the command line or through the monitor.
> + *
> + * The description will be returned in @devstr and can be NULL, eg. when
> + * passing in one of the built-in controllers. The returned string must be
> + * freed by the caller.
> + *
> + * The number pointed to by @nusbcontroller will be increased by one every
> + * time the description for a USB controller has been generated successfully.
> + *
> + * Returns: 0 on success, <0 on failure
> + */
> +int
> qemuBuildControllerDevStr(const virDomainDef *domainDef,
>virDomainControllerDefPtr def,
>virQEMUCapsPtr qemuCaps,
> + char **devstr,
>int *nusbcontroller)
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
Maybe initialize *devstr to NULL in case the caller hasn't?
Reviewed-by: Laine Stump
> @@ -2676,11 +2697,11 @@ qemuBuildControllerDevStr(const virDomainDef
> *domainDef,
>
> if (!qemuCheckCCWS390AddressSupport(domainDef, def->info, qemuCaps,
> "controller"))
> -return NULL;
> +return -1;
>
> if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> if ((qemuDomainSetSCSIControllerModel(domainDef, qemuCaps, &model))
> < 0)
> -return NULL;
> +return -1;
> }
>
> if (!(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
> @@ -2688,22 +2709,22 @@ qemuBuildControllerDevStr(const virDomainDef
> *domainDef,
> if (def->queues) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("'queues' is only supported by virtio-scsi
> controller"));
> -return NULL;
> +return -1;
> }
> if (def->cmd_per_lun) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("'cmd_per_lun' is only supported by virtio-scsi
> controller"));
> -return NULL;
> +return -1;
> }
> if (def->max_sectors) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("'max_sectors' is only supported by virtio-scsi
> controller"));
> -return NULL;
> +return -1;
> }
> if (def->ioeventfd) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("'ioeventfd' is only supported by virtio-scsi
> controller"));
> -return NULL;
> +return -1;
> }
> }
>
> @@ -3114,11 +3135,12 @@ qemuBuildControllerDevStr(const virDomainDef
> *domainDef,
> if (virBufferCheckError(&buf) < 0)
> goto error;
>
> -return virBufferContentAndReset(&buf);
> +*devstr = virBufferContentAndReset(&buf);
> +return 0;
>
> error:
> virBufferFreeAndReset(&buf);
> -return NULL;
> +return -1;
> }
>
>
> @@ -3213,12 +3235,15 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
> continue;
> }
>
> -virCommandAddArg(cmd, "-device");
> -if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps,
> - &usbcontroller)))
> +if (qemuBuildControllerDevStr(def, cont, qemuCaps,
> + &devstr, &usbcontroller) < 0)
> return -1;
> -virCommandAddArg(cmd, devstr);
> -VIR_FREE(devstr);
> +
> +if (devstr) {
> +virCommandAddArg(cmd, "-device");
> +virCommandAddArg(cmd, devstr);
> +VIR_FREE(devstr);
> +}
> }
> }
>
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 09cb00e..9a2ab29 100644
> --- a/src/qemu/qemu_comm
Re: [libvirt] [PATCH 06/26] conf: Simplify slot allocation
On 06/02/2017 12:07 PM, Andrea Bolognani wrote:
> The current algorithm for slot allocation tries to be clever
> and avoid looking at buses / slots more than once unless it's
> necessary. Unfortunately that makes the code more complex,
> and it will cause problem later on in some situations unless
> even more complex code is added.
>
> Since the performance gains are going to be pretty modest
> anyway, we can just get rid of the extra complexity and use a
> completely straighforward implementation instead.
> ---
>From looking at the current git blame you might assume that I had
something to do with this optimization, but actually it was there before
I got involved with PCI address allocation - I only preserved that
behavior. So I have no quarrel with simplifying it as you've done.
I guess the only concern I might have would be if the new algorithm
ended up give out a different address in some case (just in case someone
using transient domains with no pre-assigned PCI addresses was expecting
address stability), but I can't think of any way that would happen, and
the fact that the unit tests all pass indicates that it isn't happening.
Reviewed-by: Laine Stump
> src/conf/domain_addr.c | 43 +++
> src/conf/domain_addr.h | 2 --
> 2 files changed, 7 insertions(+), 38 deletions(-)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 6fd8bfc..d173766 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -741,34 +741,26 @@
> virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs,
> virDomainPCIConnectFlags flags,
> int function)
> {
> -/* default to starting the search for a free slot from
> - * the first slot of domain 0 bus 0...
> - */
> virPCIDeviceAddress a = { 0 };
> -bool found = false;
>
> if (addrs->nbuses == 0) {
> virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available"));
> goto error;
> }
>
> -/* ...unless this search is for the exact same type of device as
> - * last time, then continue the search from the slot where we
> - * found the previous match (it's possible there will still be a
> - * function available on that slot).
> - */
> -if (flags == addrs->lastFlags)
> -a = addrs->lastaddr;
> -else
> -a.slot = addrs->buses[0].minSlot;
> -
> /* if the caller asks for "any function", give them function 0 */
> if (function == -1)
> a.function = 0;
> else
> a.function = function;
>
> -while (a.bus < addrs->nbuses) {
> +/* "Begin at the beginning," the King said, very gravely, "and go on
> + * till you come to the end: then stop." */
> +for (a.bus = 0; a.bus < addrs->nbuses; a.bus++) {
> +bool found = false;
> +
> +a.slot = addrs->buses[a.bus].minSlot;
> +
> if (virDomainPCIAddressFindUnusedFunctionOnBus(&addrs->buses[a.bus],
> &a, function,
> flags, &found) < 0) {
> @@ -777,10 +769,6 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr
> addrs,
>
> if (found)
> goto success;
> -
> -/* nothing on this bus, go to the next bus */
> -if (++a.bus < addrs->nbuses)
> -a.slot = addrs->buses[a.bus].minSlot;
> }
>
> /* There were no free slots after the last used one */
> @@ -791,20 +779,6 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr
> addrs,
> /* this device will use the first slot of the new bus */
> a.slot = addrs->buses[a.bus].minSlot;
> goto success;
> -} else if (flags == addrs->lastFlags) {
> -/* Check the buses from 0 up to the last used one */
> -for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) {
> -a.slot = addrs->buses[a.bus].minSlot;
> -
> -if
> (virDomainPCIAddressFindUnusedFunctionOnBus(&addrs->buses[a.bus],
> - &a, function,
> - flags, &found) <
> 0) {
> -goto error;
> -}
> -
> -if (found)
> -goto success;
> -}
> }
>
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -851,9 +825,6 @@
> virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> if (virDomainPCIAddressReserveAddrInternal(addrs, &addr, flags, false) <
> 0)
> return -1;
>
> -addrs->lastaddr = addr;
> -addrs->lastFlags = flags;
> -
> if (!addrs->dryRun) {
> dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> dev->addr.pci = addr;
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index 77e19bb..7704061 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
>
Re: [libvirt] [PATCH 05/26] tests: Mock IOMMU groups
On 06/02/2017 12:07 PM, Andrea Bolognani wrote:
> Later on we're going to need access to information about IOMMU
> groups for host devices. Implement the support in virpcimock,
> and start using that mock library in a few QEMU test cases.
> ---
> tests/qemumemlocktest.c | 21 -
> tests/qemuxml2argvtest.c | 25 ++---
> tests/qemuxml2xmltest.c | 22 +-
> tests/virpcimock.c | 43 +--
> 4 files changed, 100 insertions(+), 11 deletions(-)
>
> diff --git a/tests/qemumemlocktest.c b/tests/qemumemlocktest.c
> index 6cf17a4..c0f1dc3 100644
> --- a/tests/qemumemlocktest.c
> +++ b/tests/qemumemlocktest.c
> @@ -56,11 +56,25 @@ testCompareMemLock(const void *data)
> return ret;
> }
>
> +# define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XX"
>
> static int
> mymain(void)
> {
> int ret = 0;
> +char *fakerootdir;
> +
> +if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) {
> +fprintf(stderr, "Out of memory\n");
> +abort();
> +}
> +
> +if (!mkdtemp(fakerootdir)) {
> +fprintf(stderr, "Cannot create fakerootdir");
> +abort();
> +}
> +
> +setenv("LIBVIRT_FAKE_ROOT_DIR", fakerootdir, 1);
I see all this repetition of boilerplate code and wonder if maybe it
should be done in one common place. Not a topic for this series though.
>
> abs_top_srcdir = getenv("abs_top_srcdir");
> if (!abs_top_srcdir)
> @@ -124,12 +138,17 @@ mymain(void)
> DO_TEST("pseries-hardlimit+locked+hostdev", 2147483648);
> DO_TEST("pseries-locked+hostdev", VIR_DOMAIN_MEMORY_PARAM_UNLIMITED);
>
> +if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
> +virFileDeleteTree(fakerootdir);
> +
> qemuTestDriverFree(&driver);
> +VIR_FREE(fakerootdir);
>
> return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
> }
>
> -VIR_TEST_MAIN(mymain)
> +VIR_TEST_MAIN_PRELOAD(mymain,
> + abs_builddir "/.libs/virpcimock.so")
>
> #else
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index b360185..cc6a8a8 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -534,13 +534,27 @@ testCompareXMLToArgv(const void *data)
> return ret;
> }
>
> +# define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XX"
>
> static int
> mymain(void)
> {
> int ret = 0;
> +char *fakerootdir;
> bool skipLegacyCPUs = false;
>
> +if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) {
> +fprintf(stderr, "Out of memory\n");
> +abort();
Any particular reason (other than "that's what everybody else is doing")
that you use a combination of fprintf+abort here, but the ABORT macro in
other places?
> +}
> +
> +if (!mkdtemp(fakerootdir)) {
> +fprintf(stderr, "Cannot create fakerootdir");
> +abort();
> +}
> +
> +setenv("LIBVIRT_FAKE_ROOT_DIR", fakerootdir, 1);
> +
> abs_top_srcdir = getenv("abs_top_srcdir");
> if (!abs_top_srcdir)
> abs_top_srcdir = abs_srcdir "/..";
> @@ -2567,15 +2581,20 @@ mymain(void)
> DO_TEST_PARSE_ERROR("cpu-cache-passthrough3", QEMU_CAPS_KVM);
> DO_TEST_PARSE_ERROR("cpu-cache-passthrough-l3", QEMU_CAPS_KVM);
>
> +if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
> +virFileDeleteTree(fakerootdir);
> +
> qemuTestDriverFree(&driver);
> +VIR_FREE(fakerootdir);
>
> return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
> }
>
> VIR_TEST_MAIN_PRELOAD(mymain,
> - abs_builddir "/.libs/qemuxml2argvmock.so",
> - abs_builddir "/.libs/virrandommock.so",
> - abs_builddir "/.libs/qemucpumock.so")
> + abs_builddir "/.libs/qemuxml2argvmock.so",
> + abs_builddir "/.libs/virrandommock.so",
> + abs_builddir "/.libs/qemucpumock.so",
> + abs_builddir "/.libs/virpcimock.so")
>
> #else
>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index fff13e2..3c8dbc4 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -303,14 +303,28 @@ testInfoSet(struct testInfo *info,
> return -1;
> }
>
> +# define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XX"
>
> static int
> mymain(void)
> {
> int ret = 0;
> +char *fakerootdir;
> struct testInfo info;
> virQEMUDriverConfigPtr cfg = NULL;
>
> +if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) {
> +fprintf(stderr, "Out of memory\n");
> +abort();
> +}
> +
> +if (!mkdtemp(fakerootdir)) {
> +fprintf(stderr, "Cannot create fakerootdir");
> +abort();
> +}
> +
> +setenv("LIBVIRT_FAKE_ROOT_DIR", fakerootdir, 1);
> +
> memset(&info, 0, sizeof(info));
>
> if (qemuTestDriverInit(&driver) < 0)
> @@ -1137,12 +1151,18 @@ mymain(void)
> DO_
Re: [libvirt] [PATCH 02/26] conf: Make virDomainPCIAddressSetGrow() private
On 06/12/2017 05:20 AM, Andrea Bolognani wrote: > On Mon, 2017-06-12 at 08:35 +0200, Ján Tomko wrote: >>> Reviewed-by: Laine Stump >>> >>> (This *is* the new hot way to say ACK, right?) >> >> It is not a replacement (AFAIK only rebels like John and Pavel use it) >> and it is not an equivalent (with Reviewed-by, I assume the reviewer >> wants me to make it a part of the commit history). > > Both ACK and R-B are perfectly acceptable ways to signal the > submitter you consider their code suitable for merging. As a > project we don't currently mandate or prefer either form, so > feel free to use the one you like best :) Yeah, I noticed the discussion awhile back and then noticed that some people had started using it, so wasn't sure if I'd missed some memo about "phasing it in" or something. Personally, I like the ease/brevity of "ACK", and as for having a Reviewed-by as part of the commit history, I have two comments: 1) I don't care much about having *my* reviews documented in the history, but it's sometimes useful to know who the reviewer was when it's someone else (of course others will say the same for patches that *I* review, so... :-) So I'm conflicted. It seems like a good idea but takes more time. Too bad there's not some commit hook that could search the email archives for the message with the ACK for a patch and add a Reviewed-by to the commit automatically... -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] bhyve: tests: add vnc test to bhyvexml2xmltest
John Ferlan wrote: > > > On 05/09/2017 07:52 AM, Roman Bogorodskiy wrote: > > Signed-off-by: Roman Bogorodskiy > > --- > > tests/bhyvexml2xmloutdata/bhyvexml2xmlout-vnc.xml | 41 > > +++ > > tests/bhyvexml2xmltest.c | 3 +- > > 2 files changed, 43 insertions(+), 1 deletion(-) > > create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-vnc.xml > > > > Looks reasonable... > > Reviewed-by: John Ferlan > > John Pushed, thanks! > Curious - should uefi and net-e1000 also get xml2xml tests seeing as the > xml2argv was added? Yeah, I'll add some more xml2xml tests when I finish with the thing. Roman Bogorodskiy signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: hotplug: Release address properly when redirected device attach failure
On 05/30/2017 07:22 AM, Shivaprasad G Bhat wrote: > The virDomainUSBAddressEnsure returns 0 or -1 and checking for 1 is wrong. > Fix it. > > Signed-off-by: Shivaprasad G Bhat > --- > src/qemu/qemu_hotplug.c |6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > Reviewed-by: John Ferlan FWIW: It seems commit id de325472 copied the logic from qemuDomainAttachChrDeviceAssignAddr... I altered the commit message slightly and pushed Tks, John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] bhyve: add support for video device configuration
John Ferlan wrote: > > > On 05/09/2017 07:54 AM, Roman Bogorodskiy wrote: > > Connect domain XML configuration (introduced in a > > previous patch) to bhyve command generation. > > > > Unfortunately, this option was documented just recently and at the time > > of writing official online manpages didn't have it updated, so for now > > one can refer to: > > > > https://github.com/freebsd/freebsd/blob/master/usr.sbin/bhyve/bhyve.8#L327 > > > > for the detailed description of the possible vga configuration options. > > > > Also, add some tests for this new feature. > > > > Signed-off-by: Roman Bogorodskiy > > --- > > src/bhyve/bhyve_command.c | 4 ++ > > .../bhyvexml2argv-vnc-vgaconf.args | 12 ++ > > .../bhyvexml2argv-vnc-vgaconf.ldargs | 1 + > > .../bhyvexml2argv-vnc-vgaconf.xml | 31 > > tests/bhyvexml2argvtest.c | 1 + > > .../bhyvexml2xmlout-vnc-vgaconf.xml| 43 > > ++ > > tests/bhyvexml2xmltest.c | 1 + > > 7 files changed, 93 insertions(+) > > create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.args > > create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.ldargs > > create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.xml > > create mode 100644 > > tests/bhyvexml2xmloutdata/bhyvexml2xmlout-vnc-vgaconf.xml > > > > As noted previously the xml2xml would be better served in the other > patch. The rest seems reasonable, but obviously is affected by the other > one... Still wanted to be sure to respond so that it's not left hanging > without a response. Good, I'll squash these two commits into a single one. Roman Bogorodskiy signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] conf: add video driver configuration element
John Ferlan wrote:
>
>
> On 05/09/2017 07:53 AM, Roman Bogorodskiy wrote:
> > Add support for video driver configuration. In domain xml it looks like
> > this:
> >
> >
> >
> >
> >
> >
> >
> > At present, the only supported configuration is 'vgaconf' that looks this
> > way:
> >
> >
> >
> > It was added with bhyve gop video in mind to allow users control how the
> > video device is exposed to the guest, specifically, how VGA I/O is
> > handled.
> >
> > Signed-off-by: Roman Bogorodskiy
> > ---
> > docs/schemas/domaincommon.rng | 13 +
> > src/conf/domain_conf.c| 63
> > +--
> > src/conf/domain_conf.h| 17
> > src/libvirt_private.syms | 2 ++
> > 4 files changed, 93 insertions(+), 2 deletions(-)
> >
>
> Due to languishing on list and given jtomko's changes in a similar
> place, this no longer 'git am -3' applies... Could you please rebase
> and resend? You may also need to rethink the subelement too as
> it seems jtomko's commit f5384fb4 added this as well.
Thanks, I'll rebase the change.
Also, looking at f5384fb4, it looks like I can still reuse the
subelement because it seems it goes in line with the vga-related
configuration I'm adding.
> The xml2xml test should also be added here w/ various options...
>
> There's no adjustment to the formatdomain.html.in to describe this -
> would it be strictly related to one specific ""
Yeah, I was planning to update the doc as a separate commit after this
one gets merged because I wasn't sure this schema change will go in as
is, so I don't have to re-roll the doc patches as well (yeah, I'm
lazy...)
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> > index 281309ec0..f45820383 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -3278,6 +3278,19 @@
> >
> >
> >
> > +
> > +
> > +
> > +
> > +
> > +io
> > +on
> > +off
> > +
> > +
> > +
>
> Seems strange to have an optional with one optional attribute
> "vgaconf".
>
> > +
> > +
> >
> >
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 0ff216e3a..2ab55b52f 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -553,6 +553,11 @@ VIR_ENUM_IMPL(virDomainVideo,
> > VIR_DOMAIN_VIDEO_TYPE_LAST,
> >"virtio",
> >"gop")
> >
> > +VIR_ENUM_IMPL(virDomainVideoVgaconf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
> > + "io",
> > + "on",
> > + "off")
> > +
>
> A "nit", but should it be "VGA" and not "Vga"?
Will fix this one.
> > VIR_ENUM_IMPL(virDomainInput, VIR_DOMAIN_INPUT_TYPE_LAST,
> >"mouse",
> >"tablet",
> > @@ -2280,6 +2285,7 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def)
> > virDomainDeviceInfoClear(&def->info);
> >
> > VIR_FREE(def->accel);
> > +VIR_FREE(def->driver);
> > VIR_FREE(def);
> > }
> >
> > @@ -13368,6 +13374,43 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
> > return def;
> > }
> >
> > +static virDomainVideoDriverDefPtr
> > +virDomainVideoDriverDefParseXML(xmlNodePtr node)
> > +{
> > +xmlNodePtr cur;
> > +virDomainVideoDriverDefPtr def;
> > +char *vgaconf = NULL;
> > +int val;
> > +
> > +cur = node->children;
> > +while (cur != NULL) {
> > +if (cur->type == XML_ELEMENT_NODE) {
> > +if (!vgaconf &&
> > +xmlStrEqual(cur->name, BAD_CAST "driver")) {
> > +vgaconf = virXMLPropString(cur, "vgaconf");
> > +}
> > +}
> > +cur = cur->next;
> > +}
> > +
> > +if (!vgaconf)
> > +return NULL;
> > +
> > +if (VIR_ALLOC(def) < 0)
> > +goto cleanup;
> > +
> > +if ((val = virDomainVideoVgaconfTypeFromString(vgaconf)) <= 0) {
>
> <= ?
Hm, 0 seems to correspond to the first element in the enum, so "<="
should be valid. I'll double check.
> How does VIR_DOMAIN_VIDEO_VGACONF_IO get set? Perhaps a specific xml2xml
> test...
>
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("unknown vgaconf value '%s'"), vgaconf);
> > +goto cleanup;
> > +}
> > +def->vgaconf = val;
> > +
> > + cleanup:
> > +VIR_FREE(vgaconf);
> > +return def;
> > +}
> > +
> > static virDomainVideoDefPtr
> > virDomainVideoDefParseXML(xmlNodePtr node,
> >const virDomainDef *dom,
> > @@ -13405,6 +13448,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
> > }
> >
> > def->accel = virDomainVideoAccelDefParseXML(cur);
> > +def->driver = virDomainVide
Re: [libvirt] [PATCH] bhyve: add support for video device configuration
On 05/09/2017 07:54 AM, Roman Bogorodskiy wrote:
> Connect domain XML configuration (introduced in a
> previous patch) to bhyve command generation.
>
> Unfortunately, this option was documented just recently and at the time
> of writing official online manpages didn't have it updated, so for now
> one can refer to:
>
> https://github.com/freebsd/freebsd/blob/master/usr.sbin/bhyve/bhyve.8#L327
>
> for the detailed description of the possible vga configuration options.
>
> Also, add some tests for this new feature.
>
> Signed-off-by: Roman Bogorodskiy
> ---
> src/bhyve/bhyve_command.c | 4 ++
> .../bhyvexml2argv-vnc-vgaconf.args | 12 ++
> .../bhyvexml2argv-vnc-vgaconf.ldargs | 1 +
> .../bhyvexml2argv-vnc-vgaconf.xml | 31
> tests/bhyvexml2argvtest.c | 1 +
> .../bhyvexml2xmlout-vnc-vgaconf.xml| 43
> ++
> tests/bhyvexml2xmltest.c | 1 +
> 7 files changed, 93 insertions(+)
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.args
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.ldargs
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.xml
> create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-vnc-vgaconf.xml
>
As noted previously the xml2xml would be better served in the other
patch. The rest seems reasonable, but obviously is affected by the other
one... Still wanted to be sure to respond so that it's not left hanging
without a response.
John
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index eae5cb3ca..f70e3bc60 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -408,6 +408,10 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def
> ATTRIBUTE_UNUSED,
> _("Unsupported listen type"));
> }
>
> +if (video->driver)
> +virBufferAsprintf(&opt, ",vga=%s",
> +
> virDomainVideoVgaconfTypeToString(video->driver->vgaconf));
> +
> virCommandAddArg(cmd, "-s");
> virCommandAddArgBuffer(cmd, &opt);
> return 0;
> diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.args
> b/tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.args
> new file mode 100644
> index 0..70347ee0b
> --- /dev/null
> +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.args
> @@ -0,0 +1,12 @@
> +/usr/sbin/bhyve \
> +-c 1 \
> +-m 214 \
> +-u \
> +-H \
> +-P \
> +-s 0:0,hostbridge \
> +-l bootrom,/path/to/test.fd \
> +-s 2:0,ahci,hd:/tmp/freebsd.img \
> +-s 3:0,virtio-net,faketapdev,mac=52:54:00:00:00:00 \
> +-s 4:0,fbuf,tcp=127.0.0.1:5904,vga=off \
> +-s 1,lpc bhyve
> diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.ldargs
> b/tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.ldargs
> new file mode 100644
> index 0..421376db9
> --- /dev/null
> +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.ldargs
> @@ -0,0 +1 @@
> +dummy
> diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.xml
> b/tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.xml
> new file mode 100644
> index 0..f1bcd1bde
> --- /dev/null
> +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-vnc-vgaconf.xml
> @@ -0,0 +1,31 @@
> +
> + bhyve
> + df3be7e7-a104-11e3-aeb0-50e5492bd3dc
> + 219136
> + 1
> +
> +hvm
> +/path/to/test.fd
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> + function='0x0'/>
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c
> index c8f8c685a..a369a447a 100644
> --- a/tests/bhyvexml2argvtest.c
> +++ b/tests/bhyvexml2argvtest.c
> @@ -193,6 +193,7 @@ mymain(void)
> DO_TEST("net-e1000");
> DO_TEST("uefi");
> DO_TEST("vnc");
> +DO_TEST("vnc-vgaconf");
>
> /* Address allocation tests */
> DO_TEST("addr-single-sata-disk");
> diff --git a/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-vnc-vgaconf.xml
> b/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-vnc-vgaconf.xml
> new file mode 100644
> index 0..6cc1aa088
> --- /dev/null
> +++ b/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-vnc-vgaconf.xml
> @@ -0,0 +1,43 @@
> +
> + bhyve
> + df3be7e7-a104-11e3-aeb0-50e5492bd3dc
> + 219136
> + 219136
> + 1
> +
> +hvm
> +/path/to/test.fd
> +
> +
> +
> + destroy
> + restart
> + destroy
> +
> +
> +
> +
> +
> +
> +
> +
> +
> + function='0x0'/>
> +
> +
> +
> +
> +
> + function='0x0'/>
> +
> +
> +
> +
> +
> +
> +
> +
> + function='0x0'/>
> +
> +
> +
> diff --git a/tests/bhyvexml2xmltest.c b/tests/bhyvexml2xmltest.c
> index b3759919e..70e4caeb3 100644
> --- a/tests/bhyvexml2xmltes
Re: [libvirt] [PATCH 1/2] conf: add video driver configuration element
On 05/09/2017 07:53 AM, Roman Bogorodskiy wrote:
> Add support for video driver configuration. In domain xml it looks like
> this:
>
>
>
>
>
>
>
> At present, the only supported configuration is 'vgaconf' that looks this way:
>
>
>
> It was added with bhyve gop video in mind to allow users control how the
> video device is exposed to the guest, specifically, how VGA I/O is
> handled.
>
> Signed-off-by: Roman Bogorodskiy
> ---
> docs/schemas/domaincommon.rng | 13 +
> src/conf/domain_conf.c| 63
> +--
> src/conf/domain_conf.h| 17
> src/libvirt_private.syms | 2 ++
> 4 files changed, 93 insertions(+), 2 deletions(-)
>
Due to languishing on list and given jtomko's changes in a similar
place, this no longer 'git am -3' applies... Could you please rebase
and resend? You may also need to rethink the subelement too as
it seems jtomko's commit f5384fb4 added this as well.
The xml2xml test should also be added here w/ various options...
There's no adjustment to the formatdomain.html.in to describe this -
would it be strictly related to one specific ""
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 281309ec0..f45820383 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3278,6 +3278,19 @@
>
>
>
> +
> +
> +
> +
> +
> +io
> +on
> +off
> +
> +
> +
Seems strange to have an optional with one optional attribute
"vgaconf".
> +
> +
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0ff216e3a..2ab55b52f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -553,6 +553,11 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
>"virtio",
>"gop")
>
> +VIR_ENUM_IMPL(virDomainVideoVgaconf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
> + "io",
> + "on",
> + "off")
> +
A "nit", but should it be "VGA" and not "Vga"?
> VIR_ENUM_IMPL(virDomainInput, VIR_DOMAIN_INPUT_TYPE_LAST,
>"mouse",
>"tablet",
> @@ -2280,6 +2285,7 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def)
> virDomainDeviceInfoClear(&def->info);
>
> VIR_FREE(def->accel);
> +VIR_FREE(def->driver);
> VIR_FREE(def);
> }
>
> @@ -13368,6 +13374,43 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
> return def;
> }
>
> +static virDomainVideoDriverDefPtr
> +virDomainVideoDriverDefParseXML(xmlNodePtr node)
> +{
> +xmlNodePtr cur;
> +virDomainVideoDriverDefPtr def;
> +char *vgaconf = NULL;
> +int val;
> +
> +cur = node->children;
> +while (cur != NULL) {
> +if (cur->type == XML_ELEMENT_NODE) {
> +if (!vgaconf &&
> +xmlStrEqual(cur->name, BAD_CAST "driver")) {
> +vgaconf = virXMLPropString(cur, "vgaconf");
> +}
> +}
> +cur = cur->next;
> +}
> +
> +if (!vgaconf)
> +return NULL;
> +
> +if (VIR_ALLOC(def) < 0)
> +goto cleanup;
> +
> +if ((val = virDomainVideoVgaconfTypeFromString(vgaconf)) <= 0) {
<= ?
How does VIR_DOMAIN_VIDEO_VGACONF_IO get set? Perhaps a specific xml2xml
test...
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown vgaconf value '%s'"), vgaconf);
> +goto cleanup;
> +}
> +def->vgaconf = val;
> +
> + cleanup:
> +VIR_FREE(vgaconf);
> +return def;
> +}
> +
> static virDomainVideoDefPtr
> virDomainVideoDefParseXML(xmlNodePtr node,
>const virDomainDef *dom,
> @@ -13405,6 +13448,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
> }
>
> def->accel = virDomainVideoAccelDefParseXML(cur);
> +def->driver = virDomainVideoDriverDefParseXML(cur);
> }
> }
> cur = cur->next;
> @@ -22998,6 +23042,18 @@ virDomainVideoAccelDefFormat(virBufferPtr buf,
> virBufferAddLit(buf, "/>\n");
> }
>
> +static void
> +virDomainVideoDriverDefFormat(virBufferPtr buf,
> + virDomainVideoDriverDefPtr def)
> +{
> +virBufferAddLit(buf, " +if (def->vgaconf) {
> +virBufferAsprintf(buf, " vgaconf='%s'",
> + virDomainVideoVgaconfTypeToString(def->vgaconf));
> +}
Without def->driver, the def->vgaconf doesn't exist, true?
If !vgaconf, then all you're getting is , which while
technically legal according to the RNG added here does look a little
strange. Are there plans to "enhance" this in the future? Always a bit
difficult to take that crystal ball out, but wouldn't that type of
e
Re: [libvirt] [PATCH 1/3] bhyve: tests: add vnc test to bhyvexml2xmltest
On 05/09/2017 07:52 AM, Roman Bogorodskiy wrote: > Signed-off-by: Roman Bogorodskiy > --- > tests/bhyvexml2xmloutdata/bhyvexml2xmlout-vnc.xml | 41 > +++ > tests/bhyvexml2xmltest.c | 3 +- > 2 files changed, 43 insertions(+), 1 deletion(-) > create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-vnc.xml > Looks reasonable... Reviewed-by: John Ferlan John Curious - should uefi and net-e1000 also get xml2xml tests seeing as the xml2argv was added? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/4] fix labeling for chardev source path
On Mon, May 29, 2017 at 04:31:46PM +0200, Pavel Hrdina wrote: > Pavel Hrdina (4): > conf: move seclabel for chardev source to the correct sturcture > qemu: introduce chardevStdioLogd to qemu private data > qemu: propagate chardevStdioLogd to qemuBuildChrChardevStr > security: don't relabel chardev source if virtlogd is used as stdio > handler Ping signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 0/6] Fix error reporting in streams
On 06/05/2017 10:22 AM, Michal Privoznik wrote: > diff to v3: > > -In the last patch, instead of reporting errors from callbacks, report system > error based on errno set by the callbacks. > > Michal Privoznik (6): > virfdstream: Check for thread error more frequently > fdstream: Report error from the I/O thread > virStream*All: Call virStreamAbort() more frequently > virStream*All: Preserve reported error > virFileInData: preserve errno in cleanup path > virStream*All: Report error if a callback fails > > daemon/stream.c | 18 ++--- > include/libvirt/libvirt-stream.h | 17 +++- > src/libvirt-stream.c | 83 > +++- > src/util/virfdstream.c | 22 --- > src/util/virfile.c | 5 ++- > 5 files changed, 113 insertions(+), 32 deletions(-) > Ping? I'd like to backport these and couple of other stream fixes that I've pushed earlier onto a stable branch since current release has broken streams. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: rename qemuGetProcessInfo to virProcessGetStat
On 05/23/2017 11:31 PM, Wang King wrote:
> qemuGetProcessInfo is more likely a process utility function, just rename it
> to virProcessGetStat and move it to virprocess.c source file.
> ---
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 83
> ++--
> src/util/virprocess.c| 62
> src/util/virprocess.h| 4 +++
> 4 files changed, 83 insertions(+), 67 deletions(-)
>
You should follow the guidelines about a cover letter when sending more
than one patch (http://libvirt.org/hacking.html). It would have
especially helped explain why you're doing this...
Beyond that there's quite a bit of linux specific stuff in both of these
calls and moves from the "/proc" file system to the "sysconf" call in
this patch and the comments in the second one related to CONFIG_SCHED*.
In other virprocess.c functions you'll generally note there are two
functions - one to handle linux and the other to return an error if run
on a non linux system (forcing someone to add some code in order to make
this work there).
Not sure I see the need to move and rename these, but you can try again
with a more complete set of patches.
... note one more comment below...
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d361454..3681869 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2382,6 +2382,7 @@ virProcessGetMaxMemLock;
> virProcessGetNamespaces;
> virProcessGetPids;
> virProcessGetStartTime;
> +virProcessGetStat;
> virProcessKill;
> virProcessKillPainfully;
> virProcessNamespaceAvailable;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6c79d4f..a4aa5da 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1378,68 +1378,6 @@ qemuGetSchedInfo(unsigned long long *cpuWait,
> return ret;
> }
>
> -
> -static int
> -qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
> - pid_t pid, int tid)
> -{
> -char *proc;
> -FILE *pidinfo;
> -unsigned long long usertime = 0, systime = 0;
> -long rss = 0;
> -int cpu = 0;
> -int ret;
> -
> -/* In general, we cannot assume pid_t fits in int; but /proc parsing
> - * is specific to Linux where int works fine. */
> -if (tid)
> -ret = virAsprintf(&proc, "/proc/%d/task/%d/stat", (int) pid, tid);
> -else
> -ret = virAsprintf(&proc, "/proc/%d/stat", (int) pid);
> -if (ret < 0)
> -return -1;
> -
> -pidinfo = fopen(proc, "r");
> -VIR_FREE(proc);
> -
> -/* See 'man proc' for information about what all these fields are. We're
> - * only interested in a very few of them */
> -if (!pidinfo ||
> -fscanf(pidinfo,
> - /* pid -> stime */
> - "%*d (%*[^)]) %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u
> %llu %llu"
> - /* cutime -> endcode */
> - "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
> - /* startstack -> processor */
> - "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
> - &usertime, &systime, &rss, &cpu) != 4) {
> -VIR_WARN("cannot parse process status data");
> -}
> -
> -/* We got jiffies
> - * We want nanoseconds
> - * _SC_CLK_TCK is jiffies per second
> - * So calculate thus
> - */
> -if (cpuTime)
> -*cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime)
> -/ (unsigned long long)sysconf(_SC_CLK_TCK);
> -if (lastCpu)
> -*lastCpu = cpu;
> -
> -if (vm_rss)
> -*vm_rss = rss * virGetSystemPageSizeKB();
> -
> -
> -VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld",
> - (int) pid, tid, usertime, systime, cpu, rss);
> -
> -VIR_FORCE_FCLOSE(pidinfo);
> -
> -return 0;
> -}
> -
> -
> static int
> qemuDomainHelperGetVcpus(virDomainObjPtr vm,
> virVcpuInfoPtr info,
> @@ -1482,9 +1420,9 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
> vcpuinfo->number = i;
> vcpuinfo->state = VIR_VCPU_RUNNING;
>
> -if (qemuGetProcessInfo(&vcpuinfo->cpuTime,
> - &vcpuinfo->cpu, NULL,
> - vm->pid, vcpupid) < 0) {
> +if (virProcessGetStat(vm->pid, vcpupid,
> + &vcpuinfo->cpuTime,
> + &vcpuinfo->cpu, NULL) < 0) {
> virReportSystemError(errno, "%s",
> _("cannot get vCPU placement & pCPU
> time"));
> return -1;
> @@ -2641,7 +2579,7 @@ qemuDomainGetInfo(virDomainPtr dom,
> }
>
> if (virDomainObjIsActive(vm)) {
> -if (qemuGetProcessInfo(&(info->cpuTime), NULL, NULL, vm->pid, 0) <
> 0) {
> +if (virProcessGetStat(vm->pid, 0, &(info->cpuTime), NULL, NULL) < 0)
>
Re: [libvirt] [PATCH] qemu: handle missing bind host/service on chardev hotplug
On 05/19/2017 07:43 AM, Ján Tomko wrote:
> On domain startup, bind host or bind service can be omitted
> and we will format a working command line.
>
> Extend this to hotplug as well and specify the service to QEMU
> even if the host is missing.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1452441
> ---
> src/qemu/qemu_monitor_json.c | 13 ++---
> tests/qemumonitorjsontest.c | 11 +++
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 0837290..ca9bb14 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -6429,6 +6429,8 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> virJSONValuePtr data = NULL;
> virJSONValuePtr addr = NULL;
> const char *backend_type = NULL;
> +const char *host;
> +const char *port;
> char *tlsalias = NULL;
> bool telnet;
>
> @@ -6492,9 +6494,14 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> virJSONValueObjectAppend(data, "remote", addr) < 0)
> goto error;
>
> -if (chr->data.udp.bindHost) {
> -addr =
> qemuMonitorJSONBuildInetSocketAddress(chr->data.udp.bindHost,
> -
> chr->data.udp.bindService);
> +host = chr->data.udp.bindHost;
> +port = chr->data.udp.bindService;
> +if (host || port) {
> +if (!host)
> +host = "";
> +if (!port)
> +port = "";
I believe port should be "0" instead of ""
Also, it seems connectHost could be NULL too, how does that affect the
qemuMonitorJSONBuildInetSocketAddress a few lines earlier?
FWIW: Questions are from reading the qemuBuildChrChardevStr /
qemuBuildChrArgStr code and the libxl_conf.c implementation...
ACK, if you fix or shall I say...
Reviewed-by: John Ferlan
and IDC if that's added to the committed code (it's my attempt to
conform to something new, AKA teach an old dog new tricks).
John
> +addr = qemuMonitorJSONBuildInetSocketAddress(host, port);
> if (!addr ||
> virJSONValueObjectAppend(data, "local", addr) < 0)
> goto error;
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index e9f9d47..3de901c 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -895,6 +895,17 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOptionPtr
> xmlopt)
> "'data':{'host':'localhost',"
> "'port':'4321'}");
>
> +chr.data.udp.bindHost = NULL;
> +chr.data.udp.bindService = (char *) "4321";
> +CHECK("udp", false,
> + "{'id':'alias',"
> + "'backend':{'type':'udp',"
> + "'data':{'remote':{'type':'inet',"
> +"'data':{'host':'example.com',"
> +"'port':'1234'}},"
> + "'local':{'type':'inet',"
> + "'data':{'host':'',"
> + "'port':'4321'}");
> memset(&chr, 0, sizeof(chr));
> chr.type = VIR_DOMAIN_CHR_TYPE_UNIX;
> chr.data.nix.path = (char *) "/path/to/socket";
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Explain why mdevs are assumed to be PCI Express
On Mon, Jun 12, 2017 at 06:00:13PM +0800, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/qemu/qemu_domain_address.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c > index 2106b34..b5b863f 100644 > --- a/src/qemu/qemu_domain_address.c > +++ b/src/qemu/qemu_domain_address.c > @@ -645,6 +645,11 @@ > qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, > return pcieFlags; > } > > +/* mdevs don't have corresponding files in /sys that we can poke to > + * try and figure out whether they are legacy PCI or PCI Express, so > + * the logic below would never work; instead, we just go ahead and > + * assume they're PCI Express. This is a very reasonable assumption, > + * as all current mdev-capable devices are indeed PCI Express */ > if (hostdev->source.subsys.type == > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) > return pcieFlags; > > -- > 2.7.5 > ACK Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: skip only ', ' for VNC and Spice unix socket
On 06/12/2017 05:51 AM, Pavel Hrdina wrote: > Commit 824272cb28d attempted to fix escaping of characters in unix > socket path but it was wrong. We need to escape only ',', there is > no escape character for '='. > You could have mentioned the bz again like the original commit did... > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_command.c | 4 ++-- > tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Revert "util: virqemu: introduce virQEMUBuildBufferEscape"
On 06/12/2017 05:51 AM, Pavel Hrdina wrote: > This reverts commit 22b02f44920388534d3da65cc6f0a70714dcf075. > --- > src/libvirt_private.syms | 1 - > src/util/virqemu.c | 17 - > src/util/virqemu.h | 1 - > 3 files changed, 19 deletions(-) > me wonders if anyone will ever find/use virBufferEscapeN from 726403461b Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] cpu_ppc64: Add support for host-model on POWER9
On Thu, 2017-05-18 at 14:40 +0200, Jiri Denemark wrote: > Signed-off-by: Jiri Denemark > --- > src/cpu/cpu_ppc64.c| 8 > .../qemuxml2argv-pseries-cpu-compat-power9.args| 24 >++ > .../qemuxml2argv-pseries-cpu-compat-power9.xml | 21 +++ > tests/qemuxml2argvtest.c | 7 +++ > tests/testutilsqemu.c | 13 +++- > tests/testutilsqemu.h | 1 + > 6 files changed, 69 insertions(+), 5 deletions(-) > create mode 100644 >tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-compat-power9.args > create mode 100644 >tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-compat-power9.xml Since all my concerns turned out to be just due to temporary issues in QEMU, we can go ahead and merge this. Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] fix VNC and Spice unix socket escaping
On Mon, 2017-06-12 at 11:51 +0200, Pavel Hrdina wrote: > Pavel Hrdina (2): > qemu: skip only ',' for VNC and Spice unix socket > Revert "util: virqemu: introduce virQEMUBuildBufferEscape" > > src/libvirt_private.syms | 1 - > src/qemu/qemu_command.c | 4 ++-- > src/util/virqemu.c | 17 - > src/util/virqemu.h | 1 - > tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 4 ++-- > 5 files changed, 4 insertions(+), 23 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Explain why mdevs are assumed to be PCI Express
Signed-off-by: Andrea Bolognani --- src/qemu/qemu_domain_address.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 2106b34..b5b863f 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -645,6 +645,11 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, return pcieFlags; } +/* mdevs don't have corresponding files in /sys that we can poke to + * try and figure out whether they are legacy PCI or PCI Express, so + * the logic below would never work; instead, we just go ahead and + * assume they're PCI Express. This is a very reasonable assumption, + * as all current mdev-capable devices are indeed PCI Express */ if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) return pcieFlags; -- 2.7.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] Revert "util: virqemu: introduce virQEMUBuildBufferEscape"
This reverts commit 22b02f44920388534d3da65cc6f0a70714dcf075.
---
src/libvirt_private.syms | 1 -
src/util/virqemu.c | 17 -
src/util/virqemu.h | 1 -
3 files changed, 19 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b6c828fc22..044510f091 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2422,7 +2422,6 @@ virProcessWait;
# util/virqemu.h
-virQEMUBuildBufferEscape;
virQEMUBuildBufferEscapeComma;
virQEMUBuildCommandLineJSON;
virQEMUBuildCommandLineJSONArrayBitmap;
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index f10b356781..2e9e65f9ef 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -300,23 +300,6 @@ virQEMUBuildBufferEscapeComma(virBufferPtr buf, const char
*str)
/**
- * virQEMUBuildBufferEscape:
- * @buf: buffer to append the escaped string
- * @str: the string to escape
- *
- * Some characters passed as values on the QEMU command line must be escaped.
- *
- * - ',' must by escaped by ','
- * - '=' must by escaped by '\'
- */
-void
-virQEMUBuildBufferEscape(virBufferPtr buf, const char *str)
-{
-virBufferEscapeN(buf, "%s", str, ',', ",", '\\', "=", NULL);
-}
-
-
-/**
* virQEMUBuildLuksOpts:
* @buf: buffer to build the string into
* @enc: pointer to encryption info
diff --git a/src/util/virqemu.h b/src/util/virqemu.h
index 10aeb67f4e..539d62ab14 100644
--- a/src/util/virqemu.h
+++ b/src/util/virqemu.h
@@ -50,7 +50,6 @@ char *virQEMUBuildObjectCommandlineFromJSON(const char *type,
char *virQEMUBuildDriveCommandlineFromJSON(virJSONValuePtr src);
void virQEMUBuildBufferEscapeComma(virBufferPtr buf, const char *str);
-void virQEMUBuildBufferEscape(virBufferPtr buf, const char *str);
void virQEMUBuildLuksOpts(virBufferPtr buf,
virStorageEncryptionInfoDefPtr enc,
const char *alias)
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] qemu: skip only ', ' for VNC and Spice unix socket
Commit 824272cb28d attempted to fix escaping of characters in unix
socket path but it was wrong. We need to escape only ',', there is
no escape character for '='.
Signed-off-by: Pavel Hrdina
---
src/qemu/qemu_command.c | 4 ++--
tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0f87809177..df85798ac3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7927,7 +7927,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr
cfg,
switch (glisten->type) {
case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
virBufferAddLit(&opt, "unix:");
-virQEMUBuildBufferEscape(&opt, glisten->socket);
+virQEMUBuildBufferEscapeComma(&opt, glisten->socket);
break;
case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
@@ -8060,7 +8060,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr
cfg,
}
virBufferAddLit(&opt, "unix,addr=");
-virQEMUBuildBufferEscape(&opt, glisten->socket);
+virQEMUBuildBufferEscapeComma(&opt, glisten->socket);
virBufferAddLit(&opt, ",");
hasInsecure = true;
break;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args
b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args
index ea13654bf1..d94ab76312 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args
@@ -20,7 +20,7 @@ bar=2/monitor.sock,server,nowait \
-no-acpi \
-boot c \
-usb \
--vnc 'unix:/tmp/lib/domain--1-foo\=1,,bar\=2/vnc.sock' \
--spice 'unix,addr=/tmp/lib/domain--1-foo\=1,,bar\=2/spice.sock' \
+-vnc unix:/tmp/lib/domain--1-foo=1,,bar=2/vnc.sock \
+-spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock \
-vga cirrus \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] fix VNC and Spice unix socket escaping
Pavel Hrdina (2): qemu: skip only ',' for VNC and Spice unix socket Revert "util: virqemu: introduce virQEMUBuildBufferEscape" src/libvirt_private.syms | 1 - src/qemu/qemu_command.c | 4 ++-- src/util/virqemu.c | 17 - src/util/virqemu.h | 1 - tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 4 ++-- 5 files changed, 4 insertions(+), 23 deletions(-) -- 2.13.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RFC 1/2] Resctrl: Add new xml element to support cache tune
This patch adds new xml element to support cache tune as:
...
...
id: any non-minus number
cache_id: reference of the host's cache banks id, it's from capabilities
level: cache level
type: cache bank type
size: should be multiples of the min_size of the bank on host.
vcpus: cache allocation on vcpu set, if empty, will apply the allocation
on all vcpus
Signed-off-by: Eli Qiao
---
docs/schemas/domaincommon.rng | 54 +
src/conf/domain_conf.c| 131 ++
src/conf/domain_conf.h| 21 +++
3 files changed, 206 insertions(+)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4950ddc..11dbb55 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -834,6 +834,35 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
@@ -5686,6 +5715,31 @@
-1
+
+
+ [0-9]+
+
+
+
+
+ [0-9]+
+
+
+
+
+ (3)
+
+
+
+
+ (both|code|data)
+
+
+
+
+ KiB
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5c32f93..93984bc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16250,6 +16250,104 @@ virDomainVcpuPinDefParseXML(virDomainDefPtr def,
return ret;
}
+/* Parse the XML definition for cachetune
+ * and a cachetune has the form
+ *
+ */
+static int
+virDomainCacheTuneDefParseXML(virDomainDefPtr def,
+ int n,
+ xmlNodePtr* nodes)
+{
+char* tmp = NULL;
+size_t i;
+int type = -1;
+virDomainCacheBankPtr bank = NULL;
+
+if (VIR_ALLOC_N(bank, n) < 0)
+goto cleanup;
+
+for (i = 0; i < n; i++) {
+if (!(tmp = virXMLPropString(nodes[i], "id"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing id in cache
tune"));
+goto cleanup;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].id)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("invalid setting for cache id '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+if (!(tmp = virXMLPropString(nodes[i], "cache_id"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cache id in
cache tune"));
+goto cleanup;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].cache_id)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("invalid setting for cache host id '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+if (!(tmp = virXMLPropString(nodes[i], "level"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cache level in
cache tune"));
+goto cleanup;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].level)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("invalid setting for cache level '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+
+if (!(tmp = virXMLPropString(nodes[i], "size"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing size in cache
tune"));
+goto cleanup;
+}
+if (virStrToLong_ull(tmp, NULL, 10, &(bank[i].size)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("invalid setting for cache size '%s'"), tmp);
+goto cleanup;
+}
+/* convert to B */
+bank[i].size *= 1024;
+VIR_FREE(tmp);
+
+if (!(tmp = virXMLPropString(nodes[i], "type"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing cache type"));
+goto cleanup;
+}
+if ((type = virCacheTypeFromString(tmp)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+_("'unsupported cache type '%s'"), tmp);
+goto cleanup;
+}
+
+if ((tmp = virXMLPropString(nodes[i], "vcpus"))) {
+if (virBitmapParse(tmp, &bank[i].vcpus,
+VIR_DOMAIN_CPUMASK_LEN) < 0)
+goto cleanup;
+
+if (virBitmapIsAllClear(bank[i].vcpus)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+_("Invalid value of 'vcpus': %s"), tmp);
+goto cleanup;
+}
+}
+}
+
+def->cachetune.cache_banks = bank;
+def->cachetune.n_banks = n;
+r
[libvirt] [PATCH RFC 0/2] Implement l3 CAT
This is a RFC patch to implement l3 CAT. There's old RFC V3 [1] patch, but don't get any attentions, reason maybe that it's is not a workable one. I address some of the comments for RFC V2 version from Martin. 1. Add file lock while access /sys/fs/resctrl base on kernel documents [3]. 2. Variable renaming. 3. Tested locally to create vm with cachetune defined and works well. Issues want to get some comments: I can not pass syntax-check as I reference the cache tune and cachebank definition (from conf/domain_conf.h). Since I need a complex struct to pass cachetune and host's cachetune information to resctrl. I can split patch if it's hard to review. Sorry for inconvient. Patch was pushed to github [4] for easing read. [1] https://www.redhat.com/archives/libvir-list/2017-June/msg00069.html [2] https://www.redhat.com/archives/libvir-list/2017-April/msg01466.html [3] https://www.kernel.org/doc/Documentation/x86/intel_rdt_ui.txt [4] https://github.com/taget/libvirt/commits/cache Eli Qiao (2): Resctrl: Add new xml element to support cache tune Resctrl: Add uitls functions to operate sysfs resctrl docs/schemas/domaincommon.rng | 54 ++ include/libvirt/virterror.h | 1 + src/Makefile.am | 1 + src/conf/domain_conf.c| 131 + src/conf/domain_conf.h| 21 + src/libvirt_private.syms | 9 + src/qemu/qemu_process.c | 54 ++ src/util/virerror.c | 1 + src/util/virresctrl.c | 851 ++ src/util/virresctrl.h | 79 +++ tests/Makefile.am | 8 +- tests/virresctrldata/L3-free.schemata | 1 + tests/virresctrldata/L3CODE-free.schemata | 1 + tests/virresctrldata/L3DATA-free.schemata | 1 + tests/virresctrldata/linux-resctrl| 1 + tests/virresctrldata/linux-resctrl-cdp| 1 + tests/virresctrltest.c| 119 + 17 files changed, 1333 insertions(+), 1 deletion(-) create mode 100644 src/util/virresctrl.c create mode 100644 src/util/virresctrl.h create mode 100644 tests/virresctrldata/L3-free.schemata create mode 100644 tests/virresctrldata/L3CODE-free.schemata create mode 100644 tests/virresctrldata/L3DATA-free.schemata create mode 12 tests/virresctrldata/linux-resctrl create mode 12 tests/virresctrldata/linux-resctrl-cdp create mode 100644 tests/virresctrltest.c -- 1.9.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RFC 2/2] Resctrl: Add uitls functions to operate sysfs resctrl
This patch adds 3 major private interface.
virResctrlGetFreeCache: return free cache, default cache substract cache
allocated.
virResctrlSetCachetunes: set cache banks which defined in a domain.
virResctrlRemoveCachetunes: remove cache allocation group from the
host.
There's some existed issue when do syntax-check as I reference the cache
tune and cachebank definition (from conf/domain_conf.h) in
util/virresctrl.h.
---
include/libvirt/virterror.h | 1 +
src/Makefile.am | 1 +
src/libvirt_private.syms | 9 +
src/qemu/qemu_process.c | 54 ++
src/util/virerror.c | 1 +
src/util/virresctrl.c | 851 ++
src/util/virresctrl.h | 79 +++
tests/Makefile.am | 8 +-
tests/virresctrldata/L3-free.schemata | 1 +
tests/virresctrldata/L3CODE-free.schemata | 1 +
tests/virresctrldata/L3DATA-free.schemata | 1 +
tests/virresctrldata/linux-resctrl| 1 +
tests/virresctrldata/linux-resctrl-cdp| 1 +
tests/virresctrltest.c| 119 +
14 files changed, 1127 insertions(+), 1 deletion(-)
create mode 100644 src/util/virresctrl.c
create mode 100644 src/util/virresctrl.h
create mode 100644 tests/virresctrldata/L3-free.schemata
create mode 100644 tests/virresctrldata/L3CODE-free.schemata
create mode 100644 tests/virresctrldata/L3DATA-free.schemata
create mode 12 tests/virresctrldata/linux-resctrl
create mode 12 tests/virresctrldata/linux-resctrl-cdp
create mode 100644 tests/virresctrltest.c
diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 2efee8f..4bc0c74 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -132,6 +132,7 @@ typedef enum {
VIR_FROM_PERF = 65, /* Error from perf */
VIR_FROM_LIBSSH = 66, /* Error from libssh connection transport */
+VIR_FROM_RESCTRL = 67, /* Error from resctrl */
# ifdef VIR_ENUM_SENTINELS
VIR_ERR_DOMAIN_LAST
diff --git a/src/Makefile.am b/src/Makefile.am
index eae32dc..8dbb778 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -167,6 +167,7 @@ UTIL_SOURCES =
\
util/virprocess.c util/virprocess.h \
util/virqemu.c util/virqemu.h \
util/virrandom.h util/virrandom.c \
+ util/virresctrl.h util/virresctrl.c \
util/virrotatingfile.h util/virrotatingfile.c \
util/virscsi.c util/virscsi.h \
util/virscsihost.c util/virscsihost.h \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b6c828f..7392cfa 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2440,6 +2440,15 @@ virRandomGenerateWWN;
virRandomInt;
+# util/virresctrl.h
+virResctrlBitmap2String;
+virResctrlFreeSchemata;
+virResctrlGetFreeCache;
+virResctrlRemoveCachetunes;
+virResctrlSetCachetunes;
+virResctrlTypeToString;
+
+
# util/virrotatingfile.h
virRotatingFileReaderConsume;
virRotatingFileReaderFree;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4a66f0d..8efeb19 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -70,6 +70,7 @@
#include "virbitmap.h"
#include "viratomic.h"
#include "virnuma.h"
+#include "virresctrl.h"
#include "virstring.h"
#include "virhostdev.h"
#include "secret_util.h"
@@ -5088,6 +5089,51 @@ qemuProcessSetupVcpus(virDomainObjPtr vm)
return 0;
}
+static int
+qemuProcessSetCacheBanks(virCapsHostPtr caps, virDomainObjPtr vm)
+{
+size_t i, j;
+virDomainCachetunePtr cachetune;
+unsigned int max_vcpus = virDomainDefGetVcpusMax(vm->def);
+pid_t *pids = NULL;
+virDomainVcpuDefPtr vcpu;
+size_t npid = 0;
+size_t count = 0;
+int ret = -1;
+
+cachetune = &(vm->def->cachetune);
+
+for (i = 0; i < cachetune->n_banks; i++) {
+if (cachetune->cache_banks[i].vcpus) {
+for (j = 0; j < max_vcpus; j++) {
+if (virBitmapIsBitSet(cachetune->cache_banks[i].vcpus, j)) {
+
+vcpu = virDomainDefGetVcpu(vm->def, j);
+if (!vcpu->online)
+continue;
+
+if (VIR_RESIZE_N(pids, npid, count, 1) < 0)
+goto cleanup;
+pids[count ++] = qemuDomainGetVcpuPid(vm, j);
+}
+}
+}
+}
+
+/* If not specify vcpus in cachetune, add vm->pid */
+if (pids == NULL) {
+if (VIR_ALLOC_N(pids, 1) < 0)
+goto cleanup;
+pids[0] = vm->pid;
+count = 1;
+}
+ret = virResctrlSetCachetunes(caps, cachetune, vm->def->uuid, pids, count);
+
+ cleanup:
+
Re: [libvirt] [PATCH] cpu_ppc64: Add support for host-model on POWER9
On Mon, Jun 12, 2017 at 05:06:27PM +0800, Andrea Bolognani wrote: > On Thu, 2017-06-08 at 18:40 +0200, Andrea Bolognani wrote: > > Yeah, I've been looking at downstream. I'll try again with > > upstream and file bugs as needed. > > Okay, I've tried again with upstream QEMU master (with your > CPU compatibility patches applied on top), here are the > results for both POWER8 and POWER9. Note that this is for POWER9 DD1 - different results are expected for POWER9 DD2, when it actually gets out of the fab. > > = > > * POWER8 (architected) > * POWER9 (raw) As expected. > > -cpu POWER8 > === > > * POWER8 (architected) > * qemu-system-ppc64: Register sync failed... > If you're using kvm-hv.ko, only "-cpu host" is possible > kvm_init_vcpu failed: Invalid argument As expected. > > -cpu POWER9 > === > > * qemu-system-ppc64: Register sync failed... > If you're using kvm-hv.ko, only "-cpu host" is possible > kvm_init_vcpu failed: Invalid argument As expected. > * qemu-system-ppc64: Register sync failed... > If you're using kvm-hv.ko, only "-cpu host" is possible > kvm_init_vcpu failed: Invalid argument Huh.. that's a bit weird. > > -cpu host > = > > * POWER8 (architected) > * POWER9 (raw) As expected (no parameters defaults to this). > > -cpu host,compat=power8 > === > > * POWER8 (architected) > * Unexpected error in ppc_set_compat() at target/ppc/compat.c:135: > qemu-system-ppc64: Compatibility PVR 0x0f04 not valid for CPU > Aborted Expected for DD1. > > -cpu host,compat=power9 > === > > * Unexpected error in ppc_set_compat() at target/ppc/compat.c:135: > qemu-system-ppc64: Compatibility PVR 0x0f05 not valid for CPU > Aborted Expected. > * Unexpected error in ppc_set_compat() at target/ppc/compat.c:135: > qemu-system-ppc64: Compatibility PVR 0x0f05 not valid for CPU > Aborted Expected for DD1. > AIUI, the fact that POWER9 is using raw mode rather than > architected mode for all cases where the guest actually > manages to boot is because compatibility modes are not enabled > for early silicon; this is consistent with QEMU refusing to > use the compatibility modes when explicitly asked to do so. Right. The compatibility modes are "disabled" in qemu, not in the silicon for DD1. The compatibility control registers are still there in DD1, and will have some effect. It's just that turning on compat mode won't make the DD1 bugs go away, so we're still not architecture compliant. Hence, we don't advertise them. > However, the fact that '-cpu POWER9' can't be used on the > POWER9 machine seems like a genuine bug. Even on POWER8, the > error message is pretty misleading. Yes, that does look like a bug. > Last but not least, using an unsupported compatibility mode > results in QEMU abort()ing, which seems a bit excessive for > a very reasonable guest configuration error. Ah, yes. Probably just an &error_abort where we should have an &error_fatal. > Do you want me to go ahead and file bugs for the last two > items? Sure, sonuds good. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainBlockCopyCommon: Fix the memory leak
On Fri, 2017-06-09 at 17:17 +0200, Andrea Bolognani wrote: > Looks reasonable. I'll push it on Monday unless someone raises > concerns in the meantime. Reworded the commit message and pushed. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/26] conf: Make virDomainPCIAddressSetGrow() private
On Mon, 2017-06-12 at 08:35 +0200, Ján Tomko wrote: > > Reviewed-by: Laine Stump > > > > (This *is* the new hot way to say ACK, right?) > > It is not a replacement (AFAIK only rebels like John and Pavel use it) > and it is not an equivalent (with Reviewed-by, I assume the reviewer > wants me to make it a part of the commit history). Both ACK and R-B are perfectly acceptable ways to signal the submitter you consider their code suitable for merging. As a project we don't currently mandate or prefer either form, so feel free to use the one you like best :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] cpu_ppc64: Add support for host-model on POWER9
On Thu, 2017-06-08 at 18:40 +0200, Andrea Bolognani wrote: > Yeah, I've been looking at downstream. I'll try again with > upstream and file bugs as needed. Okay, I've tried again with upstream QEMU master (with your CPU compatibility patches applied on top), here are the results for both POWER8 and POWER9. = * POWER8 (architected) * POWER9 (raw) -cpu POWER8 === * POWER8 (architected) * qemu-system-ppc64: Register sync failed... If you're using kvm-hv.ko, only "-cpu host" is possible kvm_init_vcpu failed: Invalid argument -cpu POWER9 === * qemu-system-ppc64: Register sync failed... If you're using kvm-hv.ko, only "-cpu host" is possible kvm_init_vcpu failed: Invalid argument * qemu-system-ppc64: Register sync failed... If you're using kvm-hv.ko, only "-cpu host" is possible kvm_init_vcpu failed: Invalid argument -cpu host = * POWER8 (architected) * POWER9 (raw) -cpu host,compat=power8 === * POWER8 (architected) * Unexpected error in ppc_set_compat() at target/ppc/compat.c:135: qemu-system-ppc64: Compatibility PVR 0x0f04 not valid for CPU Aborted -cpu host,compat=power9 === * Unexpected error in ppc_set_compat() at target/ppc/compat.c:135: qemu-system-ppc64: Compatibility PVR 0x0f05 not valid for CPU Aborted * Unexpected error in ppc_set_compat() at target/ppc/compat.c:135: qemu-system-ppc64: Compatibility PVR 0x0f05 not valid for CPU Aborted AIUI, the fact that POWER9 is using raw mode rather than architected mode for all cases where the guest actually manages to boot is because compatibility modes are not enabled for early silicon; this is consistent with QEMU refusing to use the compatibility modes when explicitly asked to do so. However, the fact that '-cpu POWER9' can't be used on the POWER9 machine seems like a genuine bug. Even on POWER8, the error message is pretty misleading. Last but not least, using an unsupported compatibility mode results in QEMU abort()ing, which seems a bit excessive for a very reasonable guest configuration error. Do you want me to go ahead and file bugs for the last two items? -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
