Re: [libvirt] [PATCH v2 RESEND 09/12] qemu: Generate and use zPCI device in QEMU command line
在 2018/7/24 下午11:09, Andrea Bolognani 写道:
On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
[...]
+bool
+qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
+{
+if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+((info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) ||
+ info->addr.pci.zpci))
+return true;
Missing curly braces.
Also, do you really need to check both the flags and the presence
of the zPCI address bits? It looks like either one or the other
should be enough or, if that's not the case, it should be made so
because having to check for two separate conditions makes me feel
like it would introduce bugs in the long run.
This is actually a problem. I add info->addr.pci.zpci check in order to
check
if the user specifies zpci address in xml even though it has no zpci
support.
The callers of this function checks zpci capability. If no zpci cap but
the user
specfies zpci address, report an error.
I will change the logic and remove the check on info->addr.pci.zpci.
[...]
+char *qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev);
+
+bool qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info);
Is this really necessary? Can't these two functions be static?
They are also used in qemu_hotplug.c file.
[...]
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1014,6 +1014,8 @@ mymain(void)
QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
DO_TEST("disk-virtio-scsi-ccw", QEMU_CAPS_VIRTIO_SCSI,
QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
+DO_TEST("disk-virtio-s390-zpci", QEMU_CAPS_DEVICE_ZPCI,
+QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
DO_TEST("disk-order",
QEMU_CAPS_DRIVE_BOOT, QEMU_CAPS_VIRTIO_BLK_SCSI);
DO_TEST("disk-virtio-drive-queues",
@@ -1574,6 +1576,18 @@ mymain(void)
QEMU_CAPS_DEVICE_VFIO_PCI);
DO_TEST_PARSE_ERROR("hostdev-mdev-invalid-target-address",
QEMU_CAPS_DEVICE_VFIO_PCI);
+DO_TEST("hostdev-vfio-zpci",
+QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI);
+DO_TEST("hostdev-vfio-zpci-multidomain-many",
+QEMU_CAPS_DEVICE_VFIO_PCI, X_QEMU_CAPS_HOST_PCI_MULTIDOMAIN,
Capabilities with X_QEMU prefix are no longer used, so you should
not list them here.
Sure.
+QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_ZPCI);
+DO_TEST("hostdev-vfio-zpci-autogenerate",
+QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI);
+DO_TEST("hostdev-vfio-zpci-boundaries",
+QEMU_CAPS_DEVICE_VFIO_PCI, X_QEMU_CAPS_HOST_PCI_MULTIDOMAIN,
+QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_ZPCI);
+DO_TEST_FAILURE("hostdev-vfio-zpci",
+QEMU_CAPS_DEVICE_VFIO_PCI);
Please add these to qemuxml2xmltest at the same time.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC] secdrivers remembering original labels
Dear list, we have this very old bug [1] that I just keep pushing in front of me. I made several attempts to fix it. However, none of them made into the tree. I guess it's time to have discussion what to do about it. IIRC, the algorithm that I implemented last was to keep original label in XATTRs (among with some ref counter) and the last one to restore the label will look there and find the original label. There was a problem with two libvirtds fighting over a file on shared FS. So I guess my question is can we come up with a design that would work? Or at least work to the extent that we're satisfied with? Personally, I like the XATTRs approach. And to resolve the NFS race we can invent yet another lockspace to guard labelling - I also have a bug for that [2] (although, I'm not that familiar with lockspaces). I guess doing disk metadata locking is not going to be trivial, is it? Michal 1: https://bugzilla.redhat.com/show_bug.cgi?id=547546 2: https://bugzilla.redhat.com/show_bug.cgi?id=1524792 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 00/39] qemu: Add support for -blockdev
On Thu, Jul 26, 2018 at 17:04:19 +0800, Fam Zheng wrote:
> On Thu, 07/26 10:44, Kevin Wolf wrote:
> > Am 25.07.2018 um 17:57 hat Peter Krempa geschrieben:
> > > This series adds support for starting and hotplug of disks with
> > > -blockdev/blockdev-add.
> > >
> > > Blockjobs are not supported and thus the last patch should not be
> > > applied yet as some refactoring of the jobs is required.
> > >
> > > At the beginning of the series there are a few cleanup patches which may
> > > be pushed even at this point.
> > >
> > > The main reason this is in RFC state is that block stats reporting does
> > > not work.
> > >
> > > The following command:
> > >
> > > {"execute":"query-blockstats","arguments":{"query-nodes":true}}
> >
> > query-nodes was added in commit f71eaa74c0b by Fam and Max, CCed. I'm
> > not sure what it was needed for at all and the commit message doesn't
> > help with that, but I suppose the addition was related to
> > wr_highest_offset (see below).
>
> Yes, this was part of RHBZ 1158094. Sorry about the poor commit message.
Well the problem is that when using -blockdev with 'query-nodes' false/not
present you get a
even more useless output:
virsh qemu-monitor-command --pretty upstream
'{"execute":"query-blockstats","arguments":{"query-nodes":false}}'
{
"return": [
],
"id": "libvirt-20"
}
'query-named-block-nodes' don't provide the 'stats' section either:
(other drives snipped)
{
"iops_rd": 0,
"detect_zeroes": "off",
"image": {
"virtual-size": 520032256,
"filename": "/var/lib/libvirt/images/systemrescuecd-x86-4.9.5.iso",
"format": "raw",
"actual-size": 520036352,
"dirty-flag": false
},
"iops_wr": 0,
"ro": true,
"node-name": "libvirt-7-format",
"backing_file_depth": 0,
"drv": "raw",
"iops": 0,
"bps_wr": 0,
"write_threshold": 0,
"encrypted": false,
"bps": 0,
"bps_rd": 0,
"cache": {
"no-flush": false,
"direct": false,
"writeback": true
},
"file": "/var/lib/libvirt/images/systemrescuecd-x86-4.9.5.iso",
"encryption_key_missing": false
},
{
"iops_rd": 0,
"detect_zeroes": "off",
"image": {
"virtual-size": 520032256,
"filename": "/var/lib/libvirt/images/systemrescuecd-x86-4.9.5.iso",
"format": "file",
"actual-size": 520036352,
"dirty-flag": false
},
"iops_wr": 0,
"ro": true,
"node-name": "libvirt-7-storage",
"backing_file_depth": 0,
"drv": "file",
"iops": 0,
"bps_wr": 0,
"write_threshold": 0,
"encrypted": false,
"bps": 0,
"bps_rd": 0,
"cache": {
"no-flush": false,
"direct": false,
"writeback": true
},
"file": "/var/lib/libvirt/images/systemrescuecd-x86-4.9.5.iso",
"encryption_key_missing": false
}
],
"id": "libvirt-19"
}
Thus libvirt can't provide the stats it used to with -drive. Since we
need to special-case the code for gathering stats for -blockdev it
should not be a problem if e.g. query-named-block-nodes reported the
stats section along. (perhaps with a boolean flag to do so which also
could disable the nesting)
In either case we need the stats in the same extent as we had before
with -drive and query-blockstats.
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/3] util: Rework virStringListAdd
On Thu, Jul 26, 2018 at 05:36:27PM +0200, Michal Privoznik wrote:
> So every caller does the same: they use virStringListAdd() to add
^This sounds a bit like there's a handful of them ;).
> new item into the list and then free the old copy to replace it
> with new list. It's not very memory effective, nor environmental
> friendly.
>
> Signed-off-by: Michal Privoznik
> ---
> src/util/virmacmap.c | 8 ++--
> src/util/virstring.c | 34 ++
> src/util/virstring.h | 4 ++--
> tests/virstringtest.c | 6 +-
> 4 files changed, 19 insertions(+), 33 deletions(-)
>
> diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
> index 88ca9b3f36..c7b700fa05 100644
> --- a/src/util/virmacmap.c
> +++ b/src/util/virmacmap.c
> @@ -90,7 +90,6 @@ virMacMapAddLocked(virMacMapPtr mgr,
> {
> int ret = -1;
> char **macsList = NULL;
> -char **newMacsList = NULL;
>
> if ((macsList = virHashLookup(mgr->macs, domain)) &&
> virStringListHasString((const char**) macsList, mac)) {
> @@ -98,15 +97,12 @@ virMacMapAddLocked(virMacMapPtr mgr,
> goto cleanup;
> }
>
> -if (!(newMacsList = virStringListAdd((const char **) macsList, mac)) ||
> -virHashUpdateEntry(mgr->macs, domain, newMacsList) < 0)
> +if (virStringListAdd(&macsList, mac) < 0 ||
> +virHashUpdateEntry(mgr->macs, domain, macsList) < 0)
> goto cleanup;
> -newMacsList = NULL;
> -virStringListFree(macsList);
>
> ret = 0;
> cleanup:
> -virStringListFree(newMacsList);
> return ret;
> }
>
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index 93fda69d7f..59f57a97b8 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -175,32 +175,26 @@ char *virStringListJoin(const char **strings,
> * @strings: a NULL-terminated array of strings
> * @item: string to add
> *
> - * Creates new strings list with all strings duplicated and @item
> - * at the end of the list. Callers is responsible for freeing
> - * both @strings and returned list.
> + * Appends @item into string list @strings. If *@strings is not
> + * allocated yet new string list is created.
> + *
> + * Returns: 0 on success,
> + * -1 otherwise
> */
> -char **
> -virStringListAdd(const char **strings,
> +int
> +virStringListAdd(char ***strings,
> const char *item)
> {
> -char **ret = NULL;
> -size_t i = virStringListLength(strings);
> +size_t i = virStringListLength((const char **) *strings);
>
> -if (VIR_ALLOC_N(ret, i + 2) < 0)
> -goto error;
This scales linearly, but given the number of "every" caller of this and the
projected frequency of usage, I don't really care about N sentinels.
You could consider VIR_EXPAND_N to get rid of the explicit sentinel assignment
below, your call.
> +if (VIR_REALLOC_N(*strings, i + 2) < 0)
> +return -1;
>
> -for (i = 0; strings && strings[i]; i++) {
> -if (VIR_STRDUP(ret[i], strings[i]) < 0)
> -goto error;
> -}
> +(*strings)[i + 1] = NULL;
> +if (VIR_STRDUP((*strings)[i], item) < 0)
> +return -1;
>
> -if (VIR_STRDUP(ret[i], item) < 0)
> -goto error;
> -
> -return ret;
> - error:
> -virStringListFree(ret);
> -return NULL;
> +return 0;
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] lxc: Use VIR_AUTOPTR for @veths in virLXCProcessStart
On Thu, Jul 26, 2018 at 05:36:29PM +0200, Michal Privoznik wrote: > Now that we have VIR_AUTOPTR and that @veths is a string list we > can use VIR_AUTOPTR to free it automagically. > > Signed-off-by: Michal Privoznik > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] lxc: Turn @veths into a string list in virLXCProcessStart
On Thu, Jul 26, 2018 at 05:36:28PM +0200, Michal Privoznik wrote:
> This way it will be easier to use autofree.
>
> Signed-off-by: Michal Privoznik
> ---
> src/lxc/lxc_process.c | 24 +---
> 1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> index d021a890f7..3ac39d598c 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -514,8 +514,7 @@ static int virLXCProcessSetupNamespaces(virConnectPtr
> conn,
> * virLXCProcessSetupInterfaces:
> * @conn: pointer to connection
> * @def: pointer to virtual machine structure
> - * @nveths: number of interfaces
> - * @veths: interface names
> + * @veths: string list of interface names
> *
> * Sets up the container interfaces by creating the veth device pairs and
> * attaching the parent end to the appropriate bridge. The container end
> @@ -525,7 +524,6 @@ static int virLXCProcessSetupNamespaces(virConnectPtr
> conn,
> */
> static int virLXCProcessSetupInterfaces(virConnectPtr conn,
> virDomainDefPtr def,
> -size_t *nveths,
> char ***veths)
> {
> int ret = -1;
> @@ -534,6 +532,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr
> conn,
> virDomainNetDefPtr net;
> virDomainNetType type;
>
> +*veths = NULL;
> +
> for (i = 0; i < def->nnets; i++) {
> char *veth = NULL;
> virNetDevBandwidthPtr actualBandwidth;
> @@ -549,9 +549,6 @@ static int virLXCProcessSetupInterfaces(virConnectPtr
> conn,
> if (virDomainNetAllocateActualDevice(def, net) < 0)
> goto cleanup;
>
> -if (VIR_EXPAND_N(*veths, *nveths, 1) < 0)
> -goto cleanup;
Contrary to what I said in the previous patch, I hadn't realized we were
expanding a list by 1 in a loop before I looked at this patch. That is very
inefficient and we'll keep doing that. Now I'm biased towards tracking the size
of the list so that we can do VIR_EXPAND_N(*veths, 0, def->nnets) and then just
add new elements, of course that would require tweaking the virStringListAdd
more.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 00/39] qemu: Add support for -blockdev
Am 27.07.2018 um 10:01 hat Peter Krempa geschrieben:
> On Thu, Jul 26, 2018 at 17:04:19 +0800, Fam Zheng wrote:
> > On Thu, 07/26 10:44, Kevin Wolf wrote:
> > > Am 25.07.2018 um 17:57 hat Peter Krempa geschrieben:
> > > > This series adds support for starting and hotplug of disks with
> > > > -blockdev/blockdev-add.
> > > >
> > > > Blockjobs are not supported and thus the last patch should not be
> > > > applied yet as some refactoring of the jobs is required.
> > > >
> > > > At the beginning of the series there are a few cleanup patches which may
> > > > be pushed even at this point.
> > > >
> > > > The main reason this is in RFC state is that block stats reporting does
> > > > not work.
> > > >
> > > > The following command:
> > > >
> > > > {"execute":"query-blockstats","arguments":{"query-nodes":true}}
> > >
> > > query-nodes was added in commit f71eaa74c0b by Fam and Max, CCed. I'm
> > > not sure what it was needed for at all and the commit message doesn't
> > > help with that, but I suppose the addition was related to
> > > wr_highest_offset (see below).
> >
> > Yes, this was part of RHBZ 1158094. Sorry about the poor commit message.
>
> Well the problem is that when using -blockdev with 'query-nodes' false/not
> present you get a
> even more useless output:
>
> virsh qemu-monitor-command --pretty upstream
> '{"execute":"query-blockstats","arguments":{"query-nodes":false}}'
> {
> "return": [
>
> ],
> "id": "libvirt-20"
> }
That's a problem. Apparently it ignores anonymous BlockBackends, which
it shouldn't do. I'll look into that.
> 'query-named-block-nodes' don't provide the 'stats' section either:
> (other drives snipped)
[...]
> Thus libvirt can't provide the stats it used to with -drive. Since we
> need to special-case the code for gathering stats for -blockdev it
> should not be a problem if e.g. query-named-block-nodes reported the
> stats section along. (perhaps with a boolean flag to do so which also
> could disable the nesting)
The only one it could theoretically provide is wr_highest_offset, the
other ones simply don't exist at the node level. And this is probably
not the only one you're interested in.
> In either case we need the stats in the same extent as we had before
> with -drive and query-blockstats.
Yes, that's clear.
Kevin
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] secdrivers remembering original labels
On Fri, Jul 27, 2018 at 09:56:23AM +0200, Michal Privoznik wrote: > Dear list, > > we have this very old bug [1] that I just keep pushing in front of me. I > made several attempts to fix it. However, none of them made into the > tree. I guess it's time to have discussion what to do about it. IIRC, > the algorithm that I implemented last was to keep original label in > XATTRs (among with some ref counter) and the last one to restore the > label will look there and find the original label. There was a problem > with two libvirtds fighting over a file on shared FS. IIRC there was a second problem around security of XATTRs. eg, if we set an XATTR it was possible for QEMU to change it once we given QEMU privs on the image. Thus a malicious QEMU can cause libvirtd to change the image to an arbitrary user on shutdown. I think it was possible to deal with that on Linux, because the kernel hsa some xattr namespacing that is reserved for only root. This protection is worthless though when using NFS images as you can't trust the remote NFS client to honour the same restriction. > So I guess my question is can we come up with a design that would work? > Or at least work to the extent that we're satisfied with? > > Personally, I like the XATTRs approach. And to resolve the NFS race we > can invent yet another lockspace to guard labelling - I also have a bug > for that [2] (although, I'm not that familiar with lockspaces). I guess > doing disk metadata locking is not going to be trivial, is it? Yeah, for sure we need two distinct locking regimes. Our current locking aims to protect disk content, and the lifetime of the lock is the entire duration of the QEMU process. For XATTRs, we only need write protection for the short time window in which the security drivers execute, which is at start, during hotplug/unplug, during shutdown, and finally migration. I think we could achieve this with the virtlockd if we make it use the same locking file, but a different byte offset within the file. Just need to make sure it doesn't clash with the byte offset QEMU has chosen, nor the current offset we use. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 1/2] util: Add VIR_ENUM_IMPL_LABEL
On 07/26/2018 07:49 PM, Cole Robinson wrote:
> This allows passing in a string label describing the enum, which can
> be used to autogenerate error messages
>
> Signed-off-by: Cole Robinson
> ---
> src/util/virutil.c | 20
> src/util/virutil.h | 15 ++-
> 2 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index a908422feb..6d23a26a74 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -444,16 +444,22 @@ virParseVersionString(const char *str, unsigned long
> *version,
>
> int virEnumFromString(const char *const*types,
>unsigned int ntypes,
> - const char *type)
> + const char *type,
> + const char * const label)
> {
> size_t i;
> if (!type)
> -return -1;
> +goto error;
>
> for (i = 0; i < ntypes; i++)
> if (STREQ(types[i], type))
> return i;
>
> + error:
> +if (label) {
If we rewrite all of our enums into _LABEL then @label is always going
to be non-NULL. But we need this check for now. We are not there yet.
> +virReportError(VIR_ERR_INVALID_ARG,
> + _("Unknown '%s' value '%s'"), label, type);
@type can be NULL, therefore NULLSTR(type).
> +}
> return -1;
> }
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 0/2] util: Add VIR_ENUM_IMPL_LABEL
On 07/26/2018 07:49 PM, Cole Robinson wrote: > This series adds VIR_ENUM_IMPL_LABEL and a demo conversion. > > VIR_ENUM_IMPL_LABEL allows passing in a string that briefly > describes the value the enum represents, which we use to > generate error messages for FromString and ToString > function failures. > > This will allow us to drop a lot of code and unique strings > that need translating. > > Patch 2 is an example usage, converting virDomainVirtType > enum. It's not a full conversion for this particular enum, > just a demo. The enum creation now looks like > > VIR_ENUM_IMPL_LABEL(virDomainVirt, > "domain type", > VIR_DOMAIN_VIRT_LAST, ... > > FromString failure reports this error for value '' > > invalid argument: Unknown 'domain type' value '' > > ToString failure reports this error for unknown type=83 > > internal error: Unknown 'domain type' internal value '83' > > > Seems like a win to me but I'm interested in other opinions. > This could also be a good BiteSizedTask for converting existing > enums > Agreed. > Cole Robinson (2): > util: Add VIR_ENUM_IMPL_LABEL > conf: Convert virDomainVirtType to VIR_ENUM_IMPL_LABEL > > src/conf/domain_conf.c | 10 ++ > src/util/virutil.c | 20 > src/util/virutil.h | 15 ++- > 3 files changed, 28 insertions(+), 17 deletions(-) > I like this. You can count on my ACK. But we should probably let others chime in and express their preference. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 0/2] util: Add VIR_ENUM_IMPL_LABEL
On Fri, Jul 27, 2018 at 11:17:13AM +0200, Michal Privoznik wrote: > On 07/26/2018 07:49 PM, Cole Robinson wrote: > > This series adds VIR_ENUM_IMPL_LABEL and a demo conversion. > > > > VIR_ENUM_IMPL_LABEL allows passing in a string that briefly > > describes the value the enum represents, which we use to > > generate error messages for FromString and ToString > > function failures. > > > > This will allow us to drop a lot of code and unique strings > > that need translating. > > > > Patch 2 is an example usage, converting virDomainVirtType > > enum. It's not a full conversion for this particular enum, > > just a demo. The enum creation now looks like > > > > VIR_ENUM_IMPL_LABEL(virDomainVirt, > > "domain type", > > VIR_DOMAIN_VIRT_LAST, ... > > > > FromString failure reports this error for value '' > > > > invalid argument: Unknown 'domain type' value '' > > > > ToString failure reports this error for unknown type=83 > > > > internal error: Unknown 'domain type' internal value '83' > > > > > > Seems like a win to me but I'm interested in other opinions. > > This could also be a good BiteSizedTask for converting existing > > enums > > > > Agreed. > > > Cole Robinson (2): > > util: Add VIR_ENUM_IMPL_LABEL > > conf: Convert virDomainVirtType to VIR_ENUM_IMPL_LABEL > > > > src/conf/domain_conf.c | 10 ++ > > src/util/virutil.c | 20 > > src/util/virutil.h | 15 ++- > > 3 files changed, 28 insertions(+), 17 deletions(-) > > > > I like this. You can count on my ACK. But we should probably let others > chime in and express their preference. I think its good. My only comment is whether we could come up with a way to gradually convert each file, while still finishing up with VIR_ENUM_IMPL as the macro name. A mass rename at the end is one option, but perhaps theres a more clever approach ? Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] libvirt-dbus build for virt-preview
Hi! Does it possible to add libvirt-dbus building for virt-preview repo? My use case - get latest libvirt-dbus in current fedora stable release. -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt-dbus build for virt-preview
On Fri, Jul 27, 2018 at 12:38:47PM +0300, Vasiliy Tolstov wrote: > Hi! Does it possible to add libvirt-dbus building for virt-preview repo? > My use case - get latest libvirt-dbus in current fedora stable release. Hi, I guess that we can do that, in the future it will be definitely useful, but for now I'm planning to update libvirt-dbus in stable releases as well. Currently libvirt-dbus requires libvirt-3.0.0, once the required version is higher than the libvirt in one of the stable releases I'll stop updating libvirt-dbus. Libvirt-dbus is currently under development in a way that it's still catching with all the libvirt APIs. The strategy is to implement all reasonable APIs up to libvirt-3.0.0, after that there will be separate release of libvirt-dbus to gradually cover libvirt APIs in order to make downstream maintainers life easier so they can pick specific libvirt-dbus version which will work with the shipped libvirt with all APIs covered. Pavel signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] secdrivers remembering original labels
On 07/27/2018 11:01 AM, Daniel P. Berrangé wrote: > On Fri, Jul 27, 2018 at 09:56:23AM +0200, Michal Privoznik wrote: >> Dear list, >> >> we have this very old bug [1] that I just keep pushing in front of me. I >> made several attempts to fix it. However, none of them made into the >> tree. I guess it's time to have discussion what to do about it. IIRC, >> the algorithm that I implemented last was to keep original label in >> XATTRs (among with some ref counter) and the last one to restore the >> label will look there and find the original label. There was a problem >> with two libvirtds fighting over a file on shared FS. > > IIRC there was a second problem around security of XATTRs. eg, if we set > an XATTR it was possible for QEMU to change it once we given QEMU privs > on the image. Thus a malicious QEMU can cause libvirtd to change the > image to an arbitrary user on shutdown. > > I think it was possible to deal with that on Linux, because the kernel > hsa some xattr namespacing that is reserved for only root. Yes, there are basically 4 levels (defined by prefix of attribute name): security. system. trusted. user. For the first two there is no restriction on side of VFS (but there might be some coming from underlying FS and/or security module). The trusted. can be get/set only by CAP_SYS_ADMIN process. And finally user. can be set only one regular files (plus some other restrictions), but it's available for basically everybody. IIRC, I used trusted. namespace because of CAP_SYS_ADMIN. Nota bene, if a file is stored on NFS and only CAP_SYS_ADMIN can change the attribute's value they are CAP_SYS_ADMIN already - they might change seclabel too. Why bother mangling libvirt's records. IOW, if somebody runs qemu with CAP_SYS_ADMIN all bets are off anyway. This would of course mean that there's no label restore for session daemon, but that can hardly ever work. Do we even initialize DAC/SELinux drivers for session daemon? > This protection > is worthless though when using NFS images as you can't trust the remote NFS > client to honour the same restriction. > >> So I guess my question is can we come up with a design that would work? >> Or at least work to the extent that we're satisfied with? >> >> Personally, I like the XATTRs approach. And to resolve the NFS race we >> can invent yet another lockspace to guard labelling - I also have a bug >> for that [2] (although, I'm not that familiar with lockspaces). I guess >> doing disk metadata locking is not going to be trivial, is it? > > Yeah, for sure we need two distinct locking regimes. Our current locking > aims to protect disk content, and the lifetime of the lock is the entire > duration of the QEMU process. For XATTRs, we only need write protection > for the short time window in which the security drivers execute, which > is at start, during hotplug/unplug, during shutdown, and finally migration. I guess migration would be covered by start. Since these locks would be held only for very short period (basically they would be acquired just before we try to set seclabel and released after disk content lock is successfully acquired) they can be exclusive. However, since we want to protect more than just disk seclabels, we need to acquire this new lock from more places. > > I think we could achieve this with the virtlockd if we make it use the > same locking file, but a different byte offset within the file. Just > need to make sure it doesn't clash with the byte offset QEMU has chosen, > nor the current offset we use. Makes sense. So the first step is to introduce metadata locking. I'll look into that. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] secdrivers remembering original labels
On Fri, Jul 27, 2018 at 01:27:40PM +0200, Michal Privoznik wrote: > On 07/27/2018 11:01 AM, Daniel P. Berrangé wrote: > > On Fri, Jul 27, 2018 at 09:56:23AM +0200, Michal Privoznik wrote: > >> Dear list, > >> > >> we have this very old bug [1] that I just keep pushing in front of me. I > >> made several attempts to fix it. However, none of them made into the > >> tree. I guess it's time to have discussion what to do about it. IIRC, > >> the algorithm that I implemented last was to keep original label in > >> XATTRs (among with some ref counter) and the last one to restore the > >> label will look there and find the original label. There was a problem > >> with two libvirtds fighting over a file on shared FS. > > > > IIRC there was a second problem around security of XATTRs. eg, if we set > > an XATTR it was possible for QEMU to change it once we given QEMU privs > > on the image. Thus a malicious QEMU can cause libvirtd to change the > > image to an arbitrary user on shutdown. > > > > I think it was possible to deal with that on Linux, because the kernel > > hsa some xattr namespacing that is reserved for only root. > > Yes, there are basically 4 levels (defined by prefix of attribute name): > > security. > system. > trusted. > user. > > For the first two there is no restriction on side of VFS (but there > might be some coming from underlying FS and/or security module). The > trusted. can be get/set only by CAP_SYS_ADMIN process. And finally user. > can be set only one regular files (plus some other restrictions), but > it's available for basically everybody. > > IIRC, I used trusted. namespace because of CAP_SYS_ADMIN. Nota bene, if > a file is stored on NFS and only CAP_SYS_ADMIN can change the > attribute's value they are CAP_SYS_ADMIN already - they might change > seclabel too. Why bother mangling libvirt's records. IOW, if somebody > runs qemu with CAP_SYS_ADMIN all bets are off anyway. Yep, if QEMU has CAP_SYS_ADMIN they're doomed no matter what. > > This would of course mean that there's no label restore for session > daemon, but that can hardly ever work. Do we even initialize DAC/SELinux > drivers for session daemon? The DAC driver doesn't work for session daemons (since you can't setuid obviously), but the sVirt driver *does* work. > > This protection > > is worthless though when using NFS images as you can't trust the remote NFS > > client to honour the same restriction. > > > >> So I guess my question is can we come up with a design that would work? > >> Or at least work to the extent that we're satisfied with? > >> > >> Personally, I like the XATTRs approach. And to resolve the NFS race we > >> can invent yet another lockspace to guard labelling - I also have a bug > >> for that [2] (although, I'm not that familiar with lockspaces). I guess > >> doing disk metadata locking is not going to be trivial, is it? > > > > Yeah, for sure we need two distinct locking regimes. Our current locking > > aims to protect disk content, and the lifetime of the lock is the entire > > duration of the QEMU process. For XATTRs, we only need write protection > > for the short time window in which the security drivers execute, which > > is at start, during hotplug/unplug, during shutdown, and finally migration. > > I guess migration would be covered by start. Since these locks would be > held only for very short period (basically they would be acquired just > before we try to set seclabel and released after disk content lock is > successfully acquired) they can be exclusive. > > However, since we want to protect more than just disk seclabels, we need > to acquire this new lock from more places. > > > > > I think we could achieve this with the virtlockd if we make it use the > > same locking file, but a different byte offset within the file. Just > > need to make sure it doesn't clash with the byte offset QEMU has chosen, > > nor the current offset we use. > > Makes sense. So the first step is to introduce metadata locking. I'll > look into that. Yes, in fact the metdata locking is desirable even ignoring the original task of restoring original disk labels. The metadata locking alone would better protect against startup races between system libvirtds accessing the same NFS. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 10/41] util: cgroup: use VIR_AUTOPTR for aggregate types
On Tue, Jul 24, 2018 at 09:22:11PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar
> Reviewed-by: Erik Skultety
> ---
...
> @@ -1277,10 +1268,10 @@ virCgroupNewPartition(const char *path,
>int controllers,
>virCgroupPtr *group)
> {
> -int ret = -1;
> VIR_AUTOFREE(char *) parentPath = NULL;
> VIR_AUTOFREE(char *) newPath = NULL;
> -virCgroupPtr parent = NULL;
> +VIR_AUTOPTR(virCgroup) parent = NULL;
> +VIR_AUTOPTR(virCgroup) tmpGroup = NULL;
> VIR_DEBUG("path=%s create=%d controllers=%x",
>path, create, controllers);
>
> @@ -1315,12 +1306,11 @@ virCgroupNewPartition(const char *path,
> }
> }
>
> -ret = 0;
> +return 0;
> +
> cleanup:
> -if (ret != 0)
> -virCgroupFree(*group);
> -virCgroupFree(parent);
> -return ret;
> +VIR_STEAL_PTR(tmpGroup, *group);
> +return -1;
^Not exactly what I had in mind, we're still touching the caller provided data
too much. Have a look at this diff:
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 61fafe26f8..472a8167f5 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1283,34 +1283,31 @@ virCgroupNewPartition(const char *path,
}
if (virCgroupSetPartitionSuffix(path, &newPath) < 0)
-goto cleanup;
+return -1;
-if (virCgroupNew(-1, newPath, NULL, controllers, group) < 0)
-goto cleanup;
+if (virCgroupNew(-1, newPath, NULL, controllers, &tmpGroup) < 0)
+return -1;
if (STRNEQ(newPath, "/")) {
char *tmp;
if (VIR_STRDUP(parentPath, newPath) < 0)
-goto cleanup;
+return -1;
tmp = strrchr(parentPath, '/');
tmp++;
*tmp = '\0';
if (virCgroupNew(-1, parentPath, NULL, controllers, &parent) < 0)
-goto cleanup;
+return -1;
-if (virCgroupMakeGroup(parent, *group, create, VIR_CGROUP_NONE) < 0) {
-virCgroupRemove(*group);
-goto cleanup;
+if (virCgroupMakeGroup(parent, tmpGroup, create, VIR_CGROUP_NONE) < 0)
{
+virCgroupRemove(tmpGroup);
+return -1;
}
}
+VIR_STEAL_PTR(*group, tmpGroup);
return 0;
-
- cleanup:
-VIR_STEAL_PTR(tmpGroup, *group);
-return -1;
After ^this, we'll only touch the caller provided data once we're sure nothing
can go wrong anymore. I'll squash that in.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/4] Documentation/vfio-mediated-device.txt: update for aggregation attribute
On Fri, 27 Jul 2018 10:16:58 +0800 Zhenyu Wang wrote: > On 2018.07.26 17:46:40 +0200, Cornelia Huck wrote: > > On Fri, 20 Jul 2018 10:19:28 +0800 > > Zhenyu Wang wrote: > > > > > Update mdev doc on new aggregration attribute and instances attribute > > > for mdev. > > > > > > Cc: Kirti Wankhede > > > Cc: Alex Williamson > > > Cc: Kevin Tian > > > Signed-off-by: Zhenyu Wang > > > --- > > > Documentation/vfio-mediated-device.txt | 39 ++ > > > 1 file changed, 33 insertions(+), 6 deletions(-) > > > > > > diff --git a/Documentation/vfio-mediated-device.txt > > > b/Documentation/vfio-mediated-device.txt > > > index c3f69bcaf96e..9ec9495dcbe7 100644 > > > --- a/Documentation/vfio-mediated-device.txt > > > +++ b/Documentation/vfio-mediated-device.txt > > > @@ -211,12 +211,20 @@ Directories and files under the sysfs for Each > > > Physical Device > > >| | |--- description > > >| | |--- [devices] > > >| |--- [] > > > - | |--- create > > > - | |--- name > > > - | |--- available_instances > > > - | |--- device_api > > > - | |--- description > > > - | |--- [devices] > > > + | ||--- create > > > + | ||--- name > > > + | ||--- available_instances > > > + | ||--- device_api > > > + | ||--- description > > > + | ||--- [devices] > > > + | |--- [] > > > + | ||--- create > > > + | ||--- name > > > + | ||--- available_instances > > > + | ||--- device_api > > > + | ||--- description > > > + | ||--- > > > + | ||--- [devices] > > > > > > * [mdev_supported_types] > > > > > > @@ -260,6 +268,19 @@ Directories and files under the sysfs for Each > > > Physical Device > > >This attribute should show brief features/description of the type. > > > This is > > >optional attribute. > > > > > > +* > > > + > > > + The description is to show feature for one instance of the type. > > > > > > > You are talking about "one instance" here. Can this be different for > > the same type with different physical devices? > > > > I would expect for normal mdev types, driver might expose like x2, x4, x8 > types > which split hw resource equally. But for type with aggregation feature, it can > set user wanted number of instances. Sorry maybe my use of word was not > clear, how > about "one example of type"? Maybe my question was confusing as well... is an attribute that is exposed for a particular type under a particular physical device. - If is always the same for that particular type, regardless of which physical device we're dealing with, let's just drop the "one instance" sentence. - If it instead depends on what physical device we're handling, I'd write something like "The contents of this attribute depend both on the type and on the particular instance." -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] lxc: Don't return early in virLXCProcessSetupInterfaces
There are two places in the loop body that just return instead of
jumping onto the cleanup label. The problem is the cleanup code
is not ran in those cases.
Signed-off-by: Michal Privoznik
---
Pushed under trivial rule.
src/lxc/lxc_process.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index d021a890f7..442756a6f1 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -544,7 +544,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
net = def->nets[i];
if (virLXCProcessValidateInterface(net) < 0)
-return -1;
+goto cleanup;
if (virDomainNetAllocateActualDevice(def, net) < 0)
goto cleanup;
@@ -612,7 +612,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
/* Make sure all net definitions will have a name in the container */
if (!net->ifname_guest) {
if (virAsprintf(&net->ifname_guest, "eth%zu", niface) < 0)
-return -1;
+goto cleanup;
niface++;
}
}
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] lxc: Turn @veths into a string list in virLXCProcessStart
On 07/27/2018 10:37 AM, Erik Skultety wrote:
> On Thu, Jul 26, 2018 at 05:36:28PM +0200, Michal Privoznik wrote:
>> This way it will be easier to use autofree.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/lxc/lxc_process.c | 24 +---
>> 1 file changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>> index d021a890f7..3ac39d598c 100644
>> --- a/src/lxc/lxc_process.c
>> +++ b/src/lxc/lxc_process.c
>> @@ -514,8 +514,7 @@ static int virLXCProcessSetupNamespaces(virConnectPtr
>> conn,
>> * virLXCProcessSetupInterfaces:
>> * @conn: pointer to connection
>> * @def: pointer to virtual machine structure
>> - * @nveths: number of interfaces
>> - * @veths: interface names
>> + * @veths: string list of interface names
>> *
>> * Sets up the container interfaces by creating the veth device pairs and
>> * attaching the parent end to the appropriate bridge. The container end
>> @@ -525,7 +524,6 @@ static int virLXCProcessSetupNamespaces(virConnectPtr
>> conn,
>> */
>> static int virLXCProcessSetupInterfaces(virConnectPtr conn,
>> virDomainDefPtr def,
>> -size_t *nveths,
>> char ***veths)
>> {
>> int ret = -1;
>> @@ -534,6 +532,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr
>> conn,
>> virDomainNetDefPtr net;
>> virDomainNetType type;
>>
>> +*veths = NULL;
>> +
>> for (i = 0; i < def->nnets; i++) {
>> char *veth = NULL;
>> virNetDevBandwidthPtr actualBandwidth;
>> @@ -549,9 +549,6 @@ static int virLXCProcessSetupInterfaces(virConnectPtr
>> conn,
>> if (virDomainNetAllocateActualDevice(def, net) < 0)
>> goto cleanup;
>>
>> -if (VIR_EXPAND_N(*veths, *nveths, 1) < 0)
>> -goto cleanup;
>
> Contrary to what I said in the previous patch, I hadn't realized we were
> expanding a list by 1 in a loop before I looked at this patch. That is very
> inefficient and we'll keep doing that. Now I'm biased towards tracking the
> size
> of the list so that we can do VIR_EXPAND_N(*veths, 0, def->nnets) and then
> just
> add new elements, of course that would require tweaking the virStringListAdd
> more.
>
Hold on. I'm not quite sure I follow. Are you suggesting to pre-allocate
the string list and ditch virStringListAdd completely? Something like
this?
diff --git i/src/lxc/lxc_process.c w/src/lxc/lxc_process.c
index 6eef17d1ce..33c806630b 100644
--- i/src/lxc/lxc_process.c
+++ w/src/lxc/lxc_process.c
@@ -532,7 +532,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
virDomainNetDefPtr net;
virDomainNetType type;
-*veths = NULL;
+if (VIR_ALLOC_N(*veths, def->nnets + 1) < 0)
+return -1;
for (i = 0; i < def->nnets; i++) {
char *veth = NULL;
@@ -601,8 +602,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
}
}
-if (virStringListAdd(veths, veth) < 0)
-goto cleanup;
+(*veths)[i] = veth;
if (VIR_STRDUP(def->nets[i]->ifname_guest_actual, veth) < 0)
goto cleanup;
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dbus PATCH 0/4] Fix 'make check' with libvirt 3.0.0
Andrea Bolognani (4): tests: Drop pytest.mark.usefixtures from test_nodedev tests: Move some test cases to test_nodedev tests: Don't open-code node_device_create() tests: Make node_device_create() work with libvirt 3.0.0 tests/libvirttest.py | 17 - tests/test_connect.py | 27 --- tests/test_nodedev.py | 26 +- tests/xmldata.py | 2 +- 4 files changed, 42 insertions(+), 30 deletions(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dbus PATCH 2/4] tests: Move some test cases to test_nodedev
For whatever reason, a few nodedev-related test cases
have ended up in test_connect instead of the more
appropriate test_nodedev. Move them.
Signed-off-by: Andrea Bolognani
---
tests/test_connect.py | 27 ---
tests/test_nodedev.py | 26 ++
2 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/tests/test_connect.py b/tests/test_connect.py
index f481356..9cc51db 100755
--- a/tests/test_connect.py
+++ b/tests/test_connect.py
@@ -161,19 +161,6 @@ class TestConnect(libvirttest.BaseTestClass):
self.main_loop()
-@pytest.mark.usefixtures("node_device_create")
-@pytest.mark.parametrize("lookup_method_name,lookup_item", [
-("NodeDeviceLookupByName", 'Name'),
-])
-def test_connect_node_device_lookup_by_property(self, lookup_method_name,
lookup_item, node_device_create):
-"""Parameterized test for all NodeDeviceLookupBy* API calls of Connect
interface
-"""
-original_path = node_device_create
-obj = self.bus.get_object('org.libvirt', original_path)
-prop = obj.Get('org.libvirt.NodeDevice', lookup_item,
dbus_interface=dbus.PROPERTIES_IFACE)
-path = getattr(self.connect, lookup_method_name)(prop)
-assert original_path == path
-
@pytest.mark.parametrize("lookup_method_name,lookup_item", [
("NetworkLookupByName", 'Name'),
("NetworkLookupByUUID", 'UUID'),
@@ -186,20 +173,6 @@ class TestConnect(libvirttest.BaseTestClass):
path = getattr(self.connect, lookup_method_name)(prop)
assert original_path == path
-def test_connect_node_device_create_xml(self):
-def node_device_created(path, event, _detail):
-if event != libvirttest.NodeDeviceEvent.CREATED:
-return
-assert isinstance(path, dbus.ObjectPath)
-self.loop.quit()
-
-self.connect.connect_to_signal('NodeDeviceEvent', node_device_created)
-
-path =
self.connect.NodeDeviceCreateXML(xmldata.minimal_node_device_xml, 0)
-assert isinstance(path, dbus.ObjectPath)
-
-self.main_loop()
-
def test_connect_node_get_cpu_stats(self):
stats = self.connect.NodeGetCPUStats(0, 0)
assert isinstance(stats, dbus.Dictionary)
diff --git a/tests/test_nodedev.py b/tests/test_nodedev.py
index 082cf0b..c68cb66 100755
--- a/tests/test_nodedev.py
+++ b/tests/test_nodedev.py
@@ -3,11 +3,37 @@
import dbus
import libvirttest
import pytest
+import xmldata
class TestNodeDevice(libvirttest.BaseTestClass):
""" Tests for methods and properties of the NodeDevice interface
"""
+@pytest.mark.parametrize("lookup_method_name,lookup_item", [
+("NodeDeviceLookupByName", 'Name'),
+])
+def test_connect_node_device_lookup_by_property(self, lookup_method_name,
lookup_item, node_device_create):
+"""Parameterized test for all NodeDeviceLookupBy* API calls of Connect
interface
+"""
+original_path = node_device_create
+obj = self.bus.get_object('org.libvirt', original_path)
+prop = obj.Get('org.libvirt.NodeDevice', lookup_item,
dbus_interface=dbus.PROPERTIES_IFACE)
+path = getattr(self.connect, lookup_method_name)(prop)
+assert original_path == path
+
+def test_connect_node_device_create(self):
+def node_device_created(path, event, _detail):
+if event != libvirttest.NodeDeviceEvent.CREATED:
+return
+assert isinstance(path, dbus.ObjectPath)
+self.loop.quit()
+
+self.connect.connect_to_signal('NodeDeviceEvent', node_device_created)
+
+path =
self.connect.NodeDeviceCreateXML(xmldata.minimal_node_device_xml, 0)
+assert isinstance(path, dbus.ObjectPath)
+
+self.main_loop()
def test_node_device_destroy(self, node_device_create):
def node_device_deleted(path, event, _detail):
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dbus PATCH 1/4] tests: Drop pytest.mark.usefixtures from test_nodedev
We're already using node_device_create explicitly for all
test cases in order to retrieve the result of the fixture,
so using pytest.mark.usefixtures as well is pointless.
Additionally, we're soon going to add some test cases
where we need the fixture *not* to run automatically.
Signed-off-by: Andrea Bolognani
---
tests/test_nodedev.py | 1 -
1 file changed, 1 deletion(-)
diff --git a/tests/test_nodedev.py b/tests/test_nodedev.py
index 22c8d60..082cf0b 100755
--- a/tests/test_nodedev.py
+++ b/tests/test_nodedev.py
@@ -5,7 +5,6 @@ import libvirttest
import pytest
-@pytest.mark.usefixtures("node_device_create")
class TestNodeDevice(libvirttest.BaseTestClass):
""" Tests for methods and properties of the NodeDevice interface
"""
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dbus PATCH 3/4] tests: Don't open-code node_device_create()
The fixture is pretty trivial right now, so open-coding
it is not a big deal; however, we're going to add more
logic to it soon and we definitely don't want to end up
having to worry about duplicated code.
Signed-off-by: Andrea Bolognani
---
tests/test_nodedev.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tests/test_nodedev.py b/tests/test_nodedev.py
index c68cb66..ce70dfc 100755
--- a/tests/test_nodedev.py
+++ b/tests/test_nodedev.py
@@ -3,7 +3,6 @@
import dbus
import libvirttest
import pytest
-import xmldata
class TestNodeDevice(libvirttest.BaseTestClass):
@@ -30,7 +29,7 @@ class TestNodeDevice(libvirttest.BaseTestClass):
self.connect.connect_to_signal('NodeDeviceEvent', node_device_created)
-path =
self.connect.NodeDeviceCreateXML(xmldata.minimal_node_device_xml, 0)
+path = self.node_device_create()
assert isinstance(path, dbus.ObjectPath)
self.main_loop()
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dbus PATCH 4/4] tests: Make node_device_create() work with libvirt 3.0.0
The scsi_host2 nodedev, which our test nodedev uses as its
parent, was added to the test driver with commit 5c2ff641e10c,
which is included in libvirt 3.1.0; before that, there was a
similar device called test-scsi-host-vport, which is no longer
present after libvirt 3.0.0.
Since there's no nodedev that we can use as parent both with
libvirt 3.0.0 and with later versions, our only option is to
perform a lookup at runtime and use whatever's available.
Signed-off-by: Andrea Bolognani
---
tests/libvirttest.py | 17 -
tests/xmldata.py | 2 +-
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/tests/libvirttest.py b/tests/libvirttest.py
index 776b12d..14baf5b 100644
--- a/tests/libvirttest.py
+++ b/tests/libvirttest.py
@@ -91,7 +91,22 @@ class BaseTestClass():
This fixture should be used in the setup of every test manipulating
with node devices.
"""
-path =
self.connect.NodeDeviceCreateXML(xmldata.minimal_node_device_xml, 0)
+# We need a usable parent nodedev: possible candidates are
+# scsi_host2 (available since libvirt 3.1.0) and
+# test-scsi-host-vport (available until libvirt 3.0.0).
+#
+# Looking up a non-existing nodedev raises an exception, so
+# we need to ignore them here: that's okay, because if we
+# can't find any then NodeDeviceCreateXML() itself will fail
+xml = xmldata.minimal_node_device_xml
+for parent in ['scsi_host2', 'test-scsi-host-vport']:
+try:
+if self.connect.NodeDeviceLookupByName(parent):
+xml = xml.replace('@parent@', parent)
+break
+except dbus.exceptions.DBusException:
+pass
+path = self.connect.NodeDeviceCreateXML(xml, 0)
return path
@pytest.fixture
diff --git a/tests/xmldata.py b/tests/xmldata.py
index a70bac1..8075052 100644
--- a/tests/xmldata.py
+++ b/tests/xmldata.py
@@ -41,7 +41,7 @@ minimal_network_xml = '''
minimal_node_device_xml = '''
scsi_host22
- scsi_host2
+ @parent@
22
22
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dbus PATCH] gitignore: Ignore Vim swap files
Signed-off-by: Andrea Bolognani --- Pushed as trivial. .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index e89a5d3..b4abf66 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,7 @@ *.log *.o *.pyc +*.swp *.trs *Makefile *Makefile.in -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 2/4] tests: Move some test cases to test_nodedev
On Fri, Jul 27, 2018 at 02:37:39PM +0200, Andrea Bolognani wrote: > For whatever reason, a few nodedev-related test cases > have ended up in test_connect instead of the more > appropriate test_nodedev. Move them. > > Signed-off-by: Andrea Bolognani > --- https://www.redhat.com/mailman/listinfo/libvir-list Currently a test for some method is placed in the test file refering to the interface the tested method is exposed on. So NodeDeviceCreateXML method is exposed on the Connect Interface, this test_connect.py is the place for the test for this method. Katerina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 0/4] Fix 'make check' with libvirt 3.0.0
On Fri, Jul 27, 2018 at 02:37:37PM +0200, Andrea Bolognani wrote: > Andrea Bolognani (4): > tests: Drop pytest.mark.usefixtures from test_nodedev > tests: Move some test cases to test_nodedev > tests: Don't open-code node_device_create() > tests: Make node_device_create() work with libvirt 3.0.0 > > tests/libvirttest.py | 17 - > tests/test_connect.py | 27 --- > tests/test_nodedev.py | 26 +- > tests/xmldata.py | 2 +- > 4 files changed, 42 insertions(+), 30 deletions(-) > > -- > 2.17.1 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list Please remove the second patch moving the test file - see reasoning there. So the third patch should be adjusted but in the original file, test_connect.py With these fixed: Reviewed-by: Katerina Koukiou signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] news: Usb and sata for virsh attach-disk --address
On 07/26/2018 03:33 PM, Han Han wrote: > Signed-off-by: Han Han > --- > docs/news.xml | 10 ++ > 1 file changed, 10 insertions(+) ACKed and pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 22/41] util: usb: modify virUSBDeviceListAdd to take double pointer
On Tue, Jul 24, 2018 at 09:22:23PM +0530, Sukrit Bhatnagar wrote: > Modify virUSBDeviceListAdd to take a double pointer to > virUSBDevicePtr as the second argument. This will enable usage > of cleanup macros upon the virUSBDevicePtr item which is to be > added to the list as it will be cleared by virInsertElementsN > upon success. > > Signed-off-by: Sukrit Bhatnagar > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 25/41] util: usb: use VIR_AUTOPTR for aggregate types
On Tue, Jul 24, 2018 at 09:22:26PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/virusb.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/src/util/virusb.c b/src/util/virusb.c
> index 47b407b..df94959 100644
> --- a/src/util/virusb.c
> +++ b/src/util/virusb.c
> @@ -123,8 +123,10 @@ virUSBDeviceSearch(unsigned int vendor,
> bool found = false;
> char *ignore = NULL;
> struct dirent *de;
> -virUSBDeviceListPtr list = NULL, ret = NULL;
> -virUSBDevicePtr usb;
> +virUSBDeviceListPtr list = NULL;
> +virUSBDeviceListPtr ret = NULL;
> +VIR_AUTOPTR(virUSBDevice) usb = NULL;
> +virUSBDevicePtr *ptr = NULL;
> int direrr;
>
> if (!(list = virUSBDeviceListNew()))
> @@ -173,13 +175,13 @@ virUSBDeviceSearch(unsigned int vendor,
> }
>
> usb = virUSBDeviceNew(found_bus, found_devno, vroot);
> +ptr = &usb;
I don't see a reason for the @ptr variable, you can work with @usb directly,
I'll make the change before pushing.
Reviewed-by: Erik Skultety
> +
> if (!usb)
> goto cleanup;
>
> -if (virUSBDeviceListAdd(list, &usb) < 0) {
> -virUSBDeviceFree(usb);
> +if (virUSBDeviceListAdd(list, ptr) < 0)
> goto cleanup;
> -}
>
> if (found)
> break;
> @@ -508,8 +510,7 @@ void
> virUSBDeviceListDel(virUSBDeviceListPtr list,
> virUSBDevicePtr dev)
> {
> -virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev);
> -virUSBDeviceFree(ret);
> +VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
> }
>
> virUSBDevicePtr
> --
> 1.8.3.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 RESEND 09/12] qemu: Generate and use zPCI device in QEMU command line
On Fri, 2018-07-27 at 15:53 +0800, Yi Min Zhao wrote:
> 在 2018/7/24 下午11:09, Andrea Bolognani 写道:
> > Also, do you really need to check both the flags and the presence
> > of the zPCI address bits? It looks like either one or the other
> > should be enough or, if that's not the case, it should be made so
> > because having to check for two separate conditions makes me feel
> > like it would introduce bugs in the long run.
>
> This is actually a problem. I add info->addr.pci.zpci check in order to
> check
> if the user specifies zpci address in xml even though it has no zpci
> support.
> The callers of this function checks zpci capability. If no zpci cap but
> the user
> specfies zpci address, report an error.
>
> I will change the logic and remove the check on info->addr.pci.zpci.
Yeah, you definitely want to report an error if the QEMU binary
doesn't support zPCI or the user is attempting to do something
silly like add zPCI-related information to devices attached to
an x86_64 guest...
> > > +char *qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev);
> > > +
> > > +bool qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info);
> >
> > Is this really necessary? Can't these two functions be static?
>
> They are also used in qemu_hotplug.c file.
Right, I overlooked that :)
That said, they aren't used in the hotplug code until the next
patch, so it would IMHO make sense to define them as static in
this patch and export them in the next one, at the same time as
you actually start using them outside of the file.
[...]
> > > +DO_TEST("hostdev-vfio-zpci",
> > > +QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI);
> > > +DO_TEST("hostdev-vfio-zpci-multidomain-many",
> > > +QEMU_CAPS_DEVICE_VFIO_PCI, X_QEMU_CAPS_HOST_PCI_MULTIDOMAIN,
> >
> > Capabilities with X_QEMU prefix are no longer used, so you should
> > not list them here.
>
> Sure.
I forgot to mention, please have a single capability per line and
make sure the set of capabilities used in xml2xml and xml2argv is
exactly the same.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 28/41] util: scsi: use VIR_AUTOPTR for aggregate types
On Tue, Jul 24, 2018 at 09:22:29PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar
> Reviewed-by: Erik Skultety
> ---
> src/util/virscsi.c | 37 +++--
> 1 file changed, 15 insertions(+), 22 deletions(-)
>
> diff --git a/src/util/virscsi.c b/src/util/virscsi.c
> index ba0a688..f97f6b5 100644
> --- a/src/util/virscsi.c
> +++ b/src/util/virscsi.c
> @@ -189,7 +189,8 @@ virSCSIDeviceNew(const char *sysfs_prefix,
> bool readonly,
> bool shareable)
> {
> -virSCSIDevicePtr dev, ret = NULL;
> +VIR_AUTOPTR(virSCSIDevice) dev = NULL;
> +virSCSIDevicePtr ret = NULL;
> VIR_AUTOFREE(char *) sg = NULL;
> VIR_AUTOFREE(char *) vendor_path = NULL;
> VIR_AUTOFREE(char *) model_path = NULL;
> @@ -207,46 +208,43 @@ virSCSIDeviceNew(const char *sysfs_prefix,
> dev->shareable = shareable;
>
> if (!(sg = virSCSIDeviceGetSgName(prefix, adapter, bus, target, unit)))
> -goto cleanup;
> +return NULL;
>
> if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0)
> -goto cleanup;
> +return NULL;
>
> if (virAsprintf(&dev->name, "%d:%u:%u:%llu", dev->adapter,
> dev->bus, dev->target, dev->unit) < 0 ||
> virAsprintf(&dev->sg_path, "%s/%s",
> sysfs_prefix ? sysfs_prefix : "/dev", sg) < 0)
> -goto cleanup;
> +return NULL;
>
> if (!virFileExists(dev->sg_path)) {
> virReportSystemError(errno,
> _("SCSI device '%s': could not access %s"),
> dev->name, dev->sg_path);
> -goto cleanup;
> +return NULL;
> }
>
> if (virAsprintf(&vendor_path,
> "%s/%s/vendor", prefix, dev->name) < 0 ||
> virAsprintf(&model_path,
> "%s/%s/model", prefix, dev->name) < 0)
> -goto cleanup;
> +return NULL;
>
> if (virFileReadAll(vendor_path, 1024, &vendor) < 0)
> -goto cleanup;
> +return NULL;
>
> if (virFileReadAll(model_path, 1024, &model) < 0)
> -goto cleanup;
> +return NULL;
>
> virTrimSpaces(vendor, NULL);
> virTrimSpaces(model, NULL);
>
> if (virAsprintf(&dev->id, "%s:%s", vendor, model) < 0)
> -goto cleanup;
> +return NULL;
>
> -ret = dev;
> - cleanup:
> -if (!ret)
> -virSCSIDeviceFree(dev);
> +VIR_STEAL_PTR(ret, dev);
> return ret;
> }
>
> @@ -281,21 +279,17 @@ virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev,
> const char *drvname,
> const char *domname)
> {
> -virUsedByInfoPtr copy;
> +VIR_AUTOPTR(virUsedByInfo) copy = NULL;
I'll add a new line ^here before merging.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 37/41] util: iscsi: use VIR_AUTOPTR for aggregate types
On Tue, Jul 24, 2018 at 09:22:38PM +0530, Sukrit Bhatnagar wrote: > By making use of GNU C's cleanup attribute handled by the > VIR_AUTOPTR macro for declaring aggregate pointer variables, > majority of the calls to *Free functions can be dropped, which > in turn leads to getting rid of most of our cleanup sections. > > Signed-off-by: Sukrit Bhatnagar > Reviewed-by: Erik Skultety For the time being, I'm going to retract my ^RB (the same goes for the previous patch). There are some merge conflicts while applying this and the previous patch (there's been a work on this module recently), so you should resolve them a sent as part of the next batch. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources
On Thu, Jul 26, 2018 at 06:04:10PM +0200, Cornelia Huck wrote: > On Thu, 26 Jul 2018 17:43:45 +0200 > Erik Skultety wrote: > > > On Thu, Jul 26, 2018 at 05:30:07PM +0200, Cornelia Huck wrote: > > > One thing I noticed is that we have seem to have an optional (?) > > > vendor-driver created "aggregation" attribute (which always prints > > > "true" in the Intel driver). Would it be better or worse for libvirt if > > > it contained some kind of upper boundary or so? Additionally, would it > > > > Can you be more specific? Although, I wouldn't argue against data that > > conveys > > some information, since I would treat the mere presence of the optional > > attribute as a supported feature that we can expose. Therefore, additional > > *structured* data which sets clear limits to a certain feature is only a > > plus > > that we can expose to the users/management layer. > > My question is what would be easiest for libvirt: > > - "aggregation" attribute only present when driver supports aggregation > (possibly containing max number of resources to be aggregated) > - "aggregation" attribute always present; contains '1' if driver does > not support aggregation and 'm' if driver can aggregate 'm' resources Both are fine from libvirt's POV, but IMHO the former makes a bit more sense and I'm in favour of that one, IOW the presence of an attribute denotes a new functionality which we can report, if it's missing, the feature is clearly lacking- I don't think we (libvirt) should be reporting the value 1 explicitly in the XML if the feature is missing, we can assume 1 as the default. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Plans for next release
So we are getting very close to the end of the month, I suggest to enter freeze tomorrow morning, then plan for an RC2 on Tuesday, that way if everythiong goes well next release would be on Thursday 2nd Aug, I hope this is fine with everyone's schedule, thanks, Daniel -- Daniel Veillard | Red Hat Developers Tools http://developer.redhat.com/ veill...@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] lxc: Turn @veths into a string list in virLXCProcessStart
On Fri, Jul 27, 2018 at 02:34:41PM +0200, Michal Privoznik wrote:
> On 07/27/2018 10:37 AM, Erik Skultety wrote:
> > On Thu, Jul 26, 2018 at 05:36:28PM +0200, Michal Privoznik wrote:
> >> This way it will be easier to use autofree.
> >>
> >> Signed-off-by: Michal Privoznik
> >> ---
> >> src/lxc/lxc_process.c | 24 +---
> >> 1 file changed, 9 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> >> index d021a890f7..3ac39d598c 100644
> >> --- a/src/lxc/lxc_process.c
> >> +++ b/src/lxc/lxc_process.c
> >> @@ -514,8 +514,7 @@ static int virLXCProcessSetupNamespaces(virConnectPtr
> >> conn,
> >> * virLXCProcessSetupInterfaces:
> >> * @conn: pointer to connection
> >> * @def: pointer to virtual machine structure
> >> - * @nveths: number of interfaces
> >> - * @veths: interface names
> >> + * @veths: string list of interface names
> >> *
> >> * Sets up the container interfaces by creating the veth device pairs and
> >> * attaching the parent end to the appropriate bridge. The container end
> >> @@ -525,7 +524,6 @@ static int virLXCProcessSetupNamespaces(virConnectPtr
> >> conn,
> >> */
> >> static int virLXCProcessSetupInterfaces(virConnectPtr conn,
> >> virDomainDefPtr def,
> >> -size_t *nveths,
> >> char ***veths)
> >> {
> >> int ret = -1;
> >> @@ -534,6 +532,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr
> >> conn,
> >> virDomainNetDefPtr net;
> >> virDomainNetType type;
> >>
> >> +*veths = NULL;
> >> +
> >> for (i = 0; i < def->nnets; i++) {
> >> char *veth = NULL;
> >> virNetDevBandwidthPtr actualBandwidth;
> >> @@ -549,9 +549,6 @@ static int virLXCProcessSetupInterfaces(virConnectPtr
> >> conn,
> >> if (virDomainNetAllocateActualDevice(def, net) < 0)
> >> goto cleanup;
> >>
> >> -if (VIR_EXPAND_N(*veths, *nveths, 1) < 0)
> >> -goto cleanup;
> >
> > Contrary to what I said in the previous patch, I hadn't realized we were
> > expanding a list by 1 in a loop before I looked at this patch. That is very
> > inefficient and we'll keep doing that. Now I'm biased towards tracking the
> > size
> > of the list so that we can do VIR_EXPAND_N(*veths, 0, def->nnets) and then
> > just
> > add new elements, of course that would require tweaking the virStringListAdd
> > more.
> >
>
> Hold on. I'm not quite sure I follow. Are you suggesting to pre-allocate
> the string list and ditch virStringListAdd completely? Something like
> this?
Yes, that's the idea. Originally, I didn't think dropping virStringListAdd would
be necessary, but it's clear from the snippet below that it would.
Erik
>
> diff --git i/src/lxc/lxc_process.c w/src/lxc/lxc_process.c
> index 6eef17d1ce..33c806630b 100644
> --- i/src/lxc/lxc_process.c
> +++ w/src/lxc/lxc_process.c
> @@ -532,7 +532,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr
> conn,
> virDomainNetDefPtr net;
> virDomainNetType type;
>
> -*veths = NULL;
> +if (VIR_ALLOC_N(*veths, def->nnets + 1) < 0)
> +return -1;
>
> for (i = 0; i < def->nnets; i++) {
> char *veth = NULL;
> @@ -601,8 +602,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr
> conn,
> }
> }
>
> -if (virStringListAdd(veths, veth) < 0)
> -goto cleanup;
> +(*veths)[i] = veth;
>
> if (VIR_STRDUP(def->nets[i]->ifname_guest_actual, veth) < 0)
> goto cleanup;
>
>
> Michal
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] lxc: Turn @veths into a string list in virLXCProcessStart
On Fri, Jul 27, 2018 at 05:02:36PM +0200, Erik Skultety wrote:
> On Fri, Jul 27, 2018 at 02:34:41PM +0200, Michal Privoznik wrote:
> > On 07/27/2018 10:37 AM, Erik Skultety wrote:
> > > On Thu, Jul 26, 2018 at 05:36:28PM +0200, Michal Privoznik wrote:
> > >> This way it will be easier to use autofree.
> > >>
> > >> Signed-off-by: Michal Privoznik
> > >> ---
> > >> src/lxc/lxc_process.c | 24 +---
> > >> 1 file changed, 9 insertions(+), 15 deletions(-)
> > >>
> > >> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> > >> index d021a890f7..3ac39d598c 100644
> > >> --- a/src/lxc/lxc_process.c
> > >> +++ b/src/lxc/lxc_process.c
> > >> @@ -514,8 +514,7 @@ static int
> > >> virLXCProcessSetupNamespaces(virConnectPtr conn,
> > >> * virLXCProcessSetupInterfaces:
> > >> * @conn: pointer to connection
> > >> * @def: pointer to virtual machine structure
> > >> - * @nveths: number of interfaces
> > >> - * @veths: interface names
> > >> + * @veths: string list of interface names
> > >> *
> > >> * Sets up the container interfaces by creating the veth device pairs
> > >> and
> > >> * attaching the parent end to the appropriate bridge. The container
> > >> end
> > >> @@ -525,7 +524,6 @@ static int
> > >> virLXCProcessSetupNamespaces(virConnectPtr conn,
> > >> */
> > >> static int virLXCProcessSetupInterfaces(virConnectPtr conn,
> > >> virDomainDefPtr def,
> > >> -size_t *nveths,
> > >> char ***veths)
> > >> {
> > >> int ret = -1;
> > >> @@ -534,6 +532,8 @@ static int
> > >> virLXCProcessSetupInterfaces(virConnectPtr conn,
> > >> virDomainNetDefPtr net;
> > >> virDomainNetType type;
> > >>
> > >> +*veths = NULL;
> > >> +
> > >> for (i = 0; i < def->nnets; i++) {
> > >> char *veth = NULL;
> > >> virNetDevBandwidthPtr actualBandwidth;
> > >> @@ -549,9 +549,6 @@ static int
> > >> virLXCProcessSetupInterfaces(virConnectPtr conn,
> > >> if (virDomainNetAllocateActualDevice(def, net) < 0)
> > >> goto cleanup;
> > >>
> > >> -if (VIR_EXPAND_N(*veths, *nveths, 1) < 0)
> > >> -goto cleanup;
> > >
> > > Contrary to what I said in the previous patch, I hadn't realized we were
> > > expanding a list by 1 in a loop before I looked at this patch. That is
> > > very
> > > inefficient and we'll keep doing that. Now I'm biased towards tracking
> > > the size
> > > of the list so that we can do VIR_EXPAND_N(*veths, 0, def->nnets) and
> > > then just
> > > add new elements, of course that would require tweaking the
> > > virStringListAdd
> > > more.
> > >
> >
> > Hold on. I'm not quite sure I follow. Are you suggesting to pre-allocate
> > the string list and ditch virStringListAdd completely? Something like
> > this?
>
> Yes, that's the idea. Originally, I didn't think dropping virStringListAdd
> would
> be necessary, but it's clear from the snippet below that it would.
>
> Erik
Something I forgot:
Reviewed-by: Erik Skultety
>
> >
> > diff --git i/src/lxc/lxc_process.c w/src/lxc/lxc_process.c
> > index 6eef17d1ce..33c806630b 100644
> > --- i/src/lxc/lxc_process.c
> > +++ w/src/lxc/lxc_process.c
> > @@ -532,7 +532,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr
> > conn,
> > virDomainNetDefPtr net;
> > virDomainNetType type;
> >
> > -*veths = NULL;
> > +if (VIR_ALLOC_N(*veths, def->nnets + 1) < 0)
> > +return -1;
> >
> > for (i = 0; i < def->nnets; i++) {
> > char *veth = NULL;
> > @@ -601,8 +602,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr
> > conn,
> > }
> > }
> >
> > -if (virStringListAdd(veths, veth) < 0)
> > -goto cleanup;
> > +(*veths)[i] = veth;
> >
> > if (VIR_STRDUP(def->nets[i]->ifname_guest_actual, veth) < 0)
> > goto cleanup;
> >
> >
> > Michal
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 3/3] virtestmock: Track action
As advertised in the previous commit, we need the list of
accessed files to also contain action that caused the $path to
appear on the list. Not only this enables us to fine tune our
white list rules it also helps us to see why $path is reported.
For instance:
/run/user/1000/libvirt/libvirt-sock: connect: qemuxml2argvtest: QEMU
XML-2-ARGV net-vhostuser-multiq
Signed-off-by: Michal Privoznik
---
tests/virtestmock.c | 39 ++-
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/tests/virtestmock.c b/tests/virtestmock.c
index 654af24a10..25aadf8aea 100644
--- a/tests/virtestmock.c
+++ b/tests/virtestmock.c
@@ -88,7 +88,8 @@ static void init_syms(void)
}
static void
-printFile(const char *file)
+printFile(const char *file,
+ const char *func)
{
FILE *fp;
const char *testname = getenv("VIR_TEST_MOCK_TESTNAME");
@@ -116,9 +117,9 @@ printFile(const char *file)
}
/* Now append the following line into the output file:
- * $file: $progname $testname */
+ * $file: $progname: $func: $testname */
-fprintf(fp, "%s: %s", file, progname);
+fprintf(fp, "%s: %s: %s", file, func, progname);
if (testname)
fprintf(fp, ": %s", testname);
@@ -128,8 +129,12 @@ printFile(const char *file)
fclose(fp);
}
+#define CHECK_PATH(path) \
+checkPath(path, __FUNCTION__)
+
static void
-checkPath(const char *path)
+checkPath(const char *path,
+ const char *func)
{
char *fullPath = NULL;
char *relPath = NULL;
@@ -160,7 +165,7 @@ checkPath(const char *path)
if (!STRPREFIX(path, abs_topsrcdir) &&
!STRPREFIX(path, abs_topbuilddir)) {
-printFile(path);
+printFile(path, func);
}
VIR_FREE(crippledPath);
@@ -180,7 +185,7 @@ int open(const char *path, int flags, ...)
init_syms();
-checkPath(path);
+CHECK_PATH(path);
if (flags & O_CREAT) {
va_list ap;
@@ -199,7 +204,7 @@ FILE *fopen(const char *path, const char *mode)
{
init_syms();
-checkPath(path);
+CHECK_PATH(path);
return real_fopen(path, mode);
}
@@ -209,7 +214,7 @@ int access(const char *path, int mode)
{
init_syms();
-checkPath(path);
+CHECK_PATH(path);
return real_access(path, mode);
}
@@ -239,7 +244,7 @@ int stat(const char *path, struct stat *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "stat");
return real_stat(path, sb);
}
@@ -250,7 +255,7 @@ int stat64(const char *path, struct stat64 *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "stat");
return real_stat64(path, sb);
}
@@ -262,7 +267,7 @@ __xstat(int ver, const char *path, struct stat *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "stat");
return real___xstat(ver, path, sb);
}
@@ -274,7 +279,7 @@ __xstat64(int ver, const char *path, struct stat64 *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "stat");
return real___xstat64(ver, path, sb);
}
@@ -286,7 +291,7 @@ lstat(const char *path, struct stat *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "lstat");
return real_lstat(path, sb);
}
@@ -298,7 +303,7 @@ lstat64(const char *path, struct stat64 *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "lstat");
return real_lstat64(path, sb);
}
@@ -310,7 +315,7 @@ __lxstat(int ver, const char *path, struct stat *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "lstat");
return real___lxstat(ver, path, sb);
}
@@ -322,7 +327,7 @@ __lxstat64(int ver, const char *path, struct stat64 *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "lstat");
return real___lxstat64(ver, path, sb);
}
@@ -337,7 +342,7 @@ int connect(int sockfd, const struct sockaddr *addr,
socklen_t addrlen)
if (addrlen == sizeof(struct sockaddr_un)) {
struct sockaddr_un *tmp = (struct sockaddr_un *) addr;
if (tmp->sun_family == AF_UNIX)
-checkPath(tmp->sun_path);
+CHECK_PATH(tmp->sun_path);
}
#endif
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 2/3] check-file-access: Allow specifying action
The check-file-access.pl script is used to match access list
generated by virtestmock against whitelisted rules stored in
file_access_whitelist.txt. So far the rules are in form:
$path: $progname: $testname
This is not sufficient because the rule does not take into
account 'action' that caused $path to appear in the list of
accessed files. After this commit the rule can be in new form:
$path: $action: $progname: $testname
where $action is one from ("open", "fopen", "access", "stat",
"lstat", "connect"). This way the white list can be fine tuned to
allow say access() but not connect().
Signed-off-by: Michal Privoznik
---
tests/check-file-access.pl | 32 +++-
tests/file_access_whitelist.txt | 15 ++-
2 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl
index 977a2bc533..ea0b7a18a2 100755
--- a/tests/check-file-access.pl
+++ b/tests/check-file-access.pl
@@ -27,18 +27,21 @@ use warnings;
my $access_file = "test_file_access.txt";
my $whitelist_file = "file_access_whitelist.txt";
+my @known_actions = ("open", "fopen", "access", "stat", "lstat", "connect");
+
my @files;
my @whitelist;
open FILE, "<", $access_file or die "Unable to open $access_file: $!";
while () {
chomp;
-if (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
+if (/^(\S*):\s*(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
my %rec;
${rec}{path} = $1;
-${rec}{progname} = $2;
-if (defined $4) {
-${rec}{testname} = $4;
+${rec}{action} = $2;
+${rec}{progname} = $3;
+if (defined $5) {
+${rec}{testname} = $5;
}
push (@files, \%rec);
} else {
@@ -52,7 +55,21 @@ while () {
chomp;
if (/^\s*#.*$/) {
# comment
+} elsif (/^(\S*):\s*(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/ and
+grep /^$2$/, @known_actions) {
+# $path: $action: $progname: $testname
+my %rec;
+${rec}{path} = $1;
+${rec}{action} = $3;
+if (defined $4) {
+${rec}{progname} = $4;
+}
+if (defined $6) {
+${rec}{testname} = $6;
+}
+push (@whitelist, \%rec);
} elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) {
+# $path: $progname: $testname
my %rec;
${rec}{path} = $1;
if (defined $3) {
@@ -79,6 +96,11 @@ for my $file (@files) {
next;
}
+if (defined %${rule}{action} and
+not %${file}{action} =~ m/^$rule->{action}$/) {
+next;
+}
+
if (defined %${rule}{progname} and
not %${file}{progname} =~ m/^$rule->{progname}$/) {
next;
@@ -95,7 +117,7 @@ for my $file (@files) {
if (not $match) {
$error = 1;
-print "$file->{path}: $file->{progname}";
+print "$file->{path}: $file->{action}: $file->{progname}";
print ": $file->{testname}" if defined %${file}{testname};
print "\n";
}
diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt
index 850b28506e..3fb318cbab 100644
--- a/tests/file_access_whitelist.txt
+++ b/tests/file_access_whitelist.txt
@@ -1,14 +1,17 @@
# This is a whitelist that allows accesses to files not in our
# build directory nor source directory. The records are in the
-# following format:
+# following formats:
#
# $path: $progname: $testname
+# $path: $action: $progname: $testname
#
-# All these three are evaluated as perl RE. So to allow /dev/sda
-# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
+# All these variables are evaluated as perl RE. So to allow
+# /dev/sda and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
# /proc/$pid/status you can '/proc/\d+/status' and so on.
-# Moreover, $progname and $testname can be empty, in which which
-# case $path is allowed for all tests.
+# Moreover, $action, $progname and $testname can be empty, in which
+# which case $path is allowed for all tests. However, $action (if
+# specified) must be one of "open", "fopen", "access", "stat",
+# "lstat", "connect".
/bin/cat: sysinfotest
/bin/dirname: sysinfotest: x86 sysinfo
@@ -19,5 +22,7 @@
/etc/hosts
/proc/\d+/status
+/etc/passwd: fopen
+
# This is just a dummy example, DO NOT USE IT LIKE THAT!
.*: nonexistent-test-touching-everything
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/3] qemuxml2argvtest: Set more fake drivers
So far we are setting only fake secret and storage drivers. Therefore if the code wants to call a public NWFilter API (like qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are doing) the virGetConnectNWFilter() function will try to actually spawn session daemon because there's no connection object set to handle NWFilter driver. Even though I haven't experienced the same problem with the rest of the drivers (interface, network and node dev), the reasoning above can be applied to them as well. At the same time, now that connection object is registered for the drivers, the public APIs will throw virReportUnsupportedError(). And since we don't provide any error func the error is printed to stderr. Fix this by setting dummy error func. Signed-off-by: Michal Privoznik --- tests/qemuxml2argvtest.c | 4 1 file changed, 4 insertions(+) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 84117a3e63..8901c7bde4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -471,6 +471,10 @@ testCompareXMLToArgv(const void *data) conn->secretDriver = &fakeSecretDriver; conn->storageDriver = &fakeStorageDriver; +virSetConnectInterface(conn); +virSetConnectNetwork(conn); +virSetConnectNWFilter(conn); +virSetConnectNodeDev(conn); virSetConnectSecret(conn); virSetConnectStorage(conn); -- 2.16.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 0/3] Don't touch user data from tests
v3 of: https://www.redhat.com/archives/libvir-list/2018-July/msg00747.html diff to v2: - pushed some already ACKed patches - dropped controversial line from 1/3 that turned off error reporting Michal Prívozník (3): qemuxml2argvtest: Set more fake drivers check-file-access: Allow specifying action virtestmock: Track action tests/check-file-access.pl | 32 +++- tests/file_access_whitelist.txt | 15 ++- tests/qemuxml2argvtest.c| 4 tests/virtestmock.c | 39 ++- 4 files changed, 63 insertions(+), 27 deletions(-) -- 2.16.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 00/41] use GNU C's cleanup attribute in src/util (batch II)
On Tue, Jul 24, 2018 at 09:22:01PM +0530, Sukrit Bhatnagar wrote: > This second series of patches also modifies a few files in src/util > to use VIR_AUTOFREE and VIR_AUTOPTR for automatic freeing of memory > and get rid of some VIR_FREE macro invocations and *Free function > calls. > > The argument type of virCgroupFree is changed from virCgroupPtr * > to virCgroupPtr and that of virUSBDeviceListAdd is changed to take > a double pointer to virUSBDevice. So I tweaked a few patches as noted in my review and pushed the series. I left out the iscsi patches as there were some merge conflicts that need to be resolved. Thanks, Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/3] qemuxml2argvtest: Set more fake drivers
On Fri, Jul 27, 2018 at 05:24:45PM +0200, Michal Privoznik wrote: > So far we are setting only fake secret and storage drivers. > Therefore if the code wants to call a public NWFilter API (like > qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are > doing) the virGetConnectNWFilter() function will try to actually > spawn session daemon because there's no connection object set to > handle NWFilter driver. > > Even though I haven't experienced the same problem with the rest > of the drivers (interface, network and node dev), the reasoning > above can be applied to them as well. > > At the same time, now that connection object is registered for > the drivers, the public APIs will throw > virReportUnsupportedError(). And since we don't provide any error > func the error is printed to stderr. Fix this by setting dummy > error func. Is this last paragraph stale ? you're not seeing a dummy error func in this patch ... > > Signed-off-by: Michal Privoznik > --- > tests/qemuxml2argvtest.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index 84117a3e63..8901c7bde4 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -471,6 +471,10 @@ testCompareXMLToArgv(const void *data) > conn->secretDriver = &fakeSecretDriver; > conn->storageDriver = &fakeStorageDriver; > > +virSetConnectInterface(conn); > +virSetConnectNetwork(conn); > +virSetConnectNWFilter(conn); > +virSetConnectNodeDev(conn); > virSetConnectSecret(conn); > virSetConnectStorage(conn); > > -- > 2.16.4 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/3] qemuxml2argvtest: Set more fake drivers
On 07/27/2018 05:29 PM, Daniel P. Berrangé wrote: > On Fri, Jul 27, 2018 at 05:24:45PM +0200, Michal Privoznik wrote: >> So far we are setting only fake secret and storage drivers. >> Therefore if the code wants to call a public NWFilter API (like >> qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are >> doing) the virGetConnectNWFilter() function will try to actually >> spawn session daemon because there's no connection object set to >> handle NWFilter driver. >> >> Even though I haven't experienced the same problem with the rest >> of the drivers (interface, network and node dev), the reasoning >> above can be applied to them as well. >> >> At the same time, now that connection object is registered for >> the drivers, the public APIs will throw >> virReportUnsupportedError(). And since we don't provide any error >> func the error is printed to stderr. Fix this by setting dummy >> error func. > > Is this last paragraph stale ? you're not seeing a dummy error > func in this patch ... Oh yes, I am: libvirt.git/tests $ VIR_TEST_DEBUG=1 ./qemuxml2argvtest 2>&1 | grep "this function is not supported" 441) QEMU XML-2-ARGV net-vhostuser-multiq ... libvirt: Network Filter Driver error : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev libvirt: Network Filter Driver error : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev libvirt: Network Filter Driver error : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev 2018-07-27 16:02:50.689+: 11166: error : virNWFilterBindingLookupByPortDev:599 : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev 2018-07-27 16:02:50.689+: 11166: error : virNWFilterBindingLookupByPortDev:599 : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev 2018-07-27 16:02:50.689+: 11166: error : virNWFilterBindingLookupByPortDev:599 : this function is not supported by the connection driver: virNWFilterBindingLookupByPortDev (Or even without DEBUG) But as I told Peter, at this point stopping touching live user data is more important to me than silencing some error message. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virpci: Drop unused @ret in virPCIDeviceListDel
So after 00dc991ca16730 the function is one line long and the
line is declaring a variable which is never used in fact. Replace
it with actual free() call instead of autofree.
Signed-off-by: Michal Privoznik
---
Pushed under build breaker rule as this is causing clang to complain.
Huh, who uses that? :-P
src/util/virpci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 634df8d35f..6cf2acf2d1 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2003,7 +2003,7 @@ void
virPCIDeviceListDel(virPCIDeviceListPtr list,
virPCIDevicePtr dev)
{
-VIR_AUTOPTR(virPCIDevice) ret = virPCIDeviceListSteal(list, dev);
+virPCIDeviceFree(virPCIDeviceListSteal(list, dev));
}
int
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] conf: Refactor virDomainCachetuneDefParse
[...]
>>
>> Secondary to that you've added a new error related to identical vcpus in
>> the parsing logic. Unfortunately that could make a domain disappear on a
>> libvirtd restart if for some reason it was already defined in that
>> manner. If there's a parsing error because two entries had identical
>> cpus, then there will be an error. Errors would cause a previously
>> OK/found domain to not be defined. So that type of check (now that the
>
> Sorry, I am not sure if I understand correctly. Are you talking about
> two domains(or VMs) with the same vcpus setting?
It's the:
+} else {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Identical vcpus in cachetunes found"));
+goto cleanup;
+}
that was new... Consider what would happen before any of these changes
were merged if that condition was true.
So I found tests/genericxml2xmlindata/cachetune-colliding-tunes.xml and
did a little test... Before your changes, the test fails with "error:
XML error: Overlapping vcpus in cachetunes", but with your changes the
test fails with "error: XML error: Identical vcpus in cachetunes found".
So since both fail, that's good, no issue then. It was different which
is what drew my attention. In any case, just mention it in the commit
message that part of the change will "clarify" whether it's an overlap
or a redefinition.
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] audit: Share virtType fallback logic
On 07/26/2018 01:59 PM, Cole Robinson wrote: > Signed-off-by: Cole Robinson > --- > src/conf/domain_audit.c | 91 + > 1 file changed, 28 insertions(+), 63 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: clang is failing due to unused variables.
After some recent patches, clang is throwing some errors related to
unused variables. This is not happening when we use GCC with -Werror
enabled. Only clang reports this warning.
make[3]: Entering directory '/home/julio/Desktop/virt/libvirt/src'
CC util/libvirt_util_la-virscsivhost.lo
CC util/libvirt_util_la-virusb.lo
CC util/libvirt_util_la-virmdev.lo
util/virmdev.c:373:36: error: unused variable 'ret' [-Werror,-Wunused-variable]
VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list, dev);
^
1 error generated.
Makefile:11579: recipe for target 'util/libvirt_util_la-virmdev.lo' failed
make[3]: *** [util/libvirt_util_la-virmdev.lo] Error 1
make[3]: *** Waiting for unfinished jobs
util/virscsivhost.c:112:37: error: unused variable 'tmp'
[-Werror,-Wunused-variable]
VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list,
dev);
^
1 error generated.
Makefile:11411: recipe for target 'util/libvirt_util_la-virscsivhost.lo' failed
make[3]: *** [util/libvirt_util_la-virscsivhost.lo] Error 1
util/virusb.c:511:31: error: unused variable 'ret' [-Werror,-Wunused-variable]
VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
Signed-off-by: Julio Faracco
---
src/util/virmdev.c | 3 ++-
src/util/virscsivhost.c | 3 ++-
src/util/virusb.c | 3 ++-
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 4050835cc1..63df4e40d8 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -370,7 +370,8 @@ void
virMediatedDeviceListDel(virMediatedDeviceListPtr list,
virMediatedDevicePtr dev)
{
-VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list, dev);
+VIR_AUTOPTR(virMediatedDevice) ret ATTRIBUTE_UNUSED
+= virMediatedDeviceListSteal(list, dev);
}
diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c
index 280d0dc2fd..e07cd46ba9 100644
--- a/src/util/virscsivhost.c
+++ b/src/util/virscsivhost.c
@@ -109,7 +109,8 @@ void
virSCSIVHostDeviceListDel(virSCSIVHostDeviceListPtr list,
virSCSIVHostDevicePtr dev)
{
-VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list,
dev);
+VIR_AUTOPTR(virSCSIVHostDevice) tmp ATTRIBUTE_UNUSED
+= virSCSIVHostDeviceListSteal(list, dev);
}
diff --git a/src/util/virusb.c b/src/util/virusb.c
index 609d54836f..73a1041097 100644
--- a/src/util/virusb.c
+++ b/src/util/virusb.c
@@ -508,7 +508,8 @@ void
virUSBDeviceListDel(virUSBDeviceListPtr list,
virUSBDevicePtr dev)
{
-VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
+VIR_AUTOPTR(virUSBDevice) ret ATTRIBUTE_UNUSED
+= virUSBDeviceListSteal(list, dev);
}
virUSBDevicePtr
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: clang is failing due to unused variables.
On 07/27/2018 04:50 PM, Julio Faracco wrote:
> After some recent patches, clang is throwing some errors related to
> unused variables. This is not happening when we use GCC with -Werror
> enabled. Only clang reports this warning.
>
> make[3]: Entering directory '/home/julio/Desktop/virt/libvirt/src'
> CC util/libvirt_util_la-virscsivhost.lo
> CC util/libvirt_util_la-virusb.lo
> CC util/libvirt_util_la-virmdev.lo
> util/virmdev.c:373:36: error: unused variable 'ret'
> [-Werror,-Wunused-variable]
> VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list,
> dev);
>^
> 1 error generated.
> Makefile:11579: recipe for target 'util/libvirt_util_la-virmdev.lo' failed
> make[3]: *** [util/libvirt_util_la-virmdev.lo] Error 1
> make[3]: *** Waiting for unfinished jobs
> util/virscsivhost.c:112:37: error: unused variable 'tmp'
> [-Werror,-Wunused-variable]
> VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list,
> dev);
> ^
> 1 error generated.
> Makefile:11411: recipe for target 'util/libvirt_util_la-virscsivhost.lo'
> failed
> make[3]: *** [util/libvirt_util_la-virscsivhost.lo] Error 1
> util/virusb.c:511:31: error: unused variable 'ret' [-Werror,-Wunused-variable]
> VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
>
> Signed-off-by: Julio Faracco
> ---
> src/util/virmdev.c | 3 ++-
> src/util/virscsivhost.c | 3 ++-
> src/util/virusb.c | 3 ++-
> 3 files changed, 6 insertions(+), 3 deletions(-)
>
, see:
https://www.redhat.com/archives/libvir-list/2018-July/msg01917.html
Seems like it's the same thing and we should be consistent in the manner
in which we resolve.
John
> diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> index 4050835cc1..63df4e40d8 100644
> --- a/src/util/virmdev.c
> +++ b/src/util/virmdev.c
> @@ -370,7 +370,8 @@ void
> virMediatedDeviceListDel(virMediatedDeviceListPtr list,
> virMediatedDevicePtr dev)
> {
> -VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list,
> dev);
> +VIR_AUTOPTR(virMediatedDevice) ret ATTRIBUTE_UNUSED
> += virMediatedDeviceListSteal(list, dev);
> }
>
>
> diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c
> index 280d0dc2fd..e07cd46ba9 100644
> --- a/src/util/virscsivhost.c
> +++ b/src/util/virscsivhost.c
> @@ -109,7 +109,8 @@ void
> virSCSIVHostDeviceListDel(virSCSIVHostDeviceListPtr list,
>virSCSIVHostDevicePtr dev)
> {
> -VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list,
> dev);
> +VIR_AUTOPTR(virSCSIVHostDevice) tmp ATTRIBUTE_UNUSED
> += virSCSIVHostDeviceListSteal(list, dev);
> }
>
>
> diff --git a/src/util/virusb.c b/src/util/virusb.c
> index 609d54836f..73a1041097 100644
> --- a/src/util/virusb.c
> +++ b/src/util/virusb.c
> @@ -508,7 +508,8 @@ void
> virUSBDeviceListDel(virUSBDeviceListPtr list,
> virUSBDevicePtr dev)
> {
> -VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
> +VIR_AUTOPTR(virUSBDevice) ret ATTRIBUTE_UNUSED
> += virUSBDeviceListSteal(list, dev);
> }
>
> virUSBDevicePtr
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: clang is failing due to unused variables.
On 07/27/2018 03:58 PM, John Ferlan wrote: On 07/27/2018 04:50 PM, Julio Faracco wrote: After some recent patches, clang is throwing some errors related to unused variables. This is not happening when we use GCC with -Werror enabled. Only clang reports this warning. make[3]: Entering directory '/home/julio/Desktop/virt/libvirt/src' CC util/libvirt_util_la-virscsivhost.lo CC util/libvirt_util_la-virusb.lo CC util/libvirt_util_la-virmdev.lo util/virmdev.c:373:36: error: unused variable 'ret' [-Werror,-Wunused-variable] VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list, dev); ^ clang is buggy. The variable 'ret' is very much in use, as the VIR_AUTOPTR() macro cannot work unless you attach it to a local variable to operate on when that variable goes out of scope. You should file a bug report against clang. But in the meantime, , see: https://www.redhat.com/archives/libvir-list/2018-July/msg01917.html Seems like it's the same thing and we should be consistent in the manner in which we resolve. Yes, that approach is much nicer - it is also less typing and less magic. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: clang is failing due to unused variables.
Ow,
I was pretty sure that clang would complain about not handling the function
return properly. Well, I changed it and clang is not throwing any error. Weird!
Sending a V2.
Michael's patch was applied, so we need a new one to remove the other
occurrences.
Em sex, 27 de jul de 2018 às 17:59, John Ferlan escreveu:
>
>
>
> On 07/27/2018 04:50 PM, Julio Faracco wrote:
> > After some recent patches, clang is throwing some errors related to
> > unused variables. This is not happening when we use GCC with -Werror
> > enabled. Only clang reports this warning.
> >
> > make[3]: Entering directory '/home/julio/Desktop/virt/libvirt/src'
> > CC util/libvirt_util_la-virscsivhost.lo
> > CC util/libvirt_util_la-virusb.lo
> > CC util/libvirt_util_la-virmdev.lo
> > util/virmdev.c:373:36: error: unused variable 'ret'
> > [-Werror,-Wunused-variable]
> > VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list,
> > dev);
> >^
> > 1 error generated.
> > Makefile:11579: recipe for target 'util/libvirt_util_la-virmdev.lo' failed
> > make[3]: *** [util/libvirt_util_la-virmdev.lo] Error 1
> > make[3]: *** Waiting for unfinished jobs
> > util/virscsivhost.c:112:37: error: unused variable 'tmp'
> > [-Werror,-Wunused-variable]
> > VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list,
> > dev);
> > ^
> > 1 error generated.
> > Makefile:11411: recipe for target 'util/libvirt_util_la-virscsivhost.lo'
> > failed
> > make[3]: *** [util/libvirt_util_la-virscsivhost.lo] Error 1
> > util/virusb.c:511:31: error: unused variable 'ret'
> > [-Werror,-Wunused-variable]
> > VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
> >
> > Signed-off-by: Julio Faracco
> > ---
> > src/util/virmdev.c | 3 ++-
> > src/util/virscsivhost.c | 3 ++-
> > src/util/virusb.c | 3 ++-
> > 3 files changed, 6 insertions(+), 3 deletions(-)
> >
>
> , see:
>
> https://www.redhat.com/archives/libvir-list/2018-July/msg01917.html
>
> Seems like it's the same thing and we should be consistent in the manner
> in which we resolve.
>
> John
>
> > diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> > index 4050835cc1..63df4e40d8 100644
> > --- a/src/util/virmdev.c
> > +++ b/src/util/virmdev.c
> > @@ -370,7 +370,8 @@ void
> > virMediatedDeviceListDel(virMediatedDeviceListPtr list,
> > virMediatedDevicePtr dev)
> > {
> > -VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list,
> > dev);
> > +VIR_AUTOPTR(virMediatedDevice) ret ATTRIBUTE_UNUSED
> > += virMediatedDeviceListSteal(list, dev);
> > }
> >
> >
> > diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c
> > index 280d0dc2fd..e07cd46ba9 100644
> > --- a/src/util/virscsivhost.c
> > +++ b/src/util/virscsivhost.c
> > @@ -109,7 +109,8 @@ void
> > virSCSIVHostDeviceListDel(virSCSIVHostDeviceListPtr list,
> >virSCSIVHostDevicePtr dev)
> > {
> > -VIR_AUTOPTR(virSCSIVHostDevice) tmp =
> > virSCSIVHostDeviceListSteal(list, dev);
> > +VIR_AUTOPTR(virSCSIVHostDevice) tmp ATTRIBUTE_UNUSED
> > += virSCSIVHostDeviceListSteal(list, dev);
> > }
> >
> >
> > diff --git a/src/util/virusb.c b/src/util/virusb.c
> > index 609d54836f..73a1041097 100644
> > --- a/src/util/virusb.c
> > +++ b/src/util/virusb.c
> > @@ -508,7 +508,8 @@ void
> > virUSBDeviceListDel(virUSBDeviceListPtr list,
> > virUSBDevicePtr dev)
> > {
> > -VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
> > +VIR_AUTOPTR(virUSBDevice) ret ATTRIBUTE_UNUSED
> > += virUSBDeviceListSteal(list, dev);
> > }
> >
> > virUSBDevicePtr
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] util: clang is failing to compile due to unused variables.
After some recent patches, clang is throwing some errors related to
unused variables. This is not happening when we use GCC with -Werror
enabled. Only clang reports this warning.
make[3]: Entering directory '/home/julio/Desktop/virt/libvirt/src'
CC util/libvirt_util_la-virscsivhost.lo
CC util/libvirt_util_la-virusb.lo
CC util/libvirt_util_la-virmdev.lo
util/virmdev.c:373:36: error: unused variable 'ret' [-Werror,-Wunused-variable]
VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list, dev);
^
1 error generated.
Makefile:11579: recipe for target 'util/libvirt_util_la-virmdev.lo' failed
make[3]: *** [util/libvirt_util_la-virmdev.lo] Error 1
make[3]: *** Waiting for unfinished jobs
util/virscsivhost.c:112:37: error: unused variable 'tmp'
[-Werror,-Wunused-variable]
VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list,
dev);
^
1 error generated.
Makefile:11411: recipe for target 'util/libvirt_util_la-virscsivhost.lo' failed
make[3]: *** [util/libvirt_util_la-virscsivhost.lo] Error 1
util/virusb.c:511:31: error: unused variable 'ret' [-Werror,-Wunused-variable]
VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
Signed-off-by: Julio Faracco
---
src/util/virmdev.c | 2 +-
src/util/virscsivhost.c | 2 +-
src/util/virusb.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 4050835cc1..4492fd673e 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -370,7 +370,7 @@ void
virMediatedDeviceListDel(virMediatedDeviceListPtr list,
virMediatedDevicePtr dev)
{
-VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list, dev);
+virMediatedDeviceListSteal(list, dev);
}
diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c
index 280d0dc2fd..1a069e67ff 100644
--- a/src/util/virscsivhost.c
+++ b/src/util/virscsivhost.c
@@ -109,7 +109,7 @@ void
virSCSIVHostDeviceListDel(virSCSIVHostDeviceListPtr list,
virSCSIVHostDevicePtr dev)
{
-VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list,
dev);
+virSCSIVHostDeviceListSteal(list, dev);
}
diff --git a/src/util/virusb.c b/src/util/virusb.c
index 609d54836f..d14b7623cc 100644
--- a/src/util/virusb.c
+++ b/src/util/virusb.c
@@ -508,7 +508,7 @@ void
virUSBDeviceListDel(virUSBDeviceListPtr list,
virUSBDevicePtr dev)
{
-VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
+virUSBDeviceListSteal(list, dev);
}
virUSBDevicePtr
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Remove unnecessary virQEMUDriverPtr arguments
Signed-off-by Steven Albertson
---
src/qemu/qemu_block.c | 15 +++
src/qemu/qemu_block.h | 6 ++
2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 66e6301210..8081bced9d 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -339,8 +339,7 @@ qemuBlockDiskDetectNodes(virDomainDiskDefPtr disk,
int
-qemuBlockNodeNamesDetect(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuBlockNodeNamesDetect(virDomainObjPtr vm,
qemuDomainAsyncJob asyncJob)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -354,13 +353,13 @@ qemuBlockNodeNamesDetect(virQEMUDriverPtr driver,
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_NAMED_BLOCK_NODES))
return 0;
-if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0)
return -1;
data = qemuMonitorQueryNamedBlockNodes(qemuDomainGetMonitor(vm));
blockstats = qemuMonitorQueryBlockstats(qemuDomainGetMonitor(vm), false);
-if (qemuDomainObjExitMonitor(driver, vm) < 0 || !data || !blockstats)
+if (qemuDomainObjExitMonitor(priv->driver, vm) < 0 || !data || !blockstats)
goto cleanup;
if (!(disktable = qemuBlockNodeNameGetBackingChain(data, blockstats)))
@@ -1689,14 +1688,14 @@ qemuBlockStorageSourceAttachRollback(qemuMonitorPtr mon,
* monitor internally.
*/
int
-qemuBlockStorageSourceDetachOneBlockdev(virQEMUDriverPtr driver,
-virDomainObjPtr vm,
+qemuBlockStorageSourceDetachOneBlockdev(virDomainObjPtr vm,
qemuDomainAsyncJob asyncJob,
virStorageSourcePtr src)
{
+qemuDomainObjPrivatePtr priv = vm->privateData;
int ret;
-if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0)
return -1;
ret = qemuMonitorBlockdevDel(qemuDomainGetMonitor(vm), src->nodeformat);
@@ -1704,7 +1703,7 @@ qemuBlockStorageSourceDetachOneBlockdev(virQEMUDriverPtr
driver,
if (ret == 0)
ret = qemuMonitorBlockdevDel(qemuDomainGetMonitor(vm),
src->nodestorage);
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
+if (qemuDomainObjExitMonitor(priv->driver, vm) < 0)
return -1;
return ret;
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index fd8984e60b..725ce936d5 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -47,8 +47,7 @@ qemuBlockNodeNameGetBackingChain(virJSONValuePtr
namednodesdata,
virJSONValuePtr blockstats);
int
-qemuBlockNodeNamesDetect(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuBlockNodeNamesDetect(virDomainObjPtr vm,
qemuDomainAsyncJob asyncJob);
virHashTablePtr
@@ -112,8 +111,7 @@ qemuBlockStorageSourceAttachRollback(qemuMonitorPtr mon,
qemuBlockStorageSourceAttachDataPtr data);
int
-qemuBlockStorageSourceDetachOneBlockdev(virQEMUDriverPtr driver,
-virDomainObjPtr vm,
+qemuBlockStorageSourceDetachOneBlockdev(virDomainObjPtr vm,
qemuDomainAsyncJob asyncJob,
virStorageSourcePtr src);
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] util: clang is failing to compile due to unused variables.
On 07/27/2018 05:17 PM, Julio Faracco wrote:
> After some recent patches, clang is throwing some errors related to
> unused variables. This is not happening when we use GCC with -Werror
> enabled. Only clang reports this warning.
>
> make[3]: Entering directory '/home/julio/Desktop/virt/libvirt/src'
> CC util/libvirt_util_la-virscsivhost.lo
> CC util/libvirt_util_la-virusb.lo
> CC util/libvirt_util_la-virmdev.lo
> util/virmdev.c:373:36: error: unused variable 'ret'
> [-Werror,-Wunused-variable]
> VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list,
> dev);
>^
> 1 error generated.
> Makefile:11579: recipe for target 'util/libvirt_util_la-virmdev.lo' failed
> make[3]: *** [util/libvirt_util_la-virmdev.lo] Error 1
> make[3]: *** Waiting for unfinished jobs
> util/virscsivhost.c:112:37: error: unused variable 'tmp'
> [-Werror,-Wunused-variable]
> VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list,
> dev);
> ^
> 1 error generated.
> Makefile:11411: recipe for target 'util/libvirt_util_la-virscsivhost.lo'
> failed
> make[3]: *** [util/libvirt_util_la-virscsivhost.lo] Error 1
> util/virusb.c:511:31: error: unused variable 'ret' [-Werror,-Wunused-variable]
> VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
>
> Signed-off-by: Julio Faracco
> ---
> src/util/virmdev.c | 2 +-
> src/util/virscsivhost.c | 2 +-
> src/util/virusb.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
Close, but you forgot something on each.
> diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> index 4050835cc1..4492fd673e 100644
> --- a/src/util/virmdev.c
> +++ b/src/util/virmdev.c
> @@ -370,7 +370,7 @@ void
> virMediatedDeviceListDel(virMediatedDeviceListPtr list,
> virMediatedDevicePtr dev)
> {
> -VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list,
> dev);
> +virMediatedDeviceListSteal(list, dev);
Wrap a "virMediatedDeviceFree()" around this
> }
>
>
> diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c
> index 280d0dc2fd..1a069e67ff 100644
> --- a/src/util/virscsivhost.c
> +++ b/src/util/virscsivhost.c
> @@ -109,7 +109,7 @@ void
> virSCSIVHostDeviceListDel(virSCSIVHostDeviceListPtr list,
>virSCSIVHostDevicePtr dev)
> {
> -VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list,
> dev);
> +virSCSIVHostDeviceListSteal(list, dev);
Wrap a "virSCSIVHostDeviceFree()" around this.
> }
>
>
> diff --git a/src/util/virusb.c b/src/util/virusb.c
> index 609d54836f..d14b7623cc 100644
> --- a/src/util/virusb.c
> +++ b/src/util/virusb.c
> @@ -508,7 +508,7 @@ void
> virUSBDeviceListDel(virUSBDeviceListPtr list,
> virUSBDevicePtr dev)
> {
> -VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
> +virUSBDeviceListSteal(list, dev);
Wrap a "virUSBDeviceFree()" around this.
I fixed those and pushed.
Tks,
John
> }
>
> virUSBDevicePtr
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] conf: Refactor virDomainCachetuneDefParse
On 2018年07月28日 00:58, John Ferlan wrote:
[...]
Secondary to that you've added a new error related to identical vcpus in
the parsing logic. Unfortunately that could make a domain disappear on a
libvirtd restart if for some reason it was already defined in that
manner. If there's a parsing error because two entries had identical
cpus, then there will be an error. Errors would cause a previously
OK/found domain to not be defined. So that type of check (now that the
Sorry, I am not sure if I understand correctly. Are you talking about
two domains(or VMs) with the same vcpus setting?
It's the:
+} else {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Identical vcpus in cachetunes found"));
+goto cleanup;
+}
that was new... Consider what would happen before any of these changes
were merged if that condition was true.
So I found tests/genericxml2xmlindata/cachetune-colliding-tunes.xml and
did a little test... Before your changes, the test fails with "error:
XML error: Overlapping vcpus in cachetunes", but with your changes the
test fails with "error: XML error: Identical vcpus in cachetunes found".
Yes! You are right. The error message is different.
So since both fail, that's good, no issue then. It was different which
is what drew my attention. In any case, just mention it in the commit
message that part of the change will "clarify" whether it's an overlap
or a redefinition.
Okay~, will update commit message in v2.
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 02/17] util: Refactor virResctrlGetInfo in virresctrl
From: Bing Niu
Separate resctrl common information parts from CAT specific parts,
so that common information parts can be reused among different
resource allocation technologies.
Signed-off-by: Bing Niu
Reviewed-by: John Ferlan
---
src/util/virresctrl.c | 32 +++-
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 6d69c8d..313f964 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -318,9 +318,9 @@ virResctrlUnlock(int fd)
/* virResctrlInfo-related definitions */
static int
-virResctrlGetInfo(virResctrlInfoPtr resctrl)
+virResctrlGetCacheInfo(virResctrlInfoPtr resctrl,
+ DIR *dirp)
{
-DIR *dirp = NULL;
char *endptr = NULL;
char *tmp_str = NULL;
int ret = -1;
@@ -332,12 +332,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
virResctrlInfoPerLevelPtr i_level = NULL;
virResctrlInfoPerTypePtr i_type = NULL;
-rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
-if (rv <= 0) {
-ret = rv;
-goto cleanup;
-}
-
while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) {
VIR_DEBUG("Parsing info type '%s'", ent->d_name);
if (ent->d_name[0] != 'L')
@@ -443,12 +437,32 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
ret = 0;
cleanup:
-VIR_DIR_CLOSE(dirp);
VIR_FREE(i_type);
return ret;
}
+static int
+virResctrlGetInfo(virResctrlInfoPtr resctrl)
+{
+DIR *dirp = NULL;
+int ret = -1;
+
+ret = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
+if (ret <= 0)
+goto cleanup;
+
+ret = virResctrlGetCacheInfo(resctrl, dirp);
+if (ret < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+VIR_DIR_CLOSE(dirp);
+return ret;
+}
+
+
virResctrlInfoPtr
virResctrlInfoNew(void)
{
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 05/17] util: Add MBA check to virResctrlInfoGetCache
From: Bing Niu
If we have some membw_info data, then we need to calculate the number
of MBA controllers on the system. The value cannot be obtained from a
direct query to the RDT kernel module, but it is the same as the last
level cache value which is calculated by traversing the cache hierarchy
of host(/sys/bus/cpu/devices/cpuX/cache/).
Signed-off-by: Bing Niu
Reviewed-by: John Ferlan
---
src/util/virresctrl.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index b12a05c..f454868 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -608,6 +608,20 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
if (virResctrlInfoIsEmpty(resctrl))
return 0;
+/* Let's take the opportunity to update the number of last level
+ * cache. This number of memory bandwidth controller is same with
+ * last level cache */
+if (resctrl->membw_info) {
+virResctrlInfoMemBWPtr membw_info = resctrl->membw_info;
+
+if (level > membw_info->last_level_cache) {
+membw_info->last_level_cache = level;
+membw_info->max_id = 0;
+} else if (membw_info->last_level_cache == level) {
+membw_info->max_id++;
+}
+}
+
if (level >= resctrl->nlevels)
return 0;
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 01/17] util: Rename some functions of virresctrl
From: Bing Niu
Some functions in virresctrl are for CAT only, while some of other
functions are for resource allocation, not just CAT. So change
their names to reflect the reality.
Signed-off-by: Bing Niu
Reviewed-by: John Ferlan
---
src/conf/domain_conf.c | 8
src/libvirt_private.syms | 4 ++--
src/util/virresctrl.c| 30 +++---
src/util/virresctrl.h| 26 +-
4 files changed, 34 insertions(+), 34 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index eff8af2..abb6c5e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19013,7 +19013,7 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
ULLONG_MAX, true) < 0)
goto cleanup;
-if (virResctrlAllocSetSize(alloc, level, type, cache, size) < 0)
+if (virResctrlAllocSetCacheSize(alloc, level, type, cache, size) < 0)
goto cleanup;
ret = 0;
@@ -27002,9 +27002,9 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
int ret = -1;
virBufferSetChildIndent(&childrenBuf, buf);
-virResctrlAllocForeachSize(cachetune->alloc,
- virDomainCachetuneDefFormatHelper,
- &childrenBuf);
+virResctrlAllocForeachCache(cachetune->alloc,
+virDomainCachetuneDefFormatHelper,
+&childrenBuf);
if (virBufferCheckError(&childrenBuf) < 0)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fc386e1..bc489cb 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2648,14 +2648,14 @@ virCacheTypeToString;
virResctrlAllocAddPID;
virResctrlAllocCreate;
virResctrlAllocDeterminePath;
-virResctrlAllocForeachSize;
+virResctrlAllocForeachCache;
virResctrlAllocFormat;
virResctrlAllocGetID;
virResctrlAllocGetUnused;
virResctrlAllocNew;
virResctrlAllocRemove;
+virResctrlAllocSetCacheSize;
virResctrlAllocSetID;
-virResctrlAllocSetSize;
virResctrlInfoGetCache;
virResctrlInfoNew;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index e492a63..6d69c8d 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -754,11 +754,11 @@ virResctrlAllocCheckCollision(virResctrlAllocPtr alloc,
int
-virResctrlAllocSetSize(virResctrlAllocPtr alloc,
- unsigned int level,
- virCacheType type,
- unsigned int cache,
- unsigned long long size)
+virResctrlAllocSetCacheSize(virResctrlAllocPtr alloc,
+unsigned int level,
+virCacheType type,
+unsigned int cache,
+unsigned long long size)
{
if (virResctrlAllocCheckCollision(alloc, level, type, cache)) {
virReportError(VIR_ERR_XML_ERROR,
@@ -773,9 +773,9 @@ virResctrlAllocSetSize(virResctrlAllocPtr alloc,
int
-virResctrlAllocForeachSize(virResctrlAllocPtr alloc,
- virResctrlAllocForeachSizeCallback cb,
- void *opaque)
+virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
+virResctrlAllocForeachCacheCallback cb,
+void *opaque)
{
int ret = 0;
unsigned int level = 0;
@@ -939,9 +939,9 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl,
static int
-virResctrlAllocParseProcessLine(virResctrlInfoPtr resctrl,
-virResctrlAllocPtr alloc,
-char *line)
+virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl,
+ virResctrlAllocPtr alloc,
+ char *line)
{
char **caches = NULL;
char *tmp = NULL;
@@ -1009,7 +1009,7 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl,
lines = virStringSplitCount(schemata, "\n", 0, &nlines);
for (i = 0; i < nlines; i++) {
-if (virResctrlAllocParseProcessLine(resctrl, alloc, lines[i]) < 0)
+if (virResctrlAllocParseCacheLine(resctrl, alloc, lines[i]) < 0)
goto cleanup;
}
@@ -1401,8 +1401,8 @@ virResctrlAllocCopyMasks(virResctrlAllocPtr dst,
* transforming `sizes` into `masks`.
*/
static int
-virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl,
- virResctrlAllocPtr alloc)
+virResctrlAllocAssign(virResctrlInfoPtr resctrl,
+ virResctrlAllocPtr alloc)
{
int ret = -1;
unsigned int level = 0;
@@ -1526,7 +1526,7 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
if (lockfd < 0)
goto cleanup;
-if (virResctrlAllocMasksAssign(resctrl, alloc) < 0)
+if (virResctrlAllocAssign(resctrl, alloc) < 0)
goto cleanup;
alloc_str = virResctrlAllocFormat(alloc);
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index 9052a2b..d657c06 100644
--- a/src/util/v
[libvirt] [PATCH v2 00/17] Introduce RDT memory bandwidth allocation support
From: Bing Niu This series is to introduce RDT memory bandwidth allocation support by extending current virresctrl implementation. The Memory Bandwidth Allocation (MBA) feature provides indirect and approximate control over memory bandwidth available per-core. This feature provides a method to control applications which may be over-utilizing bandwidth relative to their priority in environments such as the data-center. The details can be found in Intel's SDM 17.19.7. Kernel supports MBA through resctrl file system same as CAT. Each resctrl group have a MB parameter to control how much memory bandwidth it can utilize in unit of percentage. In this series, MBA is enabled by enhancing existing virresctrl implementation. The policy employed for MBA is similar with CAT: The sum of each MBA group's bandwidth dose not exceed 100%. The enhancement of virresctrl include two main parts: Part 1: Add two new structures virResctrlInfoMemMB and virResctrlAllocMemBW for collecting host system MBA capability and domain memory bandwidth allocation. Those two structures are the extension of existing virResctrlInfo and virResctrlAlloc. With them, virresctrl framework can support MBA and CAT concurrently. Each virResctrlAlloc represent a resource allocation including CAT, or MBA, or CAT&MBA. The policy of MBA is that: total memory bandwidth of each resctrl group, created by virresctrl, does not exceed to 100%. Part 2: On XML part, add new elements to host capabilities query and domain allocation to support memory bandwidth allocation. - For host capabilities XML, new XML format like below example, . granularity --- memory bandwidth granularity min --- minimum memory bandwidth allowed maxAllocs --- maximum concurrent memory bandwidth allocation allowed. - For domain XML, new format as below example .. .. 1024 .. id --- node where memory bandwidth allocation will happen bandwidth --- bandwidth allocated in percentage -- With this extension of the virresctrl, the overall working follow of CAT and MBA is described by below picture. XML parser will aggregate MBA and CAT configuration and represents it in one virresctrl object. The methods of virresctrl class will manipulate resctrl interface to allocate corresponding resources. +-+ | XML | + parser +---+ | | +--+ | | internal object+--v--+ virResctrlAlloc| backing object| +--+--+ | | +--+ | +--v---+ | | | schemata | /sys/fs/resctrl | tasks| | . | | . | | | +--+ - previous versions and discussion can be found at v1: https://www.redhat.com/archives/libvir-list/2018-July/msg01144.html RFC v2: https://www.redhat.com/archives/libvir-list/2018-June/msg01268.html RFC v1: https://www.redhat.com/archives/libvir-list/2018-May/msg02101.html Changelog: v1 -> this: John's comment: 1. Split calculation of number of memory bandwidth control to one patch. 2. Split virResctrlAllocMemBW relating methods to 5 patch, each provides one kind of function, eg: schemata processing, memory bandwidth calculation. 3. Use resctrl to replace cachetune in domain conf. 4. Split refactor virDomainCachetuneDefParse into 3 patches. And adjust some logic, eg: use %s format e
[libvirt] [PATCH v2 04/17] util: Add MBA capability information query to resctrl
From: Bing Niu
Introducing virResctrlInfoMemBW for the information memory bandwidth
allocation information.
Signed-off-by: Bing Niu
Reviewed-by: John Ferlan
---
src/util/virresctrl.c | 84 +++
1 file changed, 84 insertions(+)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index a38c926..b12a05c 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -80,6 +80,9 @@ typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr;
typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel;
typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;
+typedef struct _virResctrlInfoMemBW virResctrlInfoMemBW;
+typedef virResctrlInfoMemBW *virResctrlInfoMemBWPtr;
+
typedef struct _virResctrlAllocPerType virResctrlAllocPerType;
typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
@@ -116,11 +119,30 @@ struct _virResctrlInfoPerLevel {
virResctrlInfoPerTypePtr *types;
};
+/* Information about memory bandwidth allocation */
+struct _virResctrlInfoMemBW {
+/* minimum memory bandwidth allowed */
+unsigned int min_bandwidth;
+/* bandwidth granularity */
+unsigned int bandwidth_granularity;
+/* Maximum number of simultaneous allocations */
+unsigned int max_allocation;
+/* level number of last level cache */
+unsigned int last_level_cache;
+/* max id of last level cache, this is used to track
+ * how many last level cache available in host system,
+ * the number of memory bandwidth allocation controller
+ * is identical with last level cache. */
+unsigned int max_id;
+};
+
struct _virResctrlInfo {
virObject parent;
virResctrlInfoPerLevelPtr *levels;
size_t nlevels;
+
+virResctrlInfoMemBWPtr membw_info;
};
@@ -146,6 +168,7 @@ virResctrlInfoDispose(void *obj)
VIR_FREE(level);
}
+VIR_FREE(resctrl->membw_info);
VIR_FREE(resctrl->levels);
}
@@ -443,6 +466,60 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl,
static int
+virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl)
+{
+int ret = -1;
+int rv = -1;
+virResctrlInfoMemBWPtr i_membw = NULL;
+
+/* query memory bandwidth allocation info */
+if (VIR_ALLOC(i_membw) < 0)
+goto cleanup;
+rv = virFileReadValueUint(&i_membw->bandwidth_granularity,
+ SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran");
+if (rv == -2) {
+/* The file doesn't exist, so it's unusable for us,
+ * probably memory bandwidth allocation unsupported */
+VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran'"
+ "does not exist");
+ret = 0;
+goto cleanup;
+} else if (rv < 0) {
+/* Other failures are fatal, so just quit */
+goto cleanup;
+}
+
+rv = virFileReadValueUint(&i_membw->min_bandwidth,
+ SYSFS_RESCTRL_PATH "/info/MB/min_bandwidth");
+if (rv == -2) {
+/* If the previous file exists, so should this one. Hence -2 is
+ * fatal in this case (errors out in next condition) - the kernel
+ * interface might've changed too much or something else is wrong. */
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot get min bandwidth from resctrl memory info"));
+}
+if (rv < 0)
+goto cleanup;
+
+rv = virFileReadValueUint(&i_membw->max_allocation,
+ SYSFS_RESCTRL_PATH "/info/MB/num_closids");
+if (rv == -2) {
+ /* Similar reasoning to min_bandwidth above. */
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot get max allocation from resctrl memory
info"));
+}
+if (rv < 0)
+goto cleanup;
+
+VIR_STEAL_PTR(resctrl->membw_info, i_membw);
+ret = 0;
+ cleanup:
+VIR_FREE(i_membw);
+return ret;
+}
+
+
+static int
virResctrlGetInfo(virResctrlInfoPtr resctrl)
{
DIR *dirp = NULL;
@@ -452,6 +529,10 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
if (ret <= 0)
goto cleanup;
+ret = virResctrlGetMemoryBandwidthInfo(resctrl);
+if (ret < 0)
+goto cleanup;
+
ret = virResctrlGetCacheInfo(resctrl, dirp);
if (ret < 0)
goto cleanup;
@@ -493,6 +574,9 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
if (!resctrl)
return true;
+if (resctrl->membw_info)
+return false;
+
for (i = 0; i < resctrl->nlevels; i++) {
virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 08/17] util: Add support to calculate MBA utilization
From: Bing Niu
Introduce virResctrlMemoryBandwidthSubtract and
virResctrlAllocMemoryBandwidth to be used as part of
the virResctrlAllocAssign processing to configure
the available memory bandwidth.
Signed-off-by: Bing Niu
---
src/util/virresctrl.c | 105 --
1 file changed, 93 insertions(+), 12 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 1cbf9b3..c22b30f 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -1409,6 +1409,22 @@ virResctrlAllocSubtract(virResctrlAllocPtr dst,
}
+static void
+virResctrlMemoryBandwidthSubtract(virResctrlAllocPtr free,
+ virResctrlAllocPtr used)
+{
+size_t i;
+
+if (!used->mem_bw)
+return;
+
+for (i = 0; i < used->mem_bw->nbandwidths; i++) {
+if (used->mem_bw->bandwidths[i])
+*(free->mem_bw->bandwidths[i]) -= *(used->mem_bw->bandwidths[i]);
+}
+}
+
+
static virResctrlAllocPtr
virResctrlAllocNewFromInfo(virResctrlInfoPtr info)
{
@@ -1472,14 +1488,15 @@ virResctrlAllocNewFromInfo(virResctrlInfoPtr info)
}
/*
- * This function creates an allocation that represents all unused parts of all
- * caches in the system. It uses virResctrlInfo for creating a new full
- * allocation with all bits set (using virResctrlAllocNewFromInfo()) and then
- * scans for all allocations under /sys/fs/resctrl and subtracts each one of
- * them from it. That way it can then return an allocation with only bit set
- * being those that are not mentioned in any other allocation. It is used for
- * two things, a) calculating the masks when creating allocations and b) from
- * tests.
+ * This function creates an allocation that represents all unused parts of
+ * all caches and memory bandwidth in the system. It uses virResctrlInfo
+ * for creating a new full allocation with all bits set (using the
+ * virResctrlAllocNewFromInfo()), sets memory bandwidth 100%, and then scans
+ * for all allocations under /sys/fs/resctrl and subtracts each one of them
+ * from it. That way it can then return an allocation with only bit set
+ * being those that are not mentioned in any other allocation for CAT and
+ * available memory bandwidth for MBA. It is used for two things, calculating
+ * the masks and bandwidth available when creating allocations and from tests.
*/
virResctrlAllocPtr
virResctrlAllocGetUnused(virResctrlInfoPtr resctrl)
@@ -1525,6 +1542,7 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl)
goto error;
}
+virResctrlMemoryBandwidthSubtract(ret, alloc);
virResctrlAllocSubtract(ret, alloc);
virObjectUnref(alloc);
alloc = NULL;
@@ -1676,6 +1694,66 @@ virResctrlAllocFindUnused(virResctrlAllocPtr alloc,
static int
+virResctrlAllocMemoryBandwidth(virResctrlInfoPtr resctrl,
+ virResctrlAllocPtr alloc,
+ virResctrlAllocPtr free)
+{
+size_t i;
+virResctrlAllocMemBWPtr mem_bw_alloc = alloc->mem_bw;
+virResctrlAllocMemBWPtr mem_bw_free = free->mem_bw;
+virResctrlInfoMemBWPtr mem_bw_info = resctrl->membw_info;
+
+if (!mem_bw_alloc)
+return 0;
+
+if (mem_bw_alloc && !mem_bw_info) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("RDT Memory Bandwidth allocation unsupported"));
+return -1;
+}
+
+for (i = 0; i < mem_bw_alloc->nbandwidths; i++) {
+if (!mem_bw_alloc->bandwidths[i])
+continue;
+
+if (*(mem_bw_alloc->bandwidths[i]) %
mem_bw_info->bandwidth_granularity) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Memory Bandwidth allocation of size "
+ "%u is not divisible by granularity %u"),
+ *(mem_bw_alloc->bandwidths[i]),
+ mem_bw_info->bandwidth_granularity);
+return -1;
+}
+if (*(mem_bw_alloc->bandwidths[i]) < mem_bw_info->min_bandwidth) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Memory Bandwidth allocation of size "
+ "%u is smaller than the minimum "
+ "allowed allocation %u"),
+ *(mem_bw_alloc->bandwidths[i]),
+ mem_bw_info->min_bandwidth);
+return -1;
+}
+if (i > mem_bw_info->max_id) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("bandwidth controller id %zd does not "
+ "exist, max controller id %u"),
+ i, mem_bw_info->max_id);
+return -1;
+}
+if (*(mem_bw_alloc->bandwidths[i]) > *(mem_bw_free->bandwidths[i])) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Not enough room for allocation of %u%% "
+
[libvirt] [PATCH v2 03/17] util: Refactor virResctrlAllocFormat of virresctrl
From: Bing Niu
Refactor virResctrlAllocFormat so that it is easy to support other
resource allocation technologies.
Signed-off-by: Bing Niu
Reviewed-by: John Ferlan
---
src/util/virresctrl.c | 41 ++---
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 313f964..a38c926 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -849,17 +849,14 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)
}
-char *
-virResctrlAllocFormat(virResctrlAllocPtr alloc)
+static int
+virResctrlAllocFormatCache(virResctrlAllocPtr alloc,
+ virBufferPtr buf)
{
-virBuffer buf = VIR_BUFFER_INITIALIZER;
unsigned int level = 0;
unsigned int type = 0;
unsigned int cache = 0;
-if (!alloc)
-return NULL;
-
for (level = 0; level < alloc->nlevels; level++) {
virResctrlAllocPerLevelPtr a_level = alloc->levels[level];
@@ -872,7 +869,7 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc)
if (!a_type)
continue;
-virBufferAsprintf(&buf, "L%u%s:", level,
virResctrlTypeToString(type));
+virBufferAsprintf(buf, "L%u%s:", level,
virResctrlTypeToString(type));
for (cache = 0; cache < a_type->nmasks; cache++) {
virBitmapPtr mask = a_type->masks[cache];
@@ -882,21 +879,35 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc)
continue;
mask_str = virBitmapToString(mask, false, true);
-if (!mask_str) {
-virBufferFreeAndReset(&buf);
-return NULL;
-}
+if (!mask_str)
+return -1;
-virBufferAsprintf(&buf, "%u=%s;", cache, mask_str);
+virBufferAsprintf(buf, "%u=%s;", cache, mask_str);
VIR_FREE(mask_str);
}
-virBufferTrim(&buf, ";", 1);
-virBufferAddChar(&buf, '\n');
+virBufferTrim(buf, ";", 1);
+virBufferAddChar(buf, '\n');
}
}
-virBufferCheckError(&buf);
+return virBufferCheckError(buf);
+}
+
+
+char *
+virResctrlAllocFormat(virResctrlAllocPtr alloc)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+if (!alloc)
+return NULL;
+
+if (virResctrlAllocFormatCache(alloc, &buf) < 0) {
+virBufferFreeAndReset(&buf);
+return NULL;
+}
+
return virBufferContentAndReset(&buf);
}
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 07/17] util: Add MBA schemata parse and format methods
From: Bing Niu
Introduce virResctrlAllocMemoryBandwidthFormat and
virResctrlAllocParseMemoryBandwidthLine which will format
and parse an entry in the schemata file for MBA.
Signed-off-by: Bing Niu
---
src/util/virresctrl.c | 141 ++
1 file changed, 141 insertions(+)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 8a25798..1cbf9b3 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -986,6 +986,139 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)
}
+/* Format the Memory Bandwidth Allocation line that will be found in
+ * the schemata files. The line should be start with "MB:" and be
+ * followed by "id=value" pairs separated by a colon such as:
+ *
+ * MB:0=100;1=100
+ *
+ * which indicates node id 0 has 100 percent bandwith and node id 1
+ * has 100 percent bandwidth
+ */
+static int
+virResctrlAllocMemoryBandwidthFormat(virResctrlAllocPtr alloc,
+ virBufferPtr buf)
+{
+size_t i;
+
+if (!alloc->mem_bw)
+return 0;
+
+virBufferAddLit(buf, "MB:");
+
+for (i = 0; i < alloc->mem_bw->nbandwidths; i++) {
+if (alloc->mem_bw->bandwidths[i]) {
+virBufferAsprintf(buf, "%zd=%u;", i,
+ *(alloc->mem_bw->bandwidths[i]));
+}
+}
+
+virBufferTrim(buf, ";", 1);
+virBufferAddChar(buf, '\n');
+return virBufferCheckError(buf);
+}
+
+
+static int
+virResctrlAllocParseProcessMemoryBandwidth(virResctrlInfoPtr resctrl,
+ virResctrlAllocPtr alloc,
+ char *mem_bw)
+{
+unsigned int bandwidth;
+unsigned int id;
+char *tmp = NULL;
+
+tmp = strchr(mem_bw, '=');
+if (!tmp)
+return 0;
+*tmp = '\0';
+tmp++;
+
+if (virStrToLong_uip(mem_bw, NULL, 10, &id) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid node id %u "), id);
+return -1;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &bandwidth) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid bandwidth %u"), bandwidth);
+return -1;
+}
+if (bandwidth < resctrl->membw_info->min_bandwidth ||
+id > resctrl->membw_info->max_id) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Missing or inconsistent resctrl info for "
+ "memory bandwidth node '%u'"), id);
+return -1;
+}
+if (alloc->mem_bw->nbandwidths <= id &&
+VIR_EXPAND_N(alloc->mem_bw->bandwidths, alloc->mem_bw->nbandwidths,
+ id - alloc->mem_bw->nbandwidths + 1) < 0) {
+return -1;
+}
+if (!alloc->mem_bw->bandwidths[id]) {
+if (VIR_ALLOC(alloc->mem_bw->bandwidths[id]) < 0)
+return -1;
+}
+
+*(alloc->mem_bw->bandwidths[id]) = bandwidth;
+return 0;
+}
+
+
+/* Parse a schemata formatted MB: entry. Format details are described in
+ * virResctrlAllocMemoryBandwidthFormat.
+ */
+static int
+virResctrlAllocParseMemoryBandwidthLine(virResctrlInfoPtr resctrl,
+virResctrlAllocPtr alloc,
+char *line)
+{
+char **mbs = NULL;
+char *tmp = NULL;
+size_t nmbs = 0;
+size_t i;
+int ret = -1;
+
+/* For no reason there can be spaces */
+virSkipSpaces((const char **) &line);
+
+if (STRNEQLEN(line, "MB", 2))
+return 0;
+
+if (!resctrl || !resctrl->membw_info ||
+!resctrl->membw_info->min_bandwidth ||
+!resctrl->membw_info->bandwidth_granularity) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing or inconsistent resctrl info for "
+ "memory bandwidth allocation"));
+}
+
+if (!alloc->mem_bw) {
+if (VIR_ALLOC(alloc->mem_bw) < 0)
+return -1;
+}
+
+tmp = strchr(line, ':');
+if (!tmp)
+return 0;
+tmp++;
+
+mbs = virStringSplitCount(tmp, ";", 0, &nmbs);
+if (nmbs == 0)
+return 0;
+
+for (i = 0; i < nmbs; i++) {
+if (virResctrlAllocParseProcessMemoryBandwidth(resctrl, alloc, mbs[i])
< 0)
+goto cleanup;
+}
+ret = 0;
+ cleanup:
+virStringListFree(mbs);
+return ret;
+}
+
+
static int
virResctrlAllocFormatCache(virResctrlAllocPtr alloc,
virBufferPtr buf)
@@ -1045,6 +1178,11 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc)
return NULL;
}
+if (virResctrlAllocMemoryBandwidthFormat(alloc, &buf) < 0) {
+virBufferFreeAndReset(&buf);
+return NULL;
+}
+
return virBufferContentAndReset(&buf);
}
@@ -1173,6 +1311,9 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl,
for (i = 0; i < nlines; i++) {
if (virResctrlAllocParseCacheLine(resctrl, alloc, lines[i]) < 0)
goto cleanup;
+
[libvirt] [PATCH v2 09/17] util: Introduce virResctrlAllocForeachMemory
From: Bing Niu
Introduce an API that will traverse the memory bandwidth data calling
a callback function for each defined bandwidth entry.
Signed-off-by: Bing Niu
---
src/libvirt_private.syms | 1 +
src/util/virresctrl.c| 33 +
src/util/virresctrl.h| 9 +
3 files changed, 43 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bc489cb..cd598f9 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2649,6 +2649,7 @@ virResctrlAllocAddPID;
virResctrlAllocCreate;
virResctrlAllocDeterminePath;
virResctrlAllocForeachCache;
+virResctrlAllocForeachMemory;
virResctrlAllocFormat;
virResctrlAllocGetID;
virResctrlAllocGetUnused;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index c22b30f..dc57c89 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -965,6 +965,39 @@ virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
}
+/* virResctrlAllocForeachMemory
+ * @alloc: Pointer to an active allocation
+ * @cb: Callback function
+ * @opaque: Opaque data to be passed to @cb
+ *
+ * If available, traverse the defined memory bandwidth allocations and
+ * call the @cb function.
+ *
+ * Returns 0 on success, -1 and immediate failure if the @cb has any failure.
+ */
+int
+virResctrlAllocForeachMemory(virResctrlAllocPtr alloc,
+ virResctrlAllocForeachMemoryCallback cb,
+ void *opaque)
+{
+size_t i = 0;
+virResctrlAllocMemBWPtr mem_bw;
+
+if (!alloc || !alloc->mem_bw)
+return 0;
+
+mem_bw = alloc->mem_bw;
+for (i = 0; i < mem_bw->nbandwidths; i++) {
+if (mem_bw->bandwidths[i]) {
+if (cb(i, *mem_bw->bandwidths[i], opaque) < 0)
+return -1;
+}
+}
+
+return 0;
+}
+
+
int
virResctrlAllocSetID(virResctrlAllocPtr alloc,
const char *id)
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index d657c06..5ea5b27 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -73,6 +73,10 @@ typedef int virResctrlAllocForeachCacheCallback(unsigned int
level,
unsigned long long size,
void *opaque);
+typedef int virResctrlAllocForeachMemoryCallback(unsigned int id,
+ unsigned int size,
+ void *opaque);
+
virResctrlAllocPtr
virResctrlAllocNew(void);
@@ -92,6 +96,11 @@ virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
void *opaque);
int
+virResctrlAllocForeachMemory(virResctrlAllocPtr resctrl,
+ virResctrlAllocForeachMemoryCallback cb,
+ void *opaque);
+
+int
virResctrlAllocSetID(virResctrlAllocPtr alloc,
const char *id);
const char *
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 06/17] util: Add MBA allocation to virresctrl
From: Bing Niu
Add memory bandwidth allocation support to virresctrl class.
Introducing virResctrlAllocMemBW which is used for allocating memory
bandwidth. Following virResctrlAllocPerType, it also employs a
nested sparse array to indicate whether allocation is available for
particular last level cache.
Signed-off-by: Bing Niu
---
src/util/virresctrl.c | 63 +++
1 file changed, 59 insertions(+), 4 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index f454868..8a25798 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -36,9 +36,9 @@ VIR_LOG_INIT("util.virresctrl")
/* Resctrl is short for Resource Control. It might be implemented for various
- * resources, but at the time of this writing this is only supported for cache
- * allocation technology (aka CAT). Hence the reson for leaving 'Cache' out of
- * all the structure and function names for now (can be added later if needed.
+ * resources. Currently this supports cache allocation technology (aka CAT) and
+ * memory bandwidth allocation (aka MBA). More resources technologies may be
+ * added in the future.
*/
@@ -89,6 +89,9 @@ typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
typedef struct _virResctrlAllocPerLevel virResctrlAllocPerLevel;
typedef virResctrlAllocPerLevel *virResctrlAllocPerLevelPtr;
+typedef struct _virResctrlAllocMemBW virResctrlAllocMemBW;
+typedef virResctrlAllocMemBW *virResctrlAllocMemBWPtr;
+
/* Class definitions and initializations */
static virClassPtr virResctrlInfoClass;
@@ -180,7 +183,10 @@ virResctrlInfoDispose(void *obj)
* consequently a directory under /sys/fs/resctrl). Since it can have multiple
* parts of multiple caches allocated it is represented as bunch of nested
* sparse arrays (by sparse I mean array of pointers so that each might be NULL
- * in case there is no allocation for that particular one (level, cache, ...)).
+ * in case there is no allocation for that particular cache allocation (level,
+ * cache, ...) or memory allocation for particular node).
+ *
+ * =Cache allocation technology (CAT)=
*
* Since one allocation can be made for caches on different levels, the first
* nested sparse array is of types virResctrlAllocPerLevel. For example if you
@@ -205,6 +211,17 @@ virResctrlInfoDispose(void *obj)
* all of them. While doing that we store the bitmask in a sparse array of
* virBitmaps named `masks` indexed the same way as `sizes`. The upper bounds
* of the sparse arrays are stored in nmasks or nsizes, respectively.
+ + *
+ * =Memory Bandwidth allocation technology (MBA)=
+ *
+ * The memory bandwidth allocation support in virResctrlAlloc works in the
+ * same fashion as CAT. However, memory bandwidth controller doesn't have a
+ * hierarchy organization as cache, each node have one memory bandwidth
+ * controller to memory bandwidth distribution. The number of memory bandwidth
+ * controller is identical with number of last level cache. So MBA also employs
+ * a sparse array to represent whether a memory bandwidth allocation happens
+ * on corresponding node. The available memory controller number is collected
+ * in 'virResctrlInfo'.
*/
struct _virResctrlAllocPerType {
/* There could be bool saying whether this is set or not, but since
everything
@@ -225,12 +242,24 @@ struct _virResctrlAllocPerLevel {
* VIR_CACHE_TYPE_LAST number of items */
};
+/*
+ * virResctrlAllocMemBW represents one memory bandwidth allocation.
+ * Since it can have several last level caches in a NUMA system, it is
+ * also represented as a nested sparse arrays as virRestrlAllocPerLevel.
+ */
+struct _virResctrlAllocMemBW {
+unsigned int **bandwidths;
+size_t nbandwidths;
+};
+
struct _virResctrlAlloc {
virObject parent;
virResctrlAllocPerLevelPtr *levels;
size_t nlevels;
+virResctrlAllocMemBWPtr mem_bw;
+
/* The identifier (any unique string for now) */
char *id;
/* libvirt-generated path in /sys/fs/resctrl for this particular
@@ -274,6 +303,13 @@ virResctrlAllocDispose(void *obj)
VIR_FREE(level);
}
+if (alloc->mem_bw) {
+virResctrlAllocMemBWPtr mem_bw = alloc->mem_bw;
+for (i = 0; i < mem_bw->nbandwidths; i++)
+VIR_FREE(mem_bw->bandwidths[i]);
+VIR_FREE(alloc->mem_bw);
+}
+
VIR_FREE(alloc->id);
VIR_FREE(alloc->path);
VIR_FREE(alloc->levels);
@@ -692,6 +728,9 @@ virResctrlAllocIsEmpty(virResctrlAllocPtr alloc)
if (!alloc)
return true;
+if (alloc->mem_bw)
+return false;
+
for (i = 0; i < alloc->nlevels; i++) {
virResctrlAllocPerLevelPtr a_level = alloc->levels[i];
@@ -1266,6 +1305,22 @@ virResctrlAllocNewFromInfo(virResctrlInfoPtr info)
}
}
+/* set default free memory bandwidth to 100%*/
+if (info->membw_info) {
+if (VIR_ALLOC(ret->mem_bw) < 0)
+goto error;
+
+
[libvirt] [PATCH v2 11/17] conf: Rename cachetune to resctrl
From: Bing Niu
Resctrl not only supports cache tuning, but also memory bandwidth
tuning. Renaming cachetune to resctrl to reflect that. With resctrl,
all allocation for different resources (cache, memory bandwidth) are
aggregated and represented by a virResctrlAllocPtr inside
virDomainResctrlDef.
Signed-off-by: Bing Niu
---
src/conf/domain_conf.c | 44 ++--
src/conf/domain_conf.h | 10 +-
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 18 +-
4 files changed, 37 insertions(+), 37 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index abb6c5e..c1527b2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2966,14 +2966,14 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
static void
-virDomainCachetuneDefFree(virDomainCachetuneDefPtr cachetune)
+virDomainResctrlDefFree(virDomainResctrlDefPtr resctrl)
{
-if (!cachetune)
+if (!resctrl)
return;
-virObjectUnref(cachetune->alloc);
-virBitmapFree(cachetune->vcpus);
-VIR_FREE(cachetune);
+virObjectUnref(resctrl->alloc);
+virBitmapFree(resctrl->vcpus);
+VIR_FREE(resctrl);
}
@@ -3163,9 +3163,9 @@ void virDomainDefFree(virDomainDefPtr def)
virDomainShmemDefFree(def->shmems[i]);
VIR_FREE(def->shmems);
-for (i = 0; i < def->ncachetunes; i++)
-virDomainCachetuneDefFree(def->cachetunes[i]);
-VIR_FREE(def->cachetunes);
+for (i = 0; i < def->nresctrls; i++)
+virDomainResctrlDefFree(def->resctrls[i]);
+VIR_FREE(def->resctrls);
VIR_FREE(def->keywrap);
@@ -19034,7 +19034,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
xmlNodePtr *nodes = NULL;
virBitmapPtr vcpus = NULL;
virResctrlAllocPtr alloc = virResctrlAllocNew();
-virDomainCachetuneDefPtr tmp_cachetune = NULL;
+virDomainResctrlDefPtr tmp_resctrl = NULL;
char *tmp = NULL;
char *vcpus_str = NULL;
char *alloc_id = NULL;
@@ -19047,7 +19047,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
if (!alloc)
goto cleanup;
-if (VIR_ALLOC(tmp_cachetune) < 0)
+if (VIR_ALLOC(tmp_resctrl) < 0)
goto cleanup;
vcpus_str = virXMLPropString(node, "vcpus");
@@ -19088,8 +19088,8 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
goto cleanup;
}
-for (i = 0; i < def->ncachetunes; i++) {
-if (virBitmapOverlaps(def->cachetunes[i]->vcpus, vcpus)) {
+for (i = 0; i < def->nresctrls; i++) {
+if (virBitmapOverlaps(def->resctrls[i]->vcpus, vcpus)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("Overlapping vcpus in cachetunes"));
goto cleanup;
@@ -19119,16 +19119,16 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
if (virResctrlAllocSetID(alloc, alloc_id) < 0)
goto cleanup;
-VIR_STEAL_PTR(tmp_cachetune->vcpus, vcpus);
-VIR_STEAL_PTR(tmp_cachetune->alloc, alloc);
+VIR_STEAL_PTR(tmp_resctrl->vcpus, vcpus);
+VIR_STEAL_PTR(tmp_resctrl->alloc, alloc);
-if (VIR_APPEND_ELEMENT(def->cachetunes, def->ncachetunes, tmp_cachetune) <
0)
+if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, tmp_resctrl) < 0)
goto cleanup;
ret = 0;
cleanup:
ctxt->node = oldnode;
-virDomainCachetuneDefFree(tmp_cachetune);
+virDomainResctrlDefFree(tmp_resctrl);
virObjectUnref(alloc);
virBitmapFree(vcpus);
VIR_FREE(alloc_id);
@@ -26994,7 +26994,7 @@ virDomainCachetuneDefFormatHelper(unsigned int level,
static int
virDomainCachetuneDefFormat(virBufferPtr buf,
-virDomainCachetuneDefPtr cachetune,
+virDomainResctrlDefPtr resctrl,
unsigned int flags)
{
virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
@@ -27002,7 +27002,7 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
int ret = -1;
virBufferSetChildIndent(&childrenBuf, buf);
-virResctrlAllocForeachCache(cachetune->alloc,
+virResctrlAllocForeachCache(resctrl->alloc,
virDomainCachetuneDefFormatHelper,
&childrenBuf);
@@ -27015,14 +27015,14 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
goto cleanup;
}
-vcpus = virBitmapFormat(cachetune->vcpus);
+vcpus = virBitmapFormat(resctrl->vcpus);
if (!vcpus)
goto cleanup;
virBufferAsprintf(buf, "alloc);
+const char *alloc_id = virResctrlAllocGetID(resctrl->alloc);
if (!alloc_id)
goto cleanup;
@@ -27143,8 +27143,8 @@ virDomainCputuneDefFormat(virBufferPtr buf,
def->iothreadids[i]->iothread_id);
}
-for (i = 0; i < def->ncachetunes; i++)
-virDomainCachetuneDefFormat(&childrenBuf, def->cachetunes[i], flags);
+for (i = 0; i < def->nresctrls; i++)
+virDomainCachetuneDefFormat(&childrenBuf, de
[libvirt] [PATCH v2 12/17] conf: Factor out vcpus parsing part from virDomainCachetuneDefParse
From: Bing Niu
Extract vcpus parsing part from virDomainCachetuneDefParse into one
function called virDomainResctrlParseVcpus. So that vcpus parsing logic
can be reused by other resource control technologies. Adjust error
message and use node->name so that the error message can fit to all
technologies.
Signed-off-by: Bing Niu
---
src/conf/domain_conf.c | 48 +---
1 file changed, 33 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c1527b2..d6314de 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18951,6 +18951,38 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
static int
+virDomainResctrlParseVcpus(virDomainDefPtr def,
+ xmlNodePtr node,
+ virBitmapPtr *vcpus)
+{
+char *vcpus_str = NULL;
+int ret = -1;
+
+vcpus_str = virXMLPropString(node, "vcpus");
+if (!vcpus_str) {
+virReportError(VIR_ERR_XML_ERROR, _("Missing %s attribute 'vcpus'"),
+ node->name);
+goto cleanup;
+}
+if (virBitmapParse(vcpus_str, vcpus, VIR_DOMAIN_CPUMASK_LEN) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid %s attribute 'vcpus' value '%s'"),
+ vcpus_str, node->name);
+goto cleanup;
+}
+
+/* We need to limit the bitmap to number of vCPUs. If there's nothing
left,
+ * then we can just clean up and return 0 immediately */
+virBitmapShrink(*vcpus, def->maxvcpus);
+
+ret = 0;
+ cleanup:
+VIR_FREE(vcpus_str);
+return ret;
+}
+
+
+static int
virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
xmlNodePtr node,
virResctrlAllocPtr alloc)
@@ -19050,22 +19082,8 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
if (VIR_ALLOC(tmp_resctrl) < 0)
goto cleanup;
-vcpus_str = virXMLPropString(node, "vcpus");
-if (!vcpus_str) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Missing cachetune attribute 'vcpus'"));
-goto cleanup;
-}
-if (virBitmapParse(vcpus_str, &vcpus, VIR_DOMAIN_CPUMASK_LEN) < 0) {
-virReportError(VIR_ERR_XML_ERROR,
- _("Invalid cachetune attribute 'vcpus' value '%s'"),
- vcpus_str);
+if (virDomainResctrlParseVcpus(def, node, &vcpus) < 0)
goto cleanup;
-}
-
-/* We need to limit the bitmap to number of vCPUs. If there's nothing
left,
- * then we can just clean up and return 0 immediately */
-virBitmapShrink(vcpus, def->maxvcpus);
if (virBitmapIsAllClear(vcpus)) {
ret = 0;
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 13/17] conf: Factor out vcpus overlapping from virDomainCachetuneDefParse
From: Bing Niu
Factor out vcpus overlapping detecting part from
virDomainCachetuneDefParse and introduce virDomainResctrlVcpuMatch.
Instead of allocating virResctrlAllocPtr by default, allocating
virResctrlAllocPtr after confirm vcpus not overlap with existing ones.
And virDomainResctrlVcpuMatch can be reused by other resource control
technologies. virDomainResctrlVcpuMatch can clarify old vcpus overlap
error whether an overlap or a redefinition.
Signed-off-by: Bing Niu
---
src/conf/domain_conf.c | 51 ++
1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d6314de..da8681d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18983,6 +18983,31 @@ virDomainResctrlParseVcpus(virDomainDefPtr def,
static int
+virDomainResctrlVcpuMatch(virDomainDefPtr def,
+ virBitmapPtr vcpus,
+ virResctrlAllocPtr *alloc)
+{
+ssize_t i = 0;
+
+for (i = 0; i < def->nresctrls; i++) {
+/* vcpus group has been created, directly use the existing one.
+ * Just updating memory allocation information of that group
+ */
+if (virBitmapEqual(def->resctrls[i]->vcpus, vcpus)) {
+*alloc = def->resctrls[i]->alloc;
+break;
+}
+if (virBitmapOverlaps(def->resctrls[i]->vcpus, vcpus)) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Overlapping vcpus in resctrls"));
+return -1;
+}
+}
+return 0;
+}
+
+
+static int
virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
xmlNodePtr node,
virResctrlAllocPtr alloc)
@@ -19065,7 +19090,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
xmlNodePtr oldnode = ctxt->node;
xmlNodePtr *nodes = NULL;
virBitmapPtr vcpus = NULL;
-virResctrlAllocPtr alloc = virResctrlAllocNew();
+virResctrlAllocPtr alloc = NULL;
virDomainResctrlDefPtr tmp_resctrl = NULL;
char *tmp = NULL;
char *vcpus_str = NULL;
@@ -19076,9 +19101,6 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
ctxt->node = node;
-if (!alloc)
-goto cleanup;
-
if (VIR_ALLOC(tmp_resctrl) < 0)
goto cleanup;
@@ -19096,6 +19118,19 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
goto cleanup;
}
+if (virDomainResctrlVcpuMatch(def, vcpus, &alloc) < 0)
+goto cleanup;
+
+if (!alloc) {
+alloc = virResctrlAllocNew();
+if (!alloc)
+goto cleanup;
+} else {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Identical vcpus in cachetunes found"));
+goto cleanup;
+}
+
for (i = 0; i < n; i++) {
if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0)
goto cleanup;
@@ -19106,14 +19141,6 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
goto cleanup;
}
-for (i = 0; i < def->nresctrls; i++) {
-if (virBitmapOverlaps(def->resctrls[i]->vcpus, vcpus)) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Overlapping vcpus in cachetunes"));
-goto cleanup;
-}
-}
-
/* We need to format it back because we need to be consistent in the naming
* even when users specify some "sub-optimal" string there. */
VIR_FREE(vcpus_str);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 15/17] conf: Add support for memorytune XML processing for resctrl MBA
From: Bing Niu Introduce a new section memorytune to support memory bandwidth allocation. This is consistent with existing cachetune. As the example: below: .. vpus --- vpus subjected to this memory bandwidth. id--- on which node memory bandwidth to be set. bandwidth --- the memory bandwidth percent to set. Signed-off-by: Bing Niu --- docs/formatdomain.html.in | 39 +++- docs/schemas/domaincommon.rng | 17 ++ src/conf/domain_conf.c | 199 + .../memorytune-colliding-allocs.xml| 30 .../memorytune-colliding-cachetune.xml | 32 tests/genericxml2xmlindata/memorytune.xml | 33 tests/genericxml2xmltest.c | 5 + 7 files changed, 354 insertions(+), 1 deletion(-) create mode 100644 tests/genericxml2xmlindata/memorytune-colliding-allocs.xml create mode 100644 tests/genericxml2xmlindata/memorytune-colliding-cachetune.xml create mode 100644 tests/genericxml2xmlindata/memorytune.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 19b7312..1ae6e98 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -759,6 +759,10 @@+ + + ... @@ -932,7 +936,9 @@ size and required granularity are reported as well. The required attribute vcpus specifies to which vCPUs this allocation applies. A vCPU can only be member of one cachetune element -allocations. Supported subelements are: +allocations. The vCPUs specified by cachetune can be identical with those +in memorytune, however they are not allowed to overlap. +Supported subelements are: cache @@ -972,7 +978,38 @@ + + memorytuneSince 4.7.0 + +Optional memorytune element can control allocations for +memory bandwidth using the resctrl on the host. Whether or not is this +supported can be gathered from capabilities where some limitations like +minimum bandwidth and required granularity are reported as well. The +required attribute vcpus specifies to which vCPUs this +allocation applies. A vCPU can only be member of one +memorytune element allocations. The vcpus specified +by memorytune can be identical to those specified by +cachetune. However they are not allowed to overlap each other. +Supported subelements are: + + node + +This element controls the allocation of CPU memory bandwidth and has the +following attributes: + + id + +Host node id from which to allocate memory bandwidth. + + bandwidth + +The memory bandwidth to allocate from this node. The value by default +is in percentage. + + + + diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ac04af5..48f0637 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -983,6 +983,23 @@ + + + + + + + + + + + + + + + + + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0efbdf4..9acdea7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19290,6 +19290,128 @@ virDomainDefParseCaps(virDomainDefPtr def, } +static int +virDomainMemorytuneDefParseMemory(xmlXPathContextPtr ctxt, + xmlNodePtr node, + virResctrlAllocPtr alloc) +{ +xmlNodePtr oldnode = ctxt->node; +unsigned int id; +unsigned int bandwidth; +char *tmp = NULL; +int ret = -1; + +ctxt->node = node; + +tmp = virXMLPropString(node, "id"); +if (!tmp) { +virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing memorytune attribute 'id'")); +goto cleanup; +} +if (virStrToLong_uip(tmp, NULL, 10, &id) < 0) { +virReportError(VIR_ERR_XML_ERROR, + _("Invalid memorytune attribute 'id' value '%s'"), + tmp); +goto cleanup; +} +VIR_FREE(tmp); + +tmp = virXMLPropString(node, "bandwidth"); +if (!tmp) { +virReportError(VIR_ERR_X+
[libvirt] [PATCH v2 10/17] util: Introduce virResctrlAllocSetMemoryBandwidth
From: Bing Niu
Introduce an API to allow setting of the MBA from domain XML.
Signed-off-by: Bing Niu
---
src/libvirt_private.syms | 1 +
src/util/virresctrl.c| 48
src/util/virresctrl.h| 5 +
3 files changed, 54 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cd598f9..0d2fc5c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2657,6 +2657,7 @@ virResctrlAllocNew;
virResctrlAllocRemove;
virResctrlAllocSetCacheSize;
virResctrlAllocSetID;
+virResctrlAllocSetMemoryBandwidth;
virResctrlInfoGetCache;
virResctrlInfoNew;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index dc57c89..b821aac 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -965,6 +965,54 @@ virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
}
+/* virResctrlAllocSetMemoryBandwidth
+ * @alloc: Pointer to an active allocation
+ * @id: node id of MBA to be set
+ * @memory_bandwidth: new memory bandwidth value
+ *
+ * Set the @memory_bandwidth for the node @id entry in the @alloc.
+ *
+ * Returns 0 on success, -1 on failure with error message set.
+ */
+int
+virResctrlAllocSetMemoryBandwidth(virResctrlAllocPtr alloc,
+ unsigned int id,
+ unsigned int memory_bandwidth)
+{
+virResctrlAllocMemBWPtr mem_bw = alloc->mem_bw;
+
+if (memory_bandwidth > 100) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Memory Bandwidth value beyond 100 is invalid."));
+return -1;
+}
+
+if (!mem_bw) {
+if (VIR_ALLOC(mem_bw) < 0)
+return -1;
+alloc->mem_bw = mem_bw;
+}
+
+if (mem_bw->nbandwidths <= id &&
+VIR_EXPAND_N(mem_bw->bandwidths, mem_bw->nbandwidths,
+ id - mem_bw->nbandwidths + 1) < 0)
+return -1;
+
+if (mem_bw->bandwidths[id]) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("Memory Bandwidth already defined for node %u"),
+ id);
+return -1;
+}
+
+if (VIR_ALLOC(mem_bw->bandwidths[id]) < 0)
+return -1;
+
+*(mem_bw->bandwidths[id]) = memory_bandwidth;
+return 0;
+}
+
+
/* virResctrlAllocForeachMemory
* @alloc: Pointer to an active allocation
* @cb: Callback function
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index 5ea5b27..8d62517 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -96,6 +96,11 @@ virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
void *opaque);
int
+virResctrlAllocSetMemoryBandwidth(virResctrlAllocPtr alloc,
+ unsigned int id,
+ unsigned int memory_bandwidth);
+
+int
virResctrlAllocForeachMemory(virResctrlAllocPtr resctrl,
virResctrlAllocForeachMemoryCallback cb,
void *opaque);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 16/17] conf: Add return value check to virResctrlAllocForeachCache
From: Bing Niu Add return value check to virResctrlAllocForeachCache in virDomainCachetuneDefFormat. The virResctrlAllocForeachCache dose have return value, so need check return value to make sure function execute without error. Signed-off-by: Bing Niu --- src/conf/domain_conf.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9acdea7..3b68f53 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27198,10 +27198,10 @@ virDomainCachetuneDefFormat(virBufferPtr buf, int ret = -1; virBufferSetChildIndent(&childrenBuf, buf); -virResctrlAllocForeachCache(resctrl->alloc, -virDomainCachetuneDefFormatHelper, -&childrenBuf); - +if (virResctrlAllocForeachCache(resctrl->alloc, +virDomainCachetuneDefFormatHelper, +&childrenBuf) < 0) +goto cleanup; if (virBufferCheckError(&childrenBuf) < 0) goto cleanup; -- 2.7.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 17/17] conf: Add memory bandwidth allocation capability of host
From: Bing Niu
Add new XML section to report host's memory bandwidth allocation
capability. The format as below example:
.
granularity granularity of memory bandwidth, unit percentage.
min minimum memory bandwidth allowed, unit percentage.
maxAllocs maximum memory bandwidth allocation group supported.
Signed-off-by: Bing Niu
---
docs/schemas/capability.rng| 33 +++
src/conf/capabilities.c| 107 +
src/conf/capabilities.h| 11 +++
src/util/virresctrl.c | 20
src/util/virresctrl.h | 15 +++
.../linux-resctrl/resctrl/info/MB/bandwidth_gran | 1 +
.../linux-resctrl/resctrl/info/MB/min_bandwidth| 1 +
.../linux-resctrl/resctrl/info/MB/num_closids | 1 +
tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 8 ++
tests/virresctrldata/resctrl.schemata | 1 +
10 files changed, 198 insertions(+)
create mode 100644
tests/vircaps2xmldata/linux-resctrl/resctrl/info/MB/bandwidth_gran
create mode 100644
tests/vircaps2xmldata/linux-resctrl/resctrl/info/MB/min_bandwidth
create mode 100644
tests/vircaps2xmldata/linux-resctrl/resctrl/info/MB/num_closids
diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
index 52164d5..d61515c 100644
--- a/docs/schemas/capability.rng
+++ b/docs/schemas/capability.rng
@@ -51,6 +51,9 @@
+
+
+
@@ -326,6 +329,36 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 0f96500..ef2ca81 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -198,6 +198,16 @@ virCapabilitiesFreeNUMAInfo(virCapsPtr caps)
}
static void
+virCapsHostMemBWNodeFree(virCapsHostMemBWNodePtr ptr)
+{
+if (!ptr)
+return;
+
+virBitmapFree(ptr->cpus);
+VIR_FREE(ptr);
+}
+
+static void
virCapabilitiesClearSecModel(virCapsHostSecModelPtr secmodel)
{
size_t i;
@@ -239,6 +249,10 @@ virCapsDispose(void *object)
virCapsHostCacheBankFree(caps->host.caches[i]);
VIR_FREE(caps->host.caches);
+for (i = 0; i < caps->host.nnodes; i++)
+virCapsHostMemBWNodeFree(caps->host.nodes[i]);
+VIR_FREE(caps->host.nodes);
+
VIR_FREE(caps->host.netprefix);
VIR_FREE(caps->host.pagesSize);
virCPUDefFree(caps->host.cpu);
@@ -957,6 +971,58 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
return 0;
}
+static int
+virCapabilitiesFormatMemoryBandwidth(virBufferPtr buf,
+ size_t nnodes,
+ virCapsHostMemBWNodePtr *nodes)
+{
+size_t i = 0;
+virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
+
+if (!nnodes)
+return 0;
+
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+
+for (i = 0; i < nnodes; i++) {
+virCapsHostMemBWNodePtr node = nodes[i];
+virResctrlInfoMemBWPerNodePtr control = &node->control;
+char *cpus_str = virBitmapFormat(node->cpus);
+
+if (!cpus_str)
+return -1;
+
+virBufferAsprintf(buf,
+ "id, cpus_str);
+VIR_FREE(cpus_str);
+
+virBufferSetChildIndent(&controlBuf, buf);
+virBufferAsprintf(&controlBuf,
+ "\n",
+ control->granularity, control->min,
+ control->max_allocation);
+
+if (virBufferCheckError(&controlBuf) < 0)
+return -1;
+
+if (virBufferUse(&controlBuf)) {
+virBufferAddLit(buf, ">\n");
+virBufferAddBuffer(buf, &controlBuf);
+virBufferAddLit(buf, "\n");
+} else {
+virBufferAddLit(buf, "/>\n");
+}
+}
+
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+
+return 0;
+}
+
/**
* virCapabilitiesFormatXML:
* @caps: capabilities to format
@@ -1060,6 +1126,10 @@ virCapabilitiesFormatXML(virCapsPtr caps)
caps->host.caches) < 0)
goto error;
+if (virCapabilitiesFormatMemoryBandwidth(&buf, caps->host.nnodes,
+ caps->host.nodes) < 0)
+goto error;
+
for (i = 0; i < caps->host.nsecModels; i++) {
virBufferAddLit(&buf, "\n");
virBufferAdjustIndent(&buf, 2);
@@ -1602,6 +1672,40 @@ virCapabilitiesInitResctrl(virCapsPtr caps)
}
+static int
+virCapabilitiesInitRes
[libvirt] [PATCH v2 14/17] conf: Factor out virDomainResctrlDef update from virDomainCachetuneDefParse
From: Bing Niu
Factor out vcpus virDomainResctrlDef update from
virDomainCachetuneDefParse and introduce virDomainUpdateResctrl.
virDomainUpdateResctrl will format vcpus string and update
virDomainResctrlDef in virDomainDefPtr. So that this logic can
be reusable.
Signed-off-by: Bing Niu
---
src/conf/domain_conf.c | 93 +-
1 file changed, 55 insertions(+), 38 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index da8681d..0efbdf4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19082,6 +19082,58 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr
ctxt,
static int
+virDomainResctrlUpdate(virDomainDefPtr def,
+ xmlNodePtr node,
+ virResctrlAllocPtr alloc,
+ virBitmapPtr vcpus,
+ unsigned int flags)
+{
+char *vcpus_str = NULL;
+char *alloc_id = NULL;
+virDomainResctrlDefPtr tmp_resctrl = NULL;
+int ret = -1;
+
+if (VIR_ALLOC(tmp_resctrl) < 0)
+goto cleanup;
+
+/* We need to format it back because we need to be consistent in the naming
+ * even when users specify some "sub-optimal" string there. */
+vcpus_str = virBitmapFormat(vcpus);
+if (!vcpus_str)
+goto cleanup;
+
+if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
+alloc_id = virXMLPropString(node, "id");
+
+if (!alloc_id) {
+/* The number of allocations is limited and the directory structure is
flat,
+ * not hierarchical, so we need to have all same allocations in one
+ * directory, so it's nice to have it named appropriately. For now
it's
+ * 'vcpus_...' but it's designed in order for it to be changeable in
the
+ * future (it's part of the status XML). */
+if (virAsprintf(&alloc_id, "vcpus_%s", vcpus_str) < 0)
+goto cleanup;
+}
+
+if (virResctrlAllocSetID(alloc, alloc_id) < 0)
+goto cleanup;
+
+tmp_resctrl->vcpus = vcpus;
+tmp_resctrl->alloc = alloc;
+
+if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, tmp_resctrl) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+virDomainResctrlDefFree(tmp_resctrl);
+VIR_FREE(alloc_id);
+VIR_FREE(vcpus_str);
+return ret;
+}
+
+
+static int
virDomainCachetuneDefParse(virDomainDefPtr def,
xmlXPathContextPtr ctxt,
xmlNodePtr node,
@@ -19091,19 +19143,12 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
xmlNodePtr *nodes = NULL;
virBitmapPtr vcpus = NULL;
virResctrlAllocPtr alloc = NULL;
-virDomainResctrlDefPtr tmp_resctrl = NULL;
-char *tmp = NULL;
-char *vcpus_str = NULL;
-char *alloc_id = NULL;
ssize_t i = 0;
int n;
int ret = -1;
ctxt->node = node;
-if (VIR_ALLOC(tmp_resctrl) < 0)
-goto cleanup;
-
if (virDomainResctrlParseVcpus(def, node, &vcpus) < 0)
goto cleanup;
@@ -19141,45 +19186,17 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
goto cleanup;
}
-/* We need to format it back because we need to be consistent in the naming
- * even when users specify some "sub-optimal" string there. */
-VIR_FREE(vcpus_str);
-vcpus_str = virBitmapFormat(vcpus);
-if (!vcpus_str)
-goto cleanup;
-
-if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
-alloc_id = virXMLPropString(node, "id");
-
-if (!alloc_id) {
-/* The number of allocations is limited and the directory structure is
flat,
- * not hierarchical, so we need to have all same allocations in one
- * directory, so it's nice to have it named appropriately. For now
it's
- * 'vcpus_...' but it's designed in order for it to be changeable in
the
- * future (it's part of the status XML). */
-if (virAsprintf(&alloc_id, "vcpus_%s", vcpus_str) < 0)
-goto cleanup;
-}
-
-if (virResctrlAllocSetID(alloc, alloc_id) < 0)
-goto cleanup;
-
-VIR_STEAL_PTR(tmp_resctrl->vcpus, vcpus);
-VIR_STEAL_PTR(tmp_resctrl->alloc, alloc);
-
-if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, tmp_resctrl) < 0)
+if (virDomainResctrlUpdate(def, node, alloc, vcpus, flags) < 0)
goto cleanup;
+vcpus = NULL;
+alloc = NULL;
ret = 0;
cleanup:
ctxt->node = oldnode;
-virDomainResctrlDefFree(tmp_resctrl);
virObjectUnref(alloc);
virBitmapFree(vcpus);
-VIR_FREE(alloc_id);
-VIR_FREE(vcpus_str);
VIR_FREE(nodes);
-VIR_FREE(tmp);
return ret;
}
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
