Re: [libvirt] [PATCH 0/2] Fix handling of disk's WWN (world wide name)
On Mon, Apr 13, 2015 at 15:22:36 +0200, Martin Kletzander wrote: > On Tue, Apr 07, 2015 at 04:10:47PM +0200, Peter Krempa wrote: > >Peter Krempa (2): > > conf: ABI: Check WWN in disk abi stability check > > qemu: Enforce WWN to be unique among VM's disks > > > > ACK series. Pushed; Thanks. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/9] storage: Refactor iSCSI Source matching
On Mon, Apr 13, 2015 at 15:35:46 -0400, John Ferlan wrote: > > > On 04/13/2015 05:27 AM, Peter Krempa wrote: > > On Thu, Apr 02, 2015 at 13:39:38 -0400, John Ferlan wrote: > >> Create a separate iSCSI Source matching subroutine. Makes the calling > >> code a bit cleaner as well as sets up for future patches which need to > >> do better source hosts[0].name processing/checking > >> > >> Signed-off-by: John Ferlan > >> --- > >> src/conf/storage_conf.c | 35 --- > >> 1 file changed, 24 insertions(+), 11 deletions(-) > >> > >> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > >> index 8b1898b..4a38416 100644 > >> --- a/src/conf/storage_conf.c > >> +++ b/src/conf/storage_conf.c > >> @@ -2291,6 +2291,28 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, > >> } > >> > >> > >> +static bool > >> +matchISCSISource(virStoragePoolObjPtr matchpool, > > > > Please use the virStorageConf... prefix for the new function. > > > > How about "virStoragePoolSourceISCSIMatch" Sounds good to me. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] qemu: Fix condition for checking vcpu when pinning vcpus
On 04/07/2015 02:50 PM, Peter Krempa wrote:
> Previously we checked that the vcpu we are trying to set is in range of
> the number of threads presented by qemu. The problem is that if the VM
> is offline the count is 0. Since the condition subtracted 1 from the
> count the number would overflow and the check would never trigger.
>
> Change the condition for more sensible ones with specific error
> messages.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1208434
> ---
> src/qemu/qemu_driver.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
ah yes, I remember pondering this code while working through the
IOThreads pinning code.
ACK
John
BTW: As with the add/del IOThreads code - this is yet another one of
those places where using [n]vcpupids caused me to make a [n]iothreadpids
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6132674..9c6b905 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5084,10 +5084,17 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
>
> priv = vm->privateData;
>
> -if (vcpu > (priv->nvcpupids-1)) {
> +if ((flags & VIR_DOMAIN_AFFECT_LIVE) && vcpu >= vm->def->vcpus) {
> virReportError(VIR_ERR_INVALID_ARG,
> - _("vcpu number out of range %d > %d"),
> - vcpu, priv->nvcpupids - 1);
> + _("vcpu %d is out of range of live cpu count %d"),
> + vcpu, vm->def->vcpus);
> +goto endjob;
> +}
> +
> +if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && vcpu >= persistentDef->vcpus) {
> +virReportError(VIR_ERR_INVALID_ARG,
> + _("vcpu %d is out of range of persistent cpu count
> %d"),
> + vcpu, persistentDef->vcpus);
> goto endjob;
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] conf: Refactor virDomainVcpuPinDefParseXML
On 04/07/2015 02:50 PM, Peter Krempa wrote: > Refactor the code to parse the vcpupin in a similar way the iothreadpin > code is now structured. This allows to get rid of some very strange > conditions and error messages. > > Additionally since a existing bug > ( https://bugzilla.redhat.com/show_bug.cgi?id=1208434 ) allows to add > vcpupin definitions for vcpus that don't exist, this patch makes the > parser to ignore all vcpupins that don't have a matching vCPU in the > definition rather than just offlined ones. > --- > src/conf/domain_conf.c | 33 - > 1 file changed, 12 insertions(+), 21 deletions(-) > Hmm - oh I see... The deleted vcpuid >= maxvcpus check disappearing was the cause of the above mentioned bug... and of course there's a duplicated check later on... ACK, John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] conf: Error out if iothread id is missing in iothreadpin
On 04/07/2015 02:50 PM, Peter Krempa wrote: > Defining a domain with the following config: > > > ... > 1 > > > > will result in the following config formatted back: > > ... > 1 > > > > After restart the VM would vanish. Since our schema requires the > @iothread field to be present in make it required by the > code too. > --- > src/conf/domain_conf.c | 44 > 1 file changed, 24 insertions(+), 20 deletions(-) > ACK John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] conf: Split out parsing of emulatorpin
On 04/07/2015 02:50 PM, Peter Krempa wrote: > Split up parts of virDomainVcpuPinDefParseXML into > virDomainEmulatorPinDefParseXML. > --- > src/conf/domain_conf.c | 50 > +- > 1 file changed, 37 insertions(+), 13 deletions(-) > ACK John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] conf: Split up virDomainVcpuPinDefParseXML
On 04/07/2015 02:50 PM, Peter Krempa wrote:
> Extract part that parses iothreads into virDomainIothreadPinDefParseXML
> ---
> src/conf/domain_conf.c | 112
> +
> 1 file changed, 66 insertions(+), 46 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ec7f9c9..10ec17a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13153,30 +13153,19 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt,
> return idmap;
> }
>
> -/* Parse the XML definition for a vcpupin or emulatorpin.
> +/* Parse the XML definition for a vcpupin
[set bikeshed=flog... technically the above would go in patch 1, but
I'm not concerned due to where this is headed... same in about 4 lines ]
> *
> * vcpupin has the form of
> *
> - *
> - * and emulatorpin has the form of
> - *
> - *
> - * and an iothreadspin has the form
> - *
> - *
> - * A vcpuid of -1 is valid and only valid for emulatorpin. So callers
> - * have to check the returned cpuid for validity.
> */
> static virDomainPinDefPtr
> virDomainVcpuPinDefParseXML(xmlNodePtr node,
> xmlXPathContextPtr ctxt,
> -int maxvcpus,
> -bool iothreads)
> +int maxvcpus)
> {
> virDomainPinDefPtr def;
> xmlNodePtr oldnode = ctxt->node;
> int vcpuid = -1;
> -unsigned int iothreadid;
> char *tmp = NULL;
> int ret;
>
> @@ -13185,28 +13174,66 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node,
>
> ctxt->node = node;
>
> -if (!iothreads) {
> -ret = virXPathInt("string(./@vcpu)", ctxt, &vcpuid);
> -if ((ret == -2) || (vcpuid < -1)) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("vcpu id must be an unsigned integer or -1"));
> -goto error;
> -} else if (vcpuid == -1) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("vcpu id value -1 is not allowed for vcpupin"));
> -goto error;
> -}
> +ret = virXPathInt("string(./@vcpu)", ctxt, &vcpuid);
> +if ((ret == -2) || (vcpuid < -1)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("vcpu id must be an unsigned integer or -1"));
> +goto error;
> +} else if (vcpuid == -1) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("vcpu id value -1 is not allowed for vcpupin"));
> +goto error;
> +}
>
> -if (vcpuid >= maxvcpus) {
> +if (vcpuid >= maxvcpus) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("vcpu id must be less than maxvcpus"));
> +goto error;
> +}
> +
> +def->id = vcpuid;
> +
> +if (!(tmp = virXMLPropString(node, "cpuset"))) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("vcpu id must be less than maxvcpus"));
> -goto error;
> -}
> + _("missing cpuset for vcpupin"));
>
> -def->id = vcpuid;
> +goto error;
> }
>
> -if (iothreads && (tmp = virXPathString("string(./@iothread)", ctxt))) {
> +if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
> +goto error;
> +
> + cleanup:
> +VIR_FREE(tmp);
> +ctxt->node = oldnode;
> +return def;
> +
> + error:
> +VIR_FREE(def);
> +goto cleanup;
> +}
> +
> +
> +/* Parse the XML definition for a iothreadpin
> + * and an iothreadspin has the form
> + *
> + */
> +static virDomainPinDefPtr
> +virDomainIothreadPinDefParseXML(xmlNodePtr node,
> +xmlXPathContextPtr ctxt,
> +int iothreads)
s/Iothread/IOThread/
ACK with this
John
> +{
> +virDomainPinDefPtr def;
> +xmlNodePtr oldnode = ctxt->node;
> +unsigned int iothreadid;
> +char *tmp = NULL;
> +
> +if (VIR_ALLOC(def) < 0)
> +return NULL;
> +
> +ctxt->node = node;
> +
> +if ((tmp = virXPathString("string(./@iothread)", ctxt))) {
> if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) {
> virReportError(VIR_ERR_XML_ERROR,
> _("invalid setting for iothread '%s'"), tmp);
> @@ -13220,11 +13247,9 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node,
> goto error;
> }
>
> -/* NB: maxvcpus is actually def->iothreads
> - * IOThreads are numbered "iothread1...iothread", where
> - * "n" is the iothreads value
> - */
> -if (iothreadid > maxvcpus) {
> +/* IOThreads are numbered "iothread1...iothread", where
> + * "n" is the iothreads value */
> +if (iothreadid > iothreads) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("iothread id must not exceed iothreads"))
Re: [libvirt] [PATCH] lib: snapshot: Explain that only one layer of images is inserted
On 04/10/2015 04:55 AM, Peter Krempa wrote: > When creating a snapshot with _REUSE_EXTERNAL when the pre-created image > does not directly link to the current active layer libvirt would > re-detect the backing chain incorrectly and it would not match with > qemu's view. Since the configuration is an operator mistake, document > that only the top layer image gets inserted. > --- > src/libvirt-domain-snapshot.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > ACK John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 7/7] qemu: Refactor qemuDomainBlockJobAbort()
On 04/09/2015 12:45 PM, Peter Krempa wrote:
> Change few variable names and refactor the code flow. As an additional
> bonus the function now fails if the event state is not as expected.
> ---
>
> Notes:
> Version 3:
> - fixed error reporting code and success code propagation from pivot
>
> src/qemu/qemu_driver.c | 107
> +++--
> 1 file changed, 51 insertions(+), 56 deletions(-)
>
ACK 1-6...
Just one question below...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index eebed55..8d4aa97 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16273,13 +16273,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> {
> virQEMUDriverPtr driver = dom->conn->privateData;
> char *device = NULL;
> -virObjectEventPtr event = NULL;
> -virObjectEventPtr event2 = NULL;
> virDomainDiskDefPtr disk;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> bool save = false;
> int idx;
> -bool async;
> +bool modern;
> +bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT);
> +bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC);
> virDomainObjPtr vm;
> int ret = -1;
>
> @@ -16292,7 +16292,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0)
> goto cleanup;
>
> -if (qemuDomainSupportsBlockJobs(vm, &async) < 0)
> +if (qemuDomainSupportsBlockJobs(vm, &modern) < 0)
> goto cleanup;
>
> if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> @@ -16308,19 +16308,14 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> goto endjob;
> disk = vm->def->disks[idx];
>
> -if (async && !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) {
> -/* prepare state for event delivery */
> +if (modern && !async) {
> +/* prepare state for event delivery. Since qemuDomainBlockPivot is
> + * synchronous, but the event is delivered asynchronously we need to
> + * wait too */
> disk->blockJobStatus = -1;
> disk->blockJobSync = true;
> }
>
> -if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) &&
> -!(async && disk->mirror)) {
> -virReportError(VIR_ERR_OPERATION_INVALID,
> - _("pivot of disk '%s' requires an active copy job"),
> - disk->dst);
> -goto endjob;
> -}
> if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE &&
> disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> @@ -16329,31 +16324,31 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> goto endjob;
> }
>
> -if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) {
> -ret = qemuDomainBlockPivot(driver, vm, device, disk);
> -if (ret < 0 && async) {
> +if (pivot) {
> +if ((ret = qemuDomainBlockPivot(driver, vm, device, disk)) < 0) {
> disk->blockJobSync = false;
> goto endjob;
> }
> -goto waitjob;
> -}
> -if (disk->mirror) {
> -disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT;
> -save = true;
> -}
> +} else {
> +if (disk->mirror) {
> +disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT;
> +save = true;
> +}
>
> -qemuDomainObjEnterMonitor(driver, vm);
> -ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, async);
> -if (qemuDomainObjExitMonitor(driver, vm) < 0)
> -ret = -1;
> +qemuDomainObjEnterMonitor(driver, vm);
> +ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device,
> modern);
> +if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> +ret = -1;
> +goto endjob;
> +}
should this just fall through now? Asked differently - should
disk->mirrorState change before goto endjob if ExitMonitor fails?
Would "if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)" do the
trick?
ACK - in general - just making sure something wasn't missed.
John
>
> -if (ret < 0) {
> -if (disk->mirror)
> -disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
> -goto endjob;
> +if (ret < 0) {
> +if (disk->mirror)
> +disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
> +goto endjob;
> +}
> }
>
> - waitjob:
> /* If we have made changes to XML due to a copy job, make a best
> * effort to save it now. But we can ignore failure, since there
> * will be further changes when the event marks completion. */
> @@ -16368,33 +16363,37 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> * while still holding the VM job, to prevent newly scheduled
> * block jobs from confusing us. */
> if (!async) {
> -/* Older qemu that lacked async re
Re: [libvirt] [PATCH v2 3/9] conf: Add new domain XML element 'iothreadids'
On 04/13/2015 08:13 AM, Peter Krempa wrote:
> On Fri, Apr 10, 2015 at 17:36:21 -0400, John Ferlan wrote:
...
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 95cbb9c..03a0ecd 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2041,6 +2041,19 @@ struct _virDomainHugePage {
>> unsigned long long size;/* hugepage size in KiB */
>> };
>>
>> +typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef;
>> +typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr;
>> +
>> +struct _virDomainIOThreadIDDef {
>> +bool defined;
>> +unsigned int iothread_id;
>> +char *name;
>
> This structure along with the one you add in the next patch and the
> pinning structures that already exist make now three places where we
> store data regarding IO threads. I don't think we should do that.
> Keeping track of which data introduces messy code.
>
> I think it will be desirable that you move the data regarding pinning of
> iothreads into this structure along with thread ids from the monitor so
> that we can keep everything in one place.
>
>
While I don't disagree that having multiple places and data structures
is suboptimal - it is no different than the existing vcpu code which has
a [n]vcpupids list for the monitor/driver stored and separate cputune
vcpupin list. What it doesn't have is a mechanism to have different vcpu
id numbers - it forces sequential. There's also a lot to be said for
keeping the cputune stuff together as much as there is for keeping the
iothreadsid stuff together.
The whole point behind not printing out was
if it doesn't exist, we can continue with the existing algorithm and no
one is the wiser. As soon as someone goes to 'customize' by adding a
non sequential id, then things are exposed. I could care less either
way though. If the general feeling is print it regardless of whether it
was found on input, then that's fine by me - makes it easier.
Another "option" for the (if it was to be kept) is that it
becomes the alias for the thread. The current algorithm just generates
the "iothread#" as a/the mechanism for the alias. An IOThread when
"added" can use any name as long as it's unique.
All that said - I'm fine with removing - it was added mainly
because I felt "id" would be lonely all by itself ;-)... Then moving the
cputune.iothreadpin data into the internal workings for iothreadid's is
fine - just have to account for existing configurations in some manner.
Finally having the "live" information in the same data structure is
fine - I separated it mainly because existing code has it separated. I
didn't particularly like the existing code, but since no one has changed
it for all the years it's been there, well I didn't want that added onto
the "to do" list.
Finally as for some of the extraneous comments - you may in some cases
find them stating obvious facts, I've also seen that what some consider
to be self documenting code isn't in fact self documenting unless you're
the author or have become very familiar with the code due to working on
patches in the space.
I'll rework and repost tomorrow. Good to know this is moving in the
right direction
John
FWIW: In patch 8 where I used // - yes I knew that (the .0 mentioned
it), but I was trying to draw attention to it. The vcpu code doesn't do
much in the way of error recovery and while I could just do the same, I
figured I'd point it out rather than just ignore it. I knew the //
would cause someone to balk.
>> +};
>> +
>> +void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def);
>> +void virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def,
>> + int nids);
>> +
>> typedef struct _virDomainCputune virDomainCputune;
>> typedef virDomainCputune *virDomainCputunePtr;
>>
>
> I think the direction this series is taking is okay.
>
> Peter
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/9] Convert virDomainPinIsDuplicate into bool return
On 04/13/2015 06:33 AM, Peter Krempa wrote: > On Fri, Apr 10, 2015 at 17:36:20 -0400, John Ferlan wrote: >> Signed-off-by: John Ferlan >> --- >> src/conf/domain_conf.c | 8 >> src/conf/domain_conf.h | 6 +++--- >> 2 files changed, 7 insertions(+), 7 deletions(-) > > ACK, by the way, the function is exported, but is used only in > conf/domain_conf.c thus it could be converted to static. > > Peter > OK - I've separated these first two out, but since virDomainPinIsDuplicate is defined after it's used - I think it would be worthy of a separate patch "later" (I have it on my short list) to make it local and of course move it... Unless of course you want to do that in the series you have on list right now dealing with the PinParser... John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 4/8] storage: Use virStoragePoolSourceMatchSingleHost for NETFS
Rather than have duplicate code doing the same check, have the netfs matching processing code use the new virStoragePoolSourceMatchSingleHost. Signed-off-by: John Ferlan --- src/conf/storage_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 313098b..bb89bb7 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2464,9 +2464,9 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, matchpool = pool; break; case VIR_STORAGE_POOL_NETFS: -if ((STREQ(pool->def->source.dir, def->source.dir)) \ -&& (pool->def->source.nhost == 1 && def->source.nhost == 1) \ -&& (STREQ(pool->def->source.hosts[0].name, def->source.hosts[0].name))) +if (STREQ(pool->def->source.dir, def->source.dir) && +virStoragePoolSourceMatchSingleHost(&pool->def->source, +&def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_SCSI: -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 6/8] storage: Add duplicate host check for Sheepdog pool def
Check the proposed pool source host XML definition against existing sheepdog pools to ensure the incoming definition doesn't use the same source host XML definition as an existing pool. Signed-off-by: John Ferlan --- src/conf/storage_conf.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 1fadff4..2b2104d 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2544,9 +2544,13 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, case VIR_STORAGE_POOL_DISK: matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; +case VIR_STORAGE_POOL_SHEEPDOG: +if (virStoragePoolSourceMatchSingleHost(&pool->def->source, +&def->source)) +matchpool = pool; +break; case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: -case VIR_STORAGE_POOL_SHEEPDOG: case VIR_STORAGE_POOL_GLUSTER: case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_LAST: -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 7/8] storage: Add duplicate host check for Gluster pool def
Check the proposed pool source host XML definition against existing gluster pools to ensure the incoming definition doesn't use the same source host XML definition as an existing pool. Signed-off-by: John Ferlan --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 2b2104d..0a50f57 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2545,13 +2545,13 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; case VIR_STORAGE_POOL_SHEEPDOG: +case VIR_STORAGE_POOL_GLUSTER: if (virStoragePoolSourceMatchSingleHost(&pool->def->source, &def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: -case VIR_STORAGE_POOL_GLUSTER: case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_LAST: break; -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 8/8] storage: Add duplicate devices check for zfs pool def
Check proposed pool definitions to ensure they aren't trying to use the same devices as currently defined definitions - disallow the duplicate Signed-off-by: John Ferlan --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 0a50f57..9e7c575 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2542,6 +2542,7 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, case VIR_STORAGE_POOL_FS: case VIR_STORAGE_POOL_LOGICAL: case VIR_STORAGE_POOL_DISK: +case VIR_STORAGE_POOL_ZFS: matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; case VIR_STORAGE_POOL_SHEEPDOG: @@ -2552,7 +2553,6 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, break; case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: -case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_LAST: break; } -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/8] storage: Create virStoragePoolSourceMatchSingleHost
Split out the nhost == 1 and hosts[0].name logic into a separate routine
Signed-off-by: John Ferlan
---
src/conf/storage_conf.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 351eea8..f609f85 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2405,6 +2405,16 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
return false;
}
+static bool
+virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc,
+virStoragePoolSourcePtr defsrc)
+{
+if (poolsrc->nhost != 1 && defsrc->nhost != 1)
+return false;
+
+return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name);
+}
+
static bool
virStoragePoolSourceISCSIMatch(virStoragePoolObjPtr matchpool,
@@ -2413,10 +2423,7 @@ virStoragePoolSourceISCSIMatch(virStoragePoolObjPtr
matchpool,
virStoragePoolSourcePtr poolsrc = &matchpool->def->source;
virStoragePoolSourcePtr defsrc = &def->source;
-if (poolsrc->nhost != 1 && defsrc->nhost != 1)
-return false;
-
-if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name))
+if (!virStoragePoolSourceMatchSingleHost(poolsrc, defsrc))
return false;
if (STRNEQ_NULLABLE(poolsrc->initiator.iqn, defsrc->initiator.iqn))
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 5/8] storage: Remove default from switch in virStoragePoolSourceFindDuplicate
So that we can cover all the cases.
Signed-off-by: John Ferlan
---
src/conf/storage_conf.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index bb89bb7..1fadff4 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2458,7 +2458,7 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn,
virStoragePoolObjLock(pool);
-switch (pool->def->type) {
+switch ((virStoragePoolType)pool->def->type) {
case VIR_STORAGE_POOL_DIR:
if (STREQ(pool->def->target.path, def->target.path))
matchpool = pool;
@@ -2544,7 +2544,12 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn,
case VIR_STORAGE_POOL_DISK:
matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def);
break;
-default:
+case VIR_STORAGE_POOL_MPATH:
+case VIR_STORAGE_POOL_RBD:
+case VIR_STORAGE_POOL_SHEEPDOG:
+case VIR_STORAGE_POOL_GLUSTER:
+case VIR_STORAGE_POOL_ZFS:
+case VIR_STORAGE_POOL_LAST:
break;
}
virStoragePoolObjUnlock(pool);
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/8] storage: Add check for different ports for host duplicate matching
In virStoragePoolSourceMatchSingleHost, add a comparison for port number being different prior to checking the 'name' field. Signed-off-by: John Ferlan --- src/conf/storage_conf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f609f85..313098b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2412,6 +2412,9 @@ virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc, if (poolsrc->nhost != 1 && defsrc->nhost != 1) return false; +if (poolsrc->hosts[0].port != defsrc->hosts[0].port) +return false; + return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name); } -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/8] Duplicate storage pool source refactoring and checks
v1 here: http://www.redhat.com/archives/libvir-list/2015-April/msg00141.html Changes: Patches 1-3 are refactored into just 1 patch now Patch 2 is the old patch 4 and renames the function as requested and removes the comment Patch 3 is new - it adds a check for different port #'s to the new host source matching function. Patch 4 is the old patch 5, already ACK'd and only changed to use the new name Patch 5 is the old patch 6, already ACK'd Patches 6 & 7 are the old patches 7 & 8. They use the new name and the commit comment is adjusted Patch 8 is the old patch 9 and is unchanged, already ACK'd John Ferlan (8): storage: Refactor iSCSI Source matching storage: Create virStoragePoolSourceMatchSingleHost storage: Add check for different ports for host duplicate matching storage: Use virStoragePoolSourceMatchSingleHost for NETFS storage: Remove default from switch in virStoragePoolSourceFindDuplicate storage: Add duplicate host check for Sheepdog pool def storage: Add duplicate host check for Gluster pool def storage: Add duplicate devices check for zfs pool def src/conf/storage_conf.c | 62 - 1 file changed, 46 insertions(+), 16 deletions(-) -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/8] storage: Refactor iSCSI Source matching
Create a separate iSCSI Source matching subroutine. Makes the calling
code a bit cleaner as well as sets up for future patches which need to
do better source hosts[0].name processing/checking.
As part of the effort the logic will be inverted from a multi-level
if statement to a series of single level checks for better readability
and further separation
Signed-off-by: John Ferlan
Signed-off-by: John Ferlan
---
src/conf/storage_conf.c | 33 ++---
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 990a528..351eea8 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2406,6 +2406,26 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
}
+static bool
+virStoragePoolSourceISCSIMatch(virStoragePoolObjPtr matchpool,
+ virStoragePoolDefPtr def)
+{
+virStoragePoolSourcePtr poolsrc = &matchpool->def->source;
+virStoragePoolSourcePtr defsrc = &def->source;
+
+if (poolsrc->nhost != 1 && defsrc->nhost != 1)
+return false;
+
+if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name))
+return false;
+
+if (STRNEQ_NULLABLE(poolsrc->initiator.iqn, defsrc->initiator.iqn))
+return false;
+
+return true;
+}
+
+
int
virStoragePoolSourceFindDuplicate(virConnectPtr conn,
virStoragePoolObjListPtr pools,
@@ -2505,17 +2525,8 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn,
case VIR_STORAGE_POOL_ISCSI:
matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def);
if (matchpool) {
-if (matchpool->def->source.nhost == 1 && def->source.nhost ==
1) {
-if (STREQ(matchpool->def->source.hosts[0].name,
def->source.hosts[0].name)) {
-if ((matchpool->def->source.initiator.iqn) &&
(def->source.initiator.iqn)) {
-if (STREQ(matchpool->def->source.initiator.iqn,
def->source.initiator.iqn))
-break;
-matchpool = NULL;
-}
-break;
-}
-}
-matchpool = NULL;
+if (!virStoragePoolSourceISCSIMatch(matchpool, def))
+matchpool = NULL;
}
break;
case VIR_STORAGE_POOL_FS:
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] storage: Add duplicate host check for Sheepdog pool def
On 04/13/2015 06:23 AM, Peter Krempa wrote: > On Thu, Apr 02, 2015 at 13:39:44 -0400, John Ferlan wrote: >> Check proposed pool definitions to ensure they aren't trying to use the >> same host as currently defined definitions - disallow the duplicate > > This statement is invalid. Multiple pols can be hosted on a single host. > Hmm - brain shorthand... How about: Check the proposed pool source host XML definition against existing sheepdog pools to ensure the incoming definition doesn't use the same source host XML definition as an existing pool. > The check needs to do better than just check the host name. Port and > pool path may differ denoting a different pool. > Hmm.. yes 'port' is something I could add to virStoragePoolSourceMatchSingleHost and it's also extendable to iSCSI... doesn't make sense for NETFS, but would also be usable for gluster I'll squeeze in a patch in order to handle. > Btw same host can be described using multiple host strings so it also > isn't absolute. > Yep... That's where we're trying to get, but it takes a bit to get there! For example, I use "192.168.122.1' for my ' string; however, if I add to /etc/hosts: 192.168.122.1 test1 and then use 'test1' in a different definition - the new code will fail to match, but they are essentially the same thing... There's a bz for that which I'm working to fix, but was trying to avoid a 20 patch series to do so... Gotta start somewhere. John BTW: It gets worse once IPv6 is added into the mix. >> >> Signed-off-by: John Ferlan >> --- >> src/conf/storage_conf.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c >> index 5f1c151..5db7478 100644 >> --- a/src/conf/storage_conf.c >> +++ b/src/conf/storage_conf.c >> @@ -2427,9 +2427,12 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, >> case VIR_STORAGE_POOL_DISK: >> matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); >> break; >> +case VIR_STORAGE_POOL_SHEEPDOG: >> +if (matchPoolSourceHost(&pool->def->source, &def->source)) >> +matchpool = pool; >> +break; > > Peter > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/9] storage: Use matchPoolSourceHost for NETFS
On 04/13/2015 06:19 AM, Peter Krempa wrote: > On Thu, Apr 02, 2015 at 13:39:42 -0400, John Ferlan wrote: >> Rather than have duplicate code doing the same check, have the netfs >> matching processing code use the new matchPoolSourceHost. >> >> Signed-off-by: John Ferlan >> --- >> src/conf/storage_conf.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c >> index b3e930b..6ed0aa9 100644 >> --- a/src/conf/storage_conf.c >> +++ b/src/conf/storage_conf.c >> @@ -2348,9 +2348,8 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, >> matchpool = pool; >> break; >> case VIR_STORAGE_POOL_NETFS: >> -if ((STREQ(pool->def->source.dir, def->source.dir)) \ >> -&& (pool->def->source.nhost == 1 && def->source.nhost == 1) >> \ >> -&& (STREQ(pool->def->source.hosts[0].name, >> def->source.hosts[0].name))) >> +if (STREQ(pool->def->source.dir, def->source.dir) && >> +matchPoolSourceHost(&pool->def->source, &def->source)) >> matchpool = pool; >> break; >> case VIR_STORAGE_POOL_SCSI: > > ACK, no semantic change. (But of course the patch will need a change > after renaming the function. > Right - I'll wait of course for your acceptance of the name I used and adjust from there. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/9] storage: Create matchPoolSourceHost
On 04/13/2015 06:18 AM, Peter Krempa wrote:
> On Thu, Apr 02, 2015 at 13:39:41 -0400, John Ferlan wrote:
>> Split out the nhost == 1 and hosts[0].name logic into a separate routine
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/storage_conf.c | 17 -
>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index e4cb54b..b3e930b 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -2290,6 +2290,17 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
>> return false;
>> }
>>
>> +static bool
>> +matchPoolSourceHost(virStoragePoolSourcePtr poolsrc,
>
> Same compliant for the function name as in 1/9.
>
So how about: virStoragePoolSourceMatchSingleHost
As you probably eventually realized I punted on the multiple host case
of NBD - although it probably needs similar logic, but I was intent on
doing the cases where only one host was used by the backends.
BTW: Where this is headed is we currently only match the host[0] by
string, but that's not good enough for networks where one can use a
'name' or a 'number' (and then all sorts of fun with ipv4 v ipv6).
John
>> +virStoragePoolSourcePtr defsrc)
>> +{
>> +/* NB: nhost cannot be > 1 */
>> +if (poolsrc->nhost == 0 || defsrc->nhost == 0)
>> +return false;
>
> And this condition can be made explicitly state the same without the
> need for the comment.
>
> ACK with the name and condition changed.
>
> Peter
>
So up through this point the diff to master would be:
+static bool
+virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc,
+virStoragePoolSourcePtr defsrc)
+{
+if (poolsrc->nhost != 1 && defsrc->nhost != 1)
+return false;
+
+return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name);
+}
+
+
+static bool
+virStoragePoolSourceISCSIMatch(virStoragePoolObjPtr matchpool,
+ virStoragePoolDefPtr def)
+{
+virStoragePoolSourcePtr poolsrc = &matchpool->def->source;
+virStoragePoolSourcePtr defsrc = &def->source;
+
+if (!virStoragePoolSourceMatchSingleHost(poolsrc, defsrc))
+return false;
+
+if (STRNEQ_NULLABLE(poolsrc->initiator.iqn, defsrc->initiator.iqn))
+return false;
+
+return true;
+}
+
int
virStoragePoolSourceFindDuplicate(virConnectPtr conn,
@@ -2505,17 +2532,8 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn,
case VIR_STORAGE_POOL_ISCSI:
matchpool = virStoragePoolSourceFindDuplicateDevices(pool,
def);
if (matchpool) {
-if (matchpool->def->source.nhost == 1 &&
def->source.nhost == 1) {
-if (STREQ(matchpool->def->source.hosts[0].name,
def->source.hosts[0].name)) {
-if ((matchpool->def->source.initiator.iqn) &&
(def->source.initiator.iqn)) {
-if
(STREQ(matchpool->def->source.initiator.iqn, def->source.initiator.iqn))
-break;
-matchpool = NULL;
-}
-break;
-}
-}
-matchpool = NULL;
+if (!virStoragePoolSourceISCSIMatch(matchpool, def))
+matchpool = NULL;
}
break;
case VIR_STORAGE_POOL_FS:
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/9] storage: Refactor iSCSI Source matching
On 04/13/2015 05:27 AM, Peter Krempa wrote:
> On Thu, Apr 02, 2015 at 13:39:38 -0400, John Ferlan wrote:
>> Create a separate iSCSI Source matching subroutine. Makes the calling
>> code a bit cleaner as well as sets up for future patches which need to
>> do better source hosts[0].name processing/checking
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/storage_conf.c | 35 ---
>> 1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 8b1898b..4a38416 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -2291,6 +2291,28 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
>> }
>>
>>
>> +static bool
>> +matchISCSISource(virStoragePoolObjPtr matchpool,
>
> Please use the virStorageConf... prefix for the new function.
>
How about "virStoragePoolSourceISCSIMatch"
>> + virStoragePoolDefPtr def)
>> +{
>> +if (matchpool->def->source.nhost == 1 && def->source.nhost == 1) {
>> +if (STREQ(matchpool->def->source.hosts[0].name,
>> + def->source.hosts[0].name)) {
>> +if ((matchpool->def->source.initiator.iqn) &&
>> +(def->source.initiator.iqn)) {
>> +if (STREQ(matchpool->def->source.initiator.iqn,
>> + def->source.initiator.iqn))
>> +return true;
>> +else
>> +return false;
>
> Um, how about return STREQ(... ?
myopia, but in the long run it won't matter as I agree with your view to
merge patches 1-3 (something I probably thought along the way but didn't
type as I was trying to show the transformation for at least the review)
John
>
>> +}
>> +return true;
>> +}
>> +}
>> +return false;
>> +}
>> +
>> +
>> int
>> virStoragePoolSourceFindDuplicate(virConnectPtr conn,
>>virStoragePoolObjListPtr pools,
>> @@ -2390,17 +2412,8 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn,
>> case VIR_STORAGE_POOL_ISCSI:
>> matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def);
>> if (matchpool) {
>> -if (matchpool->def->source.nhost == 1 && def->source.nhost
>> == 1) {
>> -if (STREQ(matchpool->def->source.hosts[0].name,
>> def->source.hosts[0].name)) {
>> -if ((matchpool->def->source.initiator.iqn) &&
>> (def->source.initiator.iqn)) {
>> -if (STREQ(matchpool->def->source.initiator.iqn,
>> def->source.initiator.iqn))
>> -break;
>> -matchpool = NULL;
>> -}
>> -break;
>> -}
>> -}
>> -matchpool = NULL;
>> +if (!matchISCSISource(matchpool, def))
>> +matchpool = NULL;
>> }
>> break;
>> case VIR_STORAGE_POOL_FS:
>
> ACK if you rename the function and remove the redundant if.
>
> Peter
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [python PATCH] Implement the DEVICE_ADDED event
On 04/04/2015 01:17 PM, Ján Tomko wrote: > --- > examples/event-test.py | 4 +++ > libvirt-override-virConnect.py | 9 +++ > libvirt-override.c | 57 > ++ > 3 files changed, 70 insertions(+) > ACK - as it seems you've done the correct duplication of the Deleted, but changed references to Added John NOTE: You'll need to do something for libvirt-perl too... -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Emit VIR_DOMAIN_EVENT_ID_DEVICE_ADDED in the QEMU driver
On 04/04/2015 01:16 PM, Ján Tomko wrote:
> Only for devices that have an alias.
> ---
> src/qemu/qemu_driver.c | 17 -
> src/qemu/qemu_hotplug.c | 5 +
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6132674..c13f22b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7586,17 +7586,20 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> {
> virQEMUDriverPtr driver = dom->conn->privateData;
> int ret = -1;
> +const char *alias = NULL;
>
> switch ((virDomainDeviceType) dev->type) {
> case VIR_DOMAIN_DEVICE_DISK:
> qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, -1);
> ret = qemuDomainAttachDeviceDiskLive(dom->conn, driver, vm, dev);
> +alias = dev->data.disk->info.alias;
> if (!ret)
> dev->data.disk = NULL;
> break;
>
> case VIR_DOMAIN_DEVICE_CONTROLLER:
> ret = qemuDomainAttachDeviceControllerLive(driver, vm, dev);
> +alias = dev->data.controller->info.alias;
The one concern I'd have for all of these is - if (ret != 0) - is there
any path that free's anything along the way that you're using a pointer
in the alias fetching?
Additionally of course, since the only way to print the alias is if (ret
== 0) later, one could point out that setting it when ret != 0 is
pointless; however, at least if ret == 0, you should be able to assume
no one as deleted the alias!
Perhaps it's best to only get the alias if (!ret)
Your call if you want to add a "note" for case VIR_DOMAIN_DEVICE_MEMORY
that the event is elicited inside the call since the call consumes
dev->data.memory and hence the alias.
I think with the alias setting inside !ret I'd feel comfortable giving
an ACK - although I suspect in the other case it's not deleted until
after this call exits
John
> if (!ret)
> dev->data.controller = NULL;
> break;
> @@ -7612,6 +7615,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> qemuDomainObjCheckNetTaint(driver, vm, dev->data.net, -1);
> ret = qemuDomainAttachNetDevice(dom->conn, driver, vm,
> dev->data.net);
> +alias = dev->data.net->info.alias;
> if (!ret)
> dev->data.net = NULL;
> break;
> @@ -7620,6 +7624,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> qemuDomainObjCheckHostdevTaint(driver, vm, dev->data.hostdev, -1);
> ret = qemuDomainAttachHostDevice(dom->conn, driver, vm,
> dev->data.hostdev);
> +alias = dev->data.hostdev->info->alias;
> if (!ret)
> dev->data.hostdev = NULL;
> break;
> @@ -7627,6 +7632,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> case VIR_DOMAIN_DEVICE_REDIRDEV:
> ret = qemuDomainAttachRedirdevDevice(driver, vm,
> dev->data.redirdev);
> +alias = dev->data.redirdev->info.alias;
> if (!ret)
> dev->data.redirdev = NULL;
> break;
> @@ -7634,6 +7640,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> case VIR_DOMAIN_DEVICE_CHR:
> ret = qemuDomainAttachChrDevice(driver, vm,
> dev->data.chr);
> +alias = dev->data.chr->info.alias;
> if (!ret)
> dev->data.chr = NULL;
> break;
> @@ -7641,6 +7648,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> case VIR_DOMAIN_DEVICE_RNG:
> ret = qemuDomainAttachRNGDevice(driver, vm,
> dev->data.rng);
> +alias = dev->data.rng->info.alias;
> if (!ret)
> dev->data.rng = NULL;
> break;
> @@ -7673,8 +7681,15 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> break;
> }
>
> -if (ret == 0)
> +if (ret == 0) {
> ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
> +if (alias) {
> +virObjectEventPtr event;
> +event = virDomainEventDeviceAddedNewFromObj(vm, alias);
> +if (event)
> +qemuDomainEventQueue(driver, event);
> +}
> +}
>
> return ret;
> }
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 2f0549e..f07c54d 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1726,6 +1726,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
> char *objalias = NULL;
> const char *backendType;
> virJSONValuePtr props = NULL;
> +virObjectEventPtr event;
> int id;
> int ret = -1;
>
> @@ -1769,6 +1770,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> +event = virDomainEventDeviceAddedNewFromObj(vm, objalias);
> +if (event)
> +qemuDomainEventQueue(driver, event);
> +
Re: [libvirt] [PATCH] configure: Check for libxl_utils.h instead of libxlutil.h
Michal Privoznik wrote: > The file provided by xen-devel package (or xen-tools in Gentoo) > does not provide libxlutil.h. Until recently [1], libxlutil.h was not installed. > In fact the package provides > libxl_utils.h instead which is the one we are looking for anyway. > We are looking for libxlutil.h, which contains the disk parsing functions. See comment near the top of src/xenconfig/xen_xl.c. Regards, Jim [1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=8ff079803677b82195addebc0e88f1630cb7354b > Signed-off-by: Michal Privoznik > --- > configure.ac | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/configure.ac b/configure.ac > index 38fbbad..0626492 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -915,7 +915,7 @@ fi > > if test "$with_libxl" = "yes"; then > dnl If building with libxl, use the libxl utility header and lib too > -AC_CHECK_HEADERS([libxlutil.h]) > +AC_CHECK_HEADERS([libxl_utils.h]) > > LIBXL_LIBS="$LIBXL_LIBS -lxlutil" > AC_DEFINE_UNQUOTED([WITH_LIBXL], 1, [whether libxenlight driver is > enabled]) > if test "x$LIBXL_FIRMWARE_DIR" != "x"; then > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Add VIR_DOMAIN_EVENT_ID_DEVICE_ADDED event
On 04/04/2015 01:16 PM, Ján Tomko wrote:
> The counterpart to VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1206114
> ---
> daemon/remote.c | 37 +++
> include/libvirt/libvirt-domain.h | 18 ++
> src/conf/domain_event.c | 77
>
> src/conf/domain_event.h | 6
> src/libvirt_private.syms | 2 ++
> src/remote/remote_driver.c | 29 +++
> src/remote/remote_protocol.x | 14 +++-
> src/remote_protocol-structs | 6
> tools/virsh-domain.c | 20 +++
> 9 files changed, 208 insertions(+), 1 deletion(-)
>
I searched on VIR_DOMAIN_EVENT_ID_DEVICE - what about
examples/object-events/event-test.c ?
Also should 'src/libvirt-domain.c' have a description for the _ADDED
flag in 'virDomainAttachDeviceFlags' like there is for _REMOVED in
'virDomainDetachDeviceFlags'? (although even that text is a bit shy of
an 'a' - as in "or add a handler for" rather than "or add handler for"
ACK in general for what's here and with the new test and change to
AttachDevice description...
John
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 2e1f973..3a3f168 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -1045,6 +1045,42 @@ remoteRelayDomainEventAgentLifecycle(virConnectPtr
> conn,
> }
>
>
> +static int
> +remoteRelayDomainEventDeviceAdded(virConnectPtr conn,
> + virDomainPtr dom,
> + const char *devAlias,
> + void *opaque)
> +{
> +daemonClientEventCallbackPtr callback = opaque;
> +remote_domain_event_callback_device_added_msg data;
> +
> +if (callback->callbackID < 0 ||
> +!remoteRelayDomainEventCheckACL(callback->client, conn, dom))
> +return -1;
> +
> +VIR_DEBUG("Relaying domain device added event %s %d %s, callback %d",
> + dom->name, dom->id, devAlias, callback->callbackID);
> +
> +/* build return data */
> +memset(&data, 0, sizeof(data));
> +
> +if (VIR_STRDUP(data.devAlias, devAlias) < 0)
> +return -1;
> +
> +make_nonnull_domain(&data.dom, dom);
> +data.callbackID = callback->callbackID,
> +
> +remoteDispatchObjectEventSend(callback->client, remoteProgram,
> +
> REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_ADDED,
> +
> (xdrproc_t)xdr_remote_domain_event_callback_device_added_msg,
> + &data);
> +
> +return 0;
> +}
> +
> +
> +
> +
> static virConnectDomainEventGenericCallback domainEventCallbacks[] = {
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle),
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventReboot),
> @@ -1065,6 +1101,7 @@ static virConnectDomainEventGenericCallback
> domainEventCallbacks[] = {
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockJob2),
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventTunable),
> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventAgentLifecycle),
> +VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceAdded),
> };
>
> verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST);
> diff --git a/include/libvirt/libvirt-domain.h
> b/include/libvirt/libvirt-domain.h
> index 7be4219..8a4fe53 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -3202,6 +3202,23 @@ typedef void
> (*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn,
> void *opaque);
>
> /**
> + * virConnectDomainEventDeviceAddedCallback:
> + * @conn: connection object
> + * @dom: domain on which the event occurred
> + * @devAlias: device alias
> + * @opaque: application specified data
> + *
> + * This callback occurs when a device is added to the domain.
> + *
> + * The callback signature to use when registering for an event of type
> + * VIR_DOMAIN_EVENT_ID_DEVICE_ADDED with virConnectDomainEventRegisterAny()
> + */
> +typedef void (*virConnectDomainEventDeviceAddedCallback)(virConnectPtr conn,
> + virDomainPtr dom,
> + const char
> *devAlias,
> + void *opaque);
> +
> +/**
> * VIR_DOMAIN_TUNABLE_CPU_VCPUPIN:
> *
> * Macro represents formatted pinning for one vcpu specified by id which is
> @@ -3483,6 +3500,7 @@ typedef enum {
> VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2 = 16,/*
> virConnectDomainEventBlockJobCallback */
> VIR_DOMAIN_EVENT_ID_TUNABLE = 17,/*
> virConnectDomainEventTunableCallback */
> VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE = 18,/*
> virConnectDomainEventAgentLifecycleCallback */
> +VIR_DOMAIN_EVENT_ID_DEVICE_ADDED = 19, /*
> virConne
[libvirt] [PATCH] qemu: set macvtap physdevs online when macvtap is set online
A further fix for:
https://bugzilla.redhat.com/show_bug.cgi?id=1113474
Since there is no possibility that any type of macvtap will work if
the parent physdev it's attached to is offline, we should bring the
physdev online at the same time as the macvtap. When taking the
macvtap offline, it's also necessary to take the physdev offline for
macvtap passthrough mode (because the physdev has the same MAC address
as the macvtap device, so could potentially cause problems with
misdirected packets during migration, as outlined in commits 829770
and 879c13). We can't set the physdev offline for other macvtap modes
1) because there may be other macvtap devices attached to the same
physdev in the other modes whereas passthrough mode is exclusive to
one macvtap at a time, and 2) there's no practical reason to do so
anyway.
---
src/qemu/qemu_hotplug.c | 8 +++-
src/qemu/qemu_interface.c | 29 +++--
2 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 2f0549e..9be2ea3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3931,11 +3931,9 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
VIR_WARN("cannot clear bandwidth setting for device : %s",
detach->ifname);
-/* deactivate the tap/macvtap device on the host (currently this
- * isn't necessary, as everything done in
- * qemuInterfaceStopDevice() is made meaningless when the device
- * is deleted anyway, but in the future it may be important, and
- * doesn't hurt anything for now)
+/* deactivate the tap/macvtap device on the host, which could also
+ * affect the parent device (e.g. macvtap passthrough mode sets
+ * the parent device offline)
*/
ignore_value(qemuInterfaceStopDevice(detach));
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index 201a7dd..01226ac 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -63,7 +63,20 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net)
goto cleanup;
}
break;
-case VIR_DOMAIN_NET_TYPE_DIRECT:
+
+case VIR_DOMAIN_NET_TYPE_DIRECT: {
+const char *physdev = virDomainNetGetActualDirectDev(net);
+bool isOnline = true;
+
+/* set the physdev online if necessary. It may already be up,
+ * in which case we shouldn't re-up it just in case that causes
+ * some sort of "blip" in the physdev's status.
+ */
+if (physdev && virNetDevGetOnline(physdev, &isOnline) < 0)
+goto cleanup;
+if (!isOnline && virNetDevSetOnline(physdev, true) < 0)
+goto cleanup;
+
/* macvtap devices share their MAC address with the guest
* domain, and if they are set online prior to the domain CPUs
* being started, the host may send out traffic from this
@@ -79,6 +92,7 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net)
if (virNetDevSetOnline(net->ifname, true) < 0)
goto cleanup;
break;
+}
case VIR_DOMAIN_NET_TYPE_USER:
case VIR_DOMAIN_NET_TYPE_ETHERNET:
@@ -146,7 +160,9 @@ qemuInterfaceStopDevice(virDomainNetDefPtr net)
}
break;
-case VIR_DOMAIN_NET_TYPE_DIRECT:
+case VIR_DOMAIN_NET_TYPE_DIRECT: {
+const char *physdev = virDomainNetGetActualDirectDev(net);
+
/* macvtap interfaces need to be marked !IFF_UP (ie "down") to
* prevent any host-generated traffic sent from this interface
* from putting bad info into the arp caches of other machines
@@ -154,7 +170,16 @@ qemuInterfaceStopDevice(virDomainNetDefPtr net)
*/
if (virNetDevSetOnline(net->ifname, false) < 0)
goto cleanup;
+
+/* also mark the physdev down for passthrough macvtap, as the
+ * physdev has the same MAC address as the macvtap device.
+ */
+if (virDomainNetGetActualDirectMode(net) ==
+VIR_NETDEV_MACVLAN_MODE_PASSTHRU &&
+physdev && virNetDevSetOnline(physdev, false) < 0)
+goto cleanup;
break;
+}
case VIR_DOMAIN_NET_TYPE_USER:
case VIR_DOMAIN_NET_TYPE_ETHERNET:
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Introduce virnetdevtest
On 04/13/2015 11:11 AM, John Ferlan wrote:
>
> On 04/03/2015 08:36 AM, Michal Privoznik wrote:
>> This is yet another test for check of basic functionality of our
>> NIC state handling code.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virnetdev.c | 4 +-
>> src/util/virnetdev.h | 4 ++
>> tests/Makefile.am | 15 +
>> tests/virnetdevmock.c | 48 ++
>> tests/virnetdevtest.c | 94
>> +++
>> tests/virnetdevtestdata/eth0-broken/operstate | 1 +
>> tests/virnetdevtestdata/eth0-broken/speed | 1 +
>> tests/virnetdevtestdata/eth0/operstate| 1 +
>> tests/virnetdevtestdata/eth0/speed| 1 +
>> tests/virnetdevtestdata/lo/operstate | 1 +
>> tests/virnetdevtestdata/lo/speed | 1 +
>> 12 files changed, 170 insertions(+), 2 deletions(-)
>> create mode 100644 tests/virnetdevmock.c
>> create mode 100644 tests/virnetdevtest.c
>> create mode 100644 tests/virnetdevtestdata/eth0-broken/operstate
>> create mode 100644 tests/virnetdevtestdata/eth0-broken/speed
>> create mode 100644 tests/virnetdevtestdata/eth0/operstate
>> create mode 100644 tests/virnetdevtestdata/eth0/speed
>> create mode 100644 tests/virnetdevtestdata/lo/operstate
>> create mode 100644 tests/virnetdevtestdata/lo/speed
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 9f82926..0b42238 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1741,6 +1741,7 @@ virNetDevSetPromiscuous;
>> virNetDevSetRcvAllMulti;
>> virNetDevSetRcvMulti;
>> virNetDevSetupControl;
>> +virNetDevSysfsFile;
>> virNetDevValidateConfig;
>>
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 54d866e..a2d55a8 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -1519,9 +1519,9 @@ int virNetDevValidateConfig(const char *ifname
>> ATTRIBUTE_UNUSED,
>> #ifdef __linux__
>> # define NET_SYSFS "/sys/class/net/"
>>
>> -static int
>> +int
>> virNetDevSysfsFile(char **pf_sysfs_device_link, const char *ifname,
>> - const char *file)
>> + const char *file)
>> {
>>
>> if (virAsprintf(pf_sysfs_device_link, NET_SYSFS "%s/%s", ifname, file)
>> < 0)
>
> Not related specifically to this change, but there seems to be four more
> places which make up their own 'version' of this type of logic - two use
> 'SYSFS_NET_DIR' instead of 'NET_SYSFS' and two use the raw
> '/sys/class/net' path... Might be "nice" to have them use this function
> now too.
Well, the other two uses have parameters that would need to be passed in
printf-style, so that complicates any standard function. Not that I
would mind such a thing, but an in-between solution could be to provide
the #define of the base directory in a .h file, or a function that
returns that string.
>
>
>> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
>> index 856127b..999a89a 100644
>> --- a/src/util/virnetdev.h
>> +++ b/src/util/virnetdev.h
>> @@ -219,4 +219,8 @@ int virNetDevSetRcvAllMulti(const char *ifname, bool
>> receive)
>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>> int virNetDevGetRcvAllMulti(const char *ifname, bool *receive)
>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>> +int virNetDevSysfsFile(char **pf_sysfs_device_link,
>> + const char *ifname,
>> + const char *file)
>> +ATTRIBUTE_NONNULL(1);
> Seems (2) and (3) should be checked as well..
"checked" seems like an incorrect description to me - I thought that the
function of ATTRIBUTE_NONNULL ended up being exactly the opposite of
that - if you mark an arg as ATTRIBUTE_NONNULL then the code within the
function (and more importantly, static analyzers) can assume that the
arg will always be non-null, so no checking is required, i.e. it is a
more of an optimization aid than a debugging aid. And as a matter of
fact, if you turn on optimization in gcc, any code in a function that
checks for NULL in an ATTRIBUTE_NONNULL arg *will be removed* (or at
least it *would have* without the 2nd patch I point out below).
In my experience, ATTRIBUTE_NONNULL has done more to obscure failure to
check for non-null than to point it out:
https://www.redhat.com/archives/libvir-list/2012-April/msg01370.html
although I *think* this patch from 2012 eliminated that failing:
https://www.redhat.com/archives/libvir-list/2012-April/msg01376.html
so maybe I'm blathering on about nothing ;-)
>
> Also, checked current callers and found all have whatever gets used as
> arg (2) and (3) checked for non null, except for one...
>
> The virNetDevGetVirtualFunctionInfo() prototype doesn't make sure that
> it's arg (2) is non-null, but goes ahead and derefs it right away...
>
>> #endi
Re: [libvirt] [PATCH v5 0/3] Parallels disk device attach
On 04/09/2015 01:42 PM, Alexander Burluka wrote: This patchset implements disk device attachment and allows OpenStack to attach volumes to Parallels-driven instances. Parallels Cloud Server SDK supports live attachment of disk devices and virtual interfaces cards. Alexander Burluka (3): Parallels: remove disk serial number check Parallels: implement domainAttachDeviceFlags Parallels: implemented domainAttachDevice ACKED and pushed, thanks! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Introduce virnetdevtest
On 04/03/2015 08:36 AM, Michal Privoznik wrote:
> This is yet another test for check of basic functionality of our
> NIC state handling code.
>
> Signed-off-by: Michal Privoznik
> ---
> src/libvirt_private.syms | 1 +
> src/util/virnetdev.c | 4 +-
> src/util/virnetdev.h | 4 ++
> tests/Makefile.am | 15 +
> tests/virnetdevmock.c | 48 ++
> tests/virnetdevtest.c | 94
> +++
> tests/virnetdevtestdata/eth0-broken/operstate | 1 +
> tests/virnetdevtestdata/eth0-broken/speed | 1 +
> tests/virnetdevtestdata/eth0/operstate| 1 +
> tests/virnetdevtestdata/eth0/speed| 1 +
> tests/virnetdevtestdata/lo/operstate | 1 +
> tests/virnetdevtestdata/lo/speed | 1 +
> 12 files changed, 170 insertions(+), 2 deletions(-)
> create mode 100644 tests/virnetdevmock.c
> create mode 100644 tests/virnetdevtest.c
> create mode 100644 tests/virnetdevtestdata/eth0-broken/operstate
> create mode 100644 tests/virnetdevtestdata/eth0-broken/speed
> create mode 100644 tests/virnetdevtestdata/eth0/operstate
> create mode 100644 tests/virnetdevtestdata/eth0/speed
> create mode 100644 tests/virnetdevtestdata/lo/operstate
> create mode 100644 tests/virnetdevtestdata/lo/speed
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 9f82926..0b42238 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1741,6 +1741,7 @@ virNetDevSetPromiscuous;
> virNetDevSetRcvAllMulti;
> virNetDevSetRcvMulti;
> virNetDevSetupControl;
> +virNetDevSysfsFile;
> virNetDevValidateConfig;
>
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 54d866e..a2d55a8 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1519,9 +1519,9 @@ int virNetDevValidateConfig(const char *ifname
> ATTRIBUTE_UNUSED,
> #ifdef __linux__
> # define NET_SYSFS "/sys/class/net/"
>
> -static int
> +int
> virNetDevSysfsFile(char **pf_sysfs_device_link, const char *ifname,
> - const char *file)
> + const char *file)
> {
>
> if (virAsprintf(pf_sysfs_device_link, NET_SYSFS "%s/%s", ifname, file) <
> 0)
Not related specifically to this change, but there seems to be four more
places which make up their own 'version' of this type of logic - two use
'SYSFS_NET_DIR' instead of 'NET_SYSFS' and two use the raw
'/sys/class/net' path... Might be "nice" to have them use this function
now too.
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 856127b..999a89a 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -219,4 +219,8 @@ int virNetDevSetRcvAllMulti(const char *ifname, bool
> receive)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> int virNetDevGetRcvAllMulti(const char *ifname, bool *receive)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +int virNetDevSysfsFile(char **pf_sysfs_device_link,
> + const char *ifname,
> + const char *file)
> +ATTRIBUTE_NONNULL(1);
Seems (2) and (3) should be checked as well..
Also, checked current callers and found all have whatever gets used as
arg (2) and (3) checked for non null, except for one...
The virNetDevGetVirtualFunctionInfo() prototype doesn't make sure that
it's arg (2) is non-null, but goes ahead and derefs it right away...
> #endif /* __VIR_NETDEV_H__ */
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 046cd08..9ebedc3 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -176,6 +176,7 @@ test_programs = virshtest sockettest \
> domainconftest \
> virhostdevtest \
> vircaps2xmltest \
> + virnetdevtest \
> $(NULL)
>
> if WITH_REMOTE
> @@ -402,6 +403,7 @@ test_libraries = libshunload.la \
> virnetserverclientmock.la \
> vircgroupmock.la \
> virpcimock.la \
> + virnetdevmock.la \
> $(NULL)
> if WITH_QEMU
> test_libraries += libqemumonitortestutils.la \
> @@ -1029,6 +1031,19 @@ virpcimock_la_LIBADD = $(GNULIB_LIBS) \
> virpcimock_la_LDFLAGS = -module -avoid-version \
> -rpath /evil/libtool/hack/to/force/shared/lib/creation
>
> +virnetdevtest_SOURCES = \
> + virnetdevtest.c testutils.h testutils.c
> +virnetdevtest_CFLAGS = $(AM_CFLAGS) $(LIBNL_CFLAGS)
> +virnetdevtest_LDADD = $(LDADDS)
> +
> +virnetdevmock_la_SOURCES = \
> + virnetdevmock.c
> +virnetdevmock_la_CFLAGS = $(AM_CFLAGS) $(LIBNL_CFLAGS)
> +virnetdevmock_la_LIBADD = $(GNULIB_LIBS) \
> +../src/libvirt.la
> +virnetdevmock_la_LDFLAGS = -module -avoid-version \
> +-rpath /evil/libtool/hack/to/force/shared/lib/creation
> +
> if WITH_LINUX
> virusbtest_SOURCES = \
> virusbtest.c testutils.h testutils.c
[libvirt] [libvirt-glib] Support setting of compat XML node
This change adds support for setting of compat XML node in libvirt
gconfig storage volumes target
---
libvirt-gconfig/libvirt-gconfig-storage-vol-target.c | 13 +
libvirt-gconfig/libvirt-gconfig-storage-vol-target.h | 2 ++
libvirt-gconfig/libvirt-gconfig.sym | 5 +
libvirt-gconfig/tests/test-domain-create.c | 1 +
4 files changed, 21 insertions(+)
diff --git a/libvirt-gconfig/libvirt-gconfig-storage-vol-target.c
b/libvirt-gconfig/libvirt-gconfig-storage-vol-target.c
index d3151d1..b72b304 100644
--- a/libvirt-gconfig/libvirt-gconfig-storage-vol-target.c
+++ b/libvirt-gconfig/libvirt-gconfig-storage-vol-target.c
@@ -99,3 +99,16 @@ void
gvir_config_storage_vol_target_set_permissions(GVirConfigStorageVolTarget *
"permissions",
GVIR_CONFIG_OBJECT(perms));
}
+
+/**
+ * gvir_config_storage_vol_target_set_compat:
+ * @compat: (allow-none):
+ */
+void gvir_config_storage_vol_target_set_compat(GVirConfigStorageVolTarget
*target,
+ const char *compat)
+{
+g_return_if_fail(GVIR_CONFIG_IS_STORAGE_VOL_TARGET(target));
+
+gvir_config_object_set_node_content(GVIR_CONFIG_OBJECT(target),
+"compat", compat);
+}
diff --git a/libvirt-gconfig/libvirt-gconfig-storage-vol-target.h
b/libvirt-gconfig/libvirt-gconfig-storage-vol-target.h
index b572381..c165e2b 100644
--- a/libvirt-gconfig/libvirt-gconfig-storage-vol-target.h
+++ b/libvirt-gconfig/libvirt-gconfig-storage-vol-target.h
@@ -67,6 +67,8 @@ void
gvir_config_storage_vol_target_set_format(GVirConfigStorageVolTarget *targe
const char *format);
void gvir_config_storage_vol_target_set_permissions(GVirConfigStorageVolTarget
*target,
GVirConfigStoragePermissions *perms);
+void gvir_config_storage_vol_target_set_compat(GVirConfigStorageVolTarget
*target,
+ const char *compat);
G_END_DECLS
diff --git a/libvirt-gconfig/libvirt-gconfig.sym
b/libvirt-gconfig/libvirt-gconfig.sym
index 8614126..407a52f 100644
--- a/libvirt-gconfig/libvirt-gconfig.sym
+++ b/libvirt-gconfig/libvirt-gconfig.sym
@@ -714,4 +714,9 @@ global:
gvir_config_domain_cpu_set_model;
} LIBVIRT_GCONFIG_0.1.8;
+LIBVIRT_GCONFIG_0.2.0 {
+global:
+ gvir_config_storage_vol_target_set_compat;
+} LIBVIRT_GCONFIG_0.1.9;
+
# define new API here using predicted next version number
diff --git a/libvirt-gconfig/tests/test-domain-create.c
b/libvirt-gconfig/tests/test-domain-create.c
index eb4b945..66f618b 100644
--- a/libvirt-gconfig/tests/test-domain-create.c
+++ b/libvirt-gconfig/tests/test-domain-create.c
@@ -482,6 +482,7 @@ int main(int argc, char **argv)
vol_target = gvir_config_storage_vol_target_new();
gvir_config_storage_vol_target_set_format(vol_target, "qcow2");
gvir_config_storage_vol_target_set_permissions(vol_target, perms);
+gvir_config_storage_vol_target_set_compat(vol_target, "1.1");
g_object_unref(G_OBJECT(perms));
gvir_config_storage_vol_set_target(vol, vol_target);
g_object_unref(G_OBJECT(vol_target));
--
1.9.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-glib] Support setting of compat XML node
This change adds support for setting of compat XML node in libvirt
gconfig storage volumes target
---
libvirt-gconfig/libvirt-gconfig-storage-vol-target.c | 13 +
libvirt-gconfig/libvirt-gconfig-storage-vol-target.h | 2 ++
libvirt-gconfig/libvirt-gconfig.sym | 5 +
libvirt-gconfig/tests/test-domain-create.c | 1 +
4 files changed, 21 insertions(+)
diff --git a/libvirt-gconfig/libvirt-gconfig-storage-vol-target.c
b/libvirt-gconfig/libvirt-gconfig-storage-vol-target.c
index d3151d1..b72b304 100644
--- a/libvirt-gconfig/libvirt-gconfig-storage-vol-target.c
+++ b/libvirt-gconfig/libvirt-gconfig-storage-vol-target.c
@@ -99,3 +99,16 @@ void
gvir_config_storage_vol_target_set_permissions(GVirConfigStorageVolTarget *
"permissions",
GVIR_CONFIG_OBJECT(perms));
}
+
+/**
+ * gvir_config_storage_vol_target_set_compat:
+ * @compat: (allow-none):
+ */
+void gvir_config_storage_vol_target_set_compat(GVirConfigStorageVolTarget
*target,
+ const char *compat)
+{
+g_return_if_fail(GVIR_CONFIG_IS_STORAGE_VOL_TARGET(target));
+
+gvir_config_object_set_node_content(GVIR_CONFIG_OBJECT(target),
+"compat", compat);
+}
diff --git a/libvirt-gconfig/libvirt-gconfig-storage-vol-target.h
b/libvirt-gconfig/libvirt-gconfig-storage-vol-target.h
index b572381..c165e2b 100644
--- a/libvirt-gconfig/libvirt-gconfig-storage-vol-target.h
+++ b/libvirt-gconfig/libvirt-gconfig-storage-vol-target.h
@@ -67,6 +67,8 @@ void
gvir_config_storage_vol_target_set_format(GVirConfigStorageVolTarget *targe
const char *format);
void gvir_config_storage_vol_target_set_permissions(GVirConfigStorageVolTarget
*target,
GVirConfigStoragePermissions *perms);
+void gvir_config_storage_vol_target_set_compat(GVirConfigStorageVolTarget
*target,
+ const char *compat);
G_END_DECLS
diff --git a/libvirt-gconfig/libvirt-gconfig.sym
b/libvirt-gconfig/libvirt-gconfig.sym
index 8614126..407a52f 100644
--- a/libvirt-gconfig/libvirt-gconfig.sym
+++ b/libvirt-gconfig/libvirt-gconfig.sym
@@ -714,4 +714,9 @@ global:
gvir_config_domain_cpu_set_model;
} LIBVIRT_GCONFIG_0.1.8;
+LIBVIRT_GCONFIG_0.2.0 {
+global:
+ gvir_config_storage_vol_target_set_compat;
+} LIBVIRT_GCONFIG_0.1.9;
+
# define new API here using predicted next version number
diff --git a/libvirt-gconfig/tests/test-domain-create.c
b/libvirt-gconfig/tests/test-domain-create.c
index eb4b945..66f618b 100644
--- a/libvirt-gconfig/tests/test-domain-create.c
+++ b/libvirt-gconfig/tests/test-domain-create.c
@@ -482,6 +482,7 @@ int main(int argc, char **argv)
vol_target = gvir_config_storage_vol_target_new();
gvir_config_storage_vol_target_set_format(vol_target, "qcow2");
gvir_config_storage_vol_target_set_permissions(vol_target, perms);
+gvir_config_storage_vol_target_set_compat(vol_target, "1.1");
g_object_unref(G_OBJECT(perms));
gvir_config_storage_vol_set_target(vol, vol_target);
g_object_unref(G_OBJECT(vol_target));
--
1.9.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] XML Parser failing due to cryptic Serial Number.
I set virt-manager in qemu:///system space and tried to add new VM but it
didn't proceed. I figured out its duo to serial numbers is in cryptic form.
# cat /sys/devices/virtual/dmi/id/product_serial
ÿÿÿ
#virt-manager --debug
Traceback (most recent call last):
File "/usr/share/virt-manager/virtManager/libvirtobject.py", line 225, in
_reparse_xml
self._xmlobj = self._build_xmlobj(self._get_raw_xml())
File "/usr/share/virt-manager/virtManager/libvirtobject.py", line 228, in
_build_xmlobj
return self._parseclass(self.conn.get_backend(), parsexml=xml)
File "/usr/share/virt-manager/virtManager/nodedev.py", line 27, in
_parse_convert
return NodeDevice.parse(conn, parsexml)
File "/usr/share/virt-manager/virtinst/nodedev.py", line 95, in parse
tmpdev = NodeDevice(conn, parsexml=xml, allow_node_instantiate=True)
File "/usr/share/virt-manager/virtinst/nodedev.py", line 106, in __init__
XMLBuilder.__init__(self, *args, **kwargs)
File "/usr/share/virt-manager/virtinst/xmlbuilder.py", line 777, in __init__
parent_xpath, relative_object_xpath)
File "/usr/share/virt-manager/virtinst/xmlbuilder.py", line 679, in __init__
self._parse(parsexml, parsexmlnode)
File "/usr/share/virt-manager/virtinst/xmlbuilder.py", line 692, in _parse
doc = libxml2.parseDoc(xml)
File "/usr/lib/python2.7/site-packages/libxml2.py", line 1327, in parseDoc
if ret is None:raise parserError('xmlParseDoc() failed')
libxml2.parserError: xmlParseDoc() failed
[Sun, 12 Apr 2015 06:06:16 virt-manager 4241] DEBUG (create:165) Showing new vm
wizard
[Sun, 12 Apr 2015 06:06:16 virt-manager 4241] DEBUG (create:892) Guest type set
to os_type=hvm, arch=x86_64, dom_type=kvm
[Sun, 12 Apr 2015 06:06:16 virt-manager 4241] DEBUG (xmlbuilder:694) Error
parsing xml=
computer
Vostro
Dell Inc.
A10
ÿÿÿ
REMOVED
Dell Inc.
A10
05/18/2013
[Sun, 12 Apr 2015 06:06:16 virt-manager 4241] ERROR (create:346) Error setting
create wizard conn state.
Traceback (most recent call last):
File "/usr/share/virt-manager/virtManager/create.py", line 344, in reset_state
self.set_conn(activeconn, force_validate=True)
File "/usr/share/virt-manager/virtManager/create.py", line 225, in set_conn
self.set_conn_state()
File "/usr/share/virt-manager/virtManager/create.py", line 626, in
set_conn_state
self.netlist.reset_state()
File "/usr/share/virt-manager/virtManager/netlist.py", line 405, in
reset_state
self._populate_network_list()
File "/usr/share/virt-manager/virtManager/netlist.py", line 253, in
_populate_network_list
vnet_bridges)
File "/usr/share/virt-manager/virtManager/netlist.py", line 185, in
_find_physical_devices
for nodedev in self.conn.get_nodedevs("net"):
File "/usr/share/virt-manager/virtManager/connection.py", line 648, in
get_nodedevs
xmlobj = dev.get_xmlobj()
File "/usr/share/virt-manager/virtManager/libvirtobject.py", line 160, in
get_xmlobj
self._reparse_xml()
File "/usr/share/virt-manager/virtManager/libvirtobject.py", line 225, in
_reparse_xml
self._xmlobj = self._build_xmlobj(self._get_raw_xml())
File "/usr/share/virt-manager/virtManager/libvirtobject.py", line 228, in
_build_xmlobj
return self._parseclass(self.conn.get_backend(), parsexml=xml)
File "/usr/share/virt-manager/virtManager/nodedev.py", line 27, in
_parse_convert
return NodeDevice.parse(conn, parsexml)
File "/usr/share/virt-manager/virtinst/nodedev.py", line 95, in parse
tmpdev = NodeDevice(conn, parsexml=xml, allow_node_instantiate=True)
File "/usr/share/virt-manager/virtinst/nodedev.py", line 106, in __init__
XMLBuilder.__init__(self, *args, **kwargs)
File "/usr/share/virt-manager/virtinst/xmlbuilder.py", line 777, in __init__
parent_xpath, relative_object_xpath)
File "/usr/share/virt-manager/virtinst/xmlbuilder.py", line 679, in __init__
self._parse(parsexml, parsexmlnode)
File "/usr/share/virt-manager/virtinst/xmlbuilder.py", line 692, in _parse
doc = libxml2.parseDoc(xml)
File "/usr/lib/python2.7/site-packages/libxml2.py", line 1327, in parseDoc
if ret is None:raise parserError('xmlParseDoc() failed')
parserError: xmlParseDoc() failed
Regards
Roz
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] schema: Allow multiple machines for VMs
Use the same pattern for all OS types. Signed-off-by: Martin Kletzander --- docs/schemas/domaincommon.rng | 160 ++ 1 file changed, 4 insertions(+), 156 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 03fd541..cb21df7 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -328,152 +328,17 @@ - - - - - - - - - - - - hvm - - - - - i686 x86_64 - - - - - - -[a-zA-Z0-9_\.\-]+ - - - - - - - - - - mips - - - - - mips - - - - - - - - - sparc - - - - - sun4m - - - - - - - - - ppc - - - - - -g3beige -mac99 -prep -ppce500 - - - - - - - - - - +mips +ppc ppc64 ppc64le - - - - - - -pseries -pseries-2.1 -pseries-2.2 - - - - - - - - - - s390 s390x - - - - - - -s390 -s390-virtio -s390-ccw -s390-ccw-virtio - - - - - - - - - - armv7l - - - - - - -[a-zA-Z0-9_\.\-]+ - - - - - - - - - - aarch64 @@ -485,25 +350,6 @@ - - - - - - - - - i686 - x86_64 - ppc - ppc64 - mips - sparc - - - -exe - @@ -516,8 +362,10 @@ + hvm +
[libvirt] [PATCH 4/5] Change virConnectPtr into virObjectLocklable
It already had a virMutex inside, so this is just a cleanup.
Signed-off-by: Martin Kletzander
---
src/datatypes.c | 12 ++--
src/datatypes.h | 12 +---
src/libvirt-host.c | 18 +-
src/util/virerror.c | 18 +-
4 files changed, 25 insertions(+), 35 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c
index dc024f8..39f83d9 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -73,7 +73,7 @@ virDataTypesOnceInit(void)
#define DECLARE_CLASS_LOCKABLE(basename) \
DECLARE_CLASS_COMMON(basename, virClassForObjectLockable())
-DECLARE_CLASS(virConnect);
+DECLARE_CLASS_LOCKABLE(virConnect);
DECLARE_CLASS_LOCKABLE(virConnectCloseCallbackData);
DECLARE_CLASS(virDomain);
DECLARE_CLASS(virDomainSnapshot);
@@ -110,15 +110,12 @@ virGetConnect(void)
if (virDataTypesInitialize() < 0)
return NULL;
-if (!(ret = virObjectNew(virConnectClass)))
+if (!(ret = virObjectLockableNew(virConnectClass)))
return NULL;
if (!(ret->closeCallback =
virObjectLockableNew(virConnectCloseCallbackDataClass)))
goto error;
-if (virMutexInit(&ret->lock) < 0)
-goto error;
-
return ret;
error:
@@ -141,8 +138,6 @@ virConnectDispose(void *obj)
if (conn->driver)
conn->driver->connectClose(conn);
-virMutexLock(&conn->lock);
-
virResetError(&conn->err);
virURIFree(conn->uri);
@@ -154,9 +149,6 @@ virConnectDispose(void *obj)
virObjectUnref(conn->closeCallback);
}
-
-virMutexUnlock(&conn->lock);
-virMutexDestroy(&conn->lock);
}
diff --git a/src/datatypes.h b/src/datatypes.h
index 4973b07..9e19c55 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -1,7 +1,7 @@
/*
* datatypes.h: management of structs for public data types
*
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -328,7 +328,7 @@ struct _virConnectCloseCallbackData {
* Internal structure associated to a connection
*/
struct _virConnect {
-virObject object;
+virObjectLockable object;
/* All the variables from here, until the 'lock' declaration
* are setup at time of connection open, and never changed
* since. Thus no need to lock when accessing them
@@ -352,12 +352,10 @@ struct _virConnect {
void *privateData;
/*
- * The lock mutex must be acquired before accessing/changing
- * any of members following this point, or changing the ref
- * count of any virDomain/virNetwork object associated with
- * this connection
+ * Object lock must be acquired before accessing/changing any of
+ * members following this point, or changing the ref count of any
+ * virDomain/virNetwork object associated with this connection.
*/
-virMutex lock;
/* Per-connection error. */
virError err; /* the last error */
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index b4dc13e..03bee1f 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1,7 +1,7 @@
/*
* libvirt-host.c: entry points for vir{Connect,Node}Ptr APIs
*
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -51,7 +51,7 @@ VIR_LOG_INIT("libvirt.host");
int
virConnectRef(virConnectPtr conn)
{
-VIR_DEBUG("conn=%p refs=%d", conn, conn ? conn->object.u.s.refs : 0);
+VIR_DEBUG("conn=%p refs=%d", conn, conn ? conn->object.parent.u.s.refs :
0);
virResetLastError();
@@ -1219,7 +1219,7 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
virObjectRef(conn);
-virMutexLock(&conn->lock);
+virObjectLock(conn);
virObjectLock(conn->closeCallback);
virCheckNonNullArgGoto(cb, error);
@@ -1236,13 +1236,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
conn->closeCallback->freeCallback = freecb;
virObjectUnlock(conn->closeCallback);
-virMutexUnlock(&conn->lock);
+virObjectUnlock(conn);
return 0;
error:
virObjectUnlock(conn->closeCallback);
-virMutexUnlock(&conn->lock);
+virObjectUnlock(conn);
virDispatchError(conn);
virObjectUnref(conn);
return -1;
@@ -1272,7 +1272,7 @@ virConnectUnregisterCloseCallback(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
-virMutexLock(&conn->lock);
+virObjectLock(conn);
virObjectLock(conn->closeCallback);
virCheckNonNullArgGoto(cb, error);
@@ -1288,15 +1288,15 @@ virConnectUnregisterCloseCallback(virConnectPtr conn,
conn->closeCallback->freeCallback(conn->closeCallback->opaque);
conn->closeCallback->freeCallback = NULL;
-virObjectUnref(conn);
virObjectUnlock(conn->closeCallback);
-virMu
[libvirt] [PATCH 3/5] closeCallback is already lockable, initialize it as such
Luckily we are allocating structs as clean memory and
PTHREAD_MUTEX_INITIALIZER is "{ 0 }", so nothing happened, but it should
still be created as lockable object.
Signed-off-by: Martin Kletzander
---
src/datatypes.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c
index 0f535b4..dc024f8 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -1,7 +1,7 @@
/*
* datatypes.c: management of structs for public data types
*
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -113,7 +113,7 @@ virGetConnect(void)
if (!(ret = virObjectNew(virConnectClass)))
return NULL;
-if (!(ret->closeCallback = virObjectNew(virConnectCloseCallbackDataClass)))
+if (!(ret->closeCallback =
virObjectLockableNew(virConnectCloseCallbackDataClass)))
goto error;
if (virMutexInit(&ret->lock) < 0)
--
2.3.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] json: export non-static functions
Two non-static functions in virjson.c were missing their export info in libvirt_private.syms, so they couldn't be used anywhere it the code (and that's about to get changed). Signed-off-by: Martin Kletzander --- src/libvirt_private.syms | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 67ab526..d9497b5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1569,6 +1569,7 @@ virISCSIScanTargets; virJSONValueArrayAppend; virJSONValueArrayGet; virJSONValueArraySize; +virJSONValueArraySteal; virJSONValueFree; virJSONValueFromString; virJSONValueGetArrayAsBitmap; @@ -1579,6 +1580,7 @@ virJSONValueGetNumberLong; virJSONValueGetNumberUint; virJSONValueGetNumberUlong; virJSONValueGetString; +virJSONValueIsArray; virJSONValueIsNull; virJSONValueNewArray; virJSONValueNewArrayFromBitmap; -- 2.3.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] Link libvirt_util with datatypes
We were lucky enough for this to work because the datatypes files were linked to in the resulting binary, but the dependency really is already in libvirt_util. Signed-off-by: Martin Kletzander --- src/Makefile.am | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 91a4c17..8c26076 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,6 +1,6 @@ ## Process this file with automake to produce Makefile.in -## Copyright (C) 2005-2014 Red Hat, Inc. +## Copyright (C) 2005-2015 Red Hat, Inc. ## ## This library is free software; you can redistribute it and/or ## modify it under the terms of the GNU Lesser General Public @@ -86,9 +86,12 @@ augeas_DATA = augeastestdir = $(datadir)/augeas/lenses/tests augeastest_DATA = +DATATYPES_SOURCES = datatypes.h datatypes.c + # These files are not related to driver APIs. Simply generic # helper APIs for various purposes UTIL_SOURCES = \ + $(DATATYPES_SOURCES)\ util/viralloc.c util/viralloc.h \ util/virarch.h util/virarch.c \ util/viratomic.h util/viratomic.c \ @@ -185,7 +188,6 @@ util/virkeymaps.h: $(srcdir)/util/keymaps.csv \ # Internal generic driver infrastructure NODE_INFO_SOURCES = nodeinfo.h nodeinfo.c nodeinfopriv.h -DATATYPES_SOURCES = datatypes.h datatypes.c DRIVER_SOURCES = \ driver.c driver.h \ driver-hypervisor.h \ @@ -198,7 +200,6 @@ DRIVER_SOURCES = \ driver-storage.h\ driver-stream.h \ internal.h \ - $(DATATYPES_SOURCES)\ fdstream.c fdstream.h \ $(NODE_INFO_SOURCES)\ libvirt.c libvirt_internal.h\ -- 2.3.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/5] Tiny miscellaneous clean-ups
Just a few things I stumbled upon that are needed for future work. Martin Kletzander (5): configure: Align messages Link libvirt_util with datatypes closeCallback is already lockable, initialize it as such Change virConnectPtr into virObjectLocklable json: export non-static functions configure.ac | 4 ++-- src/Makefile.am | 7 --- src/datatypes.c | 16 src/datatypes.h | 12 +--- src/libvirt-host.c | 18 +- src/libvirt_private.syms | 2 ++ src/util/virerror.c | 18 +- 7 files changed, 35 insertions(+), 42 deletions(-) -- 2.3.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] configure: Align messages
The first two were a bit off. Signed-off-by: Martin Kletzander --- configure.ac | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 38fbbad..aed0934 100644 --- a/configure.ac +++ b/configure.ac @@ -2966,8 +2966,8 @@ AC_MSG_NOTICE([pm-utils: $with_pm_utils]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Test suite]) AC_MSG_NOTICE([]) -AC_MSG_NOTICE([ Coverage: $enable_coverage]) -AC_MSG_NOTICE([ Alloc OOM: $enable_oom]) +AC_MSG_NOTICE([ Coverage: $enable_coverage]) +AC_MSG_NOTICE([Alloc OOM: $enable_oom]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Miscellaneous]) AC_MSG_NOTICE([]) -- 2.3.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] schema: Allow multiple machines for sparc VMs
On Mon, Apr 13, 2015 at 04:14:53PM +0200, Martin Kletzander wrote: > Use the same pattern as there is for x86 machines. > > Signed-off-by: Martin Kletzander > --- > docs/schemas/domaincommon.rng | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 03fd541..80b30df 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -384,7 +384,9 @@ > > > > - sun4m > + > +[a-zA-Z0-9_\.\-]+ > + > > > I think you could probably simplify this all much more. All these architecture specific blocks of machine type names should just be deleted and so this: hvm Would simplify to just i686 others... [a-zA-Z0-9_\.\-]+ Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] schema: Allow multiple machines for sparc VMs
Use the same pattern as there is for x86 machines. Signed-off-by: Martin Kletzander --- docs/schemas/domaincommon.rng | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 03fd541..80b30df 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -384,7 +384,9 @@ - sun4m + +[a-zA-Z0-9_\.\-]+ + -- 2.3.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] sparc: Add default PCI root controller
It is there even with -nodefaults and -no-user-config, so count with that so we can start sparc domains. Signed-off-by: Martin Kletzander --- src/qemu/qemu_domain.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ae632c5..603360f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1,7 +1,7 @@ /* * qemu_domain.c: QEMU domain private state * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2015 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -999,6 +999,12 @@ qemuDomainDefPostParse(virDomainDefPtr def, case VIR_ARCH_S390X: addDefaultUSB = false; break; + +case VIR_ARCH_SPARC: +case VIR_ARCH_SPARC64: +addPCIRoot = true; +break; + default: break; } -- 2.3.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv1 10/13] Remove overengineered loop
On Mon, Apr 13, 2015 at 07:51:22AM +0200, Peter Krempa wrote: > On Fri, Apr 10, 2015 at 14:59:02 +0200, Ján Tomko wrote: > > Do not loop over enum with one value. > > --- > > src/storage/storage_backend.c | 31 ++- > > 1 file changed, 10 insertions(+), 21 deletions(-) > > ACK. > Thanks, I have pushed all the ACKed patches up to here (that is without patches 3, 4 and 9). Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv1 03/13] Separate out virStorageFeatureParse
On Mon, Apr 13, 2015 at 07:41:12AM +0200, Peter Krempa wrote:
> On Fri, Apr 10, 2015 at 14:58:55 +0200, Ján Tomko wrote:
> > diff --git a/src/conf/storage_feature_conf.c
> > b/src/conf/storage_feature_conf.c
> > new file mode 100644
> > index 000..77e6406
> > --- /dev/null
> > +++ b/src/conf/storage_feature_conf.c
> > @@ -0,0 +1,62 @@
> > +/*
> > + * storage_feature_conf.c: config handling for storage file features
> > + *
> > + * Copyright: Red Hat, Inc
> > + *
> > + * LGLPv2.1+
> > + */
>
> I like this compact header, but I'm not sure if the rest of upstream has
> the same opinion.
>
>
It was just a placeholder and I forgot to update it before sending the
series.
If we want to switch to a more compact header, I think it should be done
consistently all over the codebase.
I have squashed in a copy of the header from another conf file.
> > +const char *xpath,
> > +char **compat,
> > +virBitmapPtr *features)
> > +{
> > +xmlNodePtr *nodes = NULL;
> > +char *feat_xpath = NULL;
> > +size_t i;
> > +int n;
> > +int ret = -1;
> > +
> > +if (!virXPathNode(xpath, ctxt))
> > +return 0;
> > +
> > +if (!*compat && VIR_STRDUP(*compat, "1.1") < 0)
> > +return -1;
>
> Is there a specific reason that you check whether the compat string is
> not assigned previously?
>
Yes, the user might have specified a different compat level.
Jan
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 9/9] virsh: Add iothreadadd and iothreaddel commands
On Fri, Apr 10, 2015 at 17:36:27 -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1161617
>
> Add command to allow adding and removing IOThreads from the domain including
> the configuration and live domain.
>
> $ virsh iothreadadd --help
> NAME
> iothreadadd - add an IOThread to the guest domain
>
> SYNOPSIS
> iothreadadd [--name ] [--config] [--live]
> [--current]
>
> DESCRIPTION
> Add an IOThread to the guest domain.
>
> OPTIONS
> [--domain] domain name, id or uuid
> [--id] iothread for the new IOThread
> --name a name for the new IOThread
> --config affect next boot
> --live affect running domain
> --currentaffect current domain
>
> $ virsh iothreaddel --help
> NAME
> iothreaddel - delete an IOThread from the guest domain
>
> SYNOPSIS
> iothreaddel [--config] [--live] [--current]
>
> DESCRIPTION
> Delete an IOThread from the guest domain.
>
> OPTIONS
> [--domain] domain name, id or uuid
> [--id] iothread_id for the IOThread to delete
> --config affect next boot
> --live affect running domain
> --currentaffect current domain
>
> Assuming a running $dom with multiple IOThreads assigned and that
> that the $dom has disks assigned to IOThread 1 and IOThread 2:
>
> $ virsh iothreadinfo $dom
> IOThread ID CPU Affinity
> ---
> 1 2
> 2 3
> 3 0-1
>
> $ virsh iothreadadd $dom 1
> error: invalid argument: an IOThread is already using iothread_id '1' in
> iothreadpids
>
> $ virsh iothreadadd $dom 1 --config
> error: invalid argument: an IOThread is already using iothread_id '1' in
> persistent iothreadids
>
> $ virsh iothreadadd $dom 4
> $ virsh iothreadinfo $dom
> IOThread ID CPU Affinity
> ---
> 1 2
> 2 3
> 3 0-1
> 4 0-3
>
> $ virsh iothreadinfo $dom --config
> IOThread ID CPU Affinity
> ---
> 1 2
> 2 3
> 3 0-1
>
> $ virsh iothreadadd $dom 4 --config
> $ virsh iothreadinfo $dom --config
> IOThread ID CPU Affinity
> ---
> 1 2
> 2 3
> 3 0-1
> 4 0-3
>
> $ virsh iothreadadd $dom 5 userdisk
> $ virsh qemu-monitor-command $dom '{"execute":"query-iothreads"}'
> {"return":[{"thread-id":17889,"id":"iothread1"},{"thread-id":17890,"id":"iothread2"},{"thread-id":17892,"id":"iothread3"},{"thread-id":17893,"id":"iothread4"},{"thread-id":18108,"id":"userdisk_iothread5"}],"id":"libvirt-104"}
>
> $ virsh iothreaddel $dom 5 userdisk
>
> Assuming the same original configuration
>
> $ virsh iothreaddel $dom 1
> error: invalid argument: cannot remove IOThread 1 since it is being used by
> disk path '/home/vm-images/f18'
>
> $ virsh iothreaddel $dom 3
>
> $ virsh iothreadinfo $dom
> IOThread ID CPU Affinity
> ---
> 1 2
> 2 3
>
> $ virsh iothreadinfo $dom --config
> IOThread ID CPU Affinity
> ---
> 1 2
> 2 3
> 3 0-1
>
> Signed-off-by: John Ferlan
> ---
> tools/virsh-domain.c | 174
> +++
> tools/virsh.pod | 32 ++
> 2 files changed, 206 insertions(+)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 928360c..37836ce 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6977,6 +6977,168 @@ cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd)
> }
>
> /*
> + * "iothreadadd" command
> + */
> +static const vshCmdInfo info_iothreadadd[] = {
> +{.name = "help",
> + .data = N_("add an IOThread to the guest domain")
> +},
> +{.name = "desc",
> + .data = N_("Add an IOThread to the guest domain.")
> +},
> +{.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_iothreadadd[] = {
> +{.name = "domain",
> + .type = VSH_OT_DATA,
> + .flags = VSH_OFLAG_REQ,
> + .help = N_("domain name, id or uuid")
> +},
> +{.name = "id",
> + .type = VSH_OT_INT,
> + .flags = VSH_OFLAG_REQ,
> + .help = N_("iothread for the new IOThread")
> +},
> +{.name = "name",
> + .type = VSH_OT_STRING,
> + .help = N_("a name for the new IOThread")
> +},
> +{.name = "config",
> + .type = VSH_OT_BOOL,
> + .help = N_("affect next boot")
> +},
> +{.name = "live",
> + .type = VSH_OT_BOOL,
> + .help = N_("affect running domain")
> +},
> +{.name = "current",
> + .type = VSH_OT_BOOL,
> + .help = N_("affect current domain")
> +},
> +
[libvirt] Changing media on network disks (was Re: can't bot from scsi http cdrom)
On 13/04/2015 14:34, Vasiliy Tolstov wrote: > Thanks! This is works fine. Last question - does it possible to create > empty cdrom with type='network'? > I'm try this, but libvrit complains with error: > > > > > > No, unfortunately not because "virsh change-media" wouldn't be able to convert its argument to the required libvirt XML. I think you would need a new virsh change-media option, e.g. --xml, that takes a element instead of a source path + target path pair. However, I am not a libvirt developer. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 8/9] qemu: Add support to Add/Delete IOThreads
On Fri, Apr 10, 2015 at 17:36:26 -0400, John Ferlan wrote:
> Add qemuDomainAddIOThread and qemuDomainDelIOThread in order to add or
> remove an IOThread to/from the host either for live or config optoins
>
> The implementation for the 'live' option will use the iothreadpids list
> in order to make decision, while the 'config' option will use the
> iothreadids list. Additionally, for deletion each may have to adjust
> the iothreadpin list.
>
> IOThreads are implemented by qmp objects, the code makes use of the existing
> qemuMonitorAddObject or qemuMonitorDelObject APIs.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/domain_audit.c | 9 +
> src/conf/domain_audit.h | 6 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 432
> +++
> 4 files changed, 448 insertions(+)
>
...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d99f886..5b0784f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6186,6 +6186,436 @@ qemuDomainPinIOThread(virDomainPtr dom,
> return ret;
> }
>
> +static int
> +qemuDomainHotplugIOThread(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + unsigned int iothread_id,
> + const char *name,
> + bool add)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +char *alias = NULL;
> +size_t i, idx;
> +int rc = -1;
> +int ret = -1;
> +unsigned int orig_niothreads = vm->def->iothreads;
> +unsigned int exp_niothreads = vm->def->iothreads;
> +int new_niothreads = 0;
> +qemuMonitorIOThreadInfoPtr *new_iothreads = NULL;
> +virCgroupPtr cgroup_iothread = NULL;
> +char *mem_mask = NULL;
> +
> +/* Let's see if we can find this iothread_id in our iothreadpids list
> + * For add finding the same iothread_id will cause a failure since we
> + * cannot add the same iothread_id twice.
> + * For del finding our iothread_id is good since we cannot delete
> + * something that doesn't exist
> + */
The comment states obvious facts.
> +for (idx = 0; idx < priv->niothreadpids; idx++) {
> +if (iothread_id == priv->iothreadpids[idx]->iothread_id)
> +break;
> +}
> +
> +if (add) {
> +if (idx < priv->niothreadpids) {
> +virReportError(VIR_ERR_INVALID_ARG,
> + _("an IOThread is already using iothread_id "
> + "'%u' in iothreadpids"),
> + iothread_id);
> +goto cleanup;
> +}
> +} else {
> +if (idx == priv->niothreadpids) {
> +virReportError(VIR_ERR_INVALID_ARG,
> + _("cannot find IOThread '%u' in iothreadpids"),
> + iothread_id);
> +goto cleanup;
> +}
> +
> +/* The qemuDomainDelThread doesn't pass (or need to pass) the
> + * name. So we'll grab it here so that we can formulate the
> + * correct alias for qemuMonitorDelObject to find this object.
> + */
With the changes I've suggested this won't be necessary
> +name = priv->iothreadpids[idx]->name;
> +}
> +
> +/* Generate alias */
> +if (name) {
> +if (virAsprintf(&alias, "%s_iothread%u", name, iothread_id) < 0)
> +return -1;
> +} else {
> +if (virAsprintf(&alias, "iothread%u", iothread_id) < 0)
> +return -1;
> +}
Neither this.
> +
> +qemuDomainObjEnterMonitor(driver, vm);
> +
> +if (!virDomainObjIsActive(vm)) {
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("cannot change IOThreads for an inactive domain"));
> +goto exit_monitor;
> +}
This is a bit too late to check if the domain is active.
> +
> +if (add) {
> +rc = qemuMonitorAddObject(priv->mon, "iothread", alias, NULL);
> +exp_niothreads++;
> +} else {
> +rc = qemuMonitorDelObject(priv->mon, alias);
> +exp_niothreads--;
> +}
> +
> +if (rc < 0)
> +goto exit_monitor;
> +
> +/* After hotplugging the IOThreads we need to re-detect the
> + * IOThreads thread_id's, adjust the cgroups, thread affinity,
> + * and the priv->iothreadpids list.
> + */
> +if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon,
> + &new_iothreads)) < 0) {
> +virResetLastError();
> +goto exit_monitor;
In this case you'd report an empty error as exit_monitor leads to
returning -1 without reporting any new one.
> +}
> +
> +if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +goto cleanup;
> +
> +/* ohhh something went wrong */
Obvious.
> +if (new_niothreads != exp_niothreads) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("got wrong number of IOThread
Re: [libvirt] [PATCH 0/2] Fix handling of disk's WWN (world wide name)
On Tue, Apr 07, 2015 at 04:10:47PM +0200, Peter Krempa wrote: Peter Krempa (2): conf: ABI: Check WWN in disk abi stability check qemu: Enforce WWN to be unique among VM's disks ACK series. docs/formatdomain.html.in | 3 ++- src/conf/domain_conf.c| 33 + src/conf/domain_conf.h| 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 3 +++ 5 files changed, 42 insertions(+), 1 deletion(-) -- 2.2.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 7/9] remote: Add support for AddIOThread and DelIOThread
On Fri, Apr 10, 2015 at 17:36:25 -0400, John Ferlan wrote: > Add remote support for the add/delete IOThread API's > > Signed-off-by: John Ferlan > --- > src/remote/remote_driver.c | 2 ++ > src/remote/remote_protocol.x | 31 ++- > src/remote_protocol-structs | 13 + > 3 files changed, 45 insertions(+), 1 deletion(-) Looks good to me. I'd leave this one open until v3 for other people to chime in. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] configure: Check for libxl_utils.h instead of libxlutil.h
On Thu, Apr 09, 2015 at 04:08:24PM +0200, Michal Privoznik wrote: The file provided by xen-devel package (or xen-tools in Gentoo) does not provide libxlutil.h. In fact the package provides libxl_utils.h instead which is the one we are looking for anyway. It also perfectly matches src/libxl/libxl_conf.c which includes this very file. ACK. Signed-off-by: Michal Privoznik --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 38fbbad..0626492 100644 --- a/configure.ac +++ b/configure.ac @@ -915,7 +915,7 @@ fi if test "$with_libxl" = "yes"; then dnl If building with libxl, use the libxl utility header and lib too -AC_CHECK_HEADERS([libxlutil.h]) +AC_CHECK_HEADERS([libxl_utils.h]) LIBXL_LIBS="$LIBXL_LIBS -lxlutil" AC_DEFINE_UNQUOTED([WITH_LIBXL], 1, [whether libxenlight driver is enabled]) if test "x$LIBXL_FIRMWARE_DIR" != "x"; then -- 2.0.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/9] Implement virDomainAddIOThread and virDomainDelIOThread
On Fri, Apr 10, 2015 at 17:36:24 -0400, John Ferlan wrote:
> Add libvirt API's to manage adding and deleting IOThreads to/from the
> domain
>
> Signed-off-by: John Ferlan
> ---
> include/libvirt/libvirt-domain.h | 7 +++
> src/driver-hypervisor.h | 13
> src/libvirt-domain.c | 132
> +++
> src/libvirt_public.syms | 6 ++
> 4 files changed, 158 insertions(+)
>
> diff --git a/include/libvirt/libvirt-domain.h
> b/include/libvirt/libvirt-domain.h
> index 7be4219..472258c 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1615,6 +1615,13 @@ int virDomainPinIOThread(virDomainPtr
> domain,
>unsigned char *cpumap,
>int maplen,
>unsigned int flags);
> +int virDomainAddIOThread(virDomainPtr domain,
> + unsigned int iothread_id,
> + const char *name,
> + unsigned int flags);
> +int virDomainDelIOThread(virDomainPtr domain,
> + unsigned int iothread_id,
> + unsigned int flags);
>
> /**
> * VIR_USE_CPU:
> diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
> index 1b92460..283562f 100644
> --- a/src/driver-hypervisor.h
> +++ b/src/driver-hypervisor.h
> @@ -393,6 +393,17 @@ typedef int
> unsigned int flags);
>
> typedef int
> +(*virDrvDomainAddIOThread)(virDomainPtr domain,
> + unsigned int iothread_id,
> + const char *name,
> + unsigned int flags);
> +
> +typedef int
> +(*virDrvDomainDelIOThread)(virDomainPtr domain,
> + unsigned int iothread_id,
> + unsigned int flags);
> +
> +typedef int
> (*virDrvDomainGetSecurityLabel)(virDomainPtr domain,
> virSecurityLabelPtr seclabel);
>
> @@ -1273,6 +1284,8 @@ struct _virHypervisorDriver {
> virDrvDomainGetMaxVcpus domainGetMaxVcpus;
> virDrvDomainGetIOThreadInfo domainGetIOThreadInfo;
> virDrvDomainPinIOThread domainPinIOThread;
> +virDrvDomainAddIOThread domainAddIOThread;
> +virDrvDomainDelIOThread domainDelIOThread;
> virDrvDomainGetSecurityLabel domainGetSecurityLabel;
> virDrvDomainGetSecurityLabelList domainGetSecurityLabelList;
> virDrvNodeGetSecurityModel nodeGetSecurityModel;
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 0acfd13..ffd50b3 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -8020,6 +8020,138 @@ virDomainPinIOThread(virDomainPtr domain,
>
>
> /**
> + * virDomainAddIOThread:
> + * @domain: a domain object
> + * @iothread_id: the specific IOThread ID value to add
> + * @name: optional additional naming string (NUL terminated)
> + * @flags: bitwise-OR of virDomainModificationImpact
> + *
> + * Dynamically add an IOThread to the domain. If @iothread_id is a positive
> + * non-zero value, then attempt to add the specific IOThread ID and error
> + * out if the iothread id already exists. If the @name is NULL, then only
> + * the default naming scheme is used. Any name containing "iothread" will
> + * be rejected.
> + *
> + * Note that this call can fail if the underlying virtualization hypervisor
> + * does not support it or if growing the number is arbitrarily limited.
> + * This function may require privileged access to the hypervisor.
It requires, not may require.
> + *
> + * @flags may include VIR_DOMAIN_AFFECT_LIVE to affect a running
> + * domain (which may fail if domain is not active), or
> + * VIR_DOMAIN_AFFECT_CONFIG to affect the next boot via the XML
> + * description of the domain. Both flags may be set.
> + * If neither flag is specified (that is, @flags is
> VIR_DOMAIN_AFFECT_CURRENT),
> + * then an inactive domain modifies persistent setup, while an active domain
> + * is hypervisor-dependent on whether just live or both live and persistent
> + * state is changed.
I'd opt for a more sane explanation, where CURRENT with active VM means
the live definiton is modified.
> + *
> + * Not all hypervisors can support all flag combinations.
There are no flags this could potentially apply to yet.
> + *
> + * Returns 0 in case of success, -1 in case of failure.
> + */
> +int
> +virDomainAddIOThread(virDomainPtr domain,
> + unsigned int iothread_id,
> + const char *name,
> + unsigned int flags)
> +{
> +virConnectPtr conn;
> +
> +VIR_DOMAIN_DEBUG(domain, "iothread_id=%u, name=%p flags=%x",
> + iothread_id, name, flags);
> +
> +virResetLastError();
> +
> +virChe
Re: [libvirt] [PATCH v2 5/9] qemu: Use domain iothreadids to populate iothreadpids
On Fri, Apr 10, 2015 at 17:36:23 -0400, John Ferlan wrote:
> Rather than use the default numbering scheme of 1..number of iothreads
> defined for the domain, use the iothreadid's list for the iothread_id and
> possibly augmenting the alias using the iothreadsid name.
>
> This also requires adjusting the iothreadpids structure to include room for
> the iothread_id and name (if found in the alias, thus iothreadids entry).
>
> The iothreadpids will keep track of the live system, while iothreadids will
> be used for the configuration.
>
> Now that the iothreadpids list keeps track of the iothread_id's, these
> can be used in place of the many places where a for loop would "know"
> that the ID was "+ 1" from the array element.
>
> The new tests ensure usage of the values for an exact number
> of iothreads, the usage of a smaller number of values than
> iothreads that exist (and usage of the default numbering scheme), and the
> usage of the optional name value to provide the alias for the IOThread.
>
> Signed-off-by: John Ferlan
> ---
> src/qemu/qemu_cgroup.c | 13 ++-
> src/qemu/qemu_command.c| 104
> ++---
> src/qemu/qemu_command.h| 4 +
> src/qemu/qemu_domain.c | 40 +++-
> src/qemu/qemu_domain.h | 2 +
> src/qemu/qemu_driver.c | 35 ---
> src/qemu/qemu_process.c| 29 +-
> .../qemuxml2argv-iothreads-ids-partial.args| 10 ++
> .../qemuxml2argv-iothreads-ids-partial.xml | 33 +++
> .../qemuxml2argv-iothreads-ids.args| 8 ++
> .../qemuxml2argv-iothreads-ids.xml | 33 +++
> .../qemuxml2argv-iothreads-name.args | 17
> .../qemuxml2argv-iothreads-name.xml| 44 +
> tests/qemuxml2argvtest.c | 4 +
> tests/qemuxml2xmltest.c| 3 +
> 15 files changed, 339 insertions(+), 40 deletions(-)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.args
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids-partial.xml
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-ids.xml
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-name.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-name.xml
>
...
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index e7e0937..68c85e2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -678,6 +678,57 @@ qemuOpenVhostNet(virDomainDefPtr def,
> }
>
> int
> +qemuDomainParseIOThreadAlias(char *alias,
> + unsigned int *iothread_id,
> + char **name)
> +{
> +unsigned int idval;
> +char *idname = NULL;
> +
> +/* IOThread's alias is either "iothread#" or "name_iothread#", where
I don't really think we should put any user-configurable stuff into the
alias. We can keep the name internally and use it for lookup but using
it in the alias can be tricky.
If it would be part of the alias, the name definitely should not start
with the user configurable part.
> + * name is a user definable prefix for the alias and the # is the
> + * iothreadids iothread_id provided in XML or generated during
> + * post parse processing
> + */
> +if (STRPREFIX(alias, "iothread")) {
> +if (virStrToLong_ui(alias + strlen("iothread"),
> +NULL, 10, &idval) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to find iothread id for '%s'"),
> + alias);
> +return -1;
> +}
> +/* Default - no need to do anything with name */
> +} else {
> +char *spot = strstr(alias, "_iothread");
> +
> +if (!spot) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed IOThreads alias '%s'"),
> + alias);
> +return -1;
> +}
> +
> +/* Pick off the user defined name from the front */
> +if (VIR_STRNDUP(idname, alias, spot - alias) < 0)
> +return -1;
> +
> +if (virStrToLong_ui(alias + strlen(idname) + strlen("_iothread"),
> +NULL, 10, &idval) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to find iothread id for '%s'"),
> + alias);
> +VIR_FREE(idname);
> +return -1;
> +}
> +}
> +
> +*iothread_id = idval;
> +*name = idname;
> +return 0;
> +}
> +
> +int
> qemuNetworkPrepareDevices(virDomainDefPtr def)
> {
> int
Re: [libvirt] [PATCHv2] virBitmap: Place virBitmapIsAllClear check after virBitmapParse calls
On 04/13/2015 02:18 PM, Peter Krempa wrote: > On Mon, Apr 13, 2015 at 14:01:27 +0200, Erik Skultety wrote: >> This patch adds checks for empty bitmaps right after the calls of >> virBitmapParse. These only include spots where set API's are called and >> where domain's XML is parsed. >> Also, it partially reverts commit 983f5a which added a check for >> invalid nodeset "0,^0" into virBitmapParse function. This change broke >> the logic, as an empty bitmap should not cause an error. >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1210545 >> --- >> src/conf/domain_conf.c | 35 +++ >> src/conf/numa_conf.c | 23 +++ >> src/qemu/qemu_driver.c | 5 +++-- >> src/util/virbitmap.c | 3 --- >> src/xenconfig/xen_sxpr.c | 7 +++ >> tests/virbitmaptest.c| 13 ++--- >> 6 files changed, 70 insertions(+), 16 deletions(-) > > ACK, thanks for adding the test. > > Peter > Thank you, now pushed. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] virBitmap: Place virBitmapIsAllClear check after virBitmapParse calls
On Mon, Apr 13, 2015 at 14:01:27 +0200, Erik Skultety wrote: > This patch adds checks for empty bitmaps right after the calls of > virBitmapParse. These only include spots where set API's are called and > where domain's XML is parsed. > Also, it partially reverts commit 983f5a which added a check for > invalid nodeset "0,^0" into virBitmapParse function. This change broke > the logic, as an empty bitmap should not cause an error. > > https://bugzilla.redhat.com/show_bug.cgi?id=1210545 > --- > src/conf/domain_conf.c | 35 +++ > src/conf/numa_conf.c | 23 +++ > src/qemu/qemu_driver.c | 5 +++-- > src/util/virbitmap.c | 3 --- > src/xenconfig/xen_sxpr.c | 7 +++ > tests/virbitmaptest.c| 13 ++--- > 6 files changed, 70 insertions(+), 16 deletions(-) ACK, thanks for adding the test. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/9] qemu: Convert iothreadpids into an array of structures
On Fri, Apr 10, 2015 at 17:36:22 -0400, John Ferlan wrote: > Create qemuDomainIOThreadInfo which currently will just be the thread_id > returned from IOThreadInfo, but will soon expand to handle the needs of > live IOThread data for adding/deleting IOThread's As I've said in previous patch the data should be part of the structure you'll be adding there. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/9] conf: Add new domain XML element 'iothreadids'
On Fri, Apr 10, 2015 at 17:36:21 -0400, John Ferlan wrote: > Adding a new XML element 'iothreadids' in order to allow defining > specific IOThread ID's rather than relying on the algorithm to assign > IOThread ID's starting at 1 and incrementing to iothreads count. > > This will allow future patches to be able to add new IOThreads by > a specific iothread_id and of course delete any exisiting IOThread. > > Each iothreads element will have 'n' children elements > which will have attributes "id" and "name". The "id" will allow for > definition of any "valid" (eg > 0) iothread_id value. The "name" > attribute will allow for adding a name to the alias generated for > the IOThread. The name cannot contain "iothread" since that's part > of the default IOThread naming scheme already in use. > > On input, if any 's are provided, they will > be marked so that we only print out what we read in. > > On input, if no are provided, the PostParse code will > self generate a list of ID's starting at 1 and going to the number > of iothreads defined for the domain (just like the current algorithm > numbering scheme). A future patch will rework the existing algorithm > to make use of the iothreadids list. > the input XML > > On output, only print out the if they were read in. > > Signed-off-by: John Ferlan > --- > docs/formatdomain.html.in | 28 + > docs/schemas/domaincommon.rng | 17 +++ > src/conf/domain_conf.c| 245 > +- > src/conf/domain_conf.h| 23 > src/libvirt_private.syms | 6 ++ > 5 files changed, 317 insertions(+), 2 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 7ceb1fa..3224c20 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -521,6 +521,18 @@ >... > > > + > +> + ... > + > + > > >iothreads > @@ -530,7 +542,23 @@ > virtio-blk-pci and virtio-blk-ccw target storage devices. There > should be only 1 or 2 IOThreads per host CPU. There may be more > than one supported device assigned to each IOThread. > +Since 1.2.8 > > + iothreadids > + > +The optional iothreadids element provides the capability > +to specifically define the IOThread ID's for the domain. By default, > +IOThread ID's are sequentially numbered starting from 1 through the > +number of iothreads defined for the domain. The > +id attribute is used to define the IOThread ID and > +the optional name attribute is a user defined name that > +may be used to name the IOThread for the hypervisor. The id attribute > +must be a positive integer greater than 0. If there are less > +iothreadids defined than iothreads > +defined for the domain, then libvirt will sequentially fill > +iothreadids starting at 1 avoiding any predefined id. > +Since 1.2.15 > + > > > CPU Tuning > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 1f5bf62..844caf6 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c ... > @@ -3304,6 +3332,18 @@ virDomainDefPostParseInternal(virDomainDefPtr def, > return -1; > } > > +/* Fully populate the IOThread ID list */ > +if (def->iothreads && def->iothreads != def->niothreadids) { > +unsigned int iothread_id = 1; > +while (def->niothreadids != def->iothreads) { > +if (!virDomainIOThreadIDIsDuplicate(def, iothread_id)) { > +if (virDomainIOThreadIDAdd(def, iothread_id, NULL) < 0) > +return -1; > +} > +iothread_id++; > +} > +} > + > if (virDomainDefGetMemoryInitial(def) == 0) { > virReportError(VIR_ERR_XML_ERROR, "%s", > _("Memory size must be specified via or in > the " > @@ -13192,6 +13232,65 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt, > return idmap; > } > > +/* Parse the XML definition for an IOThread ID > + * > + * Format is : > + * > + * 4 > + * > + * > + * > + * > + * > + * > + */ > +static virDomainIOThreadIDDefPtr > +virDomainIOThreadIDDefParseXML(xmlNodePtr node, > + xmlXPathContextPtr ctxt) > +{ > +virDomainIOThreadIDDefPtr def; > +xmlNodePtr oldnode = ctxt->node; > +char *tmp = NULL; > + > +if (VIR_ALLOC(def) < 0) > +return NULL; > + > +ctxt->node = node; > + > +if (!(tmp = virXPathString("string(./@id)", ctxt))) { > +virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Missing element in IOThread ID"));> + > + ... > +> + > + > + > +
Re: [libvirt] [PATCH 1/2] virBitmap: Place virBitmapIsAllClear check after virBitmapParse calls
On 04/13/2015 07:10 AM, Peter Krempa wrote: > On Fri, Apr 10, 2015 at 12:41:12 +0200, Erik Skultety wrote: >> This patch adds checks for empty bitmaps right after the calls of >> virBitmapParse. These only include spots where set API's are called and >> where domain's XML is parsed. >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1210545 >> --- >> src/conf/domain_conf.c | 35 +++ >> src/conf/numa_conf.c | 23 +++ >> src/qemu/qemu_driver.c | 5 +++-- >> src/xenconfig/xen_sxpr.c | 7 +++ >> 4 files changed, 60 insertions(+), 10 deletions(-) > > I've "git grep"'d a few uses of this function and found a few places > that would also need a chceck. Few examples are the XML parser for > networks, the use in qemuProcessStart() to parse the automatic > placemenet and so on .. > > The rest looks good though. > > Peter > At the moment, there's really no need to check the bitmap 'class_id' in network.conf, because it's a status XML anyway, however even if user changed our status XML and an error occurred, it would be user's responsibility, the only exception to this would be a daemon crash which isn't this case. Moreover, if you parse the bitmap "0,^0" the empty bitmap wouldn't have almost any effect, because by default first 3 bits are always set during network object creation. To the qemuProcessStart, we call numad to get a suggestion for the nodeset and the only problem would be if numad returned empty string, however this would be handled by virBitmapParse itself successfully. The other occurrences are tests mostly, v1 included this check for XEN parser, I'm not sure about the 'Parallels' though. Anyway, I squashed the second patch into this one as you suggested and modified an existing test in virbitmaptest.c for v2. (I thought checking this specific case wasn't worth having a separate test, so I modified bitmap-overlap test, instead of just testing the bitmap parsing.) Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2] virBitmap: Place virBitmapIsAllClear check after virBitmapParse calls
This patch adds checks for empty bitmaps right after the calls of
virBitmapParse. These only include spots where set API's are called and
where domain's XML is parsed.
Also, it partially reverts commit 983f5a which added a check for
invalid nodeset "0,^0" into virBitmapParse function. This change broke
the logic, as an empty bitmap should not cause an error.
https://bugzilla.redhat.com/show_bug.cgi?id=1210545
---
src/conf/domain_conf.c | 35 +++
src/conf/numa_conf.c | 23 +++
src/qemu/qemu_driver.c | 5 +++--
src/util/virbitmap.c | 3 ---
src/xenconfig/xen_sxpr.c | 7 +++
tests/virbitmaptest.c| 13 ++---
6 files changed, 70 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 823e003..f7f68ba 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11577,6 +11577,12 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
if (virBitmapParse(nodemask, 0, &def->sourceNodes,
VIR_DOMAIN_CPUMASK_LEN) < 0)
goto cleanup;
+
+if (virBitmapIsAllClear(def->sourceNodes)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Invalid value of 'nodemask': %s"), nodemask);
+goto cleanup;
+}
}
ret = 0;
@@ -13265,6 +13271,13 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node,
if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
goto error;
+if (virBitmapIsAllClear(def->cpumask)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Invalid value of 'cpuset': %s"),
+ tmp);
+goto error;
+}
+
cleanup:
VIR_FREE(tmp);
ctxt->node = oldnode;
@@ -13366,6 +13379,12 @@ virDomainHugepagesParseXML(xmlNodePtr node,
if (virBitmapParse(nodeset, 0, &hugepage->nodemask,
VIR_DOMAIN_CPUMASK_LEN) < 0)
goto cleanup;
+
+if (virBitmapIsAllClear(hugepage->nodemask)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Invalid value of 'nodeset': %s"), nodeset);
+goto cleanup;
+}
}
ret = 0;
@@ -13487,13 +13506,14 @@ virDomainThreadSchedParse(xmlNodePtr node,
goto error;
}
-if (!virBitmapParse(tmp, 0, &sp->ids,
-VIR_DOMAIN_CPUMASK_LEN) ||
-virBitmapIsAllClear(sp->ids) ||
+if (virBitmapParse(tmp, 0, &sp->ids, VIR_DOMAIN_CPUMASK_LEN) < 0)
+goto error;
+
+if (virBitmapIsAllClear(sp->ids) ||
virBitmapNextSetBit(sp->ids, -1) < minid ||
virBitmapLastSetBit(sp->ids) > maxid) {
-virReportError(VIR_ERR_XML_ERROR,
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Invalid value of '%s': %s"),
name, tmp);
goto error;
@@ -13861,6 +13881,13 @@ virDomainDefParseXML(xmlDocPtr xml,
if (virBitmapParse(tmp, 0, &def->cpumask,
VIR_DOMAIN_CPUMASK_LEN) < 0)
goto error;
+
+if (virBitmapIsAllClear(def->cpumask)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Invalid value of 'cpuset': %s"), tmp);
+goto error;
+}
+
VIR_FREE(tmp);
}
}
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index 8a0f686..7ad3f66 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -178,6 +178,12 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr numa,
if (virBitmapParse(tmp, 0, &mem_node->nodeset,
VIR_DOMAIN_CPUMASK_LEN) < 0)
goto cleanup;
+
+if (virBitmapIsAllClear(mem_node->nodeset)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Invalid value of 'nodeset': %s"), tmp);
+goto cleanup;
+}
VIR_FREE(tmp);
}
@@ -233,10 +239,19 @@ virDomainNumatuneParseXML(virDomainNumaPtr numa,
}
VIR_FREE(tmp);
-if ((tmp = virXMLPropString(node, "nodeset")) &&
-virBitmapParse(tmp, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
-goto cleanup;
-VIR_FREE(tmp);
+tmp = virXMLPropString(node, "nodeset");
+if (tmp) {
+if (virBitmapParse(tmp, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
+goto cleanup;
+
+if (virBitmapIsAllClear(nodeset)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Invalid value of 'nodeset': %s"), tmp);
+goto cleanup;
+}
+
+VIR_FREE(tmp);
+}
}
if (virDomainNumatuneSet(numa,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f37a11e..cbb6e1b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -
Re: [libvirt] [PATCH v2 2/9] Convert virDomainPinIsDuplicate into bool return
On Fri, Apr 10, 2015 at 17:36:20 -0400, John Ferlan wrote: > Signed-off-by: John Ferlan > --- > src/conf/domain_conf.c | 8 > src/conf/domain_conf.h | 6 +++--- > 2 files changed, 7 insertions(+), 7 deletions(-) ACK, by the way, the function is exported, but is used only in conf/domain_conf.c thus it could be converted to static. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/9] Rename qemuCheckIothreads to qemuCheckIOThreads
On Fri, Apr 10, 2015 at 17:36:19 -0400, John Ferlan wrote: > Signed-off-by: John Ferlan > --- > src/qemu/qemu_command.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) ACK, Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 9/9] storage: Add duplicate devices check for zfs pool def
On Thu, Apr 02, 2015 at 13:39:46 -0400, John Ferlan wrote: > Check proposed pool definitions to ensure they aren't trying to use the > same devices as currently defined definitions - disallow the duplicate > > Signed-off-by: John Ferlan > --- > src/conf/storage_conf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) ACK, Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] storage: Add duplicate host check for Gluster pool def
On Thu, Apr 02, 2015 at 13:39:45 -0400, John Ferlan wrote: > Check proposed pool definitions to ensure they aren't trying to use the > same host as currently defined definitions - disallow the duplicate > > Signed-off-by: John Ferlan > --- > src/conf/storage_conf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Same problem as 7/9. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] storage: Add duplicate host check for Sheepdog pool def
On Thu, Apr 02, 2015 at 13:39:44 -0400, John Ferlan wrote: > Check proposed pool definitions to ensure they aren't trying to use the > same host as currently defined definitions - disallow the duplicate This statement is invalid. Multiple pols can be hosted on a single host. The check needs to do better than just check the host name. Port and pool path may differ denoting a different pool. Btw same host can be described using multiple host strings so it also isn't absolute. > > Signed-off-by: John Ferlan > --- > src/conf/storage_conf.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index 5f1c151..5db7478 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -2427,9 +2427,12 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, > case VIR_STORAGE_POOL_DISK: > matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); > break; > +case VIR_STORAGE_POOL_SHEEPDOG: > +if (matchPoolSourceHost(&pool->def->source, &def->source)) > +matchpool = pool; > +break; Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/9] storage: Remove default from switch in virStoragePoolSourceFindDuplicate
On Thu, Apr 02, 2015 at 13:39:43 -0400, John Ferlan wrote: > So that we can cover all the cases. > > Signed-off-by: John Ferlan > --- > src/conf/storage_conf.c | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) ACK, Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/9] storage: Use matchPoolSourceHost for NETFS
On Thu, Apr 02, 2015 at 13:39:42 -0400, John Ferlan wrote: > Rather than have duplicate code doing the same check, have the netfs > matching processing code use the new matchPoolSourceHost. > > Signed-off-by: John Ferlan > --- > src/conf/storage_conf.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index b3e930b..6ed0aa9 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -2348,9 +2348,8 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, > matchpool = pool; > break; > case VIR_STORAGE_POOL_NETFS: > -if ((STREQ(pool->def->source.dir, def->source.dir)) \ > -&& (pool->def->source.nhost == 1 && def->source.nhost == 1) \ > -&& (STREQ(pool->def->source.hosts[0].name, > def->source.hosts[0].name))) > +if (STREQ(pool->def->source.dir, def->source.dir) && > +matchPoolSourceHost(&pool->def->source, &def->source)) > matchpool = pool; > break; > case VIR_STORAGE_POOL_SCSI: ACK, no semantic change. (But of course the patch will need a change after renaming the function. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/9] storage: Create matchPoolSourceHost
On Thu, Apr 02, 2015 at 13:39:41 -0400, John Ferlan wrote:
> Split out the nhost == 1 and hosts[0].name logic into a separate routine
>
> Signed-off-by: John Ferlan
> ---
> src/conf/storage_conf.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index e4cb54b..b3e930b 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2290,6 +2290,17 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
> return false;
> }
>
> +static bool
> +matchPoolSourceHost(virStoragePoolSourcePtr poolsrc,
Same compliant for the function name as in 1/9.
> +virStoragePoolSourcePtr defsrc)
> +{
> +/* NB: nhost cannot be > 1 */
> +if (poolsrc->nhost == 0 || defsrc->nhost == 0)
> +return false;
And this condition can be made explicitly state the same without the
need for the comment.
ACK with the name and condition changed.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/9] storage: Invert logic for matchISCSISource
On Thu, Apr 02, 2015 at 13:39:40 -0400, John Ferlan wrote:
> Invert the logic for better readability/flow and futher separation
>
> Signed-off-by: John Ferlan
> ---
> src/conf/storage_conf.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index fa7a7f9..e4cb54b 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2297,18 +2297,19 @@ matchISCSISource(virStoragePoolObjPtr matchpool,
> {
> virStoragePoolSourcePtr poolsrc = &matchpool->def->source;
> virStoragePoolSourcePtr defsrc = &def->source;
> -if (poolsrc->nhost == 1 && defsrc->nhost == 1) {
> -if (STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
> -if (poolsrc->initiator.iqn && defsrc->initiator.iqn) {
> -if (STREQ(poolsrc->initiator.iqn, defsrc->initiator.iqn))
> -return true;
> -else
> -return false;
> -}
> -return true;
> -}
> -}
> -return false;
> +
> +
> +/* NB: nhost cannot be > 1 */
Remove this comment ...
> +if (poolsrc->nhost == 0 || defsrc->nhost == 0)
change the conditions to != 1 so that the comment is redundant.
> +return false;
> +
> +if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name))
> +return false;
> +
> +if (STRNEQ_NULLABLE(poolsrc->initiator.iqn, defsrc->initiator.iqn))
> +return false;
> +
> +return true;
> }
... and squash this together with patches 1 and 2. Doing the refactor
right away is probably better a in this case.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-glib] Support setting of compat XML node
Thanks a lot Christophe! Regards Richa On Mon, Apr 13, 2015 at 2:20 AM, Christophe Fergeau wrote: > Hey Richa, > > On Sun, Apr 12, 2015 at 03:11:41PM -0700, Richa Sehgal wrote: > > This change adds support for setting of compat XML node in libvirt > > gconfig storage volumes target > > Looks good to me, I've now pushed it with a slightly reworked commit > message: > > https://libvirt.org/git/?p=libvirt-glib.git;a=commit;h=4dad5390bbcb6afd8b733b8394c3d96529846557 > > Christophe > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/9] storage: Refactor iSCSI Source matching
On Thu, Apr 02, 2015 at 13:39:38 -0400, John Ferlan wrote:
> Create a separate iSCSI Source matching subroutine. Makes the calling
> code a bit cleaner as well as sets up for future patches which need to
> do better source hosts[0].name processing/checking
>
> Signed-off-by: John Ferlan
> ---
> src/conf/storage_conf.c | 35 ---
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 8b1898b..4a38416 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2291,6 +2291,28 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
> }
>
>
> +static bool
> +matchISCSISource(virStoragePoolObjPtr matchpool,
Please use the virStorageConf... prefix for the new function.
> + virStoragePoolDefPtr def)
> +{
> +if (matchpool->def->source.nhost == 1 && def->source.nhost == 1) {
> +if (STREQ(matchpool->def->source.hosts[0].name,
> + def->source.hosts[0].name)) {
> +if ((matchpool->def->source.initiator.iqn) &&
> +(def->source.initiator.iqn)) {
> +if (STREQ(matchpool->def->source.initiator.iqn,
> + def->source.initiator.iqn))
> +return true;
> +else
> +return false;
Um, how about return STREQ(... ?
> +}
> +return true;
> +}
> +}
> +return false;
> +}
> +
> +
> int
> virStoragePoolSourceFindDuplicate(virConnectPtr conn,
>virStoragePoolObjListPtr pools,
> @@ -2390,17 +2412,8 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn,
> case VIR_STORAGE_POOL_ISCSI:
> matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def);
> if (matchpool) {
> -if (matchpool->def->source.nhost == 1 && def->source.nhost
> == 1) {
> -if (STREQ(matchpool->def->source.hosts[0].name,
> def->source.hosts[0].name)) {
> -if ((matchpool->def->source.initiator.iqn) &&
> (def->source.initiator.iqn)) {
> -if (STREQ(matchpool->def->source.initiator.iqn,
> def->source.initiator.iqn))
> -break;
> -matchpool = NULL;
> -}
> -break;
> -}
> -}
> -matchpool = NULL;
> +if (!matchISCSISource(matchpool, def))
> +matchpool = NULL;
> }
> break;
> case VIR_STORAGE_POOL_FS:
ACK if you rename the function and remove the redundant if.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-glib] Support setting of compat XML node
Hey Richa, On Sun, Apr 12, 2015 at 03:11:41PM -0700, Richa Sehgal wrote: > This change adds support for setting of compat XML node in libvirt > gconfig storage volumes target Looks good to me, I've now pushed it with a slightly reworked commit message: https://libvirt.org/git/?p=libvirt-glib.git;a=commit;h=4dad5390bbcb6afd8b733b8394c3d96529846557 Christophe pgpoJOJ20hUre.pgp Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-glib] gconfig: Fix small leak in test-domain-create
The object returned by gvir_config_domain_disk_get_driver() must be unref'ed when no longer used. --- libvirt-gconfig/tests/test-domain-create.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libvirt-gconfig/tests/test-domain-create.c b/libvirt-gconfig/tests/test-domain-create.c index eb4b945..417d3d0 100644 --- a/libvirt-gconfig/tests/test-domain-create.c +++ b/libvirt-gconfig/tests/test-domain-create.c @@ -284,6 +284,7 @@ int main(int argc, char **argv) g_assert(gvir_config_domain_disk_driver_get_copy_on_read(driver)); g_assert(gvir_config_domain_disk_get_target_bus(disk) == GVIR_CONFIG_DOMAIN_DISK_BUS_IDE); g_str_const_check(gvir_config_domain_disk_get_target_dev(disk), "hda"); +g_object_unref(driver); /* network interfaces node */ -- 2.3.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] luajit binding
Hey, On Fri, Apr 10, 2015 at 05:01:55AM +, William Adams wrote: > I have started a luajit language binding for libvirt. It can be found here: I don't know how well https://github.com/pavouk/lgi/ works, but one alternative to writing lua bindings for libvirt would be to try to use libvirt-glib in LUA through lgi (or similar projects). Christophe pgpTY1HO2gyHK.pgp Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] qemuMigrationPrecreateDisk: Preserve sparse files
On Fri, Apr 10, 2015 at 17:25:41 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=817700
This patch certainly does not resolve this bug. See below ...
>
> When pre-creating a disk on the destination, a volume XML is
> constructed. The XML is then passed to virStorageVolCreateXML() which
> does the work. But, since there's no in the XML, the
> disk are fully allocated. This possibly breaks sparse allocation user
> has on the migration source.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_migration.c | 30 ++
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index d4757e4..7a40548 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -144,6 +144,7 @@ typedef qemuMigrationCookieNBDDisk
> *qemuMigrationCookieNBDDiskPtr;
> struct _qemuMigrationCookieNBDDisk {
> char *target; /* Disk target */
> unsigned long long capacity;/* And its capacity */
> +unsigned long long allocation; /* And its allocation */
> };
>
> typedef struct _qemuMigrationCookieNBD qemuMigrationCookieNBD;
> @@ -593,6 +594,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
> disk->dst) < 0)
> goto cleanup;
> mig->nbd->disks[mig->nbd->ndisks].capacity = entry->capacity;
> +mig->nbd->disks[mig->nbd->ndisks].allocation =
> entry->wr_highest_offset;
This "allocation" value works only for thin-provisioned LVM with qcow2
inside. Qemu docs state the following:
- "wr_highest_offset": Highest offset of a sector written since the
BlockDriverState has been opened (json-int)
As the documentation hints this information doesn't make much sense for
regular files. The file may (and certainly will) be sparse in between of
the start and the highest offset.
Additionally since the docs state that the value is "since it has been
opened" the number may actually be lower than the count of blocks that
are used.
> mig->nbd->ndisks++;
> }
>
...
> @@ -1541,6 +1560,8 @@ qemuMigrationPrecreateDisk(virConnectPtr conn,
> virBufferAdjustIndent(&buf, 2);
> virBufferEscapeString(&buf, "%s\n", volName);
> virBufferAsprintf(&buf, "%llu\n", nbd->capacity);
> +if (nbd->allocation)
> +virBufferAsprintf(&buf, "%llu\n",
> nbd->allocation);
You add this to the snippet that is used to pre-create the file, but ..
> virBufferAddLit(&buf, "\n");
> virBufferAdjustIndent(&buf, 2);
> virBufferAsprintf(&buf, "\n", format);
The @flags variable in this function is never touched for raw files. For
qcow2 files, full preallocation is not supported. This means that the
@allocation info is never used.
> @@ -1585,8 +1606,9 @@ qemuMigrationPrecreateStorage(virConnectPtr conn,
> int indx;
> const char *diskSrcPath;
>
> -VIR_DEBUG("Looking up disk target '%s' (capacity=%llu)",
> - nbd->disks[i].target, nbd->disks[i].capacity);
> +VIR_DEBUG("Looking up disk target '%s' (capacity=%llu
> allocation=%llu)",
> + nbd->disks[i].target, nbd->disks[i].capacity,
> + nbd->disks[i].allocation);
>
> if ((indx = virDomainDiskIndexByName(vm->def,
> nbd->disks[i].target, false)) <
> 0) {
The bugreport states that the user actually pre-created the file on the
destination as sparse so everything in this patch actually won't fix the
original bug.
The problem is that the block-copy job in qemu copies the entire disk
even with blocks that were not touched. For the reporter the fix would
probably be to use qcow2 as it should only copy the blocks that were
touched by the VM since qemu has the information.
As for this patch: NACK, the code you add doesn't do anything useful.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Don't output tag if it contains no information.
On Mon, 2015-04-13 at 09:39 +0200, Erik Skultety wrote: > Other than that, it looks good, so I removed the comments marked above > and pushed. Congratulations to your first patch in libvirt ;). Thank you for reviewing and pushing the patch! Have a nice day. -- Andrea Bolognani -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] An implementation bug in qemuMigrationWaitForCompletion that introduces an unnecessary sleep of 50 ms when a migration completes
On 10.04.2015 09:49, Michal Privoznik wrote:
> On 10.04.2015 00:32, Xing Lin wrote:
>>
>
> Yeah, I guess it's very unlikely that migration will complete within
> first 50ms after issuing the command on the monitor (in which case we
> would again wait pointlessly). The other approach would be to leave the
> sleep() where it is, but enclose it in if (jobInfo->type ==
> VIR_DOMAIN_JOB_UNBOUNDED) {}. I have no preference over these two
> versions though. So ACK to the patch of yours. However, I'll postpone
> merging the patch for a while and let others express their feelings.
>
As promised, pushed now.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] qemuMigrationPrecreateStorage: Fix debug message
On Fri, Apr 10, 2015 at 17:25:39 +0200, Michal Privoznik wrote: > When pre-creating storage for domains, we need to find corresponding > disk in the XML on the destination (domain XML may differ there, e.g. > disk is accessible under different path). For better debugging, I'm > printing all info I received on a disk. But there was a typo when > printing the disk capacity: "%lluu" instead of "%llu". > > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_migration.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) ACK, Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] Rewrite usb device version parsing
On Fri, Apr 10, 2015 at 16:28:24 +0200, Ján Tomko wrote:
> Simplify the function by leaving out the local copy and checking
> return values of virStrToLong.
> ---
> src/conf/domain_conf.c | 66
> +++---
> 1 file changed, 20 insertions(+), 46 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 65e2bac..bea98a1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11306,66 +11306,40 @@ virDomainRedirdevDefParseXML(xmlNodePtr node,
> }
>
> /*
> - * This is the helper function to convert USB version from a
> + * This is the helper function to convert USB device version from a
> * format of JJ.MN to a format of 0xJJMN where JJ is the major
> * version number, M is the minor version number and N is the
> * sub minor version number.
> - * e.g. USB 2.0 is reported as 0x0200,
> - * USB 1.1 as 0x0110 and USB 1.0 as 0x0100.
> + * e.g. USB version 2.0 is reported as 0x0200,
> + * USB version 4.07 as 0x0407
> */
> static int
> virDomainRedirFilterUSBVersionHelper(const char *version,
> virDomainRedirFilterUSBDevDefPtr def)
> {
> -char *version_copy = NULL;
> -char *temp = NULL;
> -int ret = -1;
> -size_t len;
> -size_t fraction_len;
> -unsigned int major;
> -unsigned int minor;
> -unsigned int hex;
> -
> -if (VIR_STRDUP(version_copy, version) < 0)
> -return -1;
> +unsigned int major, minor;
> +char *s = NULL;
>
> -len = strlen(version_copy);
> -/*
> - * The valid format of version is like 01.10, 1.10, 1.1, etc.
> - */
> -if (len > 5 ||
> -!(temp = strchr(version_copy, '.')) ||
> -temp - version_copy < 1 ||
> -temp - version_copy > 2 ||
> -!(fraction_len = strlen(temp + 1)) ||
> -fraction_len > 2) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Incorrect USB version format %s"), version);
> -goto cleanup;
> -}
> +if ((virStrToLong_ui(version, &s, 10, &major)) < 0 ||
> +*s++ != '.' ||
> +(virStrToLong_ui(s, NULL, 10, &minor)) < 0)
> +goto error;
>
> -*temp = '\0';
> -temp++;
> +if (major >= 100 || minor >= 100)
> +goto error;
>
> -if ((virStrToLong_ui(version_copy, NULL, 10, &major)) < 0 ||
> -(virStrToLong_ui(temp, NULL, 10, &minor)) < 0) {
> -virReportError(VIR_ERR_XML_ERROR,
> - _("Cannot parse USB version %s"), version);
> -goto cleanup;
> -}
> +if (strlen(s) == 1)
> +minor *= 10;
Humm, do we really want to fix user input in this case? I think that it
makes sense but a comment explaining what that part does would be
actually helpful.
>
> -hex = (major / 10) << 12 | (major % 10) << 8;
> -if (fraction_len == 1)
> -hex |= (minor % 10) << 4;
> -else
> -hex |= (minor / 10) << 4 | (minor % 10) << 0;
> +def->version = (major / 10) << 12 | (major % 10) << 8 |
> + (minor / 10) << 4 | (minor % 10) << 0;
>
> -def->version = hex;
> -ret = 0;
> +return 0;
>
> - cleanup:
> -VIR_FREE(version_copy);
> -return ret;
> + error:
> +virReportError(VIR_ERR_XML_ERROR,
> + _("Cannot parse USB device version %s"), version);
> +return -1;
> }
ACK with the comment added. It looks much better now.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Fix usb device version parsing issues
On Fri, Apr 10, 2015 at 16:28:23 +0200, Ján Tomko wrote:
> Request that the number be parsed as decimal, to allow 08
> and 09.
>
> Format it with the leading zero, 1.01 and 1.10 are two
> different versions.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1210650
> ---
> src/conf/domain_conf.c | 6 +--
> .../qemuxml2argv-usb-redir-filter-version.args | 19 +
> .../qemuxml2argv-usb-redir-filter-version.xml | 46
> ++
> tests/qemuxml2argvtest.c | 6 +++
> .../qemuxml2xmlout-usb-redir-filter-version.xml| 46
> ++
> tests/qemuxml2xmltest.c| 1 +
> 6 files changed, 121 insertions(+), 3 deletions(-)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter-version.args
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter-version.xml
> create mode 100644
> tests/qemuxml2xmloutdata/qemuxml2xmlout-usb-redir-filter-version.xml
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1763305..65e2bac 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11347,8 +11347,8 @@ virDomainRedirFilterUSBVersionHelper(const char
> *version,
> *temp = '\0';
> temp++;
>
> -if ((virStrToLong_ui(version_copy, NULL, 0, &major)) < 0 ||
> -(virStrToLong_ui(temp, NULL, 0, &minor)) < 0) {
> +if ((virStrToLong_ui(version_copy, NULL, 10, &major)) < 0 ||
> +(virStrToLong_ui(temp, NULL, 10, &minor)) < 0) {
> virReportError(VIR_ERR_XML_ERROR,
> _("Cannot parse USB version %s"), version);
> goto cleanup;
lol, funny bug
> @@ -20209,7 +20209,7 @@ virDomainRedirFilterDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, " product='0x%04X'", usbdev->product);
>
> if (usbdev->version >= 0)
> -virBufferAsprintf(buf, " version='%d.%d'",
> +virBufferAsprintf(buf, " version='%d.%02d'",
> ((usbdev->version & 0xf000) >> 12) * 10 +
> ((usbdev->version & 0x0f00) >> 8),
> ((usbdev->version & 0x00f0) >> 4) * 10 +
This too
> diff --git
> a/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter-version.args
> b/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter-version.args
> new file mode 100644
> index 000..7656ac4
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter-version.args
> @@ -0,0 +1,19 @@
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
> QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu -S \
> +-M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
> +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c \
> +-device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 \
> +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,\
> +multifunction=on,addr=0x4 \
> +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1 \
> +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x4.0x2 \
> +-chardev spicevmc,id=charredir0,name=usbredir \
> +-device 'usb-redir,chardev=charredir0,id=redir0,\
> +filter=0x08:0x15E1:0x2007:0x0109:1|0x08:0x15E1:0x2007:0x0940:1|\
> +-1:-1:-1:-1:0,bus=usb.0,port=4' \
> +-chardev spicevmc,id=charredir1,name=usbredir \
> +-device 'usb-redir,chardev=charredir1,id=redir1,\
> +filter=0x08:0x15E1:0x2007:0x0109:1|0x08:0x15E1:0x2007:0x0940:1|\
> +-1:-1:-1:-1:0,bus=usb.0,port=5' \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter-version.xml
> b/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter-version.xml
> new file mode 100644
> index 000..f1189c9
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter-version.xml
> @@ -0,0 +1,46 @@
> +
> + QEMUGuest1
> + c7a5fdbd-edaf-9455-926a-d65c16db1809
> + 219136
> + 219136
> + 1
> +
> +hvm
> +
> +
> +
> + destroy
> + restart
> + destroy
> +
> +/usr/bin/qemu
> +
> + function='0x7'/>
> +
> +
> +
> + function='0x0' multifunction='on'/>
> +
> +
> +
> + function='0x1'/>
> +
> +
> +
> + function='0x2'/>
> +
Does the test need all the controllers above?
> +
> +
> +
> +
> +
> +
> +
> +
> + allow='yes'/>
> + allow='yes'/>
> +
> +
> +
> +
> +
ACK,
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Don't output tag if it contains no information.
On 04/10/2015 03:09 PM, Andrea Bolognani wrote:
> The tag is already marked as optional in the schema, so no changes
> are needed there.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1202606
> ---
>
> Hi everyone,
>
> this is my first contribution to libvirt so I wholeheartedly welcome
> any criticism, suggestion or recommendation :)
>
> Cheers.
>
> src/conf/cpu_conf.c| 33
> --
> tests/qemuxml2argvdata/qemuxml2argv-cpu-empty.xml | 23 +++
> .../qemuxml2xmlout-cpu-empty.xml | 21 ++
> tests/qemuxml2xmltest.c| 1 +
> 4 files changed, 69 insertions(+), 9 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-empty.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-empty.xml
>
> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> index cd3882d..8f65d55 100644
> --- a/src/conf/cpu_conf.c
> +++ b/src/conf/cpu_conf.c
> @@ -435,13 +435,14 @@ virCPUDefFormatBufFull(virBufferPtr buf,
> bool updateCPU)
> {
> int ret = -1;
> +virBuffer attributeBuf = VIR_BUFFER_INITIALIZER;
> virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
> int indent = virBufferGetIndent(buf, false);
>
> if (!def)
> return 0;
>
> -virBufferAddLit(buf, " +/* Format attributes */
> if (def->type == VIR_CPU_TYPE_GUEST) {
> const char *tmp;
>
> @@ -451,7 +452,7 @@ virCPUDefFormatBufFull(virBufferPtr buf,
> _("Unexpected CPU mode %d"), def->mode);
> goto cleanup;
> }
> -virBufferAsprintf(buf, " mode='%s'", tmp);
> +virBufferAsprintf(&attributeBuf, " mode='%s'", tmp);
> }
>
> if (def->model &&
> @@ -463,10 +464,11 @@ virCPUDefFormatBufFull(virBufferPtr buf,
> def->match);
> goto cleanup;
> }
> -virBufferAsprintf(buf, " match='%s'", tmp);
> +virBufferAsprintf(&attributeBuf, " match='%s'", tmp);
> }
> }
>
> +/* Format children */
> virBufferAdjustIndent(&childrenBuf, indent + 2);
> if (def->arch)
> virBufferAsprintf(&childrenBuf, "%s\n",
> @@ -477,16 +479,29 @@ virCPUDefFormatBufFull(virBufferPtr buf,
> if (virDomainNumaDefCPUFormat(&childrenBuf, numa) < 0)
> goto cleanup;
>
> -if (virBufferUse(&childrenBuf)) {
> -virBufferAddLit(buf, ">\n");
> -virBufferAddBuffer(buf, &childrenBuf);
> -virBufferAddLit(buf, "\n");
> -} else {
> -virBufferAddLit(buf, "/>\n");
> +/* Put it all together */
> +if (virBufferUse(&attributeBuf) || virBufferUse(&childrenBuf)) {
> +
> +/* Opening tag */
Just some minor nitpicks:
Although I love the idea of commenting the code to make it as much
understandable as possible :), I think this one ^^ comments the obvious...
> +virBufferAddLit(buf, " +
> +/* Attributes (if any) */
same here ^^...
> +if (virBufferUse(&attributeBuf))
> +virBufferAddBuffer(buf, &attributeBuf);
> +
> +/* Children (if any) and closing tag */
same here ^^
> +if (virBufferUse(&childrenBuf)) {
> +virBufferAddLit(buf, ">\n");
> +virBufferAddBuffer(buf, &childrenBuf);
> +virBufferAddLit(buf, "\n");
> +} else {
> +virBufferAddLit(buf, "/>\n");
> +}
> }
>
> ret = 0;
> cleanup:
> +virBufferFreeAndReset(&attributeBuf);
> virBufferFreeAndReset(&childrenBuf);
> return ret;
> }
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-empty.xml
> b/tests/qemuxml2argvdata/qemuxml2argv-cpu-empty.xml
> new file mode 100644
> index 000..2a79826
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-empty.xml
> @@ -0,0 +1,23 @@
> +
> + cpu-empty
> + 1aed4c39-ad6e-4a78-9264-4ce996290d17
> + 4000768
> + 1048576
> + 1
> +
> +hvm
> +
> +
> +
> +
> +
> + destroy
> + restart
> + destroy
> +
> +/usr/bin/qemu-kvm
> +
> +
> +
> +
> +
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-empty.xml
> b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-empty.xml
> new file mode 100644
> index 000..e678607
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-empty.xml
> @@ -0,0 +1,21 @@
> +
> + cpu-empty
> + 1aed4c39-ad6e-4a78-9264-4ce996290d17
> + 4000768
> + 1048576
> + 1
> +
> +hvm
> +
> +
> +
> + destroy
> + restart
> + destroy
> +
> +/usr/bin/qemu-kvm
> +
> +
> +
> +
> +
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 817e408..cd0b280 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -361,6 +361,7 @@ mymain(void)
>
> DO_TEST("clock-utc");
> DO_TEST("clock-localtime");
> +DO_TEST_DIFFERENT("cpu-emp
Re: [libvirt] [PATCH 2/2] Rewrite vshParseCPUList
On Fri, Apr 10, 2015 at 16:32:33 +0200, Ján Tomko wrote:
> Use virBitmap helpers that were added after this function.
>
> Change cpumaplen to int and fill it out by this function.
> ---
> tools/virsh-domain.c | 112
> +--
> 1 file changed, 19 insertions(+), 93 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index d5352d7..90e23aa 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6335,95 +6335,23 @@ vshPrintPinInfo(unsigned char *cpumap, size_t
> cpumaplen)
> }
>
> static unsigned char *
> -vshParseCPUList(vshControl *ctl, const char *cpulist,
> -int maxcpu, size_t cpumaplen)
> +vshParseCPUList(int *cpumaplen, const char *cpulist, int maxcpu)
Hmm virBitmapToData could be refactored to take a size_t rather than
using int here
> {
> unsigned char *cpumap = NULL;
> -const char *cur;
> -bool unuse = false;
> -int cpu, lastcpu;
> -size_t i;
> -
> -cpumap = vshCalloc(ctl, cpumaplen, sizeof(*cpumap));
> -
> -/* Parse cpulist */
> -cur = cpulist;
> -if (*cur == 'r') {
> -for (cpu = 0; cpu < maxcpu; cpu++)
> -VIR_USE_CPU(cpumap, cpu);
> -return cpumap;
> -} else if (*cur == 0) {
> -goto error;
> -}
> -
> -while (*cur != 0) {
> -/* The char '^' denotes exclusive */
> -if (*cur == '^') {
> -cur++;
> -unuse = true;
> -}
> -
> -/* Parse physical CPU number */
> -if (!c_isdigit(*cur))
> -goto error;
> -
> -if ((cpu = virParseNumber(&cur)) < 0)
> -goto error;
> +virBitmapPtr map = NULL;
>
> -if (cpu >= maxcpu) {
> -vshError(ctl, _("Physical CPU %d doesn't exist."), cpu);
> -goto cleanup;
> -}
> -
> -virSkipSpaces(&cur);
> -
> -if (*cur == ',' || *cur == 0) {
> -if (unuse)
> -VIR_UNUSE_CPU(cpumap, cpu);
> -else
> -VIR_USE_CPU(cpumap, cpu);
> -} else if (*cur == '-') {
> -/* The char '-' denotes range */
> -if (unuse)
> -goto error;
> -cur++;
> -virSkipSpaces(&cur);
> -
> -/* Parse the end of range */
> -lastcpu = virParseNumber(&cur);
> -
> -if (lastcpu < cpu)
> -goto error;
> -
> -if (lastcpu >= maxcpu) {
> -vshError(ctl, _("Physical CPU %d doesn't exist."), lastcpu);
> -goto cleanup;
> -}
> -
> -for (i = cpu; i <= lastcpu; i++)
> -VIR_USE_CPU(cpumap, i);
> -
> -virSkipSpaces(&cur);
> -}
> -
> -if (*cur == ',') {
> -cur++;
> -virSkipSpaces(&cur);
> -unuse = false;
> -} else if (*cur == 0) {
> -break;
> -} else {
> -goto error;
> -}
> +if (cpulist[0] == 'r') {
> +if (!(map = virBitmapNew(maxcpu)))
> +return NULL;
> +virBitmapSetAll(map);
> +} else {
> +if (virBitmapParse(cpulist, '\0', &map, maxcpu) < 0)
> +return NULL;
> }
>
> +if (virBitmapToData(map, &cpumap, cpumaplen) < 0)
> +virBitmapFree(map);
@map needs to be freed on success too.
> return cpumap;
> -
> - error:
> -vshError(ctl, "%s", _("cpulist: Invalid format."));
> - cleanup:
> -VIR_FREE(cpumap);
> -return NULL;
> }
>
> static bool
> @@ -6434,7 +6362,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
> const char *cpulist = NULL;
> bool ret = false;
> unsigned char *cpumap = NULL;
> -size_t cpumaplen;
> +int cpumaplen;
And changing the types here on ...
> int maxcpu, ncpus;
> size_t i;
> bool config = vshCommandOptBool(cmd, "config");
> @@ -6473,7 +6401,6 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
>
> if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0)
> return false;
> -cpumaplen = VIR_CPU_MAPLEN(maxcpu);
>
> if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> return false;
> @@ -6495,6 +6422,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
> goto cleanup;
> }
>
> +cpumaplen = VIR_CPU_MAPLEN(maxcpu);
> cpumap = vshMalloc(ctl, ncpus * cpumaplen);
> if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
> cpumaplen, flags)) >= 0) {
> @@ -6514,7 +6442,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
> }
> } else {
> /* Pin mode: pinning specified vcpu to specified physical cpus*/
> -if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen)))
> +if (!(cpumap = vshParseCPUList(&cpumaplen, cpulist, maxcpu)))
> goto cleanup;
>
> if (flags == -1) {
> @@ -6580,7 +6508,7 @@ cmdEmulatorPin(vshCo
Re: [libvirt] [PATCH 1/3] Do xml->xml test for usb-redir-filter
On Fri, Apr 10, 2015 at 16:28:22 +0200, Ján Tomko wrote: > We don't format the default '-1' fields back. > --- > .../qemuxml2xmlout-usb-redir-filter.xml| 45 > ++ > tests/qemuxml2xmltest.c| 1 + > 2 files changed, 46 insertions(+) > create mode 100644 > tests/qemuxml2xmloutdata/qemuxml2xmlout-usb-redir-filter.xml ACK. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Rewrite vshPrintPinInfo
On Fri, Apr 10, 2015 at 16:32:32 +0200, Ján Tomko wrote:
> Use virBitmapDataToString instead of constructing the ranges bit
> by bit, remove the checking of parameters (that is already done
> by the callers).
>
> Let the callers choose the right bitmap, since there's only
> one that uses this helper on a matrix-in-an-array.
> ---
> tools/virsh-domain.c | 41 ++---
> 1 file changed, 10 insertions(+), 31 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 928360c..d5352d7 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
...
> @@ -6526,7 +6505,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
> continue;
>
> vshPrint(ctl, "%4zu: ", i);
> -ret = vshPrintPinInfo(cpumap, cpumaplen, maxcpu, i);
> +ret = vshPrintPinInfo(VIR_GET_CPUMAP(cpumap, cpumaplen, i),
> + cpumaplen);
> vshPrint(ctl, "\n");
> if (!ret)
> break;
> @@ -6643,12 +6623,12 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd)
> flags = VIR_DOMAIN_AFFECT_CURRENT;
>
> cpumaps = vshMalloc(ctl, cpumaplen);
> -if (virDomainGetEmulatorPinInfo(dom, cpumaps,
> +if (virDomainGetEmulatorPinInfo(dom, cpumap,
@cpumap is NULL at this point. virDomainGetEmulatorPinInfo() requires
that it's non-NULL. Additionally after this change @cpumaps is unused
just allocated and freed.
> cpumaplen, flags) >= 0) {
> vshPrintExtra(ctl, "%s %s\n", _("emulator:"), _("CPU Affinity"));
> vshPrintExtra(ctl, "--\n");
> vshPrintExtra(ctl, " *: ");
> -ret = vshPrintPinInfo(cpumaps, cpumaplen, maxcpu, 0);
> +ret = vshPrintPinInfo(cpumap, cpumaplen);
> vshPrint(ctl, "\n");
> }
> VIR_FREE(cpumaps);
ACK if you alocate @cpumap before the call and remove @cpumaps.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv1 12/13] Rework qemu-img command generation for inactive external snapshots
On Fri, Apr 10, 2015 at 14:59:04 +0200, Ján Tomko wrote:
> Reuse the code from storage backend.
>
> This also fixes the backing_fmd typo by removing it.
> ---
> src/qemu/qemu_driver.c| 46
> ++-
> src/storage/storage_backend.c | 2 +-
> 2 files changed, 11 insertions(+), 37 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4f14546..3ea42f2 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13468,7 +13468,6 @@
> qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
> size_t i;
> virDomainSnapshotDiskDefPtr snapdisk;
> virDomainDiskDefPtr defdisk;
> -virCommandPtr cmd = NULL;
> const char *qemuImgPath;
> virBitmapPtr created = NULL;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> @@ -13489,48 +13488,25 @@
> qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
> if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> continue;
>
> -if (!snapdisk->src->format)
> -snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
> -
> -/* creates cmd line args: qemu-img create -f qcow2 -o */
> -if (!(cmd = virCommandNewArgList(qemuImgPath,
> - "create",
> - "-f",
> -
> virStorageFileFormatTypeToString(snapdisk->src->format),
> - "-o",
> - NULL)))
> +if (defdisk->src->format == VIR_STORAGE_FILE_NONE &&
> +!cfg->allowDiskFormatProbing) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown image format of '%s' and "
> + "format probing is disabled"),
> + defdisk->src->path);
> goto cleanup;
> -
> -if (defdisk->src->format > 0) {
> -/* adds cmd line arg:
> backing_file=/path/to/backing/file,backing_fmd=format */
> -virCommandAddArgFormat(cmd, "backing_file=%s,backing_fmt=%s",
> - defdisk->src->path,
> -
> virStorageFileFormatTypeToString(defdisk->src->format));
> -} else {
> -if (!cfg->allowDiskFormatProbing) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("unknown image format of '%s' and "
> - "format probing is disabled"),
> - defdisk->src->path);
> -goto cleanup;
> -}
> -
> -/* adds cmd line arg: backing_file=/path/to/backing/file */
> -virCommandAddArgFormat(cmd, "backing_file=%s",
> defdisk->src->path);
> }
>
> -/* adds cmd line args: /path/to/target/file */
> -virCommandAddArg(cmd, snapdisk->src->path);
> +if (!snapdisk->src->format)
> +snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
>
> /* If the target does not exist, we're going to create it possibly */
> if (!virFileExists(snapdisk->src->path))
> ignore_value(virBitmapSetBit(created, i));
>
> -if (virCommandRun(cmd, NULL) < 0)
> +if (virStorageFileCreateWithFormat(snapdisk->src,
> snapdisk->src->path,
> + defdisk->src, qemuImgPath) < 0)
This part will need adjusting after my review to the previous patch.
> goto cleanup;
> -
> -virCommandFree(cmd);
> -cmd = NULL;
> }
>
> /* update disk definitions */
> @@ -13554,8 +13530,6 @@
> qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
> ret = 0;
>
> cleanup:
> -virCommandFree(cmd);
> -
> /* unlink images if creation has failed */
> if (ret < 0 && created) {
> ssize_t bit = -1;
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 9ffbc6e..ec8b7b5 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -819,7 +819,7 @@ virStorageBackendCreateQemuImgOpts(char **opts,
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> -if (info.backingPath)
> +if (info.backingPath && info.backingFormat)
In retrospect I think we should forbid snapshots without specifying the
format so that we don't create even more broken configurations.
> virBufferAsprintf(&buf, "backing_fmt=%s,",
>
> virStorageFileFormatTypeToString(info.backingFormat));
> if (info.encryption)
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv1 09/13] Split out qemu-img command generation
On Fri, Apr 10, 2015 at 14:59:01 +0200, Ján Tomko wrote:
> Do not require the volume definition.
> This will allow code reuse when creating snapshots.
> ---
> src/storage/storage_backend.c | 203
> +++---
> 1 file changed, 109 insertions(+), 94 deletions(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index f5b95ec..bfbc193 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -870,38 +870,16 @@ virStorageBackendCreateQemuImgOpts(char **opts,
> return -1;
> }
>
> -/* Create a qemu-img virCommand from the supplied binary path,
> - * volume definitions and imgformat
> - */
> -virCommandPtr
> -virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
> - virStoragePoolObjPtr pool,
> - virStorageVolDefPtr vol,
> - virStorageVolDefPtr inputvol,
> - unsigned int flags,
> - const char *create_tool,
> - int imgformat)
> +static virCommandPtr
> +virStorageBackendCreateQemuImgCmd(const char *create_tool,
> + int imgformat,
> + struct _virStorageBackendQemuImgInfo info)
> {
> virCommandPtr cmd = NULL;
> -const char *type;
> +const char *type = NULL;
> const char *backingType = NULL;
> const char *inputType = NULL;
> char *opts = NULL;
> -struct _virStorageBackendQemuImgInfo info = {
> -.format = vol->target.format,
> -.path = vol->target.path,
> -.encryption = vol->target.encryption != NULL,
> -.preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA),
> -.compat = vol->target.compat,
> -.features = vol->target.features,
> -.nocow = vol->target.nocow,
> -};
> -
> -virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
> -
> -/* Treat output block devices as 'raw' format */
> -if (vol->type == VIR_STORAGE_VOL_BLOCK)
> -info.format = VIR_STORAGE_FILE_RAW;
>
> if (!(type = virStorageFileFormatTypeToString(info.format))) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -926,6 +904,107 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr
> conn,
> return NULL;
> }
>
> +if (info.inputPath &&
> +!(inputType = virStorageFileFormatTypeToString(info.inputFormat))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unknown storage vol type %d"),
> + info.inputFormat);
> +return NULL;
> +}
> +
> +if (info.backingPath) {
> +if (info.preallocate) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("metadata preallocation conflicts with backing"
> + " store"));
> +return NULL;
> +}
> +
> +if (!(backingType =
> virStorageFileFormatTypeToString(info.backingFormat))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unknown storage vol backing store type %d"),
> + info.backingFormat);
> +return NULL;
> +}
> +}
> +
> +/* ignore the backing volume when we're converting a volume */
> +if (info.inputPath) {
> +info.backingPath = NULL;
> +backingType = NULL;
> +}
Shouldn't this go before you bother to check if the backing info is
correct?
> +
> +cmd = virCommandNew(create_tool);
> +
> +if (info.inputPath)
> +virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type,
> NULL);
> +else
> +virCommandAddArgList(cmd, "create", "-f", type, NULL);
> +
> +if (info.backingPath)
> +virCommandAddArgList(cmd, "-b", info.backingPath, NULL);
> +
> +if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) {
> +if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat &&
> +imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT)
> +info.compat = "0.10";
> +
> +if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) {
> +virCommandFree(cmd);
> +return NULL;
> +}
> +if (opts)
> +virCommandAddArgList(cmd, "-o", opts, NULL);
> +VIR_FREE(opts);
> +} else {
> +if (info.backingPath) {
> +if (imgformat == QEMU_IMG_BACKING_FORMAT_FLAG)
> +virCommandAddArgList(cmd, "-F", backingType, NULL);
> +else
> +VIR_DEBUG("Unable to set backing store format for %s with
> %s",
> + info.path, create_tool);
> +}
> +if (info.encryption)
> +virCommandAddArg(cmd, "-e");
> +}
> +
> +if (info.inputPath)
> +virCommandAddArg
