Re: [libvirt] [PATCH v2 RESEND 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/7/24 下午10:58, Andrea Bolognani 写道: @@ -1385,7 +1403,12 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, * parent, and will have its address collected during the scan * of the parent's device type. */ -return 0; +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || +info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) +return virDomainPCIAddressExtensionReserveAddr(addrs, addr, + info->pciAddressExtFlags); +else +return 0; This doesn't look right: the comment specifically states that the PCI address will be handled by the parent device in this case, why wouldn't the zPCI address not be handled in the same way? Thanks for other comments! I cutted off them. For this comment, we have to collect zPCI address in case that zPCI address is specified but PCI address is not. I think I shall split two checks. Original code is: if (!virDeviceInfoPCIAddressPresent(info) || ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && (device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) Separate them and only add the new code for !virDeviceInfoPCIAddressPresent(info) case. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 9/9] conf: Add memory bandwidth allocation capability of host
On 2018年07月27日 05:04, John Ferlan wrote:
On 07/18/2018 03:57 AM, bing@intel.com wrote:
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| 108 +
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, 199 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
What about virsh available views? And similar to the RDT series what
about domstats? I think you can get some good ideas from the RDT CMT
RFC that's posted. Not even sure if it's already done internally - but
pointing it out... It doesn't have to be done as part of the series,
but eventually it may be nice.
Yes. RDT CMT will follow this memory_bandwidth host capability XML.
Huaqiang will handle that.
I'll give the following a cursory look as I have other tasks needing
some attention. I'll leave it in the back of my mind that I have to be
more thorough on the next pass once I get here.
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 7a810ef..3f52296 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,11 @@ 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);
+
+
Remove one of the blank lines.
OK.
Thanks a lot~
This was the only issue I saw in my quick glance - rest seemed OK.
John
[.]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] conf: Introduce cputune/memorytune to support memory bandwidth allocation
On 2018年07月27日 04:36, John Ferlan wrote: subj: Add support for memorytune XML processing for resctrl MBA OK! On 07/18/2018 03:57 AM, bing@intel.com wrote: 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 | 35 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, 351 insertions(+) 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 Strangely I expected to actually have a merge conflict here with previous changes, but I didn't! Lucky~ diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a3afe13..4e38446 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -757,6 +757,10 @@+ + + ... @@ -950,7 +954,38 @@ + + memorytune + +Optional memorytune element can control allocations for s/Optional/Since 4.7.0, the optional/ e.g. Since 4.7.0, the optional... OK! I added that before, but deleted since I am not sure which release this series can be accepted +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. vcpus specified s/./. The/ OK! +by memorytune can be identical to those specified by +cachetune. However they are not allowed to overlap each other. The last 2 sentences are "confusing". We didn't add similar wording to cachetune... It's one of those visual things I suppose, but it seems like there's a relationship between cachetune and memtune, but only if they intersect vCPU values. Thus if cachetune "joins" vcpus='0-3', then memtune must also join them - using other combinations of 0-3 isn't allowed. Yes! Let's update cachetune part also. I have to only imagine that would get more complicated with RDT +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. Should we indicate that it must be between 1 and 100 inclusive? Yes. However the minimum allowed memory bandwidth is defined by min_bandwidth of RDT. This is reported in host capability XML. + + + + diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f24a563..b4cd96b 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 695981c..ea9276f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19139,6 +19139,128 @@ virDomainCachetuneDefParse(virDomainDefPtr def, } +static int +virDomainMemorytuneDefParseMemory(xmlXPathContextPtr ctxt, + xmlNodePtr node, + virResctrlAllocPtr alloc) +{ +xmlNodePtr oldnode = ctxt->node; ++
Re: [libvirt] [PATCH 7/9] conf: Refactor virDomainCachetuneDefParse
On 2018年07月27日 01:48, John Ferlan wrote: On 07/18/2018 03:57 AM, bing@intel.com wrote: From: Bing Niu Refactoring virDomainCachetuneDefParse, extracting vcpus extracting, overlapping detecting and new resctrl allocation creating logic. Those two logic is common among different resource allocation technologies. Signed-off-by: Bing Niu --- src/conf/domain_conf.c | 195 + 1 file changed, 131 insertions(+), 64 deletions(-) You could make 3 patches out of this - one for each reduction of virDomainCachetuneDefParse... Will split that. The virDomainFindResctrlAlloc should have used Restune instead of ResctrlAlloc, right? Of course that changes based on how the naming patch shakes out. Actually I think perhaps virDomainRestuneVcpuMatch would be even better (where Restune ends up being whatever shakes out of the previous patch naming decision). Let's use virDomainResctrlVcpuMatch. :) In this area, I'm wondering what purpose does @new_alloc serve? If @alloc was already defined, then you added an error message. So the reality is - @new_alloc is always true. As you already note in patch 8. This is used by memory bandwidth to find a resctrl group defined by CAT already. 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? If so, above vpu match is not for different domain but for the memory bandwidth allocation and the cache line allocation of one domain. And above vcpu match function doesn't change any logic of existing virDomainCachetuneDefParseCache. It just extract from virDomainCachetuneDefParseCache and packing into one function. So that memory bandwidth parsing logic in patch 8 can reuse this vcpu matching part. :) code has been around for more than a release) would need to go in some sort of Validation API. This is one of those libvirt define and libvirtd restart quirks. If my understanding is incorrect. Could you please clarify here or give me some example? :) Finally, virDomainUpdateRestune should be virDomainRestuneUpdate since you started with virDomainRestuneParseVcpus and it should go after virDomainCachetuneDefParseCache Let's use virDomainResctrlUpdate Hope this all makes sense! John [] -- 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"? > > > > + is an optional attributes to show that []'s instances can be > > > + aggregated to be assigned for one mdev device. Set number of instances > > > by > > > + appending "instances=N" parameter for create. Instances number can't > > > exceed > > > + available_instances number. Without "instances=N" parameter will be > > > default > > > + one instance to create. > > > > Could there be a case where available_instances is n, but aggregation > > is only supported for a value m < n? If yes, should m be discoverable > > via the "aggregation" attribute? > > > > I don't think there could be a case for that. As for aggregation type, > it represents minimal resource allocated for a singleton, I can't see > any reason for possible m < n. Let's think about the interface beyond the immediate needs. It seems perfectly reasonable to me to think that a vendor driver might have a large number of resources available, the ability to aggregate resources beyond what's practical to enumerate with discrete types, yet have an upper bound of how many resources can be aggregated for a single instance. Thanks, Alex -- 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 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"?
> > + is an optional attributes to show that []'s instances can be
> > + aggregated to be assigned for one mdev device. Set number of instances by
> > + appending "instances=N" parameter for create. Instances number can't
> > exceed
> > + available_instances number. Without "instances=N" parameter will be
> > default
> > + one instance to create.
>
> Could there be a case where available_instances is n, but aggregation
> is only supported for a value m < n? If yes, should m be discoverable
> via the "aggregation" attribute?
>
I don't think there could be a case for that. As for aggregation type,
it represents minimal resource allocated for a singleton, I can't see
any reason for possible m < n.
Thanks.
> > +
> > +Example::
> > +
> > + # echo ",instances=N" > create
> > +
> > Directories and Files Under the sysfs for Each mdev Device
> > --
> >
> > @@ -268,6 +289,7 @@ Directories and Files Under the sysfs for Each mdev
> > Device
> >|- [parent phy device]
> >|--- [$MDEV_UUID]
> > |--- remove
> > +|--- instances
> > |--- mdev_type {link to its type}
> > |--- vendor-specific-attributes [optional]
> >
> > @@ -281,6 +303,11 @@ Example::
> >
> > # echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
> >
> > +* instances
> > +
> > +For aggregation type show number of instances assigned for this mdev. For
> > normal
> > +type or default will just show one instance.
> > +
> > Mediated device Hot plug
> >
> >
>
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/9] conf: Rename cachetune to restune
On 2018年07月27日 00:32, John Ferlan wrote:
On 07/18/2018 03:57 AM, bing@intel.com wrote:
From: Bing Niu
Resctrl not only supports cache tuning, but also memory bandwidth
tuning. Renaming cachetune to restune(resource tuning) to reflect
that. With restune, all allocation for different resources (cache,
memory bandwidth) are aggregated and represented by a
virResctrlAllocPtr inside virDomainRestuneDef.
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(-)
As I noted previously, not much a fan of Restune instead of Cachetune,
but I understand the logic why you went that way.
I wonder if "virDomainResAllocDef" is any better (resallocs,
nresallocs)? or if that clashes with any other namespace so far? or is
too close to virResctrlAllocPtr.
Or perhaps "virDomainResCtrlDef" w/ resctrls and nresctrls to mimic the
virresctrl.{c,h} naming scheme.
virDomainResCtrlDef is better. How about we did one puny adjustment.
virDomainResctrlDef w/ resctrls and nresctrls?
Use little 'c' can align with virresctrl.c function naming. ;)
As previously stated, "Naming is hard"... Wish there was more feedback
than just me on this, but in the long run, I'll go with whatever the
Intel team agrees upon as it's not that big a deal. If someone else has
agita after things are pushed and wants to change the name, then they
know how to send patches.
John
[...]
--
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 2018.07.24 11:44:40 -0600, Alex Williamson wrote: > On Fri, 20 Jul 2018 10:19:24 +0800 > Zhenyu Wang wrote: > > > Current mdev device create interface depends on fixed mdev type, which get > > uuid > > from user to create instance of mdev device. If user wants to use customized > > number of resource for mdev device, then only can create new mdev type for > > that > > which may not be flexible. This requirement comes not only from to be able > > to > > allocate flexible resources for KVMGT, but also from Intel scalable IO > > virtualization which would use vfio/mdev to be able to allocate arbitrary > > resources on mdev instance. More info on [1] [2] [3]. > > > > To allow to create user defined resources for mdev, it trys to extend mdev > > create interface by adding new "instances=xxx" parameter following uuid, for > > target mdev type if aggregation is supported, it can create new mdev device > > which contains resources combined by number of instances, e.g > > > > echo ",instances=10" > create > > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute > > which > > can support this setting. If no "aggregation" attribute found for mdev type, > > previous behavior is still kept for one instance allocation. And new sysfs > > attribute "instances" is created for each mdev device to show allocated > > number. > > > > This trys to create new KVMGT type with minimal vGPU resources which can be > > combined with "instances=x" setting to allocate for user wanted resources. > > "instances" makes me think this is arg helps to create multiple mdev > instances rather than consuming multiple instances for a single mdev. > You're already exposing the "aggregation" attribute, so doesn't > "aggregate" perhaps make more sense as the create option? We're asking > the driver to aggregate $NUM instances into a single mdev. The mdev > attribute should then perhaps also be "aggregated_instances". yeah that seems better. > > The next user question for the interface might be what aspect of the > device gets multiplied by this aggregation? In i915 I see you're > multiplying the memory sizes by the instance, but clearly the > resolution doesn't change. I assume this is sort of like mdev types > themselves, ie. some degree of what a type means is buried in the > implementation and some degree of what some number of those types > aggregated together means is impossible to describe generically. > yeah, the purpose was to increase memory resource only, but due to current free formatted 'description', can't have a meaningful expression for that, and not sure if libvirt likes to understand vendor specific behavior e.g for intel vGPU? > We're also going to need to add aggregation to the checklist for device > compatibility for migration, for example 1) is it the same mdev_type, > 1a) are the aggregation counts the same (new), 2) is the host driver > compatible (TBD). > Right, will check with Zhi on that. > The count handling in create_store(), particularly MDEV_CREATE_OPT_LEN > looks a little suspicious. I think we should just be validating that > the string before the ',' or the entire string if there is no comma is > UUID length. Pass only that to uuid_le_to_bin(). We can then strncmp > as you have for "instances=" (or "aggregate=") but then let's be sure > to end the string we pass to kstrtouint(), ie. assume there could be > further args. Original purpose was to limit the length of string to accept, but can take this way without issue I think. > > Documentation/ABI/testing/sysfs-bus-vfio-mdev also needs to be updated > with this series. Ok. Thanks > > I'm curious what libvirt folks and Kirti think of this, it looks like > it has a nice degree of backwards compatibility, both in the sysfs > interface and the vendor driver interface. Thanks, > > Alex -- Open Source Technology Center, Intel ltd. $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827 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/8] snapshots: Avoid term 'checkpoint' for full system snapshot
On 06/26/2018 08:27 PM, Eric Blake wrote: Lets try to see an example: T1 - user create new vm marked for incremental backup - system create base volume (S1) - system create new dirty bitmap (B1) Why do you need a dirty bitmap on a brand new system? By definition, if the VM is brand new, every sector that the guest touches will be part of the first incremental backup, which is no different than taking a full backup of every sector? But if it makes life easier by following consistent patterns, I also don't see a problem with creating a first checkpoint at the time an image is first created (my API proposal would allow you to create a domain, start it in the paused state, create a checkpoint, and then resume the guest so that it can start executing). T2 - user create a snapshot - dirty bitmap in original snapshot deactivated (B1) - system create new snapshot (S2) - system starts new dirty bitmap in the new snapshot (B2) I'm still worried that interactions between snapshots (where the backing chain grows) and bitmaps may present interesting challenges. But what you are describing here is that the act of creating a snapshot (to enlarge the backing chain) also has the effect of creating a snapshot (a that should read "also has the effect of creating a checkpoint" Except that I'm not quite sure how best to handle the interaction between snapshots and checkpoints using existing qemu primitives. Right now, I'm leaning back to the idea that if you have an external backing file (that is, the act of creating a snapshot expanded the disk chain from 'S1' into 'S1 <- S2'), then creating an incremental backup that covers just the disk changes since that point in time is the same as a "sync":"top" copy of the just-created S2 image (no bitmap is needed to track what needs copying) - which works well for qemu writing out the backup file. But since we are talking about allowing third-party backups (where we provide an NBD export and the client can query which portions are dirty), then using the snapshot as the start point in time would indeed require that we either have a bitmap to expose (that is, we need to create a bitmap as part of the same transaction as creating the external snapshot file), or that we can resynthesize a bitmap based on the clusters allocated in S2 at the time we start the backup operation (that's an operation that I don't see in qemu right now). And if we DO want to allow external snapshots to automatically behave as checkpoints for use by incremental backups, that makes me wonder if I need to eventually enhance the existing virDomainSnapshotCreateXML() to also accept XML describing a checkpoint to be created simultaneously with the snapshot (the way my proposal already allows creating a checkpoint simultaneously with virDomainBackupBegin()). Another point that John and I discussed on IRC is that migrating bitmaps still has some design work to figure out. Remember, right now, there are basically three modes of operation regarding storage between source and endpoint of a migration: 1. Storage is shared. As long as qemu flushes the bitmap before inactivating on the source, then activating on the destination can load the bitmap, and everything is fine. The migration stream does not have to include the bitmaps. 2. Storage is not shared, but the storage is migrated via flags to the migrate command (we're trying to move away from this version) - there, qemu knows that it has to migrate the bitmaps as part of the migration stream. 3. Storage is not shared, and the storage is migrated via NBD (libvirt favors using this version for non-shared storage). Libvirt starts 'qemu -S' on the destination, pre-creates a destination file large enough to match the source, starts an NBD server at the destination, then on the source starts a block-mirror operation to the destination. When the drive is mirrored, libvirt then kicks off the migration using the same command as in style 1; when all state is transferred, the source then stops the block-mirror, disconnects the NBD client, the destination then stops the NBD server, and the destination can finally start executing. But note that in this mode, no bitmaps are migrated. So we need some way for libvirt to also migrate bitmap state to the destination (perhaps having the NBD server open multiple exports - one for the block itself, but another export for each bitmap that needs to be copied). At this point, I think the pressure is on me to provide a working demo of incremental backups working without any external snapshots or migration, before we expand into figuring out interactions between features. -- 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 v2 1/1] conf: Add validation of input devices
On 07/26/2018 03:27 AM, Han Han wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1591151
>
> Add function virDomainInputDefValidate to validate input devices.
> Make sure evdev attribute of source element is not used by mouse,
> keyboard, and tablet input device.
>
> Signed-off-by: Han Han
> ---
> src/conf/domain_conf.c | 22 +-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
FYI: For a single patch series, no need to create a --cover-letter
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 178c6d2711..a65b53b70c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5723,6 +5723,24 @@ virDomainVsockDefValidate(const virDomainVsockDef
> *vsock)
> return 0;
> }
>
> +static int
> +virDomainInputDefValidate(const virDomainInputDef *input)
> +{
> +switch (input->type) {
So you missed the part where I said like virDomainInputDefGetPath...
This should be
switch ((virDomainInputType) input->type)
> +case VIR_DOMAIN_INPUT_TYPE_MOUSE:
> +case VIR_DOMAIN_INPUT_TYPE_TABLET:
> +case VIR_DOMAIN_INPUT_TYPE_KBD:
> +if (input->source.evdev) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> +_("setting source evdev path only supported
> for "
> + "passthrough input devices"));
> + return -1;
> +}
Thus necessitating cases for:
case VIR_DOMAIN_INPUT_TYPE_LAST:
(int the above failure pile)
and
case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
break;
I have fixed this and pushed with my
Reviewed-by: John Ferlan
John
> +}
> +
> +return 0;
> +}
> +
>
> static int
> virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev,
> @@ -5762,9 +5780,11 @@ virDomainDeviceDefValidateInternal(const
> virDomainDeviceDef *dev,
> case VIR_DOMAIN_DEVICE_VSOCK:
> return virDomainVsockDefValidate(dev->data.vsock);
>
> +case VIR_DOMAIN_DEVICE_INPUT:
> +return virDomainInputDefValidate(dev->data.input);
> +
> case VIR_DOMAIN_DEVICE_LEASE:
> case VIR_DOMAIN_DEVICE_FS:
> -case VIR_DOMAIN_DEVICE_INPUT:
> case VIR_DOMAIN_DEVICE_SOUND:
> case VIR_DOMAIN_DEVICE_WATCHDOG:
> case VIR_DOMAIN_DEVICE_GRAPHICS:
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 40/41] util: lease: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/virlease.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/util/virlease.c b/src/util/virlease.c
index b49105d..baaceaf 100644
--- a/src/util/virlease.c
+++ b/src/util/virlease.c
@@ -55,7 +55,7 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new,
const char *ip_to_delete,
char **server_duid)
{
-char *lease_entries = NULL;
+VIR_AUTOFREE(char *) lease_entries = NULL;
virJSONValuePtr leases_array = NULL;
long long expirytime;
int custom_lease_file_len = 0;
@@ -146,7 +146,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr
leases_array_new,
cleanup:
virJSONValueFree(leases_array);
-VIR_FREE(lease_entries);
return ret;
}
@@ -235,7 +234,7 @@ virLeaseNew(virJSONValuePtr *lease_ret,
virJSONValuePtr lease_new = NULL;
const char *exptime_tmp = virGetEnvAllowSUID("DNSMASQ_LEASE_EXPIRES");
long long expirytime = 0;
-char *exptime = NULL;
+VIR_AUTOFREE(char *) exptime = NULL;
int ret = -1;
/* In case hostname is still unknown, use the last known one */
@@ -291,7 +290,6 @@ virLeaseNew(virJSONValuePtr *lease_ret,
*lease_ret = lease_new;
lease_new = NULL;
cleanup:
-VIR_FREE(exptime);
virJSONValueFree(lease_new);
return ret;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 32/41] util: hostdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/virhostdev.c | 91 +--
1 file changed, 30 insertions(+), 61 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index d5075ac..492c42f 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -186,17 +186,14 @@ virHostdevManagerNew(void)
goto error;
}
} else {
-char *rundir = NULL;
+VIR_AUTOFREE(char *) rundir = NULL;
mode_t old_umask;
if (!(rundir = virGetUserRuntimeDirectory()))
goto error;
-if (virAsprintf(&hostdevMgr->stateDir, "%s/hostdevmgr", rundir) < 0) {
-VIR_FREE(rundir);
+if (virAsprintf(&hostdevMgr->stateDir, "%s/hostdevmgr", rundir) < 0)
goto error;
-}
-VIR_FREE(rundir);
old_umask = umask(077);
@@ -289,17 +286,12 @@ virHostdevPCISysfsPath(virDomainHostdevDefPtr hostdev,
static int
virHostdevIsVirtualFunction(virDomainHostdevDefPtr hostdev)
{
-char *sysfs_path = NULL;
-int ret = -1;
+VIR_AUTOFREE(char *) sysfs_path = NULL;
if (virHostdevPCISysfsPath(hostdev, &sysfs_path) < 0)
-return ret;
+return -1;
-ret = virPCIIsVirtualFunction(sysfs_path);
-
-VIR_FREE(sysfs_path);
-
-return ret;
+return virPCIIsVirtualFunction(sysfs_path);
}
@@ -309,17 +301,15 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev,
char **linkdev,
int *vf)
{
-int ret = -1;
-char *sysfs_path = NULL;
+VIR_AUTOFREE(char *) sysfs_path = NULL;
if (virHostdevPCISysfsPath(hostdev, &sysfs_path) < 0)
-return ret;
+return -1;
if (virPCIIsVirtualFunction(sysfs_path) == 1) {
if (virPCIGetVirtualFunctionInfo(sysfs_path, pfNetDevIdx,
- linkdev, vf) < 0) {
-goto cleanup;
-}
+ linkdev, vf) < 0)
+return -1;
} else {
/* In practice this should never happen, since we currently
* only support assigning SRIOV VFs via parent.data.net);
virtPort = virDomainNetGetActualVirtPortProfile(hostdev->parent.data.net);
@@ -507,24 +487,19 @@ virHostdevSetNetConfig(virDomainHostdevDefPtr hostdev,
_("direct setting of the vlan tag is not allowed "
"for hostdev devices using %s mode"),
virNetDevVPortTypeToString(virtPort->virtPortType));
-goto cleanup;
+return -1;
}
if (virHostdevNetConfigVirtPortProfile(linkdev, vf, virtPort,
&hostdev->parent.data.net->mac,
- uuid, port_profile_associate) <
0) {
-goto cleanup;
-}
+ uuid, port_profile_associate) <
0)
+return -1;
} else {
if (virNetDevSetNetConfig(linkdev, vf, &hostdev->parent.data.net->mac,
- vlan, NULL, true) < 0) {
-goto cleanup;
-}
+ vlan, NULL, true) < 0)
+return -1;
}
-ret = 0;
- cleanup:
-VIR_FREE(linkdev);
-return ret;
+return 0;
}
@@ -540,13 +515,13 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
const char *stateDir,
const char *oldStateDir)
{
-char *linkdev = NULL;
+VIR_AUTOFREE(char *) linkdev = NULL;
+VIR_AUTOFREE(virMacAddrPtr) MAC = NULL;
+VIR_AUTOFREE(virMacAddrPtr) adminMAC = NULL;
virNetDevVPortProfilePtr virtPort;
int ret = -1;
int vf = -1;
bool port_profile_associate = false;
-virMacAddrPtr MAC = NULL;
-virMacAddrPtr adminMAC = NULL;
virNetDevVlanPtr vlan = NULL;
@@ -656,9 +631,6 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
}
cleanup:
-VIR_FREE(linkdev);
-VIR_FREE(MAC);
-VIR_FREE(adminMAC);
virNetDevVlanFree(vlan);
return ret;
@@ -763,8 +735,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
mgr->inactivePCIHostdevs) < 0)
goto reattachdevs;
} else {
-char *driverPath;
-char *driverName;
+VIR_AUTOFREE(char *) driverPath = NULL;
+VIR_AUTOFREE(char *) driverName = NULL;
int stub;
/* Unmanaged devices should already have been marked as
@@ -790,9 +762,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
stub = virPCIStubDr
[libvirt] [PATCH v2 41/41] util: lease: use VIR_AUTOPTR for aggregate types
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/virlease.c | 76 -
1 file changed, 29 insertions(+), 47 deletions(-)
diff --git a/src/util/virlease.c b/src/util/virlease.c
index baaceaf..7c6c37e 100644
--- a/src/util/virlease.c
+++ b/src/util/virlease.c
@@ -56,40 +56,36 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr
leases_array_new,
char **server_duid)
{
VIR_AUTOFREE(char *) lease_entries = NULL;
-virJSONValuePtr leases_array = NULL;
+VIR_AUTOPTR(virJSONValue) leases_array = NULL;
long long expirytime;
int custom_lease_file_len = 0;
virJSONValuePtr lease_tmp = NULL;
const char *ip_tmp = NULL;
const char *server_duid_tmp = NULL;
size_t i;
-int ret = -1;
/* Read entire contents */
if ((custom_lease_file_len = virFileReadAll(custom_lease_file,
VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX,
&lease_entries)) < 0) {
-goto cleanup;
+return -1;
}
/* Check for previous leases */
-if (custom_lease_file_len == 0) {
-ret = 0;
-goto cleanup;
-}
+if (custom_lease_file_len == 0)
+return 0;
if (!(leases_array = virJSONValueFromString(lease_entries))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("invalid json in file: %s, rewriting it"),
custom_lease_file);
-ret = 0;
-goto cleanup;
+return 0;
}
if (!virJSONValueIsArray(leases_array)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("couldn't fetch array of leases"));
-goto cleanup;
+return -1;
}
i = 0;
@@ -97,14 +93,14 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr
leases_array_new,
if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to parse json"));
-goto cleanup;
+return -1;
}
if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp, "ip-address")) ||
(virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time",
&expirytime) < 0)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to parse json"));
-goto cleanup;
+return -1;
}
/* Check whether lease has to be included or not */
@@ -121,14 +117,14 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr
leases_array_new,
/* Control reaches here when the 'action' is not for an
* ipv6 lease or, for some weird reason the env var
* DNSMASQ_SERVER_DUID wasn't set*/
-goto cleanup;
+return -1;
}
} else {
/* Inject server-duid into those ipv6 leases which
* didn't have it previously, for example, those
* created by leaseshelper from libvirt 1.2.6 */
if (virJSONValueObjectAppendString(lease_tmp, "server-duid",
*server_duid) < 0)
-goto cleanup;
+return -1;
}
}
@@ -136,17 +132,13 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr
leases_array_new,
if (virJSONValueArrayAppend(leases_array_new, lease_tmp) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to create json"));
-goto cleanup;
+return -1;
}
ignore_value(virJSONValueArraySteal(leases_array, i));
}
-ret = 0;
-
- cleanup:
-virJSONValueFree(leases_array);
-return ret;
+return 0;
}
@@ -157,7 +149,6 @@ virLeasePrintLeases(virJSONValuePtr leases_array_new,
virJSONValuePtr lease_tmp = NULL;
const char *ip_tmp = NULL;
long long expirytime = 0;
-int ret = -1;
size_t i;
/* Man page of dnsmasq says: the script (helper program, in our case)
@@ -174,7 +165,7 @@ virLeasePrintLeases(virJSONValuePtr leases_array_new,
if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp, "ip-address"))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to parse json"));
-goto cleanup;
+return -1;
}
if (!strchr(ip_tmp, ':')) {
if (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time",
@@ -198,7 +189,7 @@ virLeasePrintLeases(virJSONValuePtr leases_array_new,
if (!(ip_tmp = virJSONValueObjectGetString(le
[libvirt] [PATCH v2 22/41] util: usb: modify virUSBDeviceListAdd to take double pointer
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
---
src/util/virhostdev.c | 6 +++---
src/util/virusb.c | 10 +-
src/util/virusb.h | 2 +-
tests/virusbtest.c| 4 ++--
4 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index f4bd19d..d5075ac 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -1236,7 +1236,7 @@ virHostdevUpdateActiveUSBDevices(virHostdevManagerPtr mgr,
virUSBDeviceSetUsedBy(usb, drv_name, dom_name);
-if (virUSBDeviceListAdd(mgr->activeUSBHostdevs, usb) < 0) {
+if (virUSBDeviceListAdd(mgr->activeUSBHostdevs, &usb) < 0) {
virUSBDeviceFree(usb);
goto cleanup;
}
@@ -1406,7 +1406,7 @@ virHostdevMarkUSBDevices(virHostdevManagerPtr mgr,
* from the virUSBDeviceList that passed in on success,
* perform rollback on failure.
*/
-if (virUSBDeviceListAdd(mgr->activeUSBHostdevs, usb) < 0)
+if (virUSBDeviceListAdd(mgr->activeUSBHostdevs, &usb) < 0)
goto error;
}
@@ -1555,7 +1555,7 @@ virHostdevPrepareUSBDevices(virHostdevManagerPtr mgr,
if (virHostdevFindUSBDevice(hostdev, required, &usb) < 0)
goto cleanup;
-if (usb && virUSBDeviceListAdd(list, usb) < 0) {
+if (usb && virUSBDeviceListAdd(list, &usb) < 0) {
virUSBDeviceFree(usb);
goto cleanup;
}
diff --git a/src/util/virusb.c b/src/util/virusb.c
index 2fe1bfc..7818232 100644
--- a/src/util/virusb.c
+++ b/src/util/virusb.c
@@ -181,7 +181,7 @@ virUSBDeviceSearch(unsigned int vendor,
if (!usb)
goto cleanup;
-if (virUSBDeviceListAdd(list, usb) < 0) {
+if (virUSBDeviceListAdd(list, &usb) < 0) {
virUSBDeviceFree(usb);
goto cleanup;
}
@@ -463,15 +463,15 @@ virUSBDeviceListDispose(void *obj)
int
virUSBDeviceListAdd(virUSBDeviceListPtr list,
-virUSBDevicePtr dev)
+virUSBDevicePtr *dev)
{
-if (virUSBDeviceListFind(list, dev)) {
+if (virUSBDeviceListFind(list, *dev)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Device %s is already in use"),
- dev->name);
+ (*dev)->name);
return -1;
}
-return VIR_APPEND_ELEMENT(list->devs, list->count, dev);
+return VIR_APPEND_ELEMENT(list->devs, list->count, *dev);
}
virUSBDevicePtr
diff --git a/src/util/virusb.h b/src/util/virusb.h
index 716e8c6..078dee6 100644
--- a/src/util/virusb.h
+++ b/src/util/virusb.h
@@ -88,7 +88,7 @@ int virUSBDeviceFileIterate(virUSBDevicePtr dev,
virUSBDeviceListPtr virUSBDeviceListNew(void);
int virUSBDeviceListAdd(virUSBDeviceListPtr list,
-virUSBDevicePtr dev);
+virUSBDevicePtr *dev);
virUSBDevicePtr virUSBDeviceListGet(virUSBDeviceListPtr list,
int idx);
size_t virUSBDeviceListCount(virUSBDeviceListPtr list);
diff --git a/tests/virusbtest.c b/tests/virusbtest.c
index 8728fe9..05bba2b 100644
--- a/tests/virusbtest.c
+++ b/tests/virusbtest.c
@@ -173,7 +173,7 @@ testUSBList(const void *opaque ATTRIBUTE_UNUSED)
dev = virUSBDeviceListGet(devlist, 0);
dev = virUSBDeviceListSteal(devlist, dev);
-if (virUSBDeviceListAdd(list, dev) < 0)
+if (virUSBDeviceListAdd(list, &dev) < 0)
goto cleanup;
dev = NULL;
}
@@ -196,7 +196,7 @@ testUSBList(const void *opaque ATTRIBUTE_UNUSED)
dev = virUSBDeviceListGet(devlist, 0);
dev = virUSBDeviceListSteal(devlist, dev);
-if (virUSBDeviceListAdd(list, dev) < 0)
+if (virUSBDeviceListAdd(list, &dev) < 0)
goto cleanup;
dev = NULL;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 38/41] util: kmod: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/virkmod.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/util/virkmod.c b/src/util/virkmod.c
index 219fad6..d981cd4 100644
--- a/src/util/virkmod.c
+++ b/src/util/virkmod.c
@@ -155,13 +155,12 @@ virKModUnload(const char *module)
bool
virKModIsBlacklisted(const char *module)
{
-bool retval = false;
size_t i;
-char *drvblklst = NULL;
-char *outbuf = NULL;
+VIR_AUTOFREE(char *) drvblklst = NULL;
+VIR_AUTOFREE(char *) outbuf = NULL;
if (virAsprintfQuiet(&drvblklst, "blacklist %s\n", module) < 0)
-goto cleanup;
+return false;
/* modprobe will convert all '-' into '_', so we need to as well */
for (i = 0; i < drvblklst[i]; i++)
@@ -169,13 +168,10 @@ virKModIsBlacklisted(const char *module)
drvblklst[i] = '_';
if (doModprobe("-c", NULL, &outbuf, NULL) < 0)
-goto cleanup;
+return false;
if (strstr(outbuf, drvblklst))
-retval = true;
+return true;
- cleanup:
-VIR_FREE(drvblklst);
-VIR_FREE(outbuf);
-return retval;
+return false;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 16/41] util: firewall: use VIR_AUTOPTR for aggregate types
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/virfirewall.c | 36 +---
1 file changed, 13 insertions(+), 23 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index b4a4d06..c786d76 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -119,14 +119,13 @@ virFirewallCheckUpdateLock(bool *lockflag,
const char *const*args)
{
int status; /* Ignore failed commands without logging them */
-virCommandPtr cmd = virCommandNewArgs(args);
+VIR_AUTOPTR(virCommand) cmd = virCommandNewArgs(args);
if (virCommandRun(cmd, &status) < 0 || status) {
VIR_INFO("locking not supported by %s", args[0]);
} else {
VIR_INFO("using locking for %s", args[0]);
*lockflag = true;
}
-virCommandFree(cmd);
}
static void
@@ -673,16 +672,15 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule,
{
size_t i;
const char *bin = virFirewallLayerCommandTypeToString(rule->layer);
-virCommandPtr cmd = NULL;
+VIR_AUTOPTR(virCommand) cmd = NULL;
int status;
-int ret = -1;
VIR_AUTOFREE(char *) error = NULL;
if (!bin) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown firewall layer %d"),
rule->layer);
-goto cleanup;
+return -1;
}
cmd = virCommandNewArgList(bin, NULL);
@@ -694,7 +692,7 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule,
virCommandSetErrorBuffer(cmd, &error);
if (virCommandRun(cmd, &status) < 0)
-goto cleanup;
+return -1;
if (status != 0) {
if (ignoreErrors) {
@@ -705,14 +703,11 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule,
_("Failed to apply firewall rules %s: %s"),
NULLSTR(args), NULLSTR(error));
VIR_FREE(*output);
-goto cleanup;
+return -1;
}
}
-ret = 0;
- cleanup:
-virCommandFree(cmd);
-return ret;
+return 0;
}
@@ -805,8 +800,7 @@ virFirewallApplyRule(virFirewallPtr firewall,
{
VIR_AUTOFREE(char *) output = NULL;
VIR_AUTOFREE(char *) str = virFirewallRuleToString(rule);
-char **lines = NULL;
-int ret = -1;
+VIR_AUTOPTR(virString) lines = NULL;
VIR_INFO("Applying rule '%s'", NULLSTR(str));
if (rule->ignoreErrors)
@@ -831,28 +825,25 @@ virFirewallApplyRule(virFirewallPtr firewall,
if (rule->queryCB && output) {
if (!(lines = virStringSplit(output, "\n", -1)))
-goto cleanup;
+return -1;
VIR_DEBUG("Invoking query %p with '%s'", rule->queryCB, output);
if (rule->queryCB(firewall, (const char *const *)lines,
rule->queryOpaque) < 0)
-goto cleanup;
+return -1;
if (firewall->err == ENOMEM) {
virReportOOMError();
-goto cleanup;
+return -1;
}
if (firewall->err) {
virReportSystemError(firewall->err, "%s",
_("Unable to create rule"));
-goto cleanup;
+return -1;
}
}
-ret = 0;
- cleanup:
-virStringListFree(lines);
-return ret;
+return 0;
}
static int
@@ -926,7 +917,7 @@ virFirewallApply(virFirewallPtr firewall)
if (virFirewallApplyGroup(firewall, i) < 0) {
VIR_DEBUG("Rolling back groups up to %zu for %p", i, firewall);
size_t first = i;
-virErrorPtr saved_error = virSaveLastError();
+VIR_AUTOPTR(virError) saved_error = virSaveLastError();
/*
* Look at any inheritance markers to figure out
@@ -947,7 +938,6 @@ virFirewallApply(virFirewallPtr firewall)
}
virSetError(saved_error);
-virFreeError(saved_error);
VIR_DEBUG("Done rolling back groups for %p", firewall);
goto cleanup;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 36/41] util: iscsi: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/viriscsi.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index d4c745a..13fd02c 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -80,7 +80,7 @@ virISCSIGetSession(const char *devpath,
.session = NULL,
.devpath = devpath,
};
-char *error = NULL;
+VIR_AUTOFREE(char *) error = NULL;
int exitstatus = 0;
virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode",
@@ -101,7 +101,6 @@ virISCSIGetSession(const char *devpath,
NULLSTR(error));
cleanup:
-VIR_FREE(error);
virCommandFree(cmd);
return cbdata.session;
}
@@ -120,7 +119,10 @@ virStorageBackendIQNFound(const char *initiatoriqn,
int ret = IQN_MISSING, fd = -1;
char ebuf[64];
FILE *fp = NULL;
-char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
+VIR_AUTOFREE(char *) line = NULL;
+char *newline = NULL;
+char *iqn = NULL;
+char *token = NULL;
virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
"--mode", "iface", NULL);
@@ -192,7 +194,6 @@ virStorageBackendIQNFound(const char *initiatoriqn,
if (ret == IQN_MISSING)
VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
-VIR_FREE(line);
VIR_FORCE_FCLOSE(fp);
VIR_FORCE_CLOSE(fd);
virCommandFree(cmd);
@@ -206,7 +207,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
char **ifacename)
{
int ret = -1, exitstatus = -1;
-char *temp_ifacename;
+VIR_AUTOFREE(char *) temp_ifacename = NULL;
virCommandPtr cmd = NULL;
if (virAsprintf(&temp_ifacename,
@@ -267,7 +268,6 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
cleanup:
virCommandFree(cmd);
-VIR_FREE(temp_ifacename);
if (ret != 0)
VIR_FREE(*ifacename);
return ret;
@@ -289,7 +289,7 @@ virISCSIConnection(const char *portal,
NULL
};
virCommandPtr cmd;
-char *ifacename = NULL;
+VIR_AUTOFREE(char *) ifacename = NULL;
cmd = virCommandNewArgs(baseargv);
virCommandAddArgSet(cmd, extraargv);
@@ -326,7 +326,6 @@ virISCSIConnection(const char *portal,
cleanup:
virCommandFree(cmd);
-VIR_FREE(ifacename);
return ret;
}
@@ -377,15 +376,13 @@ virISCSIGetTargets(char **const groups,
void *data)
{
struct virISCSITargetList *list = data;
-char *target;
+VIR_AUTOFREE(char *) target = NULL;
if (VIR_STRDUP(target, groups[1]) < 0)
return -1;
-if (VIR_APPEND_ELEMENT(list->targets, list->ntargets, target) < 0) {
-VIR_FREE(target);
+if (VIR_APPEND_ELEMENT(list->targets, list->ntargets, target) < 0)
return -1;
-}
return 0;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 39/41] util: kmod: use VIR_AUTOPTR for aggregate types
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/virkmod.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/src/util/virkmod.c b/src/util/virkmod.c
index d981cd4..9d0375b 100644
--- a/src/util/virkmod.c
+++ b/src/util/virkmod.c
@@ -28,8 +28,7 @@
static int
doModprobe(const char *opts, const char *module, char **outbuf, char **errbuf)
{
-int ret = -1;
-virCommandPtr cmd = NULL;
+VIR_AUTOPTR(virCommand) cmd = NULL;
cmd = virCommandNew(MODPROBE);
if (opts)
@@ -42,32 +41,23 @@ doModprobe(const char *opts, const char *module, char
**outbuf, char **errbuf)
virCommandSetErrorBuffer(cmd, errbuf);
if (virCommandRun(cmd, NULL) < 0)
-goto cleanup;
+return -1;
-ret = 0;
-
- cleanup:
-virCommandFree(cmd);
-return ret;
+return 0;
}
static int
doRmmod(const char *module, char **errbuf)
{
-int ret = -1;
-virCommandPtr cmd = NULL;
+VIR_AUTOPTR(virCommand) cmd = NULL;
cmd = virCommandNewArgList(RMMOD, module, NULL);
virCommandSetErrorBuffer(cmd, errbuf);
if (virCommandRun(cmd, NULL) < 0)
-goto cleanup;
+return -1;
-ret = 0;
-
- cleanup:
-virCommandFree(cmd);
-return ret;
+return 0;
}
/**
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 19/41] util: pci: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When variables of types virPCIDevicePtr, virPCIDeviceAddressPtr
and virPCIEDeviceInfoPtr are declared using VIR_AUTOPTR, the functions
virPCIDeviceFree, virPCIDeviceAddressFree and virPCIEDeviceInfoFree,
respectively, will be run automatically on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar
Reviewed-by: Erik Skultety
---
src/util/virpci.c | 7 ++-
src/util/virpci.h | 7 +++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 8d02366..46f9905 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -39,7 +39,6 @@
#include "dirname.h"
#include "virlog.h"
-#include "viralloc.h"
#include "vircommand.h"
#include "virerror.h"
#include "virfile.h"
@@ -3288,3 +3287,9 @@ virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev)
VIR_FREE(dev->link_sta);
VIR_FREE(dev);
}
+
+void
+virPCIDeviceAddressFree(virPCIDeviceAddressPtr address)
+{
+VIR_FREE(address);
+}
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 794b7e5..2ac8769 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -28,6 +28,7 @@
# include "virmdev.h"
# include "virobject.h"
# include "virutil.h"
+# include "viralloc.h"
typedef struct _virPCIDevice virPCIDevice;
typedef virPCIDevice *virPCIDevicePtr;
@@ -253,4 +254,10 @@ void virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev);
ssize_t virPCIGetMdevTypes(const char *sysfspath,
virMediatedDeviceType ***types);
+void virPCIDeviceAddressFree(virPCIDeviceAddressPtr address);
+
+VIR_DEFINE_AUTOPTR_FUNC(virPCIDevice, virPCIDeviceFree)
+VIR_DEFINE_AUTOPTR_FUNC(virPCIDeviceAddress, virPCIDeviceAddressFree)
+VIR_DEFINE_AUTOPTR_FUNC(virPCIEDeviceInfo, virPCIEDeviceInfoFree)
+
#endif /* __VIR_PCI_H__ */
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 17/41] util: hook: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/virhook.c | 22 +-
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/src/util/virhook.c b/src/util/virhook.c
index facd74a..4673655 100644
--- a/src/util/virhook.c
+++ b/src/util/virhook.c
@@ -122,8 +122,7 @@ static int virHooksFound = -1;
static int
virHookCheck(int no, const char *driver)
{
-char *path;
-int ret;
+VIR_AUTOFREE(char *) path = NULL;
if (driver == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -139,18 +138,17 @@ virHookCheck(int no, const char *driver)
}
if (!virFileExists(path)) {
-ret = 0;
VIR_DEBUG("No hook script %s", path);
-} else if (!virFileIsExecutable(path)) {
-ret = 0;
+return 0;
+}
+
+if (!virFileIsExecutable(path)) {
VIR_WARN("Non-executable hook script %s", path);
-} else {
-ret = 1;
-VIR_DEBUG("Found hook script %s", path);
+return 0;
}
-VIR_FREE(path);
-return ret;
+VIR_DEBUG("Found hook script %s", path);
+return 1;
}
/*
@@ -233,7 +231,7 @@ virHookCall(int driver,
char **output)
{
int ret;
-char *path;
+VIR_AUTOFREE(char *) path = NULL;
virCommandPtr cmd;
const char *drvstr;
const char *opstr;
@@ -318,7 +316,5 @@ virHookCall(int driver,
virCommandFree(cmd);
-VIR_FREE(path);
-
return ret;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 23/41] util: usb: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When a variable of type virUSBDevicePtr is declared using VIR_AUTOPTR, the function virUSBDeviceFree will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar Reviewed-by: Erik Skultety --- src/util/virusb.c | 1 - src/util/virusb.h | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virusb.c b/src/util/virusb.c index 7818232..90f947b 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -35,7 +35,6 @@ #include "virusb.h" #include "virlog.h" -#include "viralloc.h" #include "virutil.h" #include "virerror.h" #include "virfile.h" diff --git a/src/util/virusb.h b/src/util/virusb.h index 078dee6..afaaf95 100644 --- a/src/util/virusb.h +++ b/src/util/virusb.h @@ -27,6 +27,7 @@ # include "internal.h" # include "virobject.h" +# include "viralloc.h" # define USB_DEVFS "/dev/bus/usb/" @@ -99,4 +100,6 @@ void virUSBDeviceListDel(virUSBDeviceListPtr list, virUSBDevicePtr virUSBDeviceListFind(virUSBDeviceListPtr list, virUSBDevicePtr dev); +VIR_DEFINE_AUTOPTR_FUNC(virUSBDevice, virUSBDeviceFree) + #endif /* __VIR_USB_H__ */ -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 09/41] util: cgroup: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/vircgroup.c | 528 ++-
1 file changed, 181 insertions(+), 347 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index bc5f774..6f7b5b4 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -162,7 +162,7 @@ virCgroupPartitionNeedsEscaping(const char *path)
{
FILE *fp = NULL;
int ret = 0;
-char *line = NULL;
+VIR_AUTOFREE(char *) line = NULL;
size_t buflen;
/* If it starts with 'cgroup.' or a '_' of any
@@ -222,7 +222,6 @@ virCgroupPartitionNeedsEscaping(const char *path)
}
cleanup:
-VIR_FREE(line);
VIR_FORCE_FCLOSE(fp);
return ret;
}
@@ -255,41 +254,40 @@ virCgroupValidateMachineGroup(virCgroupPtr group,
const char *machinename)
{
size_t i;
-bool valid = false;
-char *partname = NULL;
-char *scopename_old = NULL;
-char *scopename_new = NULL;
-char *partmachinename = NULL;
+VIR_AUTOFREE(char *) partname = NULL;
+VIR_AUTOFREE(char *) scopename_old = NULL;
+VIR_AUTOFREE(char *) scopename_new = NULL;
+VIR_AUTOFREE(char *) partmachinename = NULL;
if (virAsprintf(&partname, "%s.libvirt-%s",
name, drivername) < 0)
-goto cleanup;
+return false;
if (virCgroupPartitionEscape(&partname) < 0)
-goto cleanup;
+return false;
if (machinename &&
(virAsprintf(&partmachinename, "%s.libvirt-%s",
machinename, drivername) < 0 ||
virCgroupPartitionEscape(&partmachinename) < 0))
-goto cleanup;
+return false;
if (!(scopename_old = virSystemdMakeScopeName(name, drivername, true)))
-goto cleanup;
+return false;
/* We should keep trying even if this failed */
if (!machinename)
virResetLastError();
else if (!(scopename_new = virSystemdMakeScopeName(machinename,
drivername, false)))
-goto cleanup;
+return false;
if (virCgroupPartitionEscape(&scopename_old) < 0)
-goto cleanup;
+return false;
if (scopename_new &&
virCgroupPartitionEscape(&scopename_new) < 0)
-goto cleanup;
+return false;
for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
char *tmp;
@@ -302,7 +300,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group,
tmp = strrchr(group->controllers[i].placement, '/');
if (!tmp)
-goto cleanup;
+return false;
if (stripEmulatorSuffix &&
(i == VIR_CGROUP_CONTROLLER_CPU ||
@@ -312,7 +310,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group,
*tmp = '\0';
tmp = strrchr(group->controllers[i].placement, '/');
if (!tmp)
-goto cleanup;
+return false;
}
tmp++;
@@ -328,18 +326,11 @@ virCgroupValidateMachineGroup(virCgroupPtr group,
tmp, virCgroupControllerTypeToString(i),
name, NULLSTR(machinename), partname,
scopename_old, NULLSTR(scopename_new));
-goto cleanup;
+return false;
}
}
-valid = true;
-
- cleanup:
-VIR_FREE(partmachinename);
-VIR_FREE(partname);
-VIR_FREE(scopename_old);
-VIR_FREE(scopename_new);
-return valid;
+return true;
}
@@ -377,7 +368,6 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
FILE *mounts = NULL;
struct mntent entry;
char buf[CGROUP_MAX_VAL];
-char *linksrc = NULL;
int ret = -1;
mounts = fopen(path, "r");
@@ -432,8 +422,9 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
/* If it is a co-mount it has a filename like "cpu,cpuacct"
* and we must identify the symlink path */
if (checkLinks && strchr(tmp2 + 1, ',')) {
+VIR_AUTOFREE(char *) linksrc = NULL;
+
*tmp2 = '\0';
-VIR_FREE(linksrc);
if (virAsprintf(&linksrc, "%s/%s",
entry.mnt_dir, typestr) < 0)
goto cleanup;
@@ -467,7 +458,6 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
ret = 0;
cleanup:
-VIR_FREE(linksrc);
VIR_FORCE_FCLOSE(mounts);
return ret;
}
@@ -546,7 +536,7 @@ virCgroupDetectPlacement(virCgroupPtr group,
FILE *mapping = NULL;
char line[1024];
int ret = -1;
-char *procfile;
+VIR_AUTOFREE(char *) procfile = NULL;
VIR_DEBUG("
[libvirt] [PATCH v2 37/41] util: iscsi: use VIR_AUTOPTR for aggregate types
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/viriscsi.c | 68 +
1 file changed, 22 insertions(+), 46 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 13fd02c..a55e062 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -83,7 +83,7 @@ virISCSIGetSession(const char *devpath,
VIR_AUTOFREE(char *) error = NULL;
int exitstatus = 0;
-virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode",
+VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM, "--mode",
"session", NULL);
virCommandSetErrorBuffer(cmd, &error);
@@ -93,15 +93,13 @@ virISCSIGetSession(const char *devpath,
vars,
virISCSIExtractSession,
&cbdata, NULL, &exitstatus) < 0)
-goto cleanup;
+return NULL;
if (cbdata.session == NULL && !probe)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot find iscsiadm session: %s"),
NULLSTR(error));
- cleanup:
-virCommandFree(cmd);
return cbdata.session;
}
@@ -123,7 +121,7 @@ virStorageBackendIQNFound(const char *initiatoriqn,
char *newline = NULL;
char *iqn = NULL;
char *token = NULL;
-virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
+VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM,
"--mode", "iface", NULL);
if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
@@ -196,7 +194,6 @@ virStorageBackendIQNFound(const char *initiatoriqn,
VIR_FORCE_FCLOSE(fp);
VIR_FORCE_CLOSE(fd);
-virCommandFree(cmd);
return ret;
}
@@ -208,7 +205,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
{
int ret = -1, exitstatus = -1;
VIR_AUTOFREE(char *) temp_ifacename = NULL;
-virCommandPtr cmd = NULL;
+VIR_AUTOPTR(virCommand) cmd = NULL;
if (virAsprintf(&temp_ifacename,
"libvirt-iface-%08llx",
@@ -267,7 +264,6 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
ret = 0;
cleanup:
-virCommandFree(cmd);
if (ret != 0)
VIR_FREE(*ifacename);
return ret;
@@ -280,7 +276,6 @@ virISCSIConnection(const char *portal,
const char *target,
const char **extraargv)
{
-int ret = -1;
const char *const baseargv[] = {
ISCSIADM,
"--mode", "node",
@@ -288,7 +283,7 @@ virISCSIConnection(const char *portal,
"--targetname", target,
NULL
};
-virCommandPtr cmd;
+VIR_AUTOPTR(virCommand) cmd = NULL;
VIR_AUTOFREE(char *) ifacename = NULL;
cmd = virCommandNewArgs(baseargv);
@@ -301,7 +296,7 @@ virISCSIConnection(const char *portal,
break;
case IQN_MISSING:
if (virStorageBackendCreateIfaceIQN(initiatoriqn, &ifacename) != 0)
-goto cleanup;
+return -1;
/*
* iscsiadm doesn't let you send commands to the Interface IQN,
* unless you've first issued a 'sendtargets' command to the
@@ -309,25 +304,20 @@ virISCSIConnection(const char *portal,
* "iscsiadm: No records found"
*/
if (virISCSIScanTargets(portal, NULL, NULL) < 0)
-goto cleanup;
+return -1;
break;
case IQN_ERROR:
default:
-goto cleanup;
+return -1;
}
virCommandAddArgList(cmd, "--interface", ifacename, NULL);
}
if (virCommandRun(cmd, NULL) < 0)
-goto cleanup;
+return -1;
-ret = 0;
-
- cleanup:
-virCommandFree(cmd);
-
-return ret;
+return 0;
}
@@ -354,14 +344,12 @@ virISCSIConnectionLogout(const char *portal,
int
virISCSIRescanLUNs(const char *session)
{
-virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
+VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM,
"--mode", "session",
"-r", session,
"-R",
NULL);
-int ret = virCommandRun(cmd, NULL);
-virCommandFree(cmd);
-return ret;
+return virCommandRun(cmd, NULL);
}
@@ -409,8 +397,7 @@ virISCSIScanTargets(const char *portal,
int vars[] = { 2 };
struct virISCSITargetList list;
size_t i;
-int ret = -1;
-virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
+VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM,
[libvirt] [PATCH v2 27/41] util: scsi: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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 | 21 +++--
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 33292f6..ba0a688 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -115,7 +115,7 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix,
{
DIR *dir = NULL;
struct dirent *entry;
-char *path = NULL;
+VIR_AUTOFREE(char *) path = NULL;
char *sg = NULL;
unsigned int adapter_id;
const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES;
@@ -139,7 +139,6 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix,
cleanup:
VIR_DIR_CLOSE(dir);
-VIR_FREE(path);
return sg;
}
@@ -155,7 +154,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix,
{
DIR *dir = NULL;
struct dirent *entry;
-char *path = NULL;
+VIR_AUTOFREE(char *) path = NULL;
char *name = NULL;
unsigned int adapter_id;
const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES;
@@ -178,7 +177,6 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix,
cleanup:
VIR_DIR_CLOSE(dir);
-VIR_FREE(path);
return name;
}
@@ -192,11 +190,11 @@ virSCSIDeviceNew(const char *sysfs_prefix,
bool shareable)
{
virSCSIDevicePtr dev, ret = NULL;
-char *sg = NULL;
-char *vendor_path = NULL;
-char *model_path = NULL;
-char *vendor = NULL;
-char *model = NULL;
+VIR_AUTOFREE(char *) sg = NULL;
+VIR_AUTOFREE(char *) vendor_path = NULL;
+VIR_AUTOFREE(char *) model_path = NULL;
+VIR_AUTOFREE(char *) vendor = NULL;
+VIR_AUTOFREE(char *) model = NULL;
const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES;
if (VIR_ALLOC(dev) < 0)
@@ -247,11 +245,6 @@ virSCSIDeviceNew(const char *sysfs_prefix,
ret = dev;
cleanup:
-VIR_FREE(sg);
-VIR_FREE(vendor);
-VIR_FREE(model);
-VIR_FREE(vendor_path);
-VIR_FREE(model_path);
if (!ret)
virSCSIDeviceFree(dev);
return ret;
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 34/41] util: hostmem: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/virhostmem.c | 57 ---
1 file changed, 18 insertions(+), 39 deletions(-)
diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c
index c923a1e..08ed7a5 100644
--- a/src/util/virhostmem.c
+++ b/src/util/virhostmem.c
@@ -263,7 +263,7 @@ virHostMemGetStats(int cellNum ATTRIBUTE_UNUSED,
#ifdef __linux__
{
int ret;
-char *meminfo_path = NULL;
+VIR_AUTOFREE(char *) meminfo_path = NULL;
FILE *meminfo;
int max_node;
@@ -299,12 +299,10 @@ virHostMemGetStats(int cellNum ATTRIBUTE_UNUSED,
if (!meminfo) {
virReportSystemError(errno,
_("cannot open %s"), meminfo_path);
-VIR_FREE(meminfo_path);
return -1;
}
ret = virHostMemGetStatsLinux(meminfo, cellNum, params, nparams);
VIR_FORCE_FCLOSE(meminfo);
-VIR_FREE(meminfo_path);
return ret;
}
@@ -322,45 +320,36 @@ virHostMemGetStats(int cellNum ATTRIBUTE_UNUSED,
static int
virHostMemSetParameterValue(virTypedParameterPtr param)
{
-char *path = NULL;
-char *strval = NULL;
-int ret = -1;
+VIR_AUTOFREE(char *) path = NULL;
+VIR_AUTOFREE(char *) strval = NULL;
int rc = -1;
char *field = strchr(param->field, '_');
sa_assert(field);
field++;
if (virAsprintf(&path, "%s/%s",
-SYSFS_MEMORY_SHARED_PATH, field) < 0) {
-ret = -2;
-goto cleanup;
-}
+SYSFS_MEMORY_SHARED_PATH, field) < 0)
+return -2;
-if (virAsprintf(&strval, "%u", param->value.ui) == -1) {
-ret = -2;
-goto cleanup;
-}
+if (virAsprintf(&strval, "%u", param->value.ui) == -1)
+return -2;
if ((rc = virFileWriteStr(path, strval, 0)) < 0) {
virReportSystemError(-rc, _("failed to set %s"), param->field);
-goto cleanup;
+return -1;
}
-ret = 0;
- cleanup:
-VIR_FREE(path);
-VIR_FREE(strval);
-return ret;
+return 0;
}
static bool
virHostMemParametersAreAllSupported(virTypedParameterPtr params,
int nparams)
{
-char *path = NULL;
size_t i;
for (i = 0; i < nparams; i++) {
+VIR_AUTOFREE(char *) path = NULL;
virTypedParameterPtr param = ¶ms[i];
char *field = strchr(param->field, '_');
@@ -374,11 +363,8 @@ virHostMemParametersAreAllSupported(virTypedParameterPtr
params,
virReportError(VIR_ERR_OPERATION_INVALID,
_("Parameter '%s' is not supported by "
"this kernel"), param->field);
-VIR_FREE(path);
return false;
}
-
-VIR_FREE(path);
}
return true;
@@ -430,23 +416,20 @@ static int
virHostMemGetParameterValue(const char *field,
void *value)
{
-char *path = NULL;
-char *buf = NULL;
+VIR_AUTOFREE(char *) path = NULL;
+VIR_AUTOFREE(char *) buf = NULL;
char *tmp = NULL;
-int ret = -1;
int rc = -1;
if (virAsprintf(&path, "%s/%s",
SYSFS_MEMORY_SHARED_PATH, field) < 0)
-goto cleanup;
+return -1;
-if (!virFileExists(path)) {
-ret = -2;
-goto cleanup;
-}
+if (!virFileExists(path))
+return -2;
if (virFileReadAll(path, 1024, &buf) < 0)
-goto cleanup;
+return -1;
if ((tmp = strchr(buf, '\n')))
*tmp = '\0';
@@ -465,14 +448,10 @@ virHostMemGetParameterValue(const char *field,
if (rc < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to parse %s"), field);
-goto cleanup;
+return -1;
}
-ret = 0;
- cleanup:
-VIR_FREE(path);
-VIR_FREE(buf);
-return ret;
+return 0;
}
#endif
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 35/41] util: iptables: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/viriptables.c | 52 +-
1 file changed, 18 insertions(+), 34 deletions(-)
diff --git a/src/util/viriptables.c b/src/util/viriptables.c
index e921954..e65e8dc 100644
--- a/src/util/viriptables.c
+++ b/src/util/viriptables.c
@@ -215,7 +215,7 @@ static char *iptablesFormatNetwork(virSocketAddr *netaddr,
unsigned int prefix)
{
virSocketAddr network;
-char *netstr;
+VIR_AUTOFREE(char *) netstr = NULL;
char *ret;
if (!(VIR_SOCKET_ADDR_IS_FAMILY(netaddr, AF_INET) ||
@@ -238,7 +238,6 @@ static char *iptablesFormatNetwork(virSocketAddr *netaddr,
ignore_value(virAsprintf(&ret, "%s/%d", netstr, prefix));
-VIR_FREE(netstr);
return ret;
}
@@ -254,7 +253,7 @@ iptablesForwardAllowOut(virFirewallPtr fw,
const char *physdev,
int action)
{
-char *networkstr;
+VIR_AUTOFREE(char *) networkstr = NULL;
virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ?
VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6;
@@ -279,7 +278,6 @@ iptablesForwardAllowOut(virFirewallPtr fw,
"--jump", "ACCEPT",
NULL);
-VIR_FREE(networkstr);
return 0;
}
@@ -343,7 +341,7 @@ iptablesForwardAllowRelatedIn(virFirewallPtr fw,
{
virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ?
VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6;
-char *networkstr;
+VIR_AUTOFREE(char *) networkstr = NULL;
if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
return -1;
@@ -370,7 +368,6 @@ iptablesForwardAllowRelatedIn(virFirewallPtr fw,
"--jump", "ACCEPT",
NULL);
-VIR_FREE(networkstr);
return 0;
}
@@ -432,7 +429,7 @@ iptablesForwardAllowIn(virFirewallPtr fw,
{
virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ?
VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6;
-char *networkstr;
+VIR_AUTOFREE(char *) networkstr = NULL;
if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
return -1;
@@ -454,7 +451,6 @@ iptablesForwardAllowIn(virFirewallPtr fw,
"--out-interface", iface,
"--jump", "ACCEPT",
NULL);
-VIR_FREE(networkstr);
return 0;
}
@@ -661,12 +657,11 @@ iptablesForwardMasquerade(virFirewallPtr fw,
const char *protocol,
int action)
{
-int ret = -1;
-char *networkstr = NULL;
-char *addrStartStr = NULL;
-char *addrEndStr = NULL;
-char *portRangeStr = NULL;
-char *natRangeStr = NULL;
+VIR_AUTOFREE(char *) networkstr = NULL;
+VIR_AUTOFREE(char *) addrStartStr = NULL;
+VIR_AUTOFREE(char *) addrEndStr = NULL;
+VIR_AUTOFREE(char *) portRangeStr = NULL;
+VIR_AUTOFREE(char *) natRangeStr = NULL;
virFirewallRulePtr rule;
if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
@@ -677,15 +672,15 @@ iptablesForwardMasquerade(virFirewallPtr fw,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Attempted to NAT '%s'. NAT is only supported for
IPv4."),
networkstr);
-goto cleanup;
+return -1;
}
if (VIR_SOCKET_ADDR_IS_FAMILY(&addr->start, AF_INET)) {
if (!(addrStartStr = virSocketAddrFormat(&addr->start)))
-goto cleanup;
+return -1;
if (VIR_SOCKET_ADDR_IS_FAMILY(&addr->end, AF_INET)) {
if (!(addrEndStr = virSocketAddrFormat(&addr->end)))
-goto cleanup;
+return -1;
}
}
@@ -718,7 +713,7 @@ iptablesForwardMasquerade(virFirewallPtr fw,
if (port->start < port->end && port->end < 65536) {
if (virAsprintf(&portRangeStr, ":%u-%u",
port->start, port->end) < 0)
-goto cleanup;
+return -1;
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid port range '%u-%u'."),
@@ -739,7 +734,7 @@ iptablesForwardMasquerade(virFirewallPtr fw,
}
if (r < 0)
-goto cleanup;
+return -1;
virFirewallRuleAddArgList(fw, rule,
"--jump", "SNAT",
@@ -753,14 +748,7 @@ iptablesForwardMasquerade(virFirewallPtr fw,
"--to-ports", &portRangeStr[1], NULL);
}
-ret = 0;
- cleanup:
-VIR_FREE(networkstr);
-
[libvirt] [PATCH v2 31/41] util: netdevvlan: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When a variable of type virNetDevVlanPtr is declared using
VIR_AUTOPTR, the function virNetDevVlanFree will be run
automatically on it when it goes out of scope.
Signed-off-by: Sukrit Bhatnagar
Reviewed-by: Erik Skultety
---
src/util/virnetdevvlan.c | 1 -
src/util/virnetdevvlan.h | 4
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdevvlan.c b/src/util/virnetdevvlan.c
index 4c8bce5..0afc47b 100644
--- a/src/util/virnetdevvlan.c
+++ b/src/util/virnetdevvlan.c
@@ -24,7 +24,6 @@
#include "internal.h"
#include "virerror.h"
#include "virnetdevvlan.h"
-#include "viralloc.h"
#define VIR_FROM_THIS VIR_FROM_NONE
diff --git a/src/util/virnetdevvlan.h b/src/util/virnetdevvlan.h
index 7f63626..be85f59 100644
--- a/src/util/virnetdevvlan.h
+++ b/src/util/virnetdevvlan.h
@@ -23,6 +23,8 @@
# include
+# include "viralloc.h"
+
typedef enum {
VIR_NATIVE_VLAN_MODE_DEFAULT = 0,
VIR_NATIVE_VLAN_MODE_TAGGED,
@@ -48,4 +50,6 @@ void virNetDevVlanFree(virNetDevVlanPtr vlan);
int virNetDevVlanEqual(const virNetDevVlan *a, const virNetDevVlan *b);
int virNetDevVlanCopy(virNetDevVlanPtr dst, const virNetDevVlan *src);
+VIR_DEFINE_AUTOPTR_FUNC(virNetDevVlan, virNetDevVlanFree)
+
#endif /* __VIR_NETDEV_VLAN_H__ */
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 10/41] util: cgroup: use VIR_AUTOPTR for aggregate types
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/vircgroup.c | 185 +++
1 file changed, 67 insertions(+), 118 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 6f7b5b4..61fafe2 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -836,25 +836,21 @@ virCgroupGetValueForBlkDev(virCgroupPtr group,
{
VIR_AUTOFREE(char *) prefix = NULL;
VIR_AUTOFREE(char *) str = NULL;
-char **lines = NULL;
-int ret = -1;
+VIR_AUTOPTR(virString) lines = NULL;
if (virCgroupGetValueStr(group, controller, key, &str) < 0)
-goto error;
+return -1;
if (!(prefix = virCgroupGetBlockDevString(path)))
-goto error;
+return -1;
if (!(lines = virStringSplit(str, "\n", -1)))
-goto error;
+return -1;
if (VIR_STRDUP(*value, virStringListGetFirstWithPrefix(lines, prefix)) < 0)
-goto error;
+return -1;
-ret = 0;
- error:
-virStringListFree(lines);
-return ret;
+return 0;
}
@@ -1217,12 +1213,11 @@ virCgroupAddTaskController(virCgroupPtr group, pid_t
pid, int controller)
static int
virCgroupSetPartitionSuffix(const char *path, char **res)
{
-char **tokens;
+VIR_AUTOPTR(virString) tokens = NULL;
size_t i;
-int ret = -1;
if (!(tokens = virStringSplit(path, "/", 0)))
-return ret;
+return -1;
for (i = 0; tokens[i] != NULL; i++) {
/* Whitelist the 3 top level fixed dirs
@@ -1241,22 +1236,18 @@ virCgroupSetPartitionSuffix(const char *path, char
**res)
!strchr(tokens[i], '.')) {
if (VIR_REALLOC_N(tokens[i],
strlen(tokens[i]) + strlen(".partition") + 1) <
0)
-goto cleanup;
+return -1;
strcat(tokens[i], ".partition");
}
if (virCgroupPartitionEscape(&(tokens[i])) < 0)
-goto cleanup;
+return -1;
}
if (!(*res = virStringListJoin((const char **)tokens, "/")))
-goto cleanup;
+return -1;
-ret = 0;
-
- cleanup:
-virStringListFree(tokens);
-return ret;
+return 0;
}
@@ -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;
}
@@ -1502,9 +1492,9 @@ virCgroupNewMachineSystemd(const char *name,
int controllers,
virCgroupPtr *group)
{
-int ret = -1;
int rv;
-virCgroupPtr init, parent = NULL;
+VIR_AUTOPTR(virCgroup) init = NULL;
+VIR_AUTOPTR(virCgroup) parent = NULL;
VIR_AUTOFREE(char *) path = NULL;
char *offset;
@@ -1531,12 +1521,10 @@ virCgroupNewMachineSystemd(const char *name,
path = init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement;
init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement = NULL;
-virCgroupFree(init);
if (!path || STREQ(path, "/") || path[0] != '/') {
VIR_DEBUG("Systemd didn't setup its controller");
-ret = -2;
-goto cleanup;
+return -2;
}
offset = path;
@@ -1546,7 +1534,7 @@ virCgroupNewMachineSystemd(const char *name,
NULL,
controllers,
&parent) < 0)
-goto cleanup;
+return -1;
for (;;) {
@@ -1560,11 +1548,11 @@ virCgroupNewMachineSystemd(const char *name,
parent,
controllers,
&tmp) < 0)
-goto cleanup;
+return -1;
if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_NONE) < 0) {
virCgroupFree(tmp);
-goto cleanup;
+return -1;
}
if (t) {
*t = '/';
@@ -1587,10 +1575,7 @@ virCgroupNewMachineSystemd(const char *name,
}
}
-ret = 0;
- cleanup:
-virCgroupFree(parent);
-return ret;
+return 0;
}
@@ -1611,8 +1596,7 @@ virCgroupNewMachineManual(const char *name,
[libvirt] [PATCH v2 08/41] util: cgroup: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When a variable of type virCgroupPtr is declared using
VIR_AUTOPTR, the function virCgroupFree will be run
automatically on it when it goes out of scope.
This commit also adds an intermediate typedef for virCgroup
type for use with the cleanup macros.
Signed-off-by: Sukrit Bhatnagar
---
src/util/vircgroup.c | 1 -
src/util/vircgroup.h | 9 +++--
src/util/vircgrouppriv.h | 2 +-
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 140b016..bc5f774 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -50,7 +50,6 @@
#include "vircgrouppriv.h"
#include "virutil.h"
-#include "viralloc.h"
#include "virerror.h"
#include "virlog.h"
#include "virfile.h"
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index e4ffd57..065861d 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -27,9 +27,11 @@
# include "virutil.h"
# include "virbitmap.h"
+# include "viralloc.h"
-struct virCgroup;
-typedef struct virCgroup *virCgroupPtr;
+struct _virCgroup;
+typedef struct _virCgroup virCgroup;
+typedef virCgroup *virCgroupPtr;
enum {
VIR_CGROUP_CONTROLLER_CPU,
@@ -297,4 +299,7 @@ int virCgroupSetOwner(virCgroupPtr cgroup,
int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller);
bool virCgroupControllerAvailable(int controller);
+
+VIR_DEFINE_AUTOPTR_FUNC(virCgroup, virCgroupFree)
+
#endif /* __VIR_CGROUP_H__ */
diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h
index 722863e..a72bee1 100644
--- a/src/util/vircgrouppriv.h
+++ b/src/util/vircgrouppriv.h
@@ -42,7 +42,7 @@ struct virCgroupController {
char *placement;
};
-struct virCgroup {
+struct _virCgroup {
char *path;
struct virCgroupController controllers[VIR_CGROUP_CONTROLLER_LAST];
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 30/41] util: scsivhost: use VIR_AUTOPTR for aggregate types
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/virscsivhost.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c
index ef216b3..280d0dc 100644
--- a/src/util/virscsivhost.c
+++ b/src/util/virscsivhost.c
@@ -109,8 +109,7 @@ void
virSCSIVHostDeviceListDel(virSCSIVHostDeviceListPtr list,
virSCSIVHostDevicePtr dev)
{
-virSCSIVHostDevicePtr tmp = virSCSIVHostDeviceListSteal(list, dev);
-virSCSIVHostDeviceFree(tmp);
+VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list,
dev);
}
@@ -253,7 +252,8 @@ virSCSIVHostDeviceGetPath(virSCSIVHostDevicePtr dev)
virSCSIVHostDevicePtr
virSCSIVHostDeviceNew(const char *name)
{
-virSCSIVHostDevicePtr dev;
+VIR_AUTOPTR(virSCSIVHostDevice) dev = NULL;
+virSCSIVHostDevicePtr ret = NULL;
if (VIR_ALLOC(dev) < 0)
return NULL;
@@ -262,22 +262,18 @@ virSCSIVHostDeviceNew(const char *name)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("dev->name buffer overflow: %s"),
name);
-goto error;
+return NULL;
}
if (virAsprintf(&dev->path, "%s/%s",
SYSFS_VHOST_SCSI_DEVICES, name) < 0)
-goto error;
+return NULL;
VIR_DEBUG("%s: initialized", dev->name);
- cleanup:
-return dev;
+VIR_STEAL_PTR(ret, dev);
- error:
-virSCSIVHostDeviceFree(dev);
-dev = NULL;
-goto cleanup;
+return ret;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 26/41] util: scsi: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When variables of type virSCSIDevicePtr and virUsedByInfoPtr
are declared using VIR_AUTOPTR, the functions virSCSIDeviceFree
and virSCSIDeviceUsedByInfoFree, respectively, will be run
automatically on them when they go out of scope.
This commit also adds an intermediate typedef for virUsedByInfo
type for use with the cleanup macros.
Signed-off-by: Sukrit Bhatnagar
Reviewed-by: Erik Skultety
---
src/util/virscsi.c | 5 +++--
src/util/virscsi.h | 3 +++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index b51103a..33292f6 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -37,7 +37,6 @@
#include "virlog.h"
#include "virscsi.h"
-#include "viralloc.h"
#include "virfile.h"
#include "virutil.h"
#include "virstring.h"
@@ -54,7 +53,8 @@ struct _virUsedByInfo {
char *drvname; /* which driver */
char *domname; /* which domain */
};
-typedef struct _virUsedByInfo *virUsedByInfoPtr;
+typedef struct _virUsedByInfo virUsedByInfo;
+typedef virUsedByInfo *virUsedByInfoPtr;
struct _virSCSIDevice {
unsigned int adapter;
@@ -264,6 +264,7 @@ virSCSIDeviceUsedByInfoFree(virUsedByInfoPtr used_by)
VIR_FREE(used_by->domname);
VIR_FREE(used_by);
}
+VIR_DEFINE_AUTOPTR_FUNC(virUsedByInfo, virSCSIDeviceUsedByInfoFree)
void
virSCSIDeviceFree(virSCSIDevicePtr dev)
diff --git a/src/util/virscsi.h b/src/util/virscsi.h
index 9f8b3ec..b96d862 100644
--- a/src/util/virscsi.h
+++ b/src/util/virscsi.h
@@ -26,6 +26,7 @@
# include "internal.h"
# include "virobject.h"
+# include "viralloc.h"
typedef struct _virSCSIDevice virSCSIDevice;
typedef virSCSIDevice *virSCSIDevicePtr;
@@ -95,4 +96,6 @@ void virSCSIDeviceListDel(virSCSIDeviceListPtr list,
virSCSIDevicePtr virSCSIDeviceListFind(virSCSIDeviceListPtr list,
virSCSIDevicePtr dev);
+VIR_DEFINE_AUTOPTR_FUNC(virSCSIDevice, virSCSIDeviceFree)
+
#endif /* __VIR_SCSI_H__ */
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 02/41] util: buffer: typedef for struct _virBufferEscapePair
Create and use typedefs virBufferEscapePair and virBufferEscapePairPtr
for struct _virBufferEscapePair for cleaner code and for use with
cleanup macros.
Signed-off-by: Sukrit Bhatnagar
---
src/util/virbuffer.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 3d6defb..ea96704 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -648,12 +648,14 @@ virBufferEscape(virBufferPtr buf, char escape, const char
*toescape,
}
+typedef struct _virBufferEscapePair virBufferEscapePair;
+typedef virBufferEscapePair *virBufferEscapePairPtr;
+
struct _virBufferEscapePair {
char escape;
char *toescape;
};
-
/**
* virBufferEscapeN:
* @buf: the buffer to append to
@@ -678,8 +680,8 @@ virBufferEscapeN(virBufferPtr buf,
char *escaped = NULL;
char *out;
const char *cur;
-struct _virBufferEscapePair escapeItem;
-struct _virBufferEscapePair *escapeList = NULL;
+virBufferEscapePair escapeItem;
+virBufferEscapePairPtr escapeList = NULL;
size_t nescapeList = 0;
va_list ap;
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 33/41] util: hostdev: use VIR_AUTOPTR for aggregate types
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/virhostdev.c | 71 ++-
1 file changed, 25 insertions(+), 46 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 492c42f..ca79c37 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -518,11 +518,10 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
VIR_AUTOFREE(char *) linkdev = NULL;
VIR_AUTOFREE(virMacAddrPtr) MAC = NULL;
VIR_AUTOFREE(virMacAddrPtr) adminMAC = NULL;
+VIR_AUTOPTR(virNetDevVlan) vlan = NULL;
virNetDevVPortProfilePtr virtPort;
-int ret = -1;
int vf = -1;
bool port_profile_associate = false;
-virNetDevVlanPtr vlan = NULL;
/* This is only needed for PCI devices that have been defined
@@ -535,16 +534,16 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Interface type hostdev is currently supported on"
" SR-IOV Virtual Functions only"));
-return ret;
+return -1;
}
if (virHostdevNetDevice(hostdev, 0, &linkdev, &vf) < 0)
-return ret;
+return -1;
virtPort = virDomainNetGetActualVirtPortProfile(
hostdev->parent.data.net);
if (virtPort) {
-ret = virHostdevNetConfigVirtPortProfile(linkdev, vf, virtPort,
+return virHostdevNetConfigVirtPortProfile(linkdev, vf, virtPort,
&hostdev->parent.data.net->mac,
NULL,
port_profile_associate);
@@ -574,7 +573,7 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
/* 1) standard location */
if (virNetDevReadNetConfig(linkdev, vf, stateDir,
&adminMAC, &vlan, &MAC) < 0) {
-goto cleanup;
+return -1;
}
/* 2) "old" (pre-1.2.3 circa 2014) location - whenever we get
@@ -585,7 +584,7 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
if (!(adminMAC || vlan || MAC) && oldStateDir &&
virNetDevReadNetConfig(linkdev, vf, oldStateDir,
&adminMAC, &vlan, &MAC) < 0) {
-goto cleanup;
+return -1;
}
/* 3) try using the PF's "port 2" netdev as the name of the
@@ -597,7 +596,7 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) < 0 ||
virNetDevReadNetConfig(linkdev, vf, stateDir,
&adminMAC, &vlan, &MAC) < 0) {
-goto cleanup;
+return -1;
}
}
@@ -627,13 +626,8 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
ignore_value(virNetDevSetNetConfig(linkdev, vf,
adminMAC, vlan, MAC, true));
-ret = 0;
+return 0;
}
-
- cleanup:
-virNetDevVlanFree(vlan);
-
-return ret;
}
int
@@ -1117,7 +,6 @@ virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr,
const char *dom_name)
{
virDomainHostdevDefPtr hostdev = NULL;
-virPCIDevicePtr actual = NULL;
size_t i;
int ret = -1;
@@ -1128,6 +1121,7 @@ virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr,
virObjectLock(mgr->inactivePCIHostdevs);
for (i = 0; i < nhostdevs; i++) {
+VIR_AUTOPTR(virPCIDevice) actual = NULL;
virDomainHostdevSubsysPCIPtr pcisrc;
hostdev = hostdevs[i];
pcisrc = &hostdev->source.subsys.u.pci;
@@ -1165,7 +1159,6 @@ virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr,
ret = 0;
cleanup:
-virPCIDeviceFree(actual);
virObjectUnlock(mgr->activePCIHostdevs);
virObjectUnlock(mgr->inactivePCIHostdevs);
return ret;
@@ -1226,31 +1219,27 @@
virHostdevUpdateActiveSCSIHostDevices(virHostdevManagerPtr mgr,
virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
virSCSIDevicePtr scsi = NULL;
virSCSIDevicePtr tmp = NULL;
-int ret = -1;
if (!(scsi = virSCSIDeviceNew(NULL,
scsihostsrc->adapter, scsihostsrc->bus,
scsihostsrc->target, scsihostsrc->unit,
hostdev->readonly, hostdev->shareable)))
-goto cleanup;
+return -1;
if ((tmp = virSCSIDeviceListFind(mgr->activeSCSIHostdevs, scsi))) {
if (virSCSIDeviceSetUsedBy
[libvirt] [PATCH v2 29/41] util: scsivhost: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When a variable of type virSCSIVHostDevicePtr is declared using VIR_AUTOPTR, the function virSCSIVHostDeviceFree will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar Reviewed-by: Erik Skultety --- src/util/virscsivhost.c | 1 - src/util/virscsivhost.h | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c index 84c09e8..ef216b3 100644 --- a/src/util/virscsivhost.c +++ b/src/util/virscsivhost.c @@ -26,7 +26,6 @@ #include "virscsivhost.h" #include "virlog.h" -#include "viralloc.h" #include "virerror.h" #include "virfile.h" #include "virstring.h" diff --git a/src/util/virscsivhost.h b/src/util/virscsivhost.h index 21887dd..31ad429 100644 --- a/src/util/virscsivhost.h +++ b/src/util/virscsivhost.h @@ -27,6 +27,7 @@ # include "internal.h" # include "virobject.h" # include "virutil.h" +# include "viralloc.h" typedef struct _virSCSIVHostDevice virSCSIVHostDevice; typedef virSCSIVHostDevice *virSCSIVHostDevicePtr; @@ -63,4 +64,6 @@ void virSCSIVHostDeviceGetUsedBy(virSCSIVHostDevicePtr dev, void virSCSIVHostDeviceFree(virSCSIVHostDevicePtr dev); int virSCSIVHostOpenVhostSCSI(int *vhostfd) ATTRIBUTE_NOINLINE; +VIR_DEFINE_AUTOPTR_FUNC(virSCSIVHostDevice, virSCSIVHostDeviceFree) + #endif /* __VIR_SCSIHOST_H__ */ -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 18/41] util: hook: use VIR_AUTOPTR for aggregate types
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/virhook.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/util/virhook.c b/src/util/virhook.c
index 4673655..993f06d 100644
--- a/src/util/virhook.c
+++ b/src/util/virhook.c
@@ -232,7 +232,7 @@ virHookCall(int driver,
{
int ret;
VIR_AUTOFREE(char *) path = NULL;
-virCommandPtr cmd;
+VIR_AUTOPTR(virCommand) cmd = NULL;
const char *drvstr;
const char *opstr;
const char *subopstr;
@@ -314,7 +314,5 @@ virHookCall(int driver,
virGetLastErrorMessage());
}
-virCommandFree(cmd);
-
return ret;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 24/41] util: usb: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/virusb.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/util/virusb.c b/src/util/virusb.c
index 90f947b..47b407b 100644
--- a/src/util/virusb.c
+++ b/src/util/virusb.c
@@ -90,29 +90,25 @@ VIR_ONCE_GLOBAL_INIT(virUSB)
static int virUSBSysReadFile(const char *f_name, const char *d_name,
int base, unsigned int *value)
{
-int ret = -1, tmp;
-char *buf = NULL;
-char *filename = NULL;
+int tmp;
+VIR_AUTOFREE(char *) buf = NULL;
+VIR_AUTOFREE(char *) filename = NULL;
char *ignore = NULL;
tmp = virAsprintf(&filename, USB_SYSFS "/devices/%s/%s", d_name, f_name);
if (tmp < 0)
-goto cleanup;
+return -1;
if (virFileReadAll(filename, 1024, &buf) < 0)
-goto cleanup;
+return -1;
if (virStrToLong_ui(buf, &ignore, base, value) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not parse usb file %s"), filename);
-goto cleanup;
+return -1;
}
-ret = 0;
- cleanup:
-VIR_FREE(filename);
-VIR_FREE(buf);
-return ret;
+return 0;
}
static virUSBDeviceListPtr
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 11/41] util: mdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When variables of type virMediatedDevicePtr and virMediatedDeviceTypePtr
are declared using VIR_AUTOPTR, the functions virMediatedDeviceFree
and virMediatedDeviceTypeFree, respectively, will be run automatically
on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar
Reviewed-by: Erik Skultety
---
src/util/virmdev.c | 1 -
src/util/virmdev.h | 4
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 6c51388..d7bcb1d 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -21,7 +21,6 @@
#include "dirname.h"
#include "virmdev.h"
#include "virlog.h"
-#include "viralloc.h"
#include "virerror.h"
#include "virfile.h"
#include "virstring.h"
diff --git a/src/util/virmdev.h b/src/util/virmdev.h
index cfda2ca..7c93c4d 100644
--- a/src/util/virmdev.h
+++ b/src/util/virmdev.h
@@ -22,6 +22,7 @@
# include "internal.h"
# include "virobject.h"
# include "virutil.h"
+# include "viralloc.h"
typedef enum {
VIR_MDEV_MODEL_TYPE_VFIO_PCI = 0,
@@ -135,4 +136,7 @@ int
virMediatedDeviceTypeReadAttrs(const char *sysfspath,
virMediatedDeviceTypePtr *type);
+VIR_DEFINE_AUTOPTR_FUNC(virMediatedDevice, virMediatedDeviceFree)
+VIR_DEFINE_AUTOPTR_FUNC(virMediatedDeviceType, virMediatedDeviceTypeFree)
+
#endif /* __VIR_MDEV_H__ */
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 28/41] util: scsi: use VIR_AUTOPTR for aggregate types
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;
if (VIR_ALLOC(copy) < 0)
return -1;
if (VIR_STRDUP(copy->drvname, drvname) < 0 ||
VIR_STRDUP(copy->domname, domname) < 0)
-goto cleanup;
+return -1;
if (VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy) < 0)
-goto cleanup;
+return -1;
return 0;
-
- cleanup:
-virSCSIDeviceUsedByInfoFree(copy);
-return -1;
}
bool
@@ -442,7 +436,6 @@ virSCSIDeviceListDel(virSCSIDeviceListPtr list,
const char *drvname,
const char *domname)
{
-virSCSIDevicePtr tmp = NULL;
size_t i;
for (i = 0; i < dev->n_used_by; i++) {
@@ -452,8 +445,8 @@ virSCSIDeviceListDel(virSCSIDeviceListPtr list,
virSCSIDeviceUsedByInfoFree(dev->used_by[i]);
VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by);
} else {
+VIR_AUTOPTR(virSCSIDevice) tmp = NULL;
tmp = virSCSIDeviceListSteal(list, dev);
-virSCSIDeviceFree(tmp);
}
break;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 21/41] util: pci: use VIR_AUTOPTR for aggregate types
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/virpci.c | 78 +--
1 file changed, 29 insertions(+), 49 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 1b3e52f..634df8d 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -490,8 +490,6 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
ret = 1;
break;
}
-
-virPCIDeviceFree(check);
}
VIR_DIR_CLOSE(dir);
return ret;
@@ -781,7 +779,8 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev,
int cfgfd,
virPCIDeviceList *inactiveDevs)
{
-virPCIDevicePtr parent, conflict;
+VIR_AUTOPTR(virPCIDevice) parent = NULL;
+VIR_AUTOPTR(virPCIDevice) conflict = NULL;
uint8_t config_space[PCI_CONF_LEN];
uint16_t ctl;
int ret = -1;
@@ -795,7 +794,6 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Active %s devices on bus with %s, not doing bus
reset"),
conflict->name, dev->name);
-virPCIDeviceFree(conflict);
return -1;
}
@@ -848,7 +846,6 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev,
out:
virPCIDeviceConfigClose(parent, parentfd);
-virPCIDeviceFree(parent);
return ret;
}
@@ -1270,8 +1267,8 @@ virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev)
VIR_AUTOFREE(char *) stubDriverPath = NULL;
VIR_AUTOFREE(char *) driverLink = NULL;
VIR_AUTOFREE(char *) path = NULL; /* reused for different purposes */
+VIR_AUTOPTR(virError) err = NULL;
const char *stubDriverName = NULL;
-virErrorPtr err = NULL;
/* Check the device is configured to use one of the known stub drivers */
if (dev->stubDriver == VIR_PCI_STUB_DRIVER_NONE) {
@@ -1406,7 +1403,6 @@ virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev)
if (err)
virSetError(err);
-virFreeError(err);
return result;
}
@@ -1679,19 +1675,13 @@ virPCIGetAddrString(unsigned int domain,
unsigned int function,
char **pciConfigAddr)
{
-virPCIDevicePtr dev = NULL;
-int ret = -1;
+VIR_AUTOPTR(virPCIDevice) dev = NULL;
dev = virPCIDeviceNew(domain, bus, slot, function);
-if (dev != NULL) {
-if (VIR_STRDUP(*pciConfigAddr, dev->name) < 0)
-goto cleanup;
-ret = 0;
-}
+if (!dev || VIR_STRDUP(*pciConfigAddr, dev->name) < 0)
+return -1;
- cleanup:
-virPCIDeviceFree(dev);
-return ret;
+return 0;
}
virPCIDevicePtr
@@ -1700,7 +1690,8 @@ virPCIDeviceNew(unsigned int domain,
unsigned int slot,
unsigned int function)
{
-virPCIDevicePtr dev;
+virPCIDevicePtr ret = NULL;
+VIR_AUTOPTR(virPCIDevice) dev = NULL;
VIR_AUTOFREE(char *) vendor = NULL;
VIR_AUTOFREE(char *) product = NULL;
@@ -1717,17 +1708,17 @@ virPCIDeviceNew(unsigned int domain,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("dev->name buffer overflow: %.4x:%.2x:%.2x.%.1x"),
domain, bus, slot, function);
-goto error;
+goto cleanup;
}
if (virAsprintf(&dev->path, PCI_SYSFS "devices/%s/config",
dev->name) < 0)
-goto error;
+goto cleanup;
if (!virFileExists(dev->path)) {
virReportSystemError(errno,
_("Device %s not found: could not access %s"),
dev->name, dev->path);
-goto error;
+goto cleanup;
}
vendor = virPCIDeviceReadID(dev, "vendor");
@@ -1737,7 +1728,7 @@ virPCIDeviceNew(unsigned int domain,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to read product/vendor ID for %s"),
dev->name);
-goto error;
+goto cleanup;
}
/* strings contain '0x' prefix */
@@ -1746,18 +1737,15 @@ virPCIDeviceNew(unsigned int domain,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("dev->id buffer overflow: %s %s"),
&vendor[2], &product[2]);
-goto error;
+goto cleanup;
}
VIR_DEBUG("%s %s: initialized", dev->id, dev->name);
- cleanup:
-return dev;
+VIR_STEAL_PTR(ret, dev);
- error:
-virPCIDeviceFree(dev);
-dev = NULL;
-goto cleanup;;
+ cleanup:
+return ret;
}
@@ -1960,14 +1948,14 @@ virPCIDeviceListAdd(virPCIDeviceListPtr list,
int
virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev)
{
-virPCIDevicePtr copy =
[libvirt] [PATCH v2 07/41] util: cgroup: modify virCgroupFree to take virCgroupPtr
Modify virCgroupFree function signature to take a value of type
virCgroupPtr instead of virCgroupPtr * as the parameter.
Change the argument type in all calls to virCgroupFree function
from virCgroupPtr * to virCgroupPtr. This is a step towards
having consistent function signatures for Free helpers so that
they can be used with VIR_AUTOPTR cleanup macro.
Signed-off-by: Sukrit Bhatnagar
Reviewed-by: Erik Skultety
---
src/libvirt-lxc.c| 4 ++--
src/lxc/lxc_cgroup.c | 4 ++--
src/lxc/lxc_container.c | 2 +-
src/lxc/lxc_controller.c | 2 +-
src/lxc/lxc_domain.c | 2 +-
src/lxc/lxc_process.c| 10 -
src/qemu/qemu_cgroup.c | 16 +++---
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_driver.c | 34 ++---
src/qemu/qemu_process.c | 2 +-
src/util/vircgroup.c | 56
src/util/vircgroup.h | 2 +-
tests/vircgrouptest.c| 42 ++--
13 files changed, 88 insertions(+), 90 deletions(-)
diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c
index c9f2146..12be893 100644
--- a/src/libvirt-lxc.c
+++ b/src/libvirt-lxc.c
@@ -309,12 +309,12 @@ int virDomainLxcEnterCGroup(virDomainPtr domain,
if (virCgroupAddTask(cgroup, getpid()) < 0)
goto error;
-virCgroupFree(&cgroup);
+virCgroupFree(cgroup);
return 0;
error:
virDispatchError(NULL);
-virCgroupFree(&cgroup);
+virCgroupFree(cgroup);
return -1;
}
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index 8e937ec..873c843 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -306,7 +306,7 @@ int virLXCCgroupGetMeminfo(virLXCMeminfoPtr meminfo)
ret = 0;
cleanup:
-virCgroupFree(&cgroup);
+virCgroupFree(cgroup);
return ret;
}
@@ -515,7 +515,7 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
def->idmap.uidmap[0].target,
def->idmap.gidmap[0].target,
(1 << VIR_CGROUP_CONTROLLER_SYSTEMD)) < 0) {
-virCgroupFree(&cgroup);
+virCgroupFree(cgroup);
cgroup = NULL;
goto cleanup;
}
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 3a1b2d6..407214f 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -1815,7 +1815,7 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr
vmDef,
cleanup:
VIR_FREE(stateDir);
-virCgroupFree(&cgroup);
+virCgroupFree(cgroup);
VIR_FREE(sec_mount_options);
return ret;
}
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 4e84391..7be45f8 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -296,7 +296,7 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl)
VIR_FREE(ctrl->nbdpids);
VIR_FREE(ctrl->nsFDs);
-virCgroupFree(&ctrl->cgroup);
+virCgroupFree(ctrl->cgroup);
/* This must always be the last thing to be closed */
VIR_FORCE_CLOSE(ctrl->handshakeFd);
diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
index b197f9d..eb0071d 100644
--- a/src/lxc/lxc_domain.c
+++ b/src/lxc/lxc_domain.c
@@ -172,7 +172,7 @@ virLXCDomainObjPrivateFree(void *data)
{
virLXCDomainObjPrivatePtr priv = data;
-virCgroupFree(&priv->cgroup);
+virCgroupFree(priv->cgroup);
virLXCDomainObjFreeJob(priv);
VIR_FREE(priv);
}
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 14502e1..8534051 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -220,7 +220,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
if (priv->cgroup) {
virCgroupRemove(priv->cgroup);
-virCgroupFree(&priv->cgroup);
+virCgroupFree(priv->cgroup);
}
/* Get machined to terminate the machine as it may not have cleaned it
@@ -1203,26 +1203,26 @@ int virLXCProcessStart(virConnectPtr conn,
if (!virCgroupHasController(selfcgroup,
VIR_CGROUP_CONTROLLER_CPUACCT)) {
-virCgroupFree(&selfcgroup);
+virCgroupFree(selfcgroup);
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unable to find 'cpuacct' cgroups controller mount"));
return -1;
}
if (!virCgroupHasController(selfcgroup,
VIR_CGROUP_CONTROLLER_DEVICES)) {
-virCgroupFree(&selfcgroup);
+virCgroupFree(selfcgroup);
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unable to find 'devices' cgroups controller mount"));
return -1;
}
if (!virCgroupHasController(selfcgroup,
VIR_CGROUP_CONTROLLER_MEMORY)) {
-virCgroupFree(&selfcgroup);
+virCgroupFree(selfcgroup);
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unable to find 'memory' cgroups controller mount")
[libvirt] [PATCH v2 20/41] util: pci: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar
---
src/util/virpci.c | 269 ++
1 file changed, 87 insertions(+), 182 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 46f9905..1b3e52f 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -253,7 +253,7 @@ int
virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name)
{
int ret = -1;
-char *drvlink = NULL;
+VIR_AUTOFREE(char *) drvlink = NULL;
*path = *name = NULL;
/* drvlink = "/sys/bus/pci/:bb:ss.ff/driver" */
@@ -285,7 +285,6 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char
**path, char **name)
ret = 0;
cleanup:
-VIR_FREE(drvlink);
if (ret < 0) {
VIR_FREE(*path);
VIR_FREE(*name);
@@ -375,32 +374,27 @@ virPCIDeviceRead32(virPCIDevicePtr dev, int cfgfd,
unsigned int pos)
static int
virPCIDeviceReadClass(virPCIDevicePtr dev, uint16_t *device_class)
{
-char *path = NULL;
-char *id_str = NULL;
-int ret = -1;
+VIR_AUTOFREE(char *) path = NULL;
+VIR_AUTOFREE(char *) id_str = NULL;
unsigned int value;
if (!(path = virPCIFile(dev->name, "class")))
-return ret;
+return -1;
/* class string is '0xNN\n' ... i.e. 9 bytes */
if (virFileReadAll(path, 9, &id_str) < 0)
-goto cleanup;
+return -1;
id_str[8] = '\0';
if (virStrToLong_ui(id_str, NULL, 16, &value) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unusual value in %s/devices/%s/class: %s"),
PCI_SYSFS, dev->name, id_str);
-goto cleanup;
+return -1;
}
*device_class = (value >> 8) & 0x;
-ret = 0;
- cleanup:
-VIR_FREE(id_str);
-VIR_FREE(path);
-return ret;
+return 0;
}
static int
@@ -574,7 +568,7 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev,
int cfgfd)
{
uint32_t caps;
uint8_t pos;
-char *path;
+VIR_AUTOFREE(char *) path = NULL;
int found;
/* The PCIe Function Level Reset capability allows
@@ -614,7 +608,6 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev,
int cfgfd)
return -1;
found = virFileExists(path);
-VIR_FREE(path);
if (found) {
VIR_DEBUG("%s %s: buggy device didn't advertise FLR, but is a VF;
forcing flr on",
dev->id, dev->name);
@@ -926,8 +919,8 @@ virPCIDeviceReset(virPCIDevicePtr dev,
virPCIDeviceList *activeDevs,
virPCIDeviceList *inactiveDevs)
{
-char *drvPath = NULL;
-char *drvName = NULL;
+VIR_AUTOFREE(char *) drvPath = NULL;
+VIR_AUTOFREE(char *) drvName = NULL;
int ret = -1;
int fd = -1;
int hdrType = -1;
@@ -1000,8 +993,6 @@ virPCIDeviceReset(virPCIDevicePtr dev,
}
cleanup:
-VIR_FREE(drvPath);
-VIR_FREE(drvName);
virPCIDeviceConfigClose(dev, fd);
return ret;
}
@@ -1011,7 +1002,7 @@ static int
virPCIProbeStubDriver(virPCIStubDriver driver)
{
const char *drvname = NULL;
-char *drvpath = NULL;
+VIR_AUTOFREE(char *) drvpath = NULL;
bool probed = false;
if (driver == VIR_PCI_STUB_DRIVER_NONE ||
@@ -1023,20 +1014,15 @@ virPCIProbeStubDriver(virPCIStubDriver driver)
}
recheck:
-if ((drvpath = virPCIDriverDir(drvname)) && virFileExists(drvpath)) {
+if ((drvpath = virPCIDriverDir(drvname)) && virFileExists(drvpath))
/* driver already loaded, return */
-VIR_FREE(drvpath);
return 0;
-}
-
-VIR_FREE(drvpath);
if (!probed) {
-char *errbuf = NULL;
+VIR_AUTOFREE(char *) errbuf = NULL;
probed = true;
if ((errbuf = virKModLoad(drvname, true))) {
VIR_WARN("failed to load driver %s: %s", drvname, errbuf);
-VIR_FREE(errbuf);
goto cleanup;
}
@@ -1064,38 +1050,30 @@ virPCIProbeStubDriver(virPCIStubDriver driver)
int
virPCIDeviceUnbind(virPCIDevicePtr dev)
{
-char *path = NULL;
-char *drvpath = NULL;
-char *driver = NULL;
-int ret = -1;
+VIR_AUTOFREE(char *) path = NULL;
+VIR_AUTOFREE(char *) drvpath = NULL;
+VIR_AUTOFREE(char *) driver = NULL;
if (virPCIDeviceGetDriverPathAndName(dev, &drvpath, &driver) < 0)
-goto cleanup;
+return -1;
-if (!driver) {
+if (!driver)
/* The device is not bound to any driver */
-ret = 0;
-goto cleanup;
-}
+return 0;
if (!(path = virPCIFile(dev->name, "driver/unbind")))
-goto cleanup;
+return -1;
if (virFileExists(path)) {
if (virFileWriteStr(path, dev->name, 0) < 0) {
virReportSystemE
[libvirt] [PATCH v2 25/41] util: usb: use VIR_AUTOPTR for aggregate types
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;
+
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
[libvirt] [PATCH v2 15/41] util: firewall: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar
---
src/util/virfirewall.c | 16 +---
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index dfd792f..b4a4d06 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
virFirewallRulePtr rule,
const char *fmt, ...)
{
-char *arg;
+VIR_AUTOFREE(char *) arg = NULL;
va_list list;
VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule);
@@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
va_end(list);
-VIR_FREE(arg);
return;
no_memory:
firewall->err = ENOMEM;
va_end(list);
-VIR_FREE(arg);
}
@@ -678,7 +676,7 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule,
virCommandPtr cmd = NULL;
int status;
int ret = -1;
-char *error = NULL;
+VIR_AUTOFREE(char *) error = NULL;
if (!bin) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -702,11 +700,10 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule,
if (ignoreErrors) {
VIR_DEBUG("Ignoring error running command");
} else {
-char *args = virCommandToString(cmd);
+VIR_AUTOFREE(char *) args = virCommandToString(cmd);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to apply firewall rules %s: %s"),
NULLSTR(args), NULLSTR(error));
-VIR_FREE(args);
VIR_FREE(*output);
goto cleanup;
}
@@ -714,7 +711,6 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule,
ret = 0;
cleanup:
-VIR_FREE(error);
virCommandFree(cmd);
return ret;
}
@@ -807,12 +803,11 @@ virFirewallApplyRule(virFirewallPtr firewall,
virFirewallRulePtr rule,
bool ignoreErrors)
{
-char *output = NULL;
+VIR_AUTOFREE(char *) output = NULL;
+VIR_AUTOFREE(char *) str = virFirewallRuleToString(rule);
char **lines = NULL;
int ret = -1;
-char *str = virFirewallRuleToString(rule);
VIR_INFO("Applying rule '%s'", NULLSTR(str));
-VIR_FREE(str);
if (rule->ignoreErrors)
ignoreErrors = rule->ignoreErrors;
@@ -857,7 +852,6 @@ virFirewallApplyRule(virFirewallPtr firewall,
ret = 0;
cleanup:
virStringListFree(lines);
-VIR_FREE(output);
return ret;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 03/41] util: buffer: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
viralloc.h include, since that has moved from the source module into
the header.
When variables of type virBufferPtr and virBufferEscapePairPtr
are declared using VIR_AUTOPTR, the functions virBufferFreeAndReset
and virBufferEscapePairFree, respectively, will be run automatically
on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar
---
src/util/virbuffer.c | 16 ++--
src/util/virbuffer.h | 9 +++--
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index ea96704..0640cfa 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -31,10 +31,10 @@
#define __VIR_BUFFER_C__
#include "virbuffer.h"
-#include "viralloc.h"
#include "virerror.h"
#include "virstring.h"
+#define VIR_FROM_THIS VIR_FROM_NONE
/* If adding more fields, ensure to edit buf.h to match
the number of fields */
@@ -656,6 +656,18 @@ struct _virBufferEscapePair {
char *toescape;
};
+static void
+virBufferEscapePairFree(virBufferEscapePairPtr pair)
+{
+if (!pair)
+return;
+
+VIR_FREE(pair->toescape);
+VIR_FREE(pair);
+}
+VIR_DEFINE_AUTOPTR_FUNC(virBufferEscapePair, virBufferEscapePairFree)
+
+
/**
* virBufferEscapeN:
* @buf: the buffer to append to
@@ -696,7 +708,7 @@ virBufferEscapeN(virBufferPtr buf,
va_start(ap, str);
while ((escapeItem.escape = va_arg(ap, int))) {
-if (!(escapeItem.toescape = va_arg(ap, char *))) {
+if (VIR_STRDUP(escapeItem.toescape, va_arg(ap, char *)) < 0) {
virBufferSetError(buf, errno);
goto cleanup;
}
diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
index e95ee87..3b31060 100644
--- a/src/util/virbuffer.h
+++ b/src/util/virbuffer.h
@@ -23,10 +23,13 @@
#ifndef __VIR_BUFFER_H__
# define __VIR_BUFFER_H__
-# include "internal.h"
-
# include
+# include "internal.h"
+
+# include "viralloc.h"
+
+
/**
* virBuffer:
*
@@ -119,4 +122,6 @@ int virBufferGetIndent(const virBuffer *buf, bool dynamic);
void virBufferTrim(virBufferPtr buf, const char *trim, int len);
void virBufferAddStr(virBufferPtr buf, const char *str);
+VIR_DEFINE_AUTOPTR_FUNC(virBuffer, virBufferFreeAndReset)
+
#endif /* __VIR_BUFFER_H__ */
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 01/41] util: error: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When a variable of type virErrorPtr is declared using VIR_AUTOPTR, the function virFreeError will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar Reviewed-by: Erik Skultety --- src/util/virerror.c | 1 - src/util/virerror.h | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virerror.c b/src/util/virerror.c index 5f26d59..4688e01 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -30,7 +30,6 @@ #include "virerror.h" #include "datatypes.h" #include "virlog.h" -#include "viralloc.h" #include "virthread.h" #include "virutil.h" #include "virstring.h" diff --git a/src/util/virerror.h b/src/util/virerror.h index 760bfa4..31577c5 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -23,6 +23,7 @@ # define __VIRT_ERROR_H_ # include "internal.h" +# include "viralloc.h" # define VIR_ERROR_MAX_LENGTH 1024 @@ -205,4 +206,6 @@ bool virLastErrorIsSystemErrno(int errnum); void virErrorPreserveLast(virErrorPtr *saveerr); void virErrorRestore(virErrorPtr *savederr); +VIR_DEFINE_AUTOPTR_FUNC(virError, virFreeError) + #endif -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 13/41] util: mdev: use VIR_AUTOPTR for aggregate types
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/virmdev.c | 30 --
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 81fb847..4050835 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -137,20 +137,20 @@ virMediatedDevicePtr
virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model)
{
virMediatedDevicePtr ret = NULL;
-virMediatedDevicePtr dev = NULL;
+VIR_AUTOPTR(virMediatedDevice) dev = NULL;
VIR_AUTOFREE(char *) sysfspath = NULL;
if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr)))
-goto cleanup;
+return NULL;
if (!virFileExists(sysfspath)) {
virReportError(VIR_ERR_DEVICE_MISSING,
_("mediated device '%s' not found"), uuidstr);
-goto cleanup;
+return NULL;
}
if (VIR_ALLOC(dev) < 0)
-goto cleanup;
+return NULL;
VIR_STEAL_PTR(dev->path, sysfspath);
@@ -158,13 +158,11 @@ virMediatedDeviceNew(const char *uuidstr,
virMediatedDeviceModelType model)
* supported mediated device's API.
*/
if (virMediatedDeviceCheckModel(dev, model))
-goto cleanup;
+return NULL;
dev->model = model;
VIR_STEAL_PTR(ret, dev);
- cleanup:
-virMediatedDeviceFree(dev);
return ret;
}
@@ -372,8 +370,7 @@ void
virMediatedDeviceListDel(virMediatedDeviceListPtr list,
virMediatedDevicePtr dev)
{
-virMediatedDevicePtr ret = virMediatedDeviceListSteal(list, dev);
-virMediatedDeviceFree(ret);
+VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list, dev);
}
@@ -494,23 +491,22 @@ int
virMediatedDeviceTypeReadAttrs(const char *sysfspath,
virMediatedDeviceTypePtr *type)
{
-int ret = -1;
-virMediatedDeviceTypePtr tmp = NULL;
+VIR_AUTOPTR(virMediatedDeviceType) tmp = NULL;
#define MDEV_GET_SYSFS_ATTR(attr, dst, cb, optional) \
do { \
int rc; \
if ((rc = cb(dst, "%s/%s", sysfspath, attr)) < 0) { \
if (rc != -2 || !optional) \
-goto cleanup; \
+return -1; \
} \
} while (0)
if (VIR_ALLOC(tmp) < 0)
-goto cleanup;
+return -1;
if (VIR_STRDUP(tmp->id, last_component(sysfspath)) < 0)
-goto cleanup;
+return -1;
/* @name sysfs attribute is optional, so getting ENOENT is fine */
MDEV_GET_SYSFS_ATTR("name", &tmp->name, virFileReadValueString, true);
@@ -522,8 +518,6 @@ virMediatedDeviceTypeReadAttrs(const char *sysfspath,
#undef MDEV_GET_SYSFS_ATTR
VIR_STEAL_PTR(*type, tmp);
-ret = 0;
- cleanup:
-virMediatedDeviceTypeFree(tmp);
-return ret;
+
+return 0;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 14/41] util: firewall: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When a variable of type virFirewallPtr is declared using VIR_AUTOPTR, the function virFirewallFree will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar Reviewed-by: Erik Skultety --- src/util/virfirewall.c | 1 - src/util/virfirewall.h | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 10c370a..dfd792f 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -27,7 +27,6 @@ #include -#include "viralloc.h" #include "virfirewallpriv.h" #include "virerror.h" #include "virutil.h" diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index b04ab48..e024e88 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -25,6 +25,7 @@ # define __VIR_FIREWALL_H__ # include "internal.h" +# include "viralloc.h" typedef struct _virFirewall virFirewall; typedef virFirewall *virFirewallPtr; @@ -116,4 +117,6 @@ int virFirewallApply(virFirewallPtr firewall); void virFirewallSetLockOverride(bool avoid); +VIR_DEFINE_AUTOPTR_FUNC(virFirewall, virFirewallFree) + #endif /* __VIR_FIREWALL_H__ */ -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 06/41] util: hash: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When variables of type virHashTablePtr are declared using VIR_AUTOPTR, the function virHashFree will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar Reviewed-by: Erik Skultety --- src/util/virhash.c | 1 - src/util/virhash.h | 4 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virhash.c b/src/util/virhash.c index ecda55d..006ffd8 100644 --- a/src/util/virhash.c +++ b/src/util/virhash.c @@ -26,7 +26,6 @@ #include "virerror.h" #include "virhash.h" -#include "viralloc.h" #include "virlog.h" #include "virhashcode.h" #include "virrandom.h" diff --git a/src/util/virhash.h b/src/util/virhash.h index 5b24fc0..dd789c6 100644 --- a/src/util/virhash.h +++ b/src/util/virhash.h @@ -16,6 +16,8 @@ # include # include +# include "viralloc.h" + /* * The hash table. */ @@ -200,4 +202,6 @@ void *virHashSearch(const virHashTable *table, virHashSearcher iter, /* Convenience for when VIR_FREE(value) is sufficient as a data freer. */ void virHashValueFree(void *value, const void *name); +VIR_DEFINE_AUTOPTR_FUNC(virHashTable, virHashFree) + #endif /* ! __VIR_HASH_H__ */ -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 05/41] util: buffer: use VIR_AUTOPTR for aggregate types
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/virbuffer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index f70069e..7e66b4d 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -693,7 +693,7 @@ virBufferEscapeN(virBufferPtr buf, char *out; const char *cur; virBufferEscapePair escapeItem; -virBufferEscapePairPtr escapeList = NULL; +VIR_AUTOPTR(virBufferEscapePair) escapeList = NULL; size_t nescapeList = 0; va_list ap; @@ -751,7 +751,6 @@ virBufferEscapeN(virBufferPtr buf, cleanup: va_end(ap); -VIR_FREE(escapeList); } -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 9/9] conf: Add memory bandwidth allocation capability of host
On 07/18/2018 03:57 AM, bing@intel.com wrote:
> 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| 108
> +
> 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, 199 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
>
What about virsh available views? And similar to the RDT series what
about domstats? I think you can get some good ideas from the RDT CMT
RFC that's posted. Not even sure if it's already done internally - but
pointing it out... It doesn't have to be done as part of the series,
but eventually it may be nice.
I'll give the following a cursory look as I have other tasks needing
some attention. I'll leave it in the back of my mind that I have to be
more thorough on the next pass once I get here.
> 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 7a810ef..3f52296 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,11 @@ 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);
> +
> +
Remove one of the blank lines.
This was the only issue I saw in my quick glance - rest seemed OK.
John
> VIR_FREE(caps->host.netprefix);
> VIR_FREE(caps->host.pagesSize);
> virCPUDefFree(caps->host.cpu);
> @@ -957,6 +972,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,
> + " + node->id, cpus_str);
> +VIR_FREE(cpus_str);
> +
> +virBufferSetChildIndent(&controlBuf, buf);
> +virBufferAsprintf(&controlBuf,
> + " + "maxAllocs='%u'/>\n",
> + control->granularity, control->min,
> + control->max
[libvirt] [PATCH v2 04/41] util: buffer: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/virbuffer.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 0640cfa..f70069e 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -456,7 +456,8 @@ void
virBufferEscapeString(virBufferPtr buf, const char *format, const char *str)
{
int len;
-char *escaped, *out;
+VIR_AUTOFREE(char *) escaped = NULL;
+char *out;
const char *cur;
const char forbidden_characters[] = {
0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
@@ -533,7 +534,6 @@ virBufferEscapeString(virBufferPtr buf, const char *format,
const char *str)
*out = 0;
virBufferAsprintf(buf, format, escaped);
-VIR_FREE(escaped);
}
/**
@@ -612,7 +612,8 @@ virBufferEscape(virBufferPtr buf, char escape, const char
*toescape,
const char *format, const char *str)
{
int len;
-char *escaped, *out;
+VIR_AUTOFREE(char *) escaped = NULL;
+char *out;
const char *cur;
if ((format == NULL) || (buf == NULL) || (str == NULL))
@@ -644,7 +645,6 @@ virBufferEscape(virBufferPtr buf, char escape, const char
*toescape,
*out = 0;
virBufferAsprintf(buf, format, escaped);
-VIR_FREE(escaped);
}
@@ -689,7 +689,7 @@ virBufferEscapeN(virBufferPtr buf,
{
int len;
size_t i;
-char *escaped = NULL;
+VIR_AUTOFREE(char *) escaped = NULL;
char *out;
const char *cur;
virBufferEscapePair escapeItem;
@@ -752,7 +752,6 @@ virBufferEscapeN(virBufferPtr buf,
cleanup:
va_end(ap);
VIR_FREE(escapeList);
-VIR_FREE(escaped);
}
@@ -817,7 +816,8 @@ void
virBufferEscapeShell(virBufferPtr buf, const char *str)
{
int len;
-char *escaped, *out;
+VIR_AUTOFREE(char *) escaped = NULL;
+char *out;
const char *cur;
if ((buf == NULL) || (str == NULL))
@@ -861,7 +861,6 @@ virBufferEscapeShell(virBufferPtr buf, const char *str)
*out = 0;
virBufferAdd(buf, escaped, -1);
-VIR_FREE(escaped);
}
/**
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 12/41] util: mdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls 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/virmdev.c | 53 +++--
1 file changed, 19 insertions(+), 34 deletions(-)
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index d7bcb1d..81fb847 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -72,24 +72,23 @@ static int
virMediatedDeviceGetSysfsDeviceAPI(virMediatedDevicePtr dev,
char **device_api)
{
-int ret = -1;
-char *buf = NULL;
-char *file = NULL;
+VIR_AUTOFREE(char *) buf = NULL;
+VIR_AUTOFREE(char *) file = NULL;
char *tmp = NULL;
if (virAsprintf(&file, "%s/mdev_type/device_api", dev->path) < 0)
-goto cleanup;
+return -1;
/* TODO - make this a generic method to access sysfs files for various
* kinds of devices
*/
if (!virFileExists(file)) {
virReportSystemError(errno, _("failed to read '%s'"), file);
-goto cleanup;
+return -1;
}
if (virFileReadAll(file, 1024, &buf) < 0)
-goto cleanup;
+return -1;
if ((tmp = strchr(buf, '\n')))
*tmp = '\0';
@@ -97,11 +96,7 @@ virMediatedDeviceGetSysfsDeviceAPI(virMediatedDevicePtr dev,
*device_api = buf;
buf = NULL;
-ret = 0;
- cleanup:
-VIR_FREE(file);
-VIR_FREE(buf);
-return ret;
+return 0;
}
@@ -109,8 +104,7 @@ static int
virMediatedDeviceCheckModel(virMediatedDevicePtr dev,
virMediatedDeviceModelType model)
{
-int ret = -1;
-char *dev_api = NULL;
+VIR_AUTOFREE(char *) dev_api = NULL;
int actual_model;
if (virMediatedDeviceGetSysfsDeviceAPI(dev, &dev_api) < 0)
@@ -123,7 +117,7 @@ virMediatedDeviceCheckModel(virMediatedDevicePtr dev,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("device API '%s' not supported yet"),
dev_api);
-goto cleanup;
+return -1;
}
if (actual_model != model) {
@@ -132,13 +126,10 @@ virMediatedDeviceCheckModel(virMediatedDevicePtr dev,
"device only supports '%s'"),
virMediatedDeviceModelTypeToString(model),
dev->path, dev_api);
-goto cleanup;
+return -1;
}
-ret = 0;
- cleanup:
-VIR_FREE(dev_api);
-return ret;
+return 0;
}
@@ -147,7 +138,7 @@ virMediatedDeviceNew(const char *uuidstr,
virMediatedDeviceModelType model)
{
virMediatedDevicePtr ret = NULL;
virMediatedDevicePtr dev = NULL;
-char *sysfspath = NULL;
+VIR_AUTOFREE(char *) sysfspath = NULL;
if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr)))
goto cleanup;
@@ -173,7 +164,6 @@ virMediatedDeviceNew(const char *uuidstr,
virMediatedDeviceModelType model)
VIR_STEAL_PTR(ret, dev);
cleanup:
-VIR_FREE(sysfspath);
virMediatedDeviceFree(dev);
return ret;
}
@@ -218,34 +208,30 @@ virMediatedDeviceGetPath(virMediatedDevicePtr dev)
char *
virMediatedDeviceGetIOMMUGroupDev(const char *uuidstr)
{
-char *result_path = NULL;
-char *iommu_path = NULL;
+VIR_AUTOFREE(char *) result_path = NULL;
+VIR_AUTOFREE(char *) iommu_path = NULL;
+VIR_AUTOFREE(char *) dev_path = virMediatedDeviceGetSysfsPath(uuidstr);
char *vfio_path = NULL;
-char *dev_path = virMediatedDeviceGetSysfsPath(uuidstr);
if (!dev_path)
return NULL;
if (virAsprintf(&iommu_path, "%s/iommu_group", dev_path) < 0)
-goto cleanup;
+return NULL;
if (!virFileExists(iommu_path)) {
virReportSystemError(errno, _("failed to access '%s'"), iommu_path);
-goto cleanup;
+return NULL;
}
if (virFileResolveLink(iommu_path, &result_path) < 0) {
virReportSystemError(errno, _("failed to resolve '%s'"), iommu_path);
-goto cleanup;
+return NULL;
}
if (virAsprintf(&vfio_path, "/dev/vfio/%s", last_component(result_path)) <
0)
-goto cleanup;
+return NULL;
- cleanup:
-VIR_FREE(result_path);
-VIR_FREE(iommu_path);
-VIR_FREE(dev_path);
return vfio_path;
}
@@ -253,7 +239,7 @@ virMediatedDeviceGetIOMMUGroupDev(const char *uuidstr)
int
virMediatedDeviceGetIOMMUGroupNum(const char *uuidstr)
{
-char *vfio_path = NULL;
+VIR_AUTOFREE(char *) vfio_path = NULL;
char *group_num_str = NULL;
unsigned int group_num = -1;
@@ -263,7 +249,6 @@ virMediatedDeviceGetIOMMUGroupNum(const char *uuidstr)
group_num_str = last_component(vfio_path);
ignore_value(virStrToLong_ui(group_num_str, NULL, 10, &group_num));
-VIR_FREE(vfio_path);
return group_num;
}
--
1.8.3.1
--
libvir-
[libvirt] [PATCH v2 00/41] use GNU C's cleanup attribute in src/util (batch II)
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. Sukrit Bhatnagar (41): util: error: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: buffer: typedef for struct _virBufferEscapePair util: buffer: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: buffer: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: buffer: use VIR_AUTOPTR for aggregate types util: hash: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: cgroup: modify virCgroupFree to take virCgroupPtr util: cgroup: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: cgroup: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: cgroup: use VIR_AUTOPTR for aggregate types util: mdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: mdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: mdev: use VIR_AUTOPTR for aggregate types util: firewall: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: firewall: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: firewall: use VIR_AUTOPTR for aggregate types util: hook: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: hook: use VIR_AUTOPTR for aggregate types util: pci: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: pci: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: pci: use VIR_AUTOPTR for aggregate types util: usb: modify virUSBDeviceListAdd to take double pointer util: usb: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: usb: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: usb: use VIR_AUTOPTR for aggregate types util: scsi: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: scsi: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: scsi: use VIR_AUTOPTR for aggregate types util: scsivhost: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: scsivhost: use VIR_AUTOPTR for aggregate types util: netdevvlan: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: hostdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: hostdev: use VIR_AUTOPTR for aggregate types util: hostmem: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: iptables: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: iscsi: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: iscsi: use VIR_AUTOPTR for aggregate types util: kmod: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: kmod: use VIR_AUTOPTR for aggregate types util: lease: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: lease: use VIR_AUTOPTR for aggregate types src/libvirt-lxc.c| 4 +- src/lxc/lxc_cgroup.c | 4 +- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_domain.c | 2 +- src/lxc/lxc_process.c| 10 +- src/qemu/qemu_cgroup.c | 16 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 34 +-- src/qemu/qemu_process.c | 2 +- src/util/virbuffer.c | 38 ++- src/util/virbuffer.h | 9 +- src/util/vircgroup.c | 746 +-- src/util/vircgroup.h | 11 +- src/util/vircgrouppriv.h | 2 +- src/util/virerror.c | 1 - src/util/virerror.h | 3 + src/util/virfirewall.c | 53 ++-- src/util/virfirewall.h | 3 + src/util/virhash.c | 1 - src/util/virhash.h | 4 + src/util/virhook.c | 26 +- src/util/virhostdev.c| 168 --- src/util/virhostmem.c| 57 ++-- src/util/viriptables.c | 52 ++-- src/util/viriscsi.c | 89 ++ src/util/virkmod.c | 38 +-- src/util/virlease.c | 82 ++ src/util/virmdev.c | 84 ++ src/util/virmdev.h | 4 + src/util/virnetdevvlan.c | 1 - src/util/virnetdevvlan.h | 4 + src/util/virpci.c| 352 -- src/util/virpci.h| 7 + src/util/virscsi.c | 63 ++-- src/util/virscsi.h | 3 + src/util/virscsivhost.c | 19 +- src/util/virscsivhost.h | 3 + src/util/virusb.c| 42 ++- src/util/virusb.h| 5 +- tests/vircgrouptest.c| 42 +-- tests/virusbtest.c | 4 +- 42 files changed, 800 insertions(+), 1294 deletions(-) -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] conf: Introduce cputune/memorytune to support memory bandwidth allocation
subj: Add support for memorytune XML processing for resctrl MBA On 07/18/2018 03:57 AM, bing@intel.com wrote: > 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 | 35 > 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, 351 insertions(+) > 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 > Strangely I expected to actually have a merge conflict here with previous changes, but I didn't! > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index a3afe13..4e38446 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -757,6 +757,10 @@ >> > > + > + > + > >... > > @@ -950,7 +954,38 @@ > > > > + > > + memorytune > + > +Optional memorytune element can control allocations for s/Optional/Since 4.7.0, the optional/ e.g. Since 4.7.0, the optional... > +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. vcpus > specified s/./. The/ > +by memorytune can be identical to those specified by > +cachetune. However they are not allowed to overlap each > other. The last 2 sentences are "confusing". We didn't add similar wording to cachetune... It's one of those visual things I suppose, but it seems like there's a relationship between cachetune and memtune, but only if they intersect vCPU values. Thus if cachetune "joins" vcpus='0-3', then memtune must also join them - using other combinations of 0-3 isn't allowed. I have to only imagine that would get more complicated with RDT > +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. Should we indicate that it must be between 1 and 100 inclusive? > + > + > + > + > > > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index f24a563..b4cd96b 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -983,6 +983,23 @@ > > > > + > + used byame="memorytune"> > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 695981c..ea9276f 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -19139,6 +19139,128 @@ virDomainCachetuneDefParse(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; > +> +
[libvirt] [PATCH] audit: Share virtType fallback logic
Signed-off-by: Cole Robinson
---
src/conf/domain_audit.c | 91 +
1 file changed, 28 insertions(+), 63 deletions(-)
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index d9be638f05..fc13338d64 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -68,6 +68,21 @@ virDomainAuditGetRdev(const char *path ATTRIBUTE_UNUSED)
#endif
+static const char *
+virDomainAuditGetVirtType(virDomainDefPtr def)
+{
+const char *virt;
+
+if (!(virt = virDomainVirtTypeToString(def->virtType))) {
+VIR_WARN("Unexpected virt type %d while encoding audit message",
+ def->virtType);
+virt = "?";
+}
+
+return virt;
+}
+
+
static void
virDomainAuditGenericDev(virDomainObjPtr vm,
const char *type,
@@ -82,7 +97,7 @@ virDomainAuditGenericDev(virDomainObjPtr vm,
char *vmname = NULL;
char *oldsrc = NULL;
char *newsrc = NULL;
-const char *virt;
+const char *virt = virDomainAuditGetVirtType(vm->def);
/* if both new and old source aren't provided don't log anything */
if (!newsrcpath && !oldsrcpath)
@@ -99,12 +114,6 @@ virDomainAuditGenericDev(virDomainObjPtr vm,
if (!(vmname = virAuditEncode("vm", vm->def->name)))
goto no_memory;
-if (!(virt = virDomainVirtTypeToString(vm->def->virtType))) {
-VIR_WARN("Unexpected virt type %d while encoding audit message",
- vm->def->virtType);
-virt = "?";
-}
-
if (!(newsrc = virAuditEncode(newdev, VIR_AUDIT_STR(newsrcpath
goto no_memory;
@@ -312,7 +321,7 @@ virDomainAuditNetDevice(virDomainDefPtr vmDef,
virDomainNetDefPtr netDef,
char *vmname;
char *dev_name = NULL;
char *rdev;
-const char *virt;
+const char *virt = virDomainAuditGetVirtType(vmDef);
virUUIDFormat(vmDef->uuid, uuidstr);
virMacAddrFormat(&netDef->mac, macstr);
@@ -324,11 +333,6 @@ virDomainAuditNetDevice(virDomainDefPtr vmDef,
virDomainNetDefPtr netDef,
goto cleanup;
}
-if (!(virt = virDomainVirtTypeToString(vmDef->virtType))) {
-VIR_WARN("Unexpected virt type %d while encoding audit message",
vmDef->virtType);
-virt = "?";
-}
-
VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success,
"virt=%s resrc=net reason=open %s uuid=%s net=%s %s rdev=%s",
virt, vmname, uuidstr, macstr, dev_name, VIR_AUDIT_STR(rdev));
@@ -356,7 +360,8 @@ virDomainAuditHostdev(virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev,
char *vmname;
char *address = NULL;
char *device = NULL;
-const char *virt;
+const char *virt = virDomainAuditGetVirtType(vm->def);
+
virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb;
virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
@@ -369,11 +374,6 @@ virDomainAuditHostdev(virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev,
return;
}
-if (!(virt = virDomainVirtTypeToString(vm->def->virtType))) {
-VIR_WARN("Unexpected virt type %d while encoding audit message",
vm->def->virtType);
-virt = "?";
-}
-
switch ((virDomainHostdevMode) hostdev->mode) {
case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
@@ -509,7 +509,7 @@ virDomainAuditRedirdev(virDomainObjPtr vm,
virDomainRedirdevDefPtr redirdev,
char *vmname;
char *address = NULL;
char *device = NULL;
-const char *virt;
+const char *virt = virDomainAuditGetVirtType(vm->def);
virUUIDFormat(vm->def->uuid, uuidstr);
if (!(vmname = virAuditEncode("vm", vm->def->name))) {
@@ -517,11 +517,6 @@ virDomainAuditRedirdev(virDomainObjPtr vm,
virDomainRedirdevDefPtr redirdev,
return;
}
-if (!(virt = virDomainVirtTypeToString(vm->def->virtType))) {
-VIR_WARN("Unexpected virt type %d while encoding audit message",
vm->def->virtType);
-virt = "?";
-}
-
switch (redirdev->bus) {
case VIR_DOMAIN_REDIRDEV_BUS_USB:
if (VIR_STRDUP_QUIET(address, "USB redirdev") < 0) {
@@ -571,7 +566,7 @@ virDomainAuditTPM(virDomainObjPtr vm, virDomainTPMDefPtr
tpm,
char *vmname;
char *path = NULL;
char *device = NULL;
-const char *virt;
+const char *virt = virDomainAuditGetVirtType(vm->def);
virUUIDFormat(vm->def->uuid, uuidstr);
if (!(vmname = virAuditEncode("vm", vm->def->name))) {
@@ -579,11 +574,6 @@ virDomainAuditTPM(virDomainObjPtr vm, virDomainTPMDefPtr
tpm,
return;
}
-if (!(virt = virDomainVirtTypeToString(vm->def->virtType))) {
-VIR_WARN("Unexpected virt type %d while encoding audit message",
vm->def->virtType);
-virt = "?";
-}
-
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
path = tpm->data.passthrough.sou
[libvirt] [PATCH RFC 2/2] conf: Convert virDomainVirtType to VIR_ENUM_IMPL_LABEL
Signed-off-by: Cole Robinson
---
src/conf/domain_conf.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f94a90fbcc..eb40b7f349 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -103,7 +103,7 @@ VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST,
"custom-dtb",
"custom-ga-command");
-VIR_ENUM_IMPL(virDomainVirt, VIR_DOMAIN_VIRT_LAST,
+VIR_ENUM_IMPL_LABEL(virDomainVirt, "domain type", VIR_DOMAIN_VIRT_LAST,
"none",
"qemu",
"kqemu",
@@ -19141,10 +19141,7 @@ virDomainDefParseCaps(virDomainDefPtr def,
goto cleanup;
}
if ((def->virtType = virDomainVirtTypeFromString(virttype)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("invalid domain type %s"), virttype);
goto cleanup;
-}
if (!ostype) {
if (def->os.bootloader) {
@@ -27380,11 +27377,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST,
-1);
-if (!(type = virDomainVirtTypeToString(def->virtType))) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected domain type %d"), def->virtType);
+if (!(type = virDomainVirtTypeToString(def->virtType)))
goto error;
-}
if (def->id == -1)
flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RFC 0/2] util: Add VIR_ENUM_IMPL_LABEL
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 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(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RFC 1/2] util: Add VIR_ENUM_IMPL_LABEL
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) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _("Unknown '%s' value '%s'"), label, type);
+}
return -1;
}
@@ -540,10 +546,16 @@ virFormatIntPretty(unsigned long long val,
const char *virEnumToString(const char *const*types,
unsigned int ntypes,
-int type)
+int type,
+const char * const label)
{
-if (type < 0 || type >= ntypes)
+if (type < 0 || type >= ntypes) {
+if (label) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown '%s' internal value %d"), label, type);
+}
return NULL;
+}
return types[type];
}
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 1ba9635bd9..345c9e053d 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -76,26 +76,31 @@ char *virIndexToDiskName(int idx, const char *prefix);
int virEnumFromString(const char *const*types,
unsigned int ntypes,
- const char *type);
+ const char *type,
+ const char *errmsg);
const char *virEnumToString(const char *const*types,
unsigned int ntypes,
-int type);
+int type,
+const char *errmsg);
-# define VIR_ENUM_IMPL(name, lastVal, ...) \
+# define VIR_ENUM_IMPL_LABEL(name, label, lastVal, ...) \
static const char *const name ## TypeList[] = { __VA_ARGS__ }; \
verify(ARRAY_CARDINALITY(name ## TypeList) == lastVal); \
const char *name ## TypeToString(int type) { \
return virEnumToString(name ## TypeList, \
ARRAY_CARDINALITY(name ## TypeList), \
- type); \
+ type, label); \
} \
int name ## TypeFromString(const char *type) { \
return virEnumFromString(name ## TypeList, \
ARRAY_CARDINALITY(name ## TypeList), \
- type); \
+ type, label); \
}
+# define VIR_ENUM_IMPL(name, lastVal, ...) \
+VIR_ENUM_IMPL_LABEL(name, NULL, lastVal, __VA_ARGS__)
+
# define VIR_ENUM_DECL(name) \
const char *name ## TypeToString(int type); \
int name ## TypeFromString(const char*type);
--
2.17.1
--
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 07/18/2018 03:57 AM, bing@intel.com wrote:
> From: Bing Niu
>
> Refactoring virDomainCachetuneDefParse, extracting vcpus extracting,
> overlapping detecting and new resctrl allocation creating logic.
> Those two logic is common among different resource allocation
> technologies.
>
> Signed-off-by: Bing Niu
> ---
> src/conf/domain_conf.c | 195
> +
> 1 file changed, 131 insertions(+), 64 deletions(-)
>
You could make 3 patches out of this - one for each reduction of
virDomainCachetuneDefParse...
The virDomainFindResctrlAlloc should have used Restune instead of
ResctrlAlloc, right? Of course that changes based on how the naming
patch shakes out. Actually I think perhaps virDomainRestuneVcpuMatch
would be even better (where Restune ends up being whatever shakes out of
the previous patch naming decision).
In this area, I'm wondering what purpose does @new_alloc serve? If
@alloc was already defined, then you added an error message. So the
reality is - @new_alloc is always true.
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
code has been around for more than a release) would need to go in some
sort of Validation API. This is one of those libvirt define and libvirtd
restart quirks.
Finally, virDomainUpdateRestune should be virDomainRestuneUpdate since
you started with virDomainRestuneParseVcpus and it should go after
virDomainCachetuneDefParseCache
Hope this all makes sense!
John
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 24fefd1..695981c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18884,6 +18884,115 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
>
>
> static int
> +virDomainRestuneParseVcpus(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, "%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);
> +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
> +virDomainFindResctrlAlloc(virDomainDefPtr def,
> + virBitmapPtr vcpus,
> + virResctrlAllocPtr *alloc)
> +{
> +ssize_t i = 0;
> +
> +for (i = 0; i < def->nrestunes; i++) {
> +/* vcpus group has been created, directly use the existing one.
> + * Just updating memory allocation information of that group
> + */
> +if (virBitmapEqual(def->restunes[i]->vcpus, vcpus)) {
> +*alloc = def->restunes[i]->alloc;
> +break;
> +}
> +if (virBitmapOverlaps(def->restunes[i]->vcpus, vcpus)) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Overlapping vcpus in restunes"));
> +return -1;
> +}
> +}
> +return 0;
> +}
> +
> +
> +static int
> +virDomainUpdateRestune(virDomainDefPtr def,
> + xmlNodePtr node,
> + virResctrlAllocPtr alloc,
> + virBitmapPtr vcpus,
> + unsigned int flags)
> +{
> +char *vcpus_str = NULL;
> +char *alloc_id = NULL;
> +virDomainRestuneDefPtr tmp_restune = NULL;
> +int ret = -1;
> +
> +if (VIR_ALLOC(tmp_restune) < 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
Re: [libvirt] [PATCH 6/9] conf: Rename cachetune to restune
> -Original Message-
> From: John Ferlan [mailto:jfer...@redhat.com]
> Sent: Friday, July 27, 2018 12:33 AM
> To: Niu, Bing ; libvir-list@redhat.com
> Cc: Feng, Shaohe ; Wang, Huaqiang
> ; Ding, Jian-feng ;
> rui.z...@yandex.com
> Subject: Re: [libvirt] [PATCH 6/9] conf: Rename cachetune to restune
>
>
>
> On 07/18/2018 03:57 AM, bing@intel.com wrote:
> > From: Bing Niu
> >
> > Resctrl not only supports cache tuning, but also memory bandwidth
> > tuning. Renaming cachetune to restune(resource tuning) to reflect
> > that. With restune, all allocation for different resources (cache,
> > memory bandwidth) are aggregated and represented by a
> > virResctrlAllocPtr inside virDomainRestuneDef.
> >
> > 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(-)
> >
>
> As I noted previously, not much a fan of Restune instead of Cachetune, but I
> understand the logic why you went that way.
>
> I wonder if "virDomainResAllocDef" is any better (resallocs, nresallocs)? or
> if
> that clashes with any other namespace so far? or is too close to
> virResctrlAllocPtr.
>
> Or perhaps "virDomainResCtrlDef" w/ resctrls and nresctrls to mimic the
> virresctrl.{c,h} naming scheme.
>
Hi John,
I have made a comment without reading above line...
Yes! This is what I want to change for the naming. 'virDomainResCtrlDef' looks
Ok for me.
> As previously stated, "Naming is hard"... Wish there was more feedback than
> just me on this, but in the long run, I'll go with whatever the Intel team
> agrees
> upon as it's not that big a deal. If someone else has agita after things are
> pushed
> and wants to change the name, then they know how to send patches.
>
> John
>
> [...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/9] conf: Rename cachetune to restune
> -Original Message-
> From: John Ferlan [mailto:jfer...@redhat.com]
> Sent: Friday, July 27, 2018 12:33 AM
> To: Niu, Bing ; libvir-list@redhat.com
> Cc: Feng, Shaohe ; Wang, Huaqiang
> ; Ding, Jian-feng ;
> rui.z...@yandex.com
> Subject: Re: [libvirt] [PATCH 6/9] conf: Rename cachetune to restune
>
>
>
> On 07/18/2018 03:57 AM, bing@intel.com wrote:
> > From: Bing Niu
> >
> > Resctrl not only supports cache tuning, but also memory bandwidth
> > tuning. Renaming cachetune to restune(resource tuning) to reflect
> > that. With restune, all allocation for different resources (cache,
> > memory bandwidth) are aggregated and represented by a
> > virResctrlAllocPtr inside virDomainRestuneDef.
> >
> > 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(-)
> >
>
> As I noted previously, not much a fan of Restune instead of Cachetune, but I
> understand the logic why you went that way.
>
> I wonder if "virDomainResAllocDef" is any better (resallocs, nresallocs)? or
> if
> that clashes with any other namespace so far? or is too close to
> virResctrlAllocPtr.
>From Huaqiang:
Hi Bing and John, this is Huaqiang and I am working on preparing libvirt patches
to support cache monitoring (CMT) feature and memory bandwidth monitoring
feature(MBM).
I am wondering if we can consider making a further step of renaming of
'virDomainResAllocDef' in the future to accommodate RDT monitoring
feature (CMT and MBM):
Using 'virDomainResDef' or 'virDomainCPUResDef' to substitute
'virDomainResAllocDef'? My considerations are below:
- 'cache tune', 'memory bw tune' and the corresponding monitoring technologies
are integrated in same underlying CPU resource control group (aka: resctrlfs
group).
They are sharing interfaces exposed interfaces of same resctrlfs subdirectory.
- If we make a rename to 'virDomainResDef'/'virDomainCPUResDef', this data
struct could be used for CMT/MBM feature in a very straightforward way.
- And I prefer 'virDomainCPUResDef' than the other one which make it clear
that it is dealing with CPU resources other than network or disk resources.
I am also in thinking about raising a refactor patch in the future to rename
'virResctrlAllocPtr' to ' virResctrlPtr'(or virResctrlGroupPtr, still in
figuring out
an appropriate name...). Which will make it possible to reuse some of code
existing in file virresctrl.c/h now with similar reasons I talked above.
>
> Or perhaps "virDomainResCtrlDef" w/ resctrls and nresctrls to mimic the
> virresctrl.{c,h} naming scheme.
>
> As previously stated, "Naming is hard"... Wish there was more feedback than
> just me on this, but in the long run, I'll go with whatever the Intel team
> agrees
> upon as it's not that big a deal. If someone else has agita after things are
> pushed
> and wants to change the name, then they know how to send patches.
>
> John
>
> [...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/9] conf: Rename cachetune to restune
On 07/18/2018 03:57 AM, bing@intel.com wrote:
> From: Bing Niu
>
> Resctrl not only supports cache tuning, but also memory bandwidth
> tuning. Renaming cachetune to restune(resource tuning) to reflect
> that. With restune, all allocation for different resources (cache,
> memory bandwidth) are aggregated and represented by a
> virResctrlAllocPtr inside virDomainRestuneDef.
>
> 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(-)
>
As I noted previously, not much a fan of Restune instead of Cachetune,
but I understand the logic why you went that way.
I wonder if "virDomainResAllocDef" is any better (resallocs,
nresallocs)? or if that clashes with any other namespace so far? or is
too close to virResctrlAllocPtr.
Or perhaps "virDomainResCtrlDef" w/ resctrls and nresctrls to mimic the
virresctrl.{c,h} naming scheme.
As previously stated, "Naming is hard"... Wish there was more feedback
than just me on this, but in the long run, I'll go with whatever the
Intel team agrees upon as it's not that big a deal. If someone else has
agita after things are pushed and wants to change the name, then they
know how to send patches.
John
[...]
--
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, 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 -- 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 08:29:45AM -0600, Alex Williamson wrote: > On Thu, 26 Jul 2018 15:50:28 +0200 > Erik Skultety wrote: > > > On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote: > > > On Fri, 20 Jul 2018 10:19:24 +0800 > > > Zhenyu Wang wrote: > > > > > > > Current mdev device create interface depends on fixed mdev type, which > > > > get uuid > > > > from user to create instance of mdev device. If user wants to use > > > > customized > > > > number of resource for mdev device, then only can create new mdev type > > > > for that > > > > which may not be flexible. This requirement comes not only from to be > > > > able to > > > > allocate flexible resources for KVMGT, but also from Intel scalable IO > > > > virtualization which would use vfio/mdev to be able to allocate > > > > arbitrary > > > > resources on mdev instance. More info on [1] [2] [3]. > > > > > > > > To allow to create user defined resources for mdev, it trys to extend > > > > mdev > > > > create interface by adding new "instances=xxx" parameter following > > > > uuid, for > > > > target mdev type if aggregation is supported, it can create new mdev > > > > device > > > > which contains resources combined by number of instances, e.g > > > > > > > > echo ",instances=10" > create > > > > > > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute > > > > which > > > > can support this setting. If no "aggregation" attribute found for mdev > > > > type, > > > > previous behavior is still kept for one instance allocation. And new > > > > sysfs > > > > attribute "instances" is created for each mdev device to show allocated > > > > number. > > > > > > > > This trys to create new KVMGT type with minimal vGPU resources which > > > > can be > > > > combined with "instances=x" setting to allocate for user wanted > > > > resources. > > > > > > "instances" makes me think this is arg helps to create multiple mdev > > > instances rather than consuming multiple instances for a single mdev. > > > You're already exposing the "aggregation" attribute, so doesn't > > > "aggregate" perhaps make more sense as the create option? We're asking > > > the driver to aggregate $NUM instances into a single mdev. The mdev > > > attribute should then perhaps also be "aggregated_instances". > > > > > > The next user question for the interface might be what aspect of the > > > device gets multiplied by this aggregation? In i915 I see you're > > > multiplying the memory sizes by the instance, but clearly the > > > resolution doesn't change. I assume this is sort of like mdev types > > > themselves, ie. some degree of what a type means is buried in the > > > implementation and some degree of what some number of those types > > > aggregated together means is impossible to describe generically. > > > > I don't seem to clearly see the benefit here, so I have to ask, how is this > > better and/or different from allowing a heterogeneous setup if one needs a > > more > > performant instance in terms of more resources? Because to me, once you're > > able > > to aggregate instances, I would assume that a simple "echo `uuid`" with a > > different type should succeed as well and provide me (from user's > > perspective) > > with the same results. Could you please clarify this to me, as well as what > > resources/parameters are going to be impacted by aggregation? > > I think you're suggesting that we could simply define new mdev types to > account for these higher aggregate instances, for example we can > define discrete types that are 2x, 3x, 4x, etc. the resource count of a > single instance. What I think we're trying to address with this > proposal is what happens when the resources available are exceptionally > large and they can be combined in arbitrary ways. For example if a > parent device can expose 10,000 resources and the granularity with > which we can create and mdev instance is 1, is it practical to create > 10,000 mdev types or does it make more sense to expose a granularity > and method of aggregation. Okay, I got a much better picture now, thanks for the clarification. The granularity you mentioned definitely does give users more power and control (in terms of provisioning) over the devices. Erik > > Using graphics here perhaps falls a little short of the intention of > the interface because the possible types are easily enumerable and it > would be entirely practical to create discrete types for each. vGPUs > also have a lot of variables, so defining which attribute of the device > is multiplied by the number of instances is a little more fuzzy. > Thanks, > > Alex -- 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, 26 Jul 2018 09:51:26 -0600 Alex Williamson wrote: > On Thu, 26 Jul 2018 17:30:07 +0200 > Cornelia Huck wrote: > > > On Thu, 26 Jul 2018 15:50:28 +0200 > > Erik Skultety wrote: > > > Since libvirt doesn't have an API to create mdevs yet, this doesn't pose > > > an > > > issue for us at the moment. I see this adds new optional sysfs attributes > > > which > > > we could expose within our device capabilities XML, provided it doesn't > > > use a > > > free form text, like the description attribute does. > > > > 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? > > Ultimately the aggregation value should be fully specified in > Documentation/ABI, but doesn't the kernel generally use 'Y' or 'N' for > boolean attributes in sysfs? Maybe mdev core can handle the attribute > since it should exist any time the create_with_instances callback is > provided. It might make sense to print a number if the driver allows a number of resources to be aggregated which is not the same as available_instances (see my reply to the documentation patch). > > > Additionally, would it > > be easier if the "create" attribute always accepted > > ",instances=1" (calling the .create ops in that case?) > > Unless I misunderstand the code or the question, I think this is > exactly what happens: Indeed, it does. Let me blame the weather ;) > If the mdev core supplied the aggregation attribute, then the presence > of the attribute could indicate to userspace whether it can always > provide an instance (aggregate) count, even if limited to '1' when 'N', > for that mdev type. Thanks, > > Alex -- 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, 26 Jul 2018 17:30:07 +0200
Cornelia Huck wrote:
> On Thu, 26 Jul 2018 15:50:28 +0200
> Erik Skultety wrote:
>
> > On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote:
> > > On Fri, 20 Jul 2018 10:19:24 +0800
> > > Zhenyu Wang wrote:
> > >
> > > > Current mdev device create interface depends on fixed mdev type, which
> > > > get uuid
> > > > from user to create instance of mdev device. If user wants to use
> > > > customized
> > > > number of resource for mdev device, then only can create new mdev type
> > > > for that
> > > > which may not be flexible. This requirement comes not only from to be
> > > > able to
> > > > allocate flexible resources for KVMGT, but also from Intel scalable IO
> > > > virtualization which would use vfio/mdev to be able to allocate
> > > > arbitrary
> > > > resources on mdev instance. More info on [1] [2] [3].
> > > >
> > > > To allow to create user defined resources for mdev, it trys to extend
> > > > mdev
> > > > create interface by adding new "instances=xxx" parameter following
> > > > uuid, for
> > > > target mdev type if aggregation is supported, it can create new mdev
> > > > device
> > > > which contains resources combined by number of instances, e.g
> > > >
> > > > echo ",instances=10" > create
> > > >
> > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute
> > > > which
> > > > can support this setting. If no "aggregation" attribute found for mdev
> > > > type,
> > > > previous behavior is still kept for one instance allocation. And new
> > > > sysfs
> > > > attribute "instances" is created for each mdev device to show allocated
> > > > number.
> > > >
> > > > This trys to create new KVMGT type with minimal vGPU resources which
> > > > can be
> > > > combined with "instances=x" setting to allocate for user wanted
> > > > resources.
> > >
> > > "instances" makes me think this is arg helps to create multiple mdev
> > > instances rather than consuming multiple instances for a single mdev.
> > > You're already exposing the "aggregation" attribute, so doesn't
> > > "aggregate" perhaps make more sense as the create option? We're asking
> > > the driver to aggregate $NUM instances into a single mdev. The mdev
> > > attribute should then perhaps also be "aggregated_instances".
>
> I agree, that seems like a better naming scheme.
>
> (...)
>
> > > I'm curious what libvirt folks and Kirti think of this, it looks like
> > > it has a nice degree of backwards compatibility, both in the sysfs
> > > interface and the vendor driver interface. Thanks,
> >
> > Since libvirt doesn't have an API to create mdevs yet, this doesn't pose an
> > issue for us at the moment. I see this adds new optional sysfs attributes
> > which
> > we could expose within our device capabilities XML, provided it doesn't use
> > a
> > free form text, like the description attribute does.
>
> 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?
Ultimately the aggregation value should be fully specified in
Documentation/ABI, but doesn't the kernel generally use 'Y' or 'N' for
boolean attributes in sysfs? Maybe mdev core can handle the attribute
since it should exist any time the create_with_instances callback is
provided.
> Additionally, would it
> be easier if the "create" attribute always accepted
> ",instances=1" (calling the .create ops in that case?)
Unless I misunderstand the code or the question, I think this is
exactly what happens:
+ if (instances > 1)
+ ret = parent->ops->create_with_instances(kobj, mdev, instances);
+ else
+ ret = parent->ops->create(kobj, mdev);
And elsewhere:
+ if (instances > 1 && !parent->ops->create_with_instances) {
+ ret = -EINVAL;
+ goto mdev_fail;
+ }
If the mdev core supplied the aggregation attribute, then the presence
of the attribute could indicate to userspace whether it can always
provide an instance (aggregate) count, even if limited to '1' when 'N',
for that mdev type. Thanks,
Alex
--
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, 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?
> + is an optional attributes to show that []'s instances can be
> + aggregated to be assigned for one mdev device. Set number of instances by
> + appending "instances=N" parameter for create. Instances number can't exceed
> + available_instances number. Without "instances=N" parameter will be default
> + one instance to create.
Could there be a case where available_instances is n, but aggregation
is only supported for a value m < n? If yes, should m be discoverable
via the "aggregation" attribute?
> +
> +Example::
> +
> + # echo ",instances=N" > create
> +
> Directories and Files Under the sysfs for Each mdev Device
> --
>
> @@ -268,6 +289,7 @@ Directories and Files Under the sysfs for Each mdev Device
>|- [parent phy device]
>|--- [$MDEV_UUID]
> |--- remove
> + |--- instances
> |--- mdev_type {link to its type}
> |--- vendor-specific-attributes [optional]
>
> @@ -281,6 +303,11 @@ Example::
>
> # echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
>
> +* instances
> +
> +For aggregation type show number of instances assigned for this mdev. For
> normal
> +type or default will just show one instance.
> +
> Mediated device Hot plug
>
>
--
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 05:30:07PM +0200, Cornelia Huck wrote: > On Thu, 26 Jul 2018 15:50:28 +0200 > Erik Skultety wrote: > > > On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote: > > > On Fri, 20 Jul 2018 10:19:24 +0800 > > > Zhenyu Wang wrote: > > > > > > > Current mdev device create interface depends on fixed mdev type, which > > > > get uuid > > > > from user to create instance of mdev device. If user wants to use > > > > customized > > > > number of resource for mdev device, then only can create new mdev type > > > > for that > > > > which may not be flexible. This requirement comes not only from to be > > > > able to > > > > allocate flexible resources for KVMGT, but also from Intel scalable IO > > > > virtualization which would use vfio/mdev to be able to allocate > > > > arbitrary > > > > resources on mdev instance. More info on [1] [2] [3]. > > > > > > > > To allow to create user defined resources for mdev, it trys to extend > > > > mdev > > > > create interface by adding new "instances=xxx" parameter following > > > > uuid, for > > > > target mdev type if aggregation is supported, it can create new mdev > > > > device > > > > which contains resources combined by number of instances, e.g > > > > > > > > echo ",instances=10" > create > > > > > > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute > > > > which > > > > can support this setting. If no "aggregation" attribute found for mdev > > > > type, > > > > previous behavior is still kept for one instance allocation. And new > > > > sysfs > > > > attribute "instances" is created for each mdev device to show allocated > > > > number. > > > > > > > > This trys to create new KVMGT type with minimal vGPU resources which > > > > can be > > > > combined with "instances=x" setting to allocate for user wanted > > > > resources. > > > > > > "instances" makes me think this is arg helps to create multiple mdev > > > instances rather than consuming multiple instances for a single mdev. > > > You're already exposing the "aggregation" attribute, so doesn't > > > "aggregate" perhaps make more sense as the create option? We're asking > > > the driver to aggregate $NUM instances into a single mdev. The mdev > > > attribute should then perhaps also be "aggregated_instances". > > I agree, that seems like a better naming scheme. > > (...) > > > > I'm curious what libvirt folks and Kirti think of this, it looks like > > > it has a nice degree of backwards compatibility, both in the sysfs > > > interface and the vendor driver interface. Thanks, > > > > Since libvirt doesn't have an API to create mdevs yet, this doesn't pose an > > issue for us at the moment. I see this adds new optional sysfs attributes > > which > > we could expose within our device capabilities XML, provided it doesn't use > > a > > free form text, like the description attribute does. > > 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. > be easier if the "create" attribute always accepted > ",instances=1" (calling the .create ops in that case?) Is ^this libvirt related question? If so, then by accepting such syntax you'll definitely save us a few lines of code ;). However, until we have a clear vision on how to tackle the mdev migration, I'd like to avoid proposing a libvirt "create" API until then. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] vfio/mdev: Add new instances parameter for mdev create
On Fri, 20 Jul 2018 10:19:25 +0800
Zhenyu Wang wrote:
> For special mdev type which can aggregate instances for mdev device,
> this extends mdev create interface by allowing extra "instances=xxx"
> parameter, which is passed to mdev device model to be able to create
> arbitrary bundled number of instances for target mdev device.
>
> v2: create new create_with_instances operator for vendor driver
>
> Cc: Kirti Wankhede
> Cc: Alex Williamson
> Cc: Kevin Tian
> Signed-off-by: Zhenyu Wang
> ---
> drivers/vfio/mdev/mdev_core.c| 18 +
> drivers/vfio/mdev/mdev_private.h | 5 -
> drivers/vfio/mdev/mdev_sysfs.c | 34 ++--
> include/linux/mdev.h | 10 ++
> 4 files changed, 56 insertions(+), 11 deletions(-)
>
(...)
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index 249472f05509..a06e5b7c69d3 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -54,11 +54,15 @@ static const struct sysfs_ops mdev_type_sysfs_ops = {
> static ssize_t create_store(struct kobject *kobj, struct device *dev,
> const char *buf, size_t count)
> {
> - char *str;
> + char *str, *opt = NULL;
> uuid_le uuid;
> int ret;
> + unsigned int instances = 1;
>
> - if ((count < UUID_STRING_LEN) || (count > UUID_STRING_LEN + 1))
> + if (count < UUID_STRING_LEN)
> + return -EINVAL;
> +
> + if (count > UUID_STRING_LEN + 1 + MDEV_CREATE_OPT_LEN)
Do you plan to have other optional parameters? If you don't, you could
probably do a quick exit here if count is between UUID_STRING_LEN + 1
and UUID_STRING_LEN + 12 (for ",instances=")?
> return -EINVAL;
>
> str = kstrndup(buf, count, GFP_KERNEL);
(...)
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index b6e048e1045f..cfb702600f95 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -30,6 +30,13 @@ struct mdev_device;
> * @kobj: kobject of type for which 'create' is called.
> * @mdev: mdev_device structure on of mediated device
> * that is being created
> + * @create_with_instances: Allocate aggregated instances' resources in
> parent device's
> + * driver for a particular mediated device. It is optional
> + * if doesn't support aggregated resources.
"Optional if aggregated resources are not supported"
> + * @kobj: kobject of type for which 'create' is called.
> + * @mdev: mdev_device structure on of mediated device
> + * that is being created
> + * @instances: number of instances to aggregate
> * Returns integer: success (0) or error (< 0)
You need that "Returns" line for both the old and the new ops.
> * @remove: Called to free resources in parent device's driver for a
> * a mediated device. It is mandatory to provide 'remove'
> @@ -71,6 +78,9 @@ struct mdev_parent_ops {
> struct attribute_group **supported_type_groups;
>
> int (*create)(struct kobject *kobj, struct mdev_device *mdev);
> + int (*create_with_instances)(struct kobject *kobj,
> + struct mdev_device *mdev,
> + unsigned int instances);
> int (*remove)(struct mdev_device *mdev);
> int (*open)(struct mdev_device *mdev);
> void(*release)(struct mdev_device *mdev);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] lxc: Turn @veths into a string list in virLXCProcessStart
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;
-
type = virDomainNetGetActualType(net);
switch (type) {
case VIR_DOMAIN_NET_TYPE_NETWORK:
@@ -604,7 +601,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
}
}
-(*veths)[(*nveths)-1] = veth;
+if (virStringListAdd(veths, veth) < 0)
+goto cleanup;
if (VIR_STRDUP(def->nets[i]->ifname_guest_actual, veth) < 0)
goto cleanup;
@@ -902,7 +900,6 @@ int virLXCProcessStop(virLXCDriverPtr driver,
static virCommandPtr
virLXCProcessBuildControllerCmd(virLXCDriverPtr driver,
virDomainObjPtr vm,
-int nveths,
char **veths,
int *ttyFDs,
size_t nttyFDs,
@@ -987,7 +984,7 @@ virLXCProcessBuildControllerCmd(virLXCDriverPtr driver,
virCommandAddArg(cmd, "--handshake");
virCommandAddArgFormat(cmd, "%d", handshakefd);
-for (i = 0; i < nveths; i++)
+for (i = 0; veths && veths[i]; i++)
virCommandAddArgList(cmd, "--veth", veths[i], NULL);
virCommandPassFD(cmd, handshakefd, 0);
@@ -1184,7 +1181,6 @@ int virLXCProcessStart(virConnectPtr conn,
size_t i;
char *logfile = NULL;
int logfd = -1;
-size_t nveths = 0;
char **veths = NULL;
int handshakefds[2] = { -1, -1 };
off_t pos = -1;
@@ -1355,7 +1351,7 @@ int virLXCProcessStart(virConnectPtr conn,
}
VIR_DEBUG("Setting up Interfaces");
-if (virLXCProcessSetupInterfaces(conn, vm->def, &nveths, &veths) < 0)
+if (virLXCProcessSetupInterfaces(conn, vm->def, &veths) < 0)
goto cleanup;
VIR_DEBUG("Setting up namespaces if any");
@@ -1379,7 +1375,7 @@ int virLXCProcessStart(virConnectPtr conn,
if (!(cmd = virLXCProcessBuildControllerCmd(driver,
vm,
-nveths, veths,
+veths,
ttyFDs, nttyFDs,
nsInheritFDs,
files, nfiles,
@@ -1559,9 +1555,7 @@ int virLXCProcessStart(virConnectPtr conn,
virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
}
virCommandFree(cmd);
-for (i = 0; i < nveths; i++)
-VIR_FREE(veths[i]);
-VIR_FREE(veths);
+virStringListFree(veths);
for (i = 0; i < nttyFDs; i++)
VIR_FORCE_CLOSE(ttyFDs[i]);
VIR_FREE(ttyFDs);
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] lxc: Use VIR_AUTOPTR for @veths in virLXCProcessStart
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
---
src/lxc/lxc_process.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 3ac39d598c..6eef17d1ce 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1181,7 +1181,7 @@ int virLXCProcessStart(virConnectPtr conn,
size_t i;
char *logfile = NULL;
int logfd = -1;
-char **veths = NULL;
+VIR_AUTOPTR(virString) veths = NULL;
int handshakefds[2] = { -1, -1 };
off_t pos = -1;
char ebuf[1024];
@@ -1555,7 +1555,6 @@ int virLXCProcessStart(virConnectPtr conn,
virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
}
virCommandFree(cmd);
-virStringListFree(veths);
for (i = 0; i < nttyFDs; i++)
VIR_FORCE_CLOSE(ttyFDs[i]);
VIR_FREE(ttyFDs);
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] util: Rework virStringListAdd
So every caller does the same: they use virStringListAdd() to add
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;
+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;
}
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 125fd4eede..a2133ab7ce 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -44,8 +44,8 @@ char *virStringListJoin(const char **strings,
const char *delim)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-char **virStringListAdd(const char **strings,
-const char *item);
+int virStringListAdd(char ***strings,
+ const char *item);
void virStringListRemove(char ***strings,
const char *item);
diff --git a/tests/virstringtest.c b/tests/virstringtest.c
index 1230aba5b7..1a1e6364d1 100644
--- a/tests/virstringtest.c
+++ b/tests/virstringtest.c
@@ -179,12 +179,8 @@ static int testAdd(const void *args)
size_t i;
for (i = 0; data->tokens[i]; i++) {
-char **tmp = virStringListAdd((const char **)list, data->tokens[i]);
-if (!tmp)
+if (virStringListAdd(&list, data->tokens[i]) < 0)
goto cleanup;
-virStringListFree(list);
-list = tmp;
-tmp = NULL;
}
if (!list &&
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] lxc: Simplify virLXCProcessStart a tiny bit
These are basically follow up patches as suggested by Erik in his review: https://www.redhat.com/archives/libvir-list/2018-July/msg01767.html Michal Prívozník (3): util: Rework virStringListAdd lxc: Turn @veths into a string list in virLXCProcessStart lxc: Use VIR_AUTOPTR for @veths in virLXCProcessStart src/lxc/lxc_process.c | 25 + src/util/virmacmap.c | 8 ++-- src/util/virstring.c | 34 ++ src/util/virstring.h | 4 ++-- tests/virstringtest.c | 6 +- 5 files changed, 28 insertions(+), 49 deletions(-) -- 2.16.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: fix virtType FromString check
On Thu, Jul 26, 2018 at 11:30:07AM -0400, Cole Robinson wrote:
> In the domain definition, virtType is virDomainVirtType which is
> unsigned, so comparing against -1 doesn't work. Cast to int first
>
> Fixes 8e2982b5767
>
> Signed-off-by: Cole Robinson
> ---
> This regressed in the past as well, when virtType was changed to
> virDomainVirtType in 7383b8cc068, fixed by the follow up 5e06a4f063.
> It's strange that virDomainVirtType is unsigned but VirtTypeFromString
> can return -1... it should probably either work like virArch, or we
> should switch virDomainVirtType to int in the DomainDef, but that
> requires fixing a few other places too
>
> src/conf/domain_conf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c27c874d9e..30806d1e59 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -19140,7 +19140,7 @@ virDomainDefParseCaps(virDomainDefPtr def,
> "%s", _("missing domain type attribute"));
> goto cleanup;
> }
> -if ((def->virtType = virDomainVirtTypeFromString(virttype)) < 0) {
> +if ((int)(def->virtType = virDomainVirtTypeFromString(virttype)) < 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("invalid domain type %s"), virttype);
> goto cleanup;
I've already pushed the fix for this problem, changing "virtType"
back to an int.
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 0/4] New mdev type handling for aggregated resources
On Thu, 26 Jul 2018 15:50:28 +0200 Erik Skultety wrote: > On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote: > > On Fri, 20 Jul 2018 10:19:24 +0800 > > Zhenyu Wang wrote: > > > > > Current mdev device create interface depends on fixed mdev type, which > > > get uuid > > > from user to create instance of mdev device. If user wants to use > > > customized > > > number of resource for mdev device, then only can create new mdev type > > > for that > > > which may not be flexible. This requirement comes not only from to be > > > able to > > > allocate flexible resources for KVMGT, but also from Intel scalable IO > > > virtualization which would use vfio/mdev to be able to allocate arbitrary > > > resources on mdev instance. More info on [1] [2] [3]. > > > > > > To allow to create user defined resources for mdev, it trys to extend mdev > > > create interface by adding new "instances=xxx" parameter following uuid, > > > for > > > target mdev type if aggregation is supported, it can create new mdev > > > device > > > which contains resources combined by number of instances, e.g > > > > > > echo ",instances=10" > create > > > > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute > > > which > > > can support this setting. If no "aggregation" attribute found for mdev > > > type, > > > previous behavior is still kept for one instance allocation. And new sysfs > > > attribute "instances" is created for each mdev device to show allocated > > > number. > > > > > > This trys to create new KVMGT type with minimal vGPU resources which can > > > be > > > combined with "instances=x" setting to allocate for user wanted > > > resources. > > > > "instances" makes me think this is arg helps to create multiple mdev > > instances rather than consuming multiple instances for a single mdev. > > You're already exposing the "aggregation" attribute, so doesn't > > "aggregate" perhaps make more sense as the create option? We're asking > > the driver to aggregate $NUM instances into a single mdev. The mdev > > attribute should then perhaps also be "aggregated_instances". I agree, that seems like a better naming scheme. (...) > > I'm curious what libvirt folks and Kirti think of this, it looks like > > it has a nice degree of backwards compatibility, both in the sysfs > > interface and the vendor driver interface. Thanks, > > Since libvirt doesn't have an API to create mdevs yet, this doesn't pose an > issue for us at the moment. I see this adds new optional sysfs attributes > which > we could expose within our device capabilities XML, provided it doesn't use a > free form text, like the description attribute does. 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 be easier if the "create" attribute always accepted ",instances=1" (calling the .create ops in that case?) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: fix virtType FromString check
In the domain definition, virtType is virDomainVirtType which is
unsigned, so comparing against -1 doesn't work. Cast to int first
Fixes 8e2982b5767
Signed-off-by: Cole Robinson
---
This regressed in the past as well, when virtType was changed to
virDomainVirtType in 7383b8cc068, fixed by the follow up 5e06a4f063.
It's strange that virDomainVirtType is unsigned but VirtTypeFromString
can return -1... it should probably either work like virArch, or we
should switch virDomainVirtType to int in the DomainDef, but that
requires fixing a few other places too
src/conf/domain_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c27c874d9e..30806d1e59 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19140,7 +19140,7 @@ virDomainDefParseCaps(virDomainDefPtr def,
"%s", _("missing domain type attribute"));
goto cleanup;
}
-if ((def->virtType = virDomainVirtTypeFromString(virttype)) < 0) {
+if ((int)(def->virtType = virDomainVirtTypeFromString(virttype)) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("invalid domain type %s"), virttype);
goto cleanup;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: don't use virDomainVirtType in struct field
Use of enum types for struct fields is generally avoided since it causes
warnings if the compiler assumes the enum is unsigned. For example
commit 8e2982b5767a25e5da6533c65bfdc648c95b3c69
Author: Cole Robinson
Date: Tue Jul 24 16:27:54 2018 -0400
conf: Clean up virDomainDefParseCaps
Introduced a line:
if ((def->virtType = virDomainVirtTypeFromString(virttype)) < 0) {
which causes a build failure with CLang
conf/domain_conf.c:19143:65: error: comparison of unsigned enum expression <
0 is always false [-Werror,-Wtautological-compare]
as the compiler is free to optimize away the "< 0" check due to the
assumption that the enum type is unsigned and always in range.
Signed-off-by: Daniel P. Berrangé
---
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 2 +-
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_process.c | 4 +++-
4 files changed, 6 insertions(+), 4 deletions(-)
Pushing as a build fix for CLang
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c27c874d9e..f94a90fbcc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15075,7 +15075,7 @@ virDomainVideoDefaultRAM(const virDomainDef *def,
int
virDomainVideoDefaultType(const virDomainDef *def)
{
-switch (def->virtType) {
+switch ((virDomainVirtType)def->virtType) {
case VIR_DOMAIN_VIRT_TEST:
case VIR_DOMAIN_VIRT_XEN:
if (def->os.type == VIR_DOMAIN_OSTYPE_XEN ||
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a804e86f6c..c1dfa37fdf 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2386,7 +2386,7 @@ struct _virDomainVirtioOptions {
typedef struct _virDomainDef virDomainDef;
typedef virDomainDef *virDomainDefPtr;
struct _virDomainDef {
-virDomainVirtType virtType;
+int virtType; /* enum virDomainVirtType */
int id;
unsigned char uuid[VIR_UUID_BUFLEN];
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ae45c45b7f..d148db90fa 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7163,7 +7163,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
virCommandAddArg(cmd, "-machine");
virBufferAdd(&buf, def->os.machine, -1);
-switch (def->virtType) {
+switch ((virDomainVirtType)def->virtType) {
case VIR_DOMAIN_VIRT_QEMU:
virBufferAddLit(&buf, ",accel=tcg");
break;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 27bd8b9465..c4e33723d1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7191,6 +7191,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
virCapsPtr caps = NULL;
bool active = false;
+virDomainVirtType virtType;
VIR_DEBUG("Beginning VM attach process");
@@ -7342,8 +7343,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
goto exit_monitor;
if (qemuMonitorGetStatus(priv->mon, &running, &reason) < 0)
goto exit_monitor;
-if (qemuMonitorGetVirtType(priv->mon, &vm->def->virtType) < 0)
+if (qemuMonitorGetVirtType(priv->mon, &virtType) < 0)
goto exit_monitor;
+vm->def->virtType = virtType;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto error;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_monitor: Fix regression in getting disk capacity
On Thu, Jul 26, 2018 at 04:39:54PM +0200, Michal Privoznik wrote: In dbf990fd31e8 the qemuMonitorJSONBlockStatsUpdateCapacityOne() was split. However, due to a bug the return value was never set to something meaningful. Signed-off-by: Michal Privoznik --- src/qemu/qemu_monitor_json.c | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu_monitor: Fix regression in getting disk capacity
In dbf990fd31e8 the qemuMonitorJSONBlockStatsUpdateCapacityOne()
was split. However, due to a bug the return value was never set
to something meaningful.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_monitor_json.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 75d0738b5d..9acf62e0bb 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2492,14 +2492,15 @@
qemuMonitorJSONBlockStatsUpdateCapacityOne(virJSONValuePtr image,
goto cleanup;
if (backingChain &&
-(backing = virJSONValueObjectGetObject(image, "backing-image"))) {
-ret = qemuMonitorJSONBlockStatsUpdateCapacityOne(backing,
- dev_name,
- depth + 1,
- stats,
- true);
-}
+(backing = virJSONValueObjectGetObject(image, "backing-image")) &&
+qemuMonitorJSONBlockStatsUpdateCapacityOne(backing,
+ dev_name,
+ depth + 1,
+ stats,
+ true) < 0)
+goto cleanup;
+ret = 0;
cleanup:
VIR_FREE(entry_name);
return ret;
--
2.16.4
--
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, 26 Jul 2018 15:50:28 +0200 Erik Skultety wrote: > On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote: > > On Fri, 20 Jul 2018 10:19:24 +0800 > > Zhenyu Wang wrote: > > > > > Current mdev device create interface depends on fixed mdev type, which > > > get uuid > > > from user to create instance of mdev device. If user wants to use > > > customized > > > number of resource for mdev device, then only can create new mdev type > > > for that > > > which may not be flexible. This requirement comes not only from to be > > > able to > > > allocate flexible resources for KVMGT, but also from Intel scalable IO > > > virtualization which would use vfio/mdev to be able to allocate arbitrary > > > resources on mdev instance. More info on [1] [2] [3]. > > > > > > To allow to create user defined resources for mdev, it trys to extend mdev > > > create interface by adding new "instances=xxx" parameter following uuid, > > > for > > > target mdev type if aggregation is supported, it can create new mdev > > > device > > > which contains resources combined by number of instances, e.g > > > > > > echo ",instances=10" > create > > > > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute > > > which > > > can support this setting. If no "aggregation" attribute found for mdev > > > type, > > > previous behavior is still kept for one instance allocation. And new sysfs > > > attribute "instances" is created for each mdev device to show allocated > > > number. > > > > > > This trys to create new KVMGT type with minimal vGPU resources which can > > > be > > > combined with "instances=x" setting to allocate for user wanted > > > resources. > > > > "instances" makes me think this is arg helps to create multiple mdev > > instances rather than consuming multiple instances for a single mdev. > > You're already exposing the "aggregation" attribute, so doesn't > > "aggregate" perhaps make more sense as the create option? We're asking > > the driver to aggregate $NUM instances into a single mdev. The mdev > > attribute should then perhaps also be "aggregated_instances". > > > > The next user question for the interface might be what aspect of the > > device gets multiplied by this aggregation? In i915 I see you're > > multiplying the memory sizes by the instance, but clearly the > > resolution doesn't change. I assume this is sort of like mdev types > > themselves, ie. some degree of what a type means is buried in the > > implementation and some degree of what some number of those types > > aggregated together means is impossible to describe generically. > > I don't seem to clearly see the benefit here, so I have to ask, how is this > better and/or different from allowing a heterogeneous setup if one needs a > more > performant instance in terms of more resources? Because to me, once you're > able > to aggregate instances, I would assume that a simple "echo `uuid`" with a > different type should succeed as well and provide me (from user's perspective) > with the same results. Could you please clarify this to me, as well as what > resources/parameters are going to be impacted by aggregation? I think you're suggesting that we could simply define new mdev types to account for these higher aggregate instances, for example we can define discrete types that are 2x, 3x, 4x, etc. the resource count of a single instance. What I think we're trying to address with this proposal is what happens when the resources available are exceptionally large and they can be combined in arbitrary ways. For example if a parent device can expose 10,000 resources and the granularity with which we can create and mdev instance is 1, is it practical to create 10,000 mdev types or does it make more sense to expose a granularity and method of aggregation. Using graphics here perhaps falls a little short of the intention of the interface because the possible types are easily enumerable and it would be entirely practical to create discrete types for each. vGPUs also have a lot of variables, so defining which attribute of the device is multiplied by the number of instances is a little more fuzzy. Thanks, Alex -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Fix typo in error msg of virDomainVideoDefValidate
On Thu, Jul 26, 2018 at 09:52:46PM +0800, Han Han wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1607825
>
> Fix typo of error msg when 'none' type is not the only video device
> defined in VM.
>
> Signed-off-by: Han Han
> ---
I tweaked the commit message and pushed.
Thanks,
Erik
> src/conf/domain_conf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 33b0b4ab68..c2ccbcca91 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5687,7 +5687,7 @@ virDomainVideoDefValidate(const virDomainVideoDef
> *video,
> if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_NONE &&
> def->nvideos > 1) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("a '%s' video type must be the only video
> device "
> + _("a 'none' video type must be the only video
> device "
> "defined for the domain"));
> return -1;
> }
> --
> 2.18.0
>
> --
> 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 3/7] tests: qemuhotplug: Fix segfault when XML loading fails
On 07/26/2018 02:44 AM, Michal Privoznik wrote:
> On 07/24/2018 11:23 PM, Cole Robinson wrote:
>> Some tests use the same VM state multiple times in a row. But if we
>> failed loading the VM XML, subsequent tests crash on the NULL def
>> pointer
>>
>> Signed-off-by: Cole Robinson
>> ---
>> I hit this with failing tests while writing this series
>>
>> tests/qemuhotplugtest.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
>> index 674ba92b27..4f9e127f88 100644
>> --- a/tests/qemuhotplugtest.c
>> +++ b/tests/qemuhotplugtest.c
>> @@ -268,6 +268,8 @@ testQemuHotplug(const void *data)
>>
>> if (test->vm) {
>> vm = test->vm;
>> +if (!vm->def)
>> +goto cleanup;
>> } else {
>> if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_xml,
>> test->deviceDeletedEvent) < 0)
>>
>
> I wonder if we should fprintf(stderr, "test skipped because of an
> failure in dependant test"); or something among these lines. The idea
> being it's easier to debug. Look at all places where we 'goto cleanup'.
> At least libvirt error is reported there (virAsprintf reports OOM,
> virTestLoadFile reports error too, etc.).
>
I'll use VIR_TEST_VERBOSE() which will give similarish behavior to
virReportError in this case: output will only be shown if
VIR_TEST_DEBUG=1 or similar is used
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH v2 2/2] Add syntax-check target in Makefile.am
On Thu, Jul 26, 2018 at 03:20:49PM +0200, Katerina Koukiou wrote: For now syntax checking is performed only on the python files using flake8. E501: (line too long) warning is ignored. Signed-off-by: Katerina Koukiou --- HACKING.md | 9 - Makefile.am | 6 ++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/HACKING.md b/HACKING.md index bb22fd6..9327959 100644 --- a/HACKING.md +++ b/HACKING.md @@ -29,7 +29,7 @@ Running from git repository ``` - * Before posting a patch, you should run tests: + * Before posting a patch, you should run tests and perform syntax-checking: ``` make check @@ -37,6 +37,13 @@ Running from git repository The test tool requires python3, python3-pytest and python3-dbus. +``` +make syntax-check +``` + +Syntax checking currently is applied only on the python code. s/currently is/is currently/ or just drop the line completely, that way it won't need updating. +To run this flake8 package is required. + * To run libvirt-dbus directly from the build dir without installing it use the run script: diff --git a/Makefile.am b/Makefile.am index 2c9e588..5c95e96 100644 --- a/Makefile.am +++ b/Makefile.am @@ -42,3 +42,9 @@ gen-AUTHORS: mv -f $(distdir)/AUTHORS-tmp $(distdir)/AUTHORS && \ rm -f all.list maint.list contrib.list; \ fi + I'd add the list of ignored warnings here: # E501: (line too long) warning is ignored. +.PHONY: flake8 +flake8: + flake8 --ignore=E501 Maybe add --show-source as well. + +syntax-check: flake8 Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH v2 1/2] tests: fix all coding style issues to comply with flake8
On Thu, Jul 26, 2018 at 03:20:48PM +0200, Katerina Koukiou wrote: Signed-off-by: Katerina Koukiou --- tests/libvirttest.py| 9 + tests/test_connect.py | 2 +- tests/test_domain.py| 5 +++-- tests/test_interface.py | 12 +++- tests/test_network.py | 8 tests/test_storage.py | 1 + 6 files changed, 21 insertions(+), 16 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: Fix typo in error msg of virDomainVideoDefValidate
https://bugzilla.redhat.com/show_bug.cgi?id=1607825
Fix typo of error msg when 'none' type is not the only video device
defined in VM.
Signed-off-by: Han Han
---
src/conf/domain_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 33b0b4ab68..c2ccbcca91 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5687,7 +5687,7 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_NONE &&
def->nvideos > 1) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("a '%s' video type must be the only video device "
+ _("a 'none' video type must be the only video
device "
"defined for the domain"));
return -1;
}
--
2.18.0
--
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 Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote: > On Fri, 20 Jul 2018 10:19:24 +0800 > Zhenyu Wang wrote: > > > Current mdev device create interface depends on fixed mdev type, which get > > uuid > > from user to create instance of mdev device. If user wants to use customized > > number of resource for mdev device, then only can create new mdev type for > > that > > which may not be flexible. This requirement comes not only from to be able > > to > > allocate flexible resources for KVMGT, but also from Intel scalable IO > > virtualization which would use vfio/mdev to be able to allocate arbitrary > > resources on mdev instance. More info on [1] [2] [3]. > > > > To allow to create user defined resources for mdev, it trys to extend mdev > > create interface by adding new "instances=xxx" parameter following uuid, for > > target mdev type if aggregation is supported, it can create new mdev device > > which contains resources combined by number of instances, e.g > > > > echo ",instances=10" > create > > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute > > which > > can support this setting. If no "aggregation" attribute found for mdev type, > > previous behavior is still kept for one instance allocation. And new sysfs > > attribute "instances" is created for each mdev device to show allocated > > number. > > > > This trys to create new KVMGT type with minimal vGPU resources which can be > > combined with "instances=x" setting to allocate for user wanted resources. > > "instances" makes me think this is arg helps to create multiple mdev > instances rather than consuming multiple instances for a single mdev. > You're already exposing the "aggregation" attribute, so doesn't > "aggregate" perhaps make more sense as the create option? We're asking > the driver to aggregate $NUM instances into a single mdev. The mdev > attribute should then perhaps also be "aggregated_instances". > > The next user question for the interface might be what aspect of the > device gets multiplied by this aggregation? In i915 I see you're > multiplying the memory sizes by the instance, but clearly the > resolution doesn't change. I assume this is sort of like mdev types > themselves, ie. some degree of what a type means is buried in the > implementation and some degree of what some number of those types > aggregated together means is impossible to describe generically. I don't seem to clearly see the benefit here, so I have to ask, how is this better and/or different from allowing a heterogeneous setup if one needs a more performant instance in terms of more resources? Because to me, once you're able to aggregate instances, I would assume that a simple "echo `uuid`" with a different type should succeed as well and provide me (from user's perspective) with the same results. Could you please clarify this to me, as well as what resources/parameters are going to be impacted by aggregation? ... > > I'm curious what libvirt folks and Kirti think of this, it looks like > it has a nice degree of backwards compatibility, both in the sysfs > interface and the vendor driver interface. Thanks, Since libvirt doesn't have an API to create mdevs yet, this doesn't pose an issue for us at the moment. I see this adds new optional sysfs attributes which we could expose within our device capabilities XML, provided it doesn't use a free form text, like the description attribute does. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] news: Usb and sata for virsh attach-disk --address
Signed-off-by: Han Han --- docs/news.xml | 10 ++ 1 file changed, 10 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index fc7924ad5f..a68ef2dc1c 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -76,6 +76,16 @@ using attach-disk or attach-interface commands. + + + virsh: Support usb and sata address to attach-disk + + + Usb or sata address could be used when attach-disk with --address. + For example, use usb address as usb:. , use + sata address as . . . + + -- 2.18.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/7] conf: Sync caps data even when SKIP_OSTYPE_CHECKS
On 07/26/2018 02:44 AM, Michal Privoznik wrote:
> On 07/24/2018 11:23 PM, Cole Robinson wrote:
>> We should still make an effort to fill in data, just not raise
>> an error if say an ostype/virttype combo disappeared from caps.
>>
>> Signed-off-by: Cole Robinson
>> ---
>> src/conf/domain_conf.c | 13 ++---
>> tests/qemuxml2argvdata/missing-machine.xml | 2 +-
>> tests/qemuxml2argvtest.c | 3 +++
>> 3 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index b7f6a22e20..78ee000857 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -19178,18 +19178,17 @@ virDomainDefParseCaps(virDomainDefPtr def,
>> goto cleanup;
>> }
>>
>> -if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) {
>> -if (!(capsdata = virCapabilitiesDomainDataLookup(caps,
>> -def->os.type, def->os.arch, def->virtType,
>> -NULL, NULL)))
>> +if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type,
>> +def->os.arch, def->virtType, NULL, NULL))) {
>
> This looks misaligned ;-)
>
>> +if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))
>> goto cleanup;
>
>
> So you're changing the flag here even though I believe it belongs to the
> next patch. It helps downstream maintainers, but in the end the code
> will look the same.
>
Good catch, mistake on my part. I'll fix before pushing. Thanks for the
review!
- Cole
>> -
>> +virResetLastError();
>> +} else {
>> if (!def->os.arch)
>> def->os.arch = capsdata->arch;
>> if ((!def->os.machine &&
>> - VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) {
>> + VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0))
>> goto cleanup;
>> -}
>> }
>>
>> ret = 0;
>> diff --git a/tests/qemuxml2argvdata/missing-machine.xml
>> b/tests/qemuxml2argvdata/missing-machine.xml
>> index 4ce7b377a5..2900baec90 100644
>> --- a/tests/qemuxml2argvdata/missing-machine.xml
>> +++ b/tests/qemuxml2argvdata/missing-machine.xml
>> @@ -6,7 +6,7 @@
>>219100
>>1
>>
>> -hvm
>> +hvm
>
> Firstly I was wondering why is this change needed, but then I read the
> comment in the next hunk and it makes sense. We need to have
> non-existent pair of arch and os type so that the error is triggered.
>
>>
>>
>>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index 1a936faef1..03b6d92912 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -2773,6 +2773,9 @@ mymain(void)
>> QEMU_CAPS_OBJECT_GPEX,
>> QEMU_CAPS_NEC_USB_XHCI);
>>
>> +/* VM XML has invalid arch/ostype/virttype combo, but the SKIP flag
>> + * will avoid the error. Still, we expect qemu driver to complain about
>> + * missing machine error, and not crash */
>> DO_TEST_PARSE_FLAGS_ERROR("missing-machine",
>>VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS,
>>NONE);
>>
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/9] util: Add MBA allocation to virresctrl
On 07/26/2018 02:00 AM, bing.niu wrote:
>
>
> On 2018年07月26日 06:37, John Ferlan wrote:
>>
>>
>> On 07/18/2018 03:57 AM, bing@intel.com wrote:
>>> 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 | 346
>>> --
>>> src/util/virresctrl.h | 13 ++
>>> 2 files changed, 346 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>>> index 06e2702..bec2afd 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 feature.
>>
>> s/feature/the future/
>
> Will change.
>>
>>> */
>>>
> []
>>> 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
>>> @@ -275,6 +302,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);
>>
>> NIT: Could be inside the if condition.
>
> OK~
>>
>>> VIR_FREE(alloc->id);
>>> VIR_FREE(alloc->path);
>>> VIR_FREE(alloc->levels);
>>> @@ -697,6 +731,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];
>>> @@ -890,6 +927,27 @@ virResctrlAllocSetCacheSize(virResctrlAllocPtr
>>> alloc,
>>> int
>>> +virResctrlAllocForeachMemory(virResctrlAllocPtr alloc,
>>> + virResctrlAllocForeachMemoryCallback cb,
>>> + void *opaque)
>>> +{
>>> + size_t i = 0;
>>> +
>>> + if (!alloc)
>>> + return 0;
>>> +
>>> + if (alloc->mem_bw) {
>>> + virResctrlAllocMemBWPtr mem_bw = alloc->mem_bw;
>>> + for (i = 0; i < mem_bw->nbandwidths; i++)
>>> + if (mem_bw->bandwidths[i])
>>> + cb(i, *mem_bw->bandwidths[i], opaque);
>>
>> @cb is an int function, but only ever used in what amounts to a void
>> function in patch8 (virDomainMemorytuneDefFormatHelper - although
>> defined as int, it only ever returns 0, so it could be void too).
>>
>> So either this changes to handle that or we change the *Callback to be
>> void. Do you have a preference?
>
> Let's add a return value testing in patch8.
> I think we should change cachetune virDomainCachetuneDefFormatHelper
> part also. Since it also doesn't check return value of
> virResctrlAllocForeachCache.
>
> IMO, Giving a return value of function is always a good practice. ;)
> It's also give some spaces for the future extension. If logic inside @cb
> became complex. :)
>
Yeah, typically those Foreach type function want to return something
such that we stop processing in the event of some failure, no need to
keep going and pile on the errors or have some unexpected success!
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +
>>> +int
>>> virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
>>> virResctrlAllocForeachCacheCallback cb,
>>> void *opaque)
>>> @@ -952,6 +1010,240 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)
>>> }
>>>
>>
>> The next hunk of changes has perhaps more to do with the functionality
>> of the code vs. the pure alloc/free of the structures. I think that
>> alloc/free needs to be split from Parse/Format - it'll make things
>> easier. Then there's Parse of XML vs Parse of the schemata for the MB:
>> lines which just jumbles things (in my mind ;-))
>>
>> This particular change may even be a 3rd patch... Keep reading ;-)
>
> Yes. I just realize put ev
[libvirt] [dbus PATCH v2 2/2] Add syntax-check target in Makefile.am
For now syntax checking is performed only on the python files using flake8. E501: (line too long) warning is ignored. Signed-off-by: Katerina Koukiou --- HACKING.md | 9 - Makefile.am | 6 ++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/HACKING.md b/HACKING.md index bb22fd6..9327959 100644 --- a/HACKING.md +++ b/HACKING.md @@ -29,7 +29,7 @@ Running from git repository ``` - * Before posting a patch, you should run tests: + * Before posting a patch, you should run tests and perform syntax-checking: ``` make check @@ -37,6 +37,13 @@ Running from git repository The test tool requires python3, python3-pytest and python3-dbus. +``` +make syntax-check +``` + +Syntax checking currently is applied only on the python code. +To run this flake8 package is required. + * To run libvirt-dbus directly from the build dir without installing it use the run script: diff --git a/Makefile.am b/Makefile.am index 2c9e588..5c95e96 100644 --- a/Makefile.am +++ b/Makefile.am @@ -42,3 +42,9 @@ gen-AUTHORS: mv -f $(distdir)/AUTHORS-tmp $(distdir)/AUTHORS && \ rm -f all.list maint.list contrib.list; \ fi + +.PHONY: flake8 +flake8: + flake8 --ignore=E501 + +syntax-check: flake8 -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dbus PATCH v2 1/2] tests: fix all coding style issues to comply with flake8
Signed-off-by: Katerina Koukiou
---
tests/libvirttest.py| 9 +
tests/test_connect.py | 2 +-
tests/test_domain.py| 5 +++--
tests/test_interface.py | 12 +++-
tests/test_network.py | 8
tests/test_storage.py | 1 +
6 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/tests/libvirttest.py b/tests/libvirttest.py
index 7a75ff3..776b12d 100644
--- a/tests/libvirttest.py
+++ b/tests/libvirttest.py
@@ -107,7 +107,6 @@ class BaseTestClass():
path =
interface_obj.StorageVolCreateXML(xmldata.minimal_storage_vol_xml, 0)
yield path
-
def get_test_domain(self):
path = self.connect.ListDomains(0)[0]
obj = self.bus.get_object('org.libvirt', path)
@@ -196,7 +195,7 @@ class DomainEventStoppedDetailType(IntEnum):
SHUTDOWN = 0
DESTROYED = 1
CRASHED = 2
-MIGRATED = 3
+MIGRATED = 3
SAVED = 4
FAILED = 5
FROM_SNAPSHOT = 6
@@ -205,9 +204,9 @@ class DomainEventStoppedDetailType(IntEnum):
class DomainEventSuspendedDetailType(IntEnum):
PAUSED = 0
-MIGRATED = 1
+MIGRATED = 1
IOERROR = 2
-WATCHDOG = 3
+WATCHDOG = 3
RESTORED = 4
FROM_SNAPSHOT = 5
API_ERROR = 6
@@ -240,10 +239,12 @@ class NetworkEvent(IntEnum):
STARTED = 2
STOPPED = 3
+
class NodeDeviceEvent(IntEnum):
CREATED = 0
DELETED = 1
+
class StoragePoolBuildFlags(IntEnum):
NEW = 0
REPAIR = 1
diff --git a/tests/test_connect.py b/tests/test_connect.py
index bb2d767..f481356 100755
--- a/tests/test_connect.py
+++ b/tests/test_connect.py
@@ -96,7 +96,7 @@ class TestConnect(libvirttest.BaseTestClass):
def test_connect_interface_lookup_by_property(self, lookup_method_name,
lookup_item, interface_create):
"""Parameterized test for all InterfaceLookupBy* API calls of Connect
interface
"""
-original_path,_ = interface_create
+original_path, _ = interface_create
obj = self.bus.get_object('org.libvirt', original_path)
prop = obj.Get('org.libvirt.Interface', lookup_item,
dbus_interface=dbus.PROPERTIES_IFACE)
path = getattr(self.connect, lookup_method_name)(prop)
diff --git a/tests/test_domain.py b/tests/test_domain.py
index b9a6d33..b5879b4 100755
--- a/tests/test_domain.py
+++ b/tests/test_domain.py
@@ -5,6 +5,7 @@ import libvirttest
DBUS_EXCEPTION_MISSING_FUNCTION = 'this function is not supported by the
connection driver'
+
class TestDomain(libvirttest.BaseTestClass):
def test_api(self):
obj, domain = self.get_test_domain()
@@ -153,8 +154,8 @@ class TestDomain(libvirttest.BaseTestClass):
def test_domain_vcpu_pin_info(self):
obj, domain = self.get_test_domain()
pinInfo_expected = [
-[ True, True, True, True, True, True, True, True ],
-[ True, True, True, True, True, True, True, True ]
+[True, True, True, True, True, True, True, True],
+[True, True, True, True, True, True, True, True]
]
pinInfo = domain.GetVcpuPinInfo(0)
assert pinInfo == pinInfo_expected
diff --git a/tests/test_interface.py b/tests/test_interface.py
index 7fd9dbf..3f6b513 100755
--- a/tests/test_interface.py
+++ b/tests/test_interface.py
@@ -4,38 +4,40 @@ import dbus
import libvirttest
import pytest
+
@pytest.mark.usefixtures("interface_create")
class TestInterface(libvirttest.BaseTestClass):
""" Tests for methods and properties of the Interface interface
"""
def test_interface_undefine(self, interface_create):
-_,interface_obj = interface_create
+_, interface_obj = interface_create
interface_obj.Destroy(0)
interface_obj.Undefine()
def test_interface_destroy(self, interface_create):
-_,interface_obj = interface_create
+_, interface_obj = interface_create
interface_obj.Destroy(0)
def test_interface_create(self, interface_create):
-_,interface_obj = interface_create
+_, interface_obj = interface_create
interface_obj.Destroy(0)
interface_obj.Create(0)
def test_interface_get_xml_description(self, interface_create):
-_,interface_obj = interface_create
+_, interface_obj = interface_create
assert isinstance(interface_obj.GetXMLDesc(0), dbus.String)
def test_interface_properties_type(self, interface_create):
""" Ensure correct return type for Interface properties
"""
-test_interface_path,_ = interface_create
+test_interface_path, _ = interface_create
obj = self.bus.get_object('org.libvirt', test_interface_path)
props = obj.GetAll('org.libvirt.Interface',
dbus_interface=dbus.PROPERTIES_IFACE)
assert isinstance(props['Name'], dbus.String)
assert isinstance(props['MAC'], dbus.String)
assert isinstance(props['Active'], dbus.Boolean)
+
if __name__ == '__main__':
libvirt
[libvirt] [dbus PATCH v2 0/2] Make correct use of setup fixtures & cleanup
diff from v1: * fixtures patch merged * fixed reminder flake8 issues * added patch introducing syntax-check target in the Makefile Katerina Koukiou (2): tests: fix all coding style issues to comply with flake8 Add syntax-check target in Makefile.am HACKING.md | 9 - Makefile.am | 6 ++ tests/libvirttest.py| 9 + tests/test_connect.py | 2 +- tests/test_domain.py| 5 +++-- tests/test_interface.py | 12 +++- tests/test_network.py | 8 tests/test_storage.py | 1 + 8 files changed, 35 insertions(+), 17 deletions(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 2/2] tests: fix all coding style issues to comply with pep8
On Thu, Jul 26, 2018 at 02:39:37PM +0200, Ján Tomko wrote: > On Thu, Jul 26, 2018 at 02:09:58PM +0200, Katerina Koukiou wrote: > > Is there nothing that could be said here in the commit message? > > Also, it might be a good time to introduce a 'syntax-check' target > to Makefile, to make sure these won't get merged again. OK, I 'll add syntax check with flake8, with reminders and repost. 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 2/2] tests: fix all coding style issues to comply with pep8
On Thu, Jul 26, 2018 at 02:09:58PM +0200, Katerina Koukiou wrote: Is there nothing that could be said here in the commit message? Also, it might be a good time to introduce a 'syntax-check' target to Makefile, to make sure these won't get merged again. Signed-off-by: Katerina Koukiou --- tests/test_connect.py | 2 +- tests/test_domain.py| 5 +++-- tests/test_interface.py | 11 ++- tests/test_network.py | 8 tests/test_storage.py | 1 + 5 files changed, 15 insertions(+), 12 deletions(-) diff --git a/tests/test_domain.py b/tests/test_domain.py index b9a6d33..96b282c 100755 --- a/tests/test_domain.py +++ b/tests/test_domain.py @@ -5,6 +5,7 @@ import libvirttest DBUS_EXCEPTION_MISSING_FUNCTION = 'this function is not supported by the connection driver' + class TestDomain(libvirttest.BaseTestClass): def test_api(self): obj, domain = self.get_test_domain() @@ -153,8 +154,8 @@ class TestDomain(libvirttest.BaseTestClass): def test_domain_vcpu_pin_info(self): obj, domain = self.get_test_domain() pinInfo_expected = [ -[ True, True, True, True, True, True, True, True ], -[ True, True, True, True, True, True, True, True ] +[True, True, True, True, True, True, True, True], +[True, True, True, True, True, True, True, True] The 'flake8 --ignore=E501' command you proposed months ago [0] still shows some issues, including: ./tests/test_domain.py:157:17: E126 continuation line over-indented for hanging indent [True, True, True, True, True, True, True, True], ^ Jano [0] https://www.redhat.com/archives/libvir-list/2018-March/msg01368.html signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 1/2] tests: {interface, node_device}_create should be used as setup fixtures
On Thu, Jul 26, 2018 at 02:09:57PM +0200, Katerina Koukiou wrote: * node_device_create was a fixture but we were calling it as normal function, thus it got triggered twice. * interface_create was not a fixture. This patch makes sure that the setup work these two functions are doing, will run before the actual test call phase. Signed-off-by: Katerina Koukiou --- tests/libvirttest.py| 1 + tests/test_connect.py | 9 + tests/test_interface.py | 22 -- tests/test_nodedev.py | 16 4 files changed, 26 insertions(+), 22 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dbus PATCH 2/2] tests: fix all coding style issues to comply with pep8
Signed-off-by: Katerina Koukiou
---
tests/test_connect.py | 2 +-
tests/test_domain.py| 5 +++--
tests/test_interface.py | 11 ++-
tests/test_network.py | 8
tests/test_storage.py | 1 +
5 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/tests/test_connect.py b/tests/test_connect.py
index bb2d767..f481356 100755
--- a/tests/test_connect.py
+++ b/tests/test_connect.py
@@ -96,7 +96,7 @@ class TestConnect(libvirttest.BaseTestClass):
def test_connect_interface_lookup_by_property(self, lookup_method_name,
lookup_item, interface_create):
"""Parameterized test for all InterfaceLookupBy* API calls of Connect
interface
"""
-original_path,_ = interface_create
+original_path, _ = interface_create
obj = self.bus.get_object('org.libvirt', original_path)
prop = obj.Get('org.libvirt.Interface', lookup_item,
dbus_interface=dbus.PROPERTIES_IFACE)
path = getattr(self.connect, lookup_method_name)(prop)
diff --git a/tests/test_domain.py b/tests/test_domain.py
index b9a6d33..96b282c 100755
--- a/tests/test_domain.py
+++ b/tests/test_domain.py
@@ -5,6 +5,7 @@ import libvirttest
DBUS_EXCEPTION_MISSING_FUNCTION = 'this function is not supported by the
connection driver'
+
class TestDomain(libvirttest.BaseTestClass):
def test_api(self):
obj, domain = self.get_test_domain()
@@ -153,8 +154,8 @@ class TestDomain(libvirttest.BaseTestClass):
def test_domain_vcpu_pin_info(self):
obj, domain = self.get_test_domain()
pinInfo_expected = [
-[ True, True, True, True, True, True, True, True ],
-[ True, True, True, True, True, True, True, True ]
+[True, True, True, True, True, True, True, True],
+[True, True, True, True, True, True, True, True]
]
pinInfo = domain.GetVcpuPinInfo(0)
assert pinInfo == pinInfo_expected
diff --git a/tests/test_interface.py b/tests/test_interface.py
index bcda8d5..38a1cc1 100755
--- a/tests/test_interface.py
+++ b/tests/test_interface.py
@@ -4,33 +4,34 @@ import dbus
import libvirttest
import pytest
+
@pytest.mark.usefixtures("interface_create")
class TestInterface(libvirttest.BaseTestClass):
""" Tests for methods and properties of the Interface interface
"""
def test_interface_undefine(self, interface_create):
-_,interface_obj = interface_create
+_, interface_obj = interface_create
interface_obj.Destroy(0)
interface_obj.Undefine()
def test_interface_destroy(self, interface_create):
-_,interface_obj = interface_create
+_, interface_obj = interface_create
interface_obj.Destroy(0)
def test_interface_create(self, interface_create):
-_,interface_obj = interface_create
+_, interface_obj = interface_create
interface_obj.Destroy(0)
interface_obj.Create(0)
def test_interface_get_xml_description(self, interface_create):
-_,interface_obj = interface_create
+_, interface_obj = interface_create
assert isinstance(interface_obj.GetXMLDesc(0), dbus.String)
def test_interface_properties_type(self, interface_create):
""" Ensure correct return type for Interface properties
"""
-test_interface_path,_ = interface_create
+test_interface_path, _ = interface_create
obj = self.bus.get_object('org.libvirt', test_interface_path)
props = obj.GetAll('org.libvirt.Interface',
dbus_interface=dbus.PROPERTIES_IFACE)
assert isinstance(props['Name'], dbus.String)
diff --git a/tests/test_network.py b/tests/test_network.py
index 11418cb..2e3a8f0 100755
--- a/tests/test_network.py
+++ b/tests/test_network.py
@@ -25,7 +25,7 @@ class TestNetwork(libvirttest.BaseTestClass):
assert isinstance(props['UUID'], dbus.String)
def test_network_autostart(self):
-_,test_network = self.get_test_network()
+_, test_network = self.get_test_network()
interface_obj = dbus.Interface(test_network, 'org.libvirt.Network')
autostart_expected = True
interface_obj.Set('org.libvirt.Network', 'Autostart',
autostart_expected, dbus_interface=dbus.PROPERTIES_IFACE)
@@ -41,7 +41,7 @@ class TestNetwork(libvirttest.BaseTestClass):
self.connect.connect_to_signal('NetworkEvent', domain_started)
-_,test_network = self.get_test_network()
+_, test_network = self.get_test_network()
interface_obj = dbus.Interface(test_network, 'org.libvirt.Network')
interface_obj.Destroy()
interface_obj.Create()
@@ -64,7 +64,7 @@ class TestNetwork(libvirttest.BaseTestClass):
self.main_loop()
def test_network_get_xml_description(self):
-_,test_network = self.get_test_network()
+_, test_network = self.get_test_network()
interface_obj = dbus.Interface(test_network, 'org.libvirt.Network'
[libvirt] [dbus PATCH 1/2] tests: {interface, node_device}_create should be used as setup fixtures
* node_device_create was a fixture but we were calling it as normal function,
thus it got triggered twice.
* interface_create was not a fixture.
This patch makes sure that the setup work these two functions are doing, will
run before the actual test call phase.
Signed-off-by: Katerina Koukiou
---
tests/libvirttest.py| 1 +
tests/test_connect.py | 9 +
tests/test_interface.py | 22 --
tests/test_nodedev.py | 16
4 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/tests/libvirttest.py b/tests/libvirttest.py
index 9db6e0a..7a75ff3 100644
--- a/tests/libvirttest.py
+++ b/tests/libvirttest.py
@@ -71,6 +71,7 @@ class BaseTestClass():
if self.timeout:
raise TimeoutError()
+@pytest.fixture
def interface_create(self):
""" Fixture to define dummy interface on the test driver
diff --git a/tests/test_connect.py b/tests/test_connect.py
index 042c568..bb2d767 100755
--- a/tests/test_connect.py
+++ b/tests/test_connect.py
@@ -88,14 +88,15 @@ class TestConnect(libvirttest.BaseTestClass):
path = self.connect.InterfaceDefineXML(xmldata.minimal_interface_xml,
0)
assert isinstance(path, dbus.ObjectPath)
+@pytest.mark.usefixtures("interface_create")
@pytest.mark.parametrize("lookup_method_name,lookup_item", [
("InterfaceLookupByName", 'Name'),
("InterfaceLookupByMAC", 'MAC'),
])
-def test_connect_interface_lookup_by_property(self, lookup_method_name,
lookup_item):
+def test_connect_interface_lookup_by_property(self, lookup_method_name,
lookup_item, interface_create):
"""Parameterized test for all InterfaceLookupBy* API calls of Connect
interface
"""
-original_path,_ = self.interface_create()
+original_path,_ = interface_create
obj = self.bus.get_object('org.libvirt', original_path)
prop = obj.Get('org.libvirt.Interface', lookup_item,
dbus_interface=dbus.PROPERTIES_IFACE)
path = getattr(self.connect, lookup_method_name)(prop)
@@ -164,10 +165,10 @@ class TestConnect(libvirttest.BaseTestClass):
@pytest.mark.parametrize("lookup_method_name,lookup_item", [
("NodeDeviceLookupByName", 'Name'),
])
-def test_connect_node_device_lookup_by_property(self, lookup_method_name,
lookup_item):
+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 = self.node_device_create()
+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)
diff --git a/tests/test_interface.py b/tests/test_interface.py
index 9503eef..bcda8d5 100755
--- a/tests/test_interface.py
+++ b/tests/test_interface.py
@@ -2,33 +2,35 @@
import dbus
import libvirttest
+import pytest
+@pytest.mark.usefixtures("interface_create")
class TestInterface(libvirttest.BaseTestClass):
""" Tests for methods and properties of the Interface interface
"""
-def test_interface_undefine(self):
-_,interface_obj = self.interface_create()
+def test_interface_undefine(self, interface_create):
+_,interface_obj = interface_create
interface_obj.Destroy(0)
interface_obj.Undefine()
-def test_interface_destroy(self):
-_,interface_obj = self.interface_create()
+def test_interface_destroy(self, interface_create):
+_,interface_obj = interface_create
interface_obj.Destroy(0)
-def test_interface_create(self):
-_,interface_obj = self.interface_create()
+def test_interface_create(self, interface_create):
+_,interface_obj = interface_create
interface_obj.Destroy(0)
interface_obj.Create(0)
-def test_interface_get_xml_description(self):
-_,interface_obj = self.interface_create()
+def test_interface_get_xml_description(self, interface_create):
+_,interface_obj = interface_create
assert isinstance(interface_obj.GetXMLDesc(0), dbus.String)
-def test_interface_properties_type(self):
+def test_interface_properties_type(self, interface_create):
""" Ensure correct return type for Interface properties
"""
-test_interface_path,_ = self.interface_create()
+test_interface_path,_ = interface_create
obj = self.bus.get_object('org.libvirt', test_interface_path)
props = obj.GetAll('org.libvirt.Interface',
dbus_interface=dbus.PROPERTIES_IFACE)
assert isinstance(props['Name'], dbus.String)
diff --git a/tests/test_nodedev.py b/tests/test_nodedev.py
index 6aa6211..22c8d60 100755
--- a/tests/test_nodedev.py
+++ b/tests/test_nodedev.py
@@ -
[libvirt] [dbus PATCH 0/2] Make correct use of setup fixtures & cleanup
Katerina Koukiou (2):
tests: {interface, node_device}_create should be used as setup
fixtures
tests: fix all coding style issues to comply with pep8
tests/libvirttest.py| 1 +
tests/test_connect.py | 9 +
tests/test_domain.py| 5 +++--
tests/test_interface.py | 23 +--
tests/test_network.py | 8
tests/test_nodedev.py | 16
tests/test_storage.py | 1 +
7 files changed, 35 insertions(+), 28 deletions(-)
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390
On Thu, 2018-07-26 at 12:52 +0100, Daniel P. Berrangé wrote: > On Thu, Jul 26, 2018 at 01:47:52PM +0200, Andrea Bolognani wrote: > > On Thu, 2018-07-26 at 12:22 +0100, Daniel P. Berrangé wrote: > > > From the libvirt side we must avoid any scenario where QEMU auto-adds > > > devices behind our back. If adding a device requires adding a controller > > > libvirt must do this explicitly and record it in the XML. > > > > Definitely. My question was whether the corresponding zpci device > > should be created as well... > > I'm not sure I understand it fully, but it sounds like zpci devices are > providing info that is guest ABI sensitive, which would mean libvirt must > control and record it. So from that POV we should create zpci devices Making sure the guest-visible address is stable is taken care of by the additional PCI address attributes uid and fid, which are then used when creating the zpci device. PCI controllers such as pci-bridge are, however, not visible to the guest and as such it would be possible to skip the corresponding zpci devices; it's been argued, though, that it's better to create them regardless in order to maintain consistency. -- 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 RESEND 00/12] PCI passthrough support on s390
On Thu, Jul 26, 2018 at 01:47:52PM +0200, Andrea Bolognani wrote: > On Thu, 2018-07-26 at 12:22 +0100, Daniel P. Berrangé wrote: > > On Thu, Jul 26, 2018 at 07:17:03PM +0800, Yi Min Zhao wrote: > > > 在 2018/7/26 下午7:00, Andrea Bolognani 写道: > > > > From the test cases I see a zpci devices, with its own uid and fid, > > > > is created for the pci-bridge as well... Is that intentional? > > > > > > Firstly pci bridge can be auto-generated if a pci device is to be plugged > > > to > > > non-existing pci bus. > > > IIUC, pci-bridge is treated as a controller device in libvirt. So I think, > > > it's pretty readable not only > > > in libvirt xml but also in qtree, if we assign zpci device for it. > > > Otherwise > > > address type of pci-bridge > > > is pci type but has no uid and fid. Isn't it odd? > > Everything about zPCI is odd ;) > > I guess there's no harm in creating an additional zpci device, > and as you say it will keep things a bit more consistent, which > is good. > > > From the libvirt side we must avoid any scenario where QEMU auto-adds > > devices behind our back. If adding a device requires adding a controller > > libvirt must do this explicitly and record it in the XML. > > Definitely. My question was whether the corresponding zpci device > should be created as well... I'm not sure I understand it fully, but it sounds like zpci devices are providing info that is guest ABI sensitive, which would mean libvirt must control and record it. So from that POV we should create zpci devices 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
