Re: [libvirt] libvirt external snapshot error
23 нояб. 2015 г. 15:13 пользователь "Vasiliy Tolstov" написал: > > 2015-11-23 15:10 GMT+03:00 Peter Krempa : > > The file should not exist prior to the snapshot. If you want to > > pre-create it (with correct size and format), you need to specify > > --reuse-external in that case. Virsh manual already documents that: > > > >If --reuse-external is specified, and the snapshot XML requests an > >external snapshot with a destination of an existing file, then the > >destination must exist and be pre-created with correct format and > >metadata. The file is then reused; otherwise, a snapshot is refused > >to avoid losing contents of the existing files. > > > > > If file is not exists i get error: > error: internal error: unable to execute QEMU command 'transaction': > Could not open '/test.raw': Invalid argument > Any new suggestions? > > -- > Vasiliy Tolstov, > e-mail: v.tols...@selfip.ru -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/8] perf: implement the public APIs for perf event
> -Original Message- > From: Jiri Denemark [mailto:jdene...@redhat.com] > Sent: Tuesday, November 24, 2015 12:25 AM > To: Ren, Qiaowei > Cc: libvir-list@redhat.com > Subject: Re: [libvirt] [PATCH 3/8] perf: implement the public APIs for perf > event > > On Tue, Nov 17, 2015 at 16:00:43 +0800, Qiaowei Ren wrote: > > * src/libvirt-domain.c: Implement virDomainGetPerfEvents and > > virDomainSetPerfEvents. > > > > Signed-off-by: Qiaowei Ren > > --- > > src/libvirt-domain.c | 106 > > +++ > > 1 file changed, 106 insertions(+) > > > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index > > de7eb04..e767606 100644 > > --- a/src/libvirt-domain.c > > +++ b/src/libvirt-domain.c > > @@ -9572,6 +9572,112 @@ virDomainOpenChannel(virDomainPtr dom, > > > > > > /** > > + * virDomainGetPerfEvents: > > + * @domain: pointer to domain object > > + * @params: pointer to perf events parameter object > > + * (return value, allocated by the caller) > > + * @nparams: pointer to number of perf event parameters > > + * > > + * Get all perf events setting. On input, @nparams gives the size of > > + the > > + * @params array; on output, @nparams gives how many slots were > > + filled > > + * with parameter information, which might be less but will not > > + exceed > > + * the input value. > > + * > > + * As a special case, calling with @params as NULL and @nparams as 0 > > + on > > + * input will cause @nparams on output to contain the number of > > + parameters > > + * supported by the hypervisor. The caller should then allocate > > + @params > > + * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call > > + the > > + * API again. > > + * > > + * See virDomainGetMemoryParameters() for an equivalent usage example. > > You took a wrong API to copy. Requiring the caller to run the API with @params > == NULL to get the number of parameters and then pass the allocated @params > to get the values is the original design which we do not use for new APIs. The > API should just allocate @params, fill it with all the values it knows about, > and > return the count in @nparams. In other words, no caller allocated arrays, > please. > You can look at, e.g., virDomainGetJobStats to see how it works. > Thanks, Jirka. I will update this patch according to new API design. Thanks, Qiaowei -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/8] perf: add new public APIs for perf event
> -Original Message-
> From: Jiri Denemark [mailto:jdene...@redhat.com]
> Sent: Monday, November 23, 2015 11:26 PM
> To: Ren, Qiaowei
> Cc: libvir-list@redhat.com
> Subject: Re: [libvirt] [PATCH 1/8] perf: add new public APIs for perf event
>
> On Tue, Nov 17, 2015 at 16:00:41 +0800, Qiaowei Ren wrote:
> > API agreed on in
> > https://www.redhat.com/archives/libvir-list/2015-October/msg00872.html
> >
> > * include/libvirt/libvirt-domain.h (virDomainGetPerfEvents,
> > virDomainSetPerfEvents): New declarations.
> > * src/libvirt_public.syms: Export new symbols.
> >
> > Signed-off-by: Qiaowei Ren
> > ---
> > include/libvirt/libvirt-domain.h | 18 ++
> > src/libvirt_public.syms | 6 ++
> > 2 files changed, 24 insertions(+)
> >
> > diff --git a/include/libvirt/libvirt-domain.h
> > b/include/libvirt/libvirt-domain.h
> > index a1ea6a5..69965e6 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -1802,6 +1802,24 @@ int virDomainListGetStats(virDomainPtr *doms,
> > void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats);
> >
> > /*
> > + * Perf Event API
> > + */
> > +
> > +/**
> > + * VIR_DOMAIN_PERF_CMT:
> > + *
> > + * Macro for typed parameter name that represents CMT perf event.
> > + */
> > +# define VIR_DOMAIN_PERF_CMT "cmt"
> > +
> > +int virDomainGetPerfEvents(virDomainPtr dom,
> > + virTypedParameterPtr params,
> > + int * nparams);
>
> We don't put a space between * and the name.
>
> > +int virDomainSetPerfEvents(virDomainPtr dom,
> > + virTypedParameterPtr params,
> > + int nparams);
> > +
> > +/*
> > * BlockJob API
> > */
> >
> > diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index
> > dd94191..03206e7 100644
> > --- a/src/libvirt_public.syms
> > +++ b/src/libvirt_public.syms
> > @@ -725,4 +725,10 @@ LIBVIRT_1.2.19 {
> > virDomainRename;
> > } LIBVIRT_1.2.17;
> >
> > +LIBVIRT_1.2.23 {
> > +global:
> > +virDomainGetPerfEvents;
> > +virDomainSetPerfEvents;
> > +} LIBVIRT_1.2.19;
> > +
> > # define new API here using predicted next version number
>
> This patch does not compile, you should squash patches 1, 2, and 3 together,
> since 'make all check syntax-check' should pass after each patch in the
> series.
>
Ok. This is just from the sample about "Implementing a new API in Libvirt"
(https://libvirt.org/api_extension.html ). ^-^
I will make these 3 patches together in next version.
Thanks,
Qiaowei
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC] libxl: use libxl_event_wait to process libxl events
On 11/23/2015 01:59 PM, Jim Fehlig wrote: > > Thanks for your comments and ACK'ing the change . I'll submit a V2 that > retains > handling of shutdown event in a thread. While testing V2, I noticed occasionally missing a shutdown event. I can see from the logs that domain_death_xswatch_callback is called, a matching domain is found, and shutdown is being reported 2015-11-23 17:01:09 MST libxl: debug: libxl.c:1227:domain_death_xswatch_callback: [evg=0x7fa2fc002570:0] nentries=200 rc=2 0..61 2015-11-23 17:01:09 MST libxl: debug: libxl.c:1238:domain_death_xswatch_callback: [evg=0x7fa2fc002570:0] got=domaininfos[0] got->domain=0 2015-11-23 17:01:09 MST libxl: debug: libxl.c:1264:domain_death_xswatch_callback: exists shutdown_reported=0 dominf.flags=0020 2015-11-23 17:01:09 MST libxl: debug: libxl.c:1238:domain_death_xswatch_callback: [evg=0x7fa34c007e20:61] got=domaininfos[0] got->domain=0 2015-11-23 17:01:09 MST libxl: debug: libxl.c:1238:domain_death_xswatch_callback: [evg=0x7fa34c007e20:61] got=domaininfos[1] got->domain=61 2015-11-23 17:01:09 MST libxl: debug: libxl.c:1264:domain_death_xswatch_callback: exists shutdown_reported=0 dominf.flags=4 2015-11-23 17:01:09 MST libxl: debug: libxl.c:1276:domain_death_xswatch_callback: shutdown reporting Seems the call to libxl__event_occurred never pokes libvirt's call to libxl_event_wait. Interestingly, starting another domain does cause libxl_event_wait to return with the missing shutdown event. I'm testing on a fairly recent (2 weeks) git master. I'm not familiar with the libxl poller code and will need to instrument that a bit to understand why libxl_event_wait isn't poked. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 31/34] qemu: Replace checking for vcpu<->pid mapping availability with a helper
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Add qemuDomainHasVCpuPids to do the checking and replace in place checks
> with it.
> ---
> src/qemu/qemu_cgroup.c | 7 ++-
> src/qemu/qemu_domain.c | 15 +++
> src/qemu/qemu_domain.h | 2 ++
> src/qemu/qemu_driver.c | 29 +
> src/qemu/qemu_process.c | 7 ---
> 5 files changed, 36 insertions(+), 24 deletions(-)
>
Well I got close - reached critical mass here.
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 3c7694a..56c2e90 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -1005,12 +1005,9 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
> !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET))
> return 0;
>
> -if (priv->nvcpupids == 0 || priv->vcpupids[0] == vm->pid) {
> -/* If we don't know VCPU<->PID mapping or all vcpu runs in the same
> - * thread, we cannot control each vcpu.
> - */
I'm curious about the vcpupids[0] == vm->pid checks... I feel like I
missed some nuance with the last patch. Perhaps a bit more explanation
with regard to what happened will help. What up to this point so far
including this patch allows the "safe" removal of that check
> +/* If vCPU<->pid mapping is missing we can't do vCPU pinning */
> +if (!qemuDomainHasVCpuPids(vm))
> return 0;
> -}
>
> if (virDomainNumatuneGetMode(vm->def->numa, -1, &mem_mode) == 0 &&
> mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4913a3b..8a45825 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3956,3 +3956,18 @@ qemuDomainRequiresMlock(virDomainDefPtr def)
>
> return false;
> }
> +
> +
> +/**
> + * qemuDomainHasVCpuPids:
> + * @vm: Domain object
> + *
> + * Returns true if we were able to successfully detect vCPU pids for the VM.
> + */
> +bool
> +qemuDomainHasVCpuPids(virDomainObjPtr vm)
Use of "Vcpu" or "VCPU" rather than "VCpu"
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +return priv->nvcpupids > 0;
> +}
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 03cf6ef..7f2eca1 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -491,4 +491,6 @@ int qemuDomainDefValidateMemoryHotplug(const virDomainDef
> *def,
> virQEMUCapsPtr qemuCaps,
> const virDomainMemoryDef *mem);
>
> +bool qemuDomainHasVCpuPids(virDomainObjPtr vm);
> +
> #endif /* __QEMU_DOMAIN_H__ */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8047d36..4b7452c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1428,7 +1428,7 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
> virVcpuInfoPtr info, int maxinfo,
> size_t i, v;
> qemuDomainObjPrivatePtr priv = vm->privateData;
>
> -if (priv->vcpupids == NULL) {
> +if (!qemuDomainHasVCpuPids(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("cpu affinity is not supported"));
> return -1;
> @@ -5118,7 +5118,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
> }
>
> if (def) {
> -if (priv->vcpupids == NULL) {
> +if (!qemuDomainHasVCpuPids(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("cpu affinity is not supported"));
> goto endjob;
> @@ -10287,21 +10287,18 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr
> cgroup,
> if (period == 0 && quota == 0)
> return 0;
>
> -/* If we does not know VCPU<->PID mapping or all vcpu runs in the same
> - * thread, we cannot control each vcpu. So we only modify cpu bandwidth
> - * when each vcpu has a separated thread.
> - */
> -if (priv->nvcpupids != 0 && priv->vcpupids[0] != vm->pid) {
> -for (i = 0; i < priv->nvcpupids; i++) {
> -if (virCgroupNewThread(cgroup, VIR_CGROUP_THREAD_VCPU, i,
> - false, &cgroup_vcpu) < 0)
> -goto cleanup;
> +if (!qemuDomainHasVCpuPids(vm))
> +return 0;
Here again the vcpupids[0] thing.
John
>
> -if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0)
> -goto cleanup;
> +for (i = 0; i < priv->nvcpupids; i++) {
> +if (virCgroupNewThread(cgroup, VIR_CGROUP_THREAD_VCPU, i,
> + false, &cgroup_vcpu) < 0)
> +goto cleanup;
>
> -virCgroupFree(&cgroup_vcpu);
> -}
> +if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0)
> +goto cleanup;
> +
> +virCgroupFree(&cgroup_vcpu);
> }
>
> return 0;
> @@ -10604,7 +10601,7 @@ qemuGetVcpusBWLive(virDomainObjPtr vm,
> int ret = -1;
>
> priv = vm->privateData;
> -if (pri
Re: [libvirt] [PATCH 30/34] qemu: Drop checking vcpu threads in emulator bandwidth getter/setter
On 11/20/2015 10:22 AM, Peter Krempa wrote: > The vCPU threads make sense in the counterparts that set the vCPU > bandwidth/quota, not in the emulator one. The emulator tunables are set > all the time anyways. > > Drop the extra check and remove the now unneeded vm argument. > --- > src/qemu/qemu_driver.c | 33 ++--- > 1 file changed, 10 insertions(+), 23 deletions(-) > Seems reasonable - ACK John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 29/34] qemu: cgroup: Remove now unreachable check
On 11/20/2015 10:22 AM, Peter Krempa wrote: > Since commit 0c04906fa the check for priv->cgroup doesn't make sense as > the calls to virCgroupHasController return the same information. Remove > it and move it's comment partially to the new check. > > The already spurious check was also later copied to the iothreads code. naturally ;-)! > --- > src/qemu/qemu_cgroup.c | 18 ++ > 1 file changed, 2 insertions(+), 16 deletions(-) > ACK - John (losing steam... only 5 more to go) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-glib v3] gobject: Port to GTask API
On Mon, Nov 23, 2015 at 10:45 PM, Zeeshan Ali (Khattak) wrote: > Drop usage of deprecated GSimpleAsyncResult API. > --- Sorry, forgot to pass --annotate option to git-send-email. v3 just drops some of the private context structures in favour of GPOINTER_TO_UINT/GUINT_TO_POINTER usage. -- Regards, Zeeshan Ali (Khattak) Befriend GNOME: http://www.gnome.org/friends/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 28/34] conf: Add helper to get pointer to a certain vCPU definition
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Once more stuff will be moved into the vCPU data structure it will be
> necessary to get a specific one in some ocasions. Add a helper that will
> simplify this task.
> ---
> src/conf/domain_conf.c | 15 +++
> src/conf/domain_conf.h | 4
> src/libvirt_private.syms | 1 +
> 3 files changed, 20 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 66fc6d3..f4b4700 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1520,6 +1520,21 @@ virDomainDefGetVCpus(const virDomainDef *def)
> }
>
>
> +virDomainVCpuInfoPtr
> +virDomainDefGetVCpu(virDomainDefPtr def,
Use of "VCpu" rather than "Vcpu" or "VCPU"
Think this should be "GetVcpuInfo"...
> +unsigned int vcpu)
> +{
> +if (vcpu > def->maxvcpus) {
Should be an accessor for def->maxvcpus
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("vCPU '%u' is not present in domain definition"),
> + vcpu);
> +return NULL;
> +}
> +
> +return &def->vcpus[vcpu];
yeah - thinking about my comments in 27 - I can foresee a problem with
the ABI check going forward.
> +}
> +
> +
> virDomainDiskDefPtr
> virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt)
> {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 68f82c6..7c9457a 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2135,6 +2135,8 @@ typedef virDomainVCpuInfo *virDomainVCpuInfoPtr;
>
> struct _virDomainVCpuInfo {
> bool online;
> +
> +virBitmapPtr cpumask;
This should be in some future patch...
ACK with changes.
John
> };
>
> typedef struct _virDomainBlkiotune virDomainBlkiotune;
> @@ -2338,6 +2340,8 @@ bool virDomainDefHasVCpusOffline(const virDomainDef
> *def);
> unsigned int virDomainDefGetVCpusMax(const virDomainDef *def);
> int virDomainDefSetVCpus(virDomainDefPtr def, unsigned int vcpus);
> unsigned int virDomainDefGetVCpus(const virDomainDef *def);
> +virDomainVCpuInfoPtr virDomainDefGetVCpu(virDomainDefPtr def, unsigned int
> vcpu)
> +ATTRIBUTE_RETURN_CHECK;
>
> unsigned long long virDomainDefGetMemoryInitial(const virDomainDef *def);
> void virDomainDefSetMemoryTotal(virDomainDefPtr def, unsigned long long
> size);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d2c4945..449caf6 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -217,6 +217,7 @@ virDomainDefGetDefaultEmulator;
> virDomainDefGetMemoryActual;
> virDomainDefGetMemoryInitial;
> virDomainDefGetSecurityLabelDef;
> +virDomainDefGetVCpu;
> virDomainDefGetVCpus;
> virDomainDefGetVCpusMax;
> virDomainDefHasDeviceAddress;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 26/34] conf: turn def->vcpus into a structure
On 11/23/2015 05:22 PM, John Ferlan wrote:
>
>
> On 11/20/2015 10:22 AM, Peter Krempa wrote:
>> To allow collecting all relevant data at one place let's make def->vcpus
>> a structure and then we can start moving stuff into it.
>> ---
>> src/conf/domain_conf.c | 55
>> --
>> src/conf/domain_conf.h | 10 -
>> 2 files changed, 58 insertions(+), 7 deletions(-)
>>
[...]
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 3490f02..68f82c6 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2129,6 +2129,14 @@ struct _virDomainCputune {
>> virDomainThreadSchedParamPtr iothreadsched;
>> };
>>
>> +
>> +typedef struct _virDomainVCpuInfo virDomainVCpuInfo;
>> +typedef virDomainVCpuInfo *virDomainVCpuInfoPtr;
>> +
>> +struct _virDomainVCpuInfo {
>> +bool online;
>> +};
Missed noting these 'VcpuInfo' or 'VCPUInfo" not "VCpuInfo"
John
>> +
>> typedef struct _virDomainBlkiotune virDomainBlkiotune;
>> typedef virDomainBlkiotune *virDomainBlkiotunePtr;
>>
>> @@ -2202,7 +2210,7 @@ struct _virDomainDef {
>> virDomainBlkiotune blkio;
>> virDomainMemtune mem;
>>
>> -unsigned int vcpus;
>> +virDomainVCpuInfoPtr vcpus;
>> size_t maxvcpus;
>> int placement_mode;
>> virBitmapPtr cpumask;
>>
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-glib v3] gobject: Port to GTask API
Drop usage of deprecated GSimpleAsyncResult API.
---
libvirt-gobject/libvirt-gobject-domain.c| 290 +---
libvirt-gobject/libvirt-gobject-input-stream.c | 77 +++
libvirt-gobject/libvirt-gobject-output-stream.c | 75 +++---
libvirt-gobject/libvirt-gobject-storage-pool.c | 281 ++-
libvirt-gobject/libvirt-gobject-stream.c| 52 +++--
5 files changed, 322 insertions(+), 453 deletions(-)
diff --git a/libvirt-gobject/libvirt-gobject-domain.c
b/libvirt-gobject/libvirt-gobject-domain.c
index 5509ce3..c768cd3 100644
--- a/libvirt-gobject/libvirt-gobject-domain.c
+++ b/libvirt-gobject/libvirt-gobject-domain.c
@@ -370,28 +370,20 @@ gboolean gvir_domain_start(GVirDomain *dom,
return TRUE;
}
-typedef struct {
-guint flags;
-} DomainStartData;
-
-static void domain_start_data_free(DomainStartData *data)
-{
-g_slice_free(DomainStartData, data);
-}
-
static void
-gvir_domain_start_helper(GSimpleAsyncResult *res,
- GObject *object,
+gvir_domain_start_helper(GTask *task,
+ gpointer source_object,
+ gpointer task_data,
GCancellable *cancellable G_GNUC_UNUSED)
{
-GVirDomain *dom = GVIR_DOMAIN(object);
-DomainStartData *data;
+GVirDomain *dom = GVIR_DOMAIN(source_object);
+guint flags = GPOINTER_TO_UINT(task_data);
GError *err = NULL;
-data = g_simple_async_result_get_op_res_gpointer(res);
-
-if (!gvir_domain_start(dom, data->flags, &err))
-g_simple_async_result_take_error(res, err);
+if (!gvir_domain_start(dom, flags, &err))
+g_task_return_error(task, err);
+else
+g_task_return_boolean(task, TRUE);
}
/**
@@ -410,25 +402,18 @@ void gvir_domain_start_async(GVirDomain *dom,
GAsyncReadyCallback callback,
gpointer user_data)
{
-GSimpleAsyncResult *res;
-DomainStartData *data;
+GTask *task;
g_return_if_fail(GVIR_IS_DOMAIN(dom));
g_return_if_fail((cancellable == NULL) || G_IS_CANCELLABLE(cancellable));
-data = g_slice_new0(DomainStartData);
-data->flags = flags;
-
-res = g_simple_async_result_new(G_OBJECT(dom),
-callback,
-user_data,
-gvir_domain_start_async);
-g_simple_async_result_set_op_res_gpointer (res, data,
(GDestroyNotify)domain_start_data_free);
-g_simple_async_result_run_in_thread(res,
-gvir_domain_start_helper,
-G_PRIORITY_DEFAULT,
-cancellable);
-g_object_unref(res);
+task = g_task_new(G_OBJECT(dom),
+ cancellable,
+ callback,
+ user_data);
+g_task_set_task_data(task, GUINT_TO_POINTER(flags), NULL);
+g_task_run_in_thread(task, gvir_domain_start_helper);
+g_object_unref(task);
}
gboolean gvir_domain_start_finish(GVirDomain *dom,
@@ -436,13 +421,10 @@ gboolean gvir_domain_start_finish(GVirDomain *dom,
GError **err)
{
g_return_val_if_fail(GVIR_IS_DOMAIN(dom), FALSE);
-g_return_val_if_fail(g_simple_async_result_is_valid(result, G_OBJECT(dom),
gvir_domain_start_async), FALSE);
+g_return_val_if_fail(g_task_is_valid(result, dom), FALSE);
g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
-if (g_simple_async_result_propagate_error(G_SIMPLE_ASYNC_RESULT(result),
err))
-return FALSE;
-
-return TRUE;
+return g_task_propagate_boolean(G_TASK(result), err);
}
/**
@@ -472,15 +454,18 @@ gboolean gvir_domain_resume(GVirDomain *dom,
}
static void
-gvir_domain_resume_helper(GSimpleAsyncResult *res,
- GObject *object,
+gvir_domain_resume_helper(GTask *task,
+ gpointer source_object,
+ gpointer task_data G_GNUC_UNUSED,
GCancellable *cancellable G_GNUC_UNUSED)
{
-GVirDomain *dom = GVIR_DOMAIN(object);
+GVirDomain *dom = GVIR_DOMAIN(source_object);
GError *err = NULL;
if (!gvir_domain_resume(dom, &err))
-g_simple_async_result_take_error(res, err);
+g_task_return_error(task, err);
+else
+g_task_return_boolean(task, TRUE);
}
/**
@@ -497,20 +482,17 @@ void gvir_domain_resume_async(GVirDomain *dom,
GAsyncReadyCallback callback,
gpointer user_data)
{
-GSimpleAsyncResult *res;
+GTask *task;
g_return_if_fail(GVIR_IS_DOMAIN(dom));
g_return_if_fail((cancellable == NULL) || G_IS_CANCELLABLE(cancellable));
-res = g_simple_async_result_new(G_OBJECT(dom),
-callback,
-user_data,
-
Re: [libvirt] [PATCH 27/34] conf: ABI: Split up and improve vcpu info ABI checking
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Extract the checking code into a separate function and prepare the
> infrastructure for checking the new structure type.
> ---
> src/conf/domain_conf.c | 41 ++---
> 1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 631e1db..66fc6d3 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17835,6 +17835,35 @@
> virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src,
> }
>
>
> +static bool
> +virDomainDefVcpuCheckAbiStability(virDomainDefPtr src,
> + virDomainDefPtr dst)
I see use of "Vcpu" here...
> +{
> +size_t i;
> +
> +if (src->maxvcpus != dst->maxvcpus) {
Should these be accessors? Like they were in the moved code?
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Target domain vCPU max %zu does not match source
> %zu"),
> + dst->maxvcpus, src->maxvcpus);
> +return false;
> +}
> +
> +for (i = 0; i < src->maxvcpus; i++) {
Allowing for this to be an accessor/local too.
> +virDomainVCpuInfoPtr svcpu = &src->vcpus[i];
> +virDomainVCpuInfoPtr dvcpu = &dst->vcpus[i];
> +
> +if (svcpu->online != dvcpu->online) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("State of vCPU '%zu' differs between source and
> "
> + "destination definitions"), i);
> +return false;
> +}
This changes the original code/design just slightly from a counted value
online to having the same order between source/dest. If the current
algorithm of using the first 'current' vcpus doesn't change, then I
foresee perhaps an interesting issue/question.
Say we start with 2 current (0,1) and 4 total (0,1,2,3). If we allow
someone to start/hotplug #3, then a migration occurs. Would the "target"
start "0,1,2" or "0,1,3"?
If I think about the current algorithm, it's get the # of vCPU's
"current" (virDomainDefGetVCpus) and then set online in order 0, 1, 2
(virDomainDefSetVCpus).
That causes a failure for this algorithm, but should it? Again only an
issue if you're ultimate goal is to allow the user to choose which
vCPU's to place online or offline. I haven't looked that far forward yet.
Conditional ACK depending on response.
John
> +}
> +
> +return true;
> +}
> +
> +
> /* This compares two configurations and looks for any differences
> * which will affect the guest ABI. This is primarily to allow
> * validation of custom XML config passed in during migration
> @@ -17908,18 +17937,8 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
> goto error;
> }
>
> -if (virDomainDefGetVCpus(src) != virDomainDefGetVCpus(dst)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Target domain vCPU count %d does not match source
> %d"),
> - virDomainDefGetVCpus(dst), virDomainDefGetVCpus(src));
> +if (!virDomainDefVcpuCheckAbiStability(src, dst))
> goto error;
> -}
> -if (virDomainDefGetVCpusMax(src) != virDomainDefGetVCpusMax(dst)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Target domain vCPU max %d does not match source
> %d"),
> - virDomainDefGetVCpusMax(dst),
> virDomainDefGetVCpusMax(src));
> -goto error;
> -}
>
> if (src->iothreads != dst->iothreads) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 26/34] conf: turn def->vcpus into a structure
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> To allow collecting all relevant data at one place let's make def->vcpus
> a structure and then we can start moving stuff into it.
> ---
> src/conf/domain_conf.c | 55
> --
> src/conf/domain_conf.h | 10 -
> 2 files changed, 58 insertions(+), 7 deletions(-)
>
Well I have to assume at this point neither of us builds w/ bhyve or
vz/parallels enabled! (true for me); otherwise, the build would have
gone down in a flaming mess.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 897b643..631e1db 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1424,20 +1424,38 @@ void virDomainLeaseDefFree(virDomainLeaseDefPtr def)
> }
>
>
> +static void
> +virDomainVCpuInfoClear(virDomainVCpuInfoPtr info)'
Use of "Vcpus" or "VCPUs" is preferred.
> +{
> +if (!info)
> +return;
> +}
> +
> +
> int
> virDomainDefSetVCpusMax(virDomainDefPtr def,
> unsigned int vcpus)
H. check that earlier thought... maybe "newvcpus"? I dunno, my eyes
are getting tired though!
> {
> +size_t i;
> +
> +if (def->maxvcpus == vcpus)
> +return 0;
> +
> if (vcpus == 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("domain config can't have 0 maximum vCPUs"));
> return -1;
> }
>
> -if (vcpus < def->vcpus)
> -virDomainDefSetVCpus(def, vcpus);
> +if (def->maxvcpus < vcpus) {
> +if (VIR_EXPAND_N(def->vcpus, def->maxvcpus, vcpus - def->maxvcpus) <
> 0)
> +return -1;
> +} else {
> +for (i = vcpus; i < def->maxvcpus; i++)
> +virDomainVCpuInfoClear(&def->vcpus[i]);
>
> -def->maxvcpus = vcpus;
> +VIR_SHRINK_N(def->vcpus, def->maxvcpus, def->maxvcpus - vcpus);
> +}
>
> return 0;
> }
> @@ -1446,7 +1464,14 @@ virDomainDefSetVCpusMax(virDomainDefPtr def,
> bool
> virDomainDefHasVCpusOffline(const virDomainDef *def)
> {
> -return def->vcpus < def->maxvcpus;
> +size_t i;
> +
> +for (i = 0; i < def->maxvcpus; i++) {
Should there be an accessor to def->maxvcpus?
> +if (!def->vcpus[i].online)
> +return true;
> +}
> +
> +return false;
> }
>
>
> @@ -1461,6 +1486,8 @@ int
> virDomainDefSetVCpus(virDomainDefPtr def,
> unsigned int vcpus)
> {
> +size_t i;
> +
> if (vcpus > def->maxvcpus) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("maxvcpus must not be less than current vcpus (%u <
> %zu)"),
> @@ -1468,7 +1495,11 @@ virDomainDefSetVCpus(virDomainDefPtr def,
> return -1;
> }
>
> -def->vcpus = vcpus;
> +for (i = 0; i < vcpus; i++)
> +def->vcpus[i].online = true;
> +
> +for (i = vcpus; i < def->maxvcpus; i++)
Should there be an accessor to def->maxvcpus? That'd be for both uses.
> +def->vcpus[i].online = false;
>
> return 0;
> }
> @@ -1477,7 +1508,15 @@ virDomainDefSetVCpus(virDomainDefPtr def,
> unsigned int
> virDomainDefGetVCpus(const virDomainDef *def)
> {
> -return def->vcpus;
> +size_t i;
> +unsigned int ret = 0;
> +
> +for (i = 0; i < def->maxvcpus; i++) {
Should there be accessor to "def->maxvcpus"?
ACK with some adjustments... More importantly the "VCpus" change, but
less so the accessor to ->maxvcpus
John
> +if (def->vcpus[i].online)
> +ret++;
> +}
> +
> +return ret;
> }
>
>
> @@ -2505,6 +2544,10 @@ void virDomainDefFree(virDomainDefPtr def)
>
> virDomainResourceDefFree(def->resource);
>
> +for (i = 0; i < def->maxvcpus; i++)
> +virDomainVCpuInfoClear(&def->vcpus[i]);
> +VIR_FREE(def->vcpus);
> +
> /* hostdevs must be freed before nets (or any future "intelligent
> * hostdevs") because the pointer to the hostdev is really
> * pointing into the middle of the higher level device's object,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3490f02..68f82c6 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2129,6 +2129,14 @@ struct _virDomainCputune {
> virDomainThreadSchedParamPtr iothreadsched;
> };
>
> +
> +typedef struct _virDomainVCpuInfo virDomainVCpuInfo;
> +typedef virDomainVCpuInfo *virDomainVCpuInfoPtr;
> +
> +struct _virDomainVCpuInfo {
> +bool online;
> +};
> +
> typedef struct _virDomainBlkiotune virDomainBlkiotune;
> typedef virDomainBlkiotune *virDomainBlkiotunePtr;
>
> @@ -2202,7 +2210,7 @@ struct _virDomainDef {
> virDomainBlkiotune blkio;
> virDomainMemtune mem;
>
> -unsigned int vcpus;
> +virDomainVCpuInfoPtr vcpus;
> size_t maxvcpus;
> int placement_mode;
> virBitmapPtr cpumask;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/34] conf: Replace writes to def->maxvcpus with accessor
On 11/20/2015 10:21 AM, Peter Krempa wrote:
> To support further refactors replace all write access to def->maxvcpus
> with a accessor function.
> ---
> src/conf/domain_conf.c | 18 --
> src/conf/domain_conf.h | 2 ++
> src/hyperv/hyperv_driver.c | 5 -
> src/libvirt_private.syms | 1 +
> src/libxl/libxl_driver.c | 8 ++--
> src/lxc/lxc_native.c | 4 +++-
> src/openvz/openvz_conf.c | 4 +++-
> src/openvz/openvz_driver.c | 5 -
> src/phyp/phyp_driver.c | 4 +++-
> src/qemu/qemu_command.c| 9 +++--
> src/qemu/qemu_driver.c | 4 +++-
> src/test/test_driver.c | 4 +++-
> src/vbox/vbox_common.c | 11 +--
> src/vmx/vmx.c | 5 -
> src/vz/vz_sdk.c| 4 +++-
> src/xen/xm_internal.c | 4 +++-
> src/xenapi/xenapi_driver.c | 4 +++-
> src/xenconfig/xen_common.c | 4 +++-
> src/xenconfig/xen_sxpr.c | 3 ++-
> 19 files changed, 82 insertions(+), 21 deletions(-)
>
Now that I'm much further along...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a744412..e0fc09c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1424,6 +1424,16 @@ void virDomainLeaseDefFree(virDomainLeaseDefPtr def)
> }
>
>
> +int
> +virDomainDefSetVCpusMax(virDomainDefPtr def,
> +unsigned int vcpus)
should this change to "maxvcpus"?
Not all that important, but it may make for easier reading later on when
def->maxvcpus intersperses with def->vcpus and there's a vcpus variable
that relates to max and not current.
John
> +{
> +def->maxvcpus = vcpus;
> +
> +return 0;
> +}
> +
> +
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 25/34] qemu: refactor qemuDomainHotunplugVcpus
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Refactor the code flow so that 'exit_monitor:' can be removed.
>
> This patch moves the auditing functions into places where it's certain
> that hotunplug was or was not successful and reports errors from
> qemuMonitorGetCPUInfo properly.
> ---
> src/qemu/qemu_driver.c | 50
> +++---
> 1 file changed, 15 insertions(+), 35 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9e0e334..614c7f8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4798,48 +4798,36 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> int ret = -1;
> +int rc;
> int oldvcpus = virDomainDefGetVCpus(vm->def);
> -int vcpus = oldvcpus;
> pid_t *cpupids = NULL;
> -int ncpupids;
> +int ncpupids = 0;
>
> qemuDomainObjEnterMonitor(driver, vm);
>
> -if (qemuMonitorSetCPU(priv->mon, vcpu, false) < 0)
> -goto exit_monitor;
> +rc = qemuMonitorSetCPU(priv->mon, vcpu, false);
>
> -vcpus--;
> +if (rc == 0)
> +ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids);
>
> -/* After hotplugging the CPUs we need to re-detect threads corresponding
> - * to the virtual CPUs. Some older versions don't provide the thread ID
> - * or don't have the "info cpus" command (and they don't support multiple
> - * CPUs anyways), so errors in the re-detection will not be treated
> - * fatal */
> -if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) {
> -virResetLastError();
> -ret = 0;
> -goto exit_monitor;
> -}
> if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
>
> -/* check if hotunplug has failed */
> -if (ncpupids == oldvcpus) {
> -virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> - _("qemu didn't unplug the vCPUs properly"));
> -vcpus = oldvcpus;
> +virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update",
> + rc == 0 && ncpupids == oldvcpus -1);
> +
Similar comments to 24 w/r/t ExitMonitor failure and lack of Audit and
overwritten last message possible from GetCPUInfo failure.
w/r/t "&& ncpupids == oldvcpus - 1" in the audit message - if ncpupids
== 0 here, then unless we've dropped to zero vcpu's, this will always
trip strangely.
IOW: The ncpupids == 0 has been lost...
> +if (rc < 0 || ncpupids < 0)
> goto cleanup;
> -}
>
> -if (ncpupids != vcpus) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("got wrong number of vCPU pids from QEMU monitor. "
> - "got %d, wanted %d"),
> - ncpupids, vcpus);
> -vcpus = oldvcpus;
> +/* check if hotunplug has failed */
> +if (ncpupids != oldvcpus - 1) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("qemu didn't unplug vCPU '%u' properly"), vcpu);
> goto cleanup;
> }
>
> +ignore_value(virDomainDefSetVCpus(vm->def, oldvcpus - 1));
> +
Again - I would hope it wouldn't fail and not sure why ignore_value
should be used... I would think we'd have after the (rc < 0 || ncpupids
< 0) check (e.g. similar to Hotplug order):
if (virDomainDefSetVCpus(vm->def, oldvcpus - 1) < 0)
goto cleanup;
/* Gratuitous comment here ... */
if (ncpupids == 0) {
ret = 0;
goto cleanup;
}
I'm sure you'll figure out a better order for an ACK...
John
> if (qemuDomainDelCgroupForThread(priv->cgroup,
> VIR_CGROUP_THREAD_VCPU, vcpu) < 0)
> goto cleanup;
> @@ -4858,15 +4846,7 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
>
> cleanup:
> VIR_FREE(cpupids);
> -if (virDomainObjIsActive(vm) &&
> -virDomainDefSetVCpus(vm->def, vcpus) < 0)
> -ret = -1;
> -virDomainAuditVcpu(vm, oldvcpus, vcpus, "update", ret == 0);
> return ret;
> -
> - exit_monitor:
> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
> -goto cleanup;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] nodedev: fix virtual_functions element
Moshe Levi pointed out yesterday in an email to libvir-list that there was no way to tell from the node device API that a PCI was capable of having SRIOV virtual functions (VFs) if it was currently setup to not have any: https://www.redhat.com/archives/libvir-list/2015-November/msg00894.html These two patches modify the node device XML in two ways: 1) if the device has a file called sriov_totalvfs, the value contained in that file will be included in the capabilities element as the attribute "maxCount". 2) in this case, even if there are currently no VFs active/visible for the device, the virtual_functions capability will still be reported. Laine Stump (2): conf: support reporting maxCount attribute for virtual_functions cap nodedev: report maxCount for virtual_functions capability docs/formatnode.html.in | 11 ++- src/conf/node_device_conf.c | 32 +++ src/conf/node_device_conf.h | 1 + src/network/bridge_driver.c | 5 +++-- src/node_device/node_device_linux_sysfs.c | 7 +-- src/util/virnetdev.c | 9 ++--- src/util/virnetdev.h | 5 +++-- src/util/virpci.c | 30 + src/util/virpci.h | 5 +++-- 9 files changed, 77 insertions(+), 28 deletions(-) -- 2.4.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] conf: support reporting maxCount attribute for virtual_functions cap
Report the maximum possible number of VFs for an SRIOV PF, like this:
...
I've just discovered that the virtual_functions and physical_functions
capabilities are not supported in the virNodeDeviceParse functions,
only in virNodeDeviceFormat (I suppose because they are only reported,
not set from XML). This should probably be remedied, but is less
immediately useful than the current patch.
---
docs/formatnode.html.in | 11 ++-
src/conf/node_device_conf.c | 32
src/conf/node_device_conf.h | 1 +
3 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index ed00af5..79e2448 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -108,7 +108,16 @@
the type is virtual_functions, then this
device is an SRIOV PF, and the capability element will
have a list of address subelements, one
-for each VF on this PF.
+for each VF on this PF. If the host system supports
+reporting it (via the "sriov_maxvfs" file in the
+device's sysfs directory) the capability element will
+also have an attribute named maxCount
+which is the maximum number of SRIOV VFs supported by
+this device, which could be higher than the number of
+VFs that are curently active since
+1.2.22; in this case, even if there are
+currently no active VFs the virtual_functions
+capabililty will still be shown.
numa
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index e6f3f27..c04739f 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -361,19 +361,27 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def)
virBufferAddLit(&buf, "\n");
}
if (data->pci_dev.flags &
VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION) {
-virBufferAddLit(&buf, "\n");
-virBufferAdjustIndent(&buf, 2);
-for (i = 0; i < data->pci_dev.num_virtual_functions; i++) {
-virBufferAsprintf(&buf,
- "\n",
-
data->pci_dev.virtual_functions[i]->domain,
- data->pci_dev.virtual_functions[i]->bus,
- data->pci_dev.virtual_functions[i]->slot,
-
data->pci_dev.virtual_functions[i]->function);
+virBufferAddLit(&buf, "pci_dev.max_virtual_functions)
+virBufferAsprintf(&buf, " maxCount='%u'",
+ data->pci_dev.max_virtual_functions);
+if (data->pci_dev.num_virtual_functions == 0) {
+virBufferAddLit(&buf, "/>\n");
+} else {
+virBufferAddLit(&buf, ">\n");
+virBufferAdjustIndent(&buf, 2);
+for (i = 0; i < data->pci_dev.num_virtual_functions; i++) {
+virBufferAsprintf(&buf,
+ "\n",
+
data->pci_dev.virtual_functions[i]->domain,
+
data->pci_dev.virtual_functions[i]->bus,
+
data->pci_dev.virtual_functions[i]->slot,
+
data->pci_dev.virtual_functions[i]->function);
+}
+virBufferAdjustIndent(&buf, -2);
+virBufferAddLit(&buf, "\n");
}
-virBufferAdjustIndent(&buf, -2);
-virBufferAddLit(&buf, "\n");
}
if (data->pci_dev.nIommuGroupDevices) {
virBufferAsprintf(&buf, "\n",
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 7dd39ca..d007186 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -112,6 +112,7 @@ typedef struct _virNodeDevCapData {
virPCIDeviceAddressPtr physical_function;
virPCIDeviceAddressPtr *virtual_functions;
size_t num_virtual_functions;
+unsigned int max_virtual_functions;
unsigned int flags;
virPCIDeviceAddressPtr *iommuGroupDevices;
size_t nIommuGroupDevices;
--
2.4.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] nodedev: report maxCount for virtual_functions capability
A PCI device may have the capability to setup virtual functions (VFs)
but have them currently all disabled. Prior to this patch, if that was
the case the the node device XML for the device wouldn't report any
virtual_functions capability.
With this patch, if a file called "sriov_totalvfs" is found in the
device's sysfs directory, its contents will be interpreted as a
decimal number, and that value will be reported as "maxCount" in a
capability element of the device's XML, e.g.:
This will be reported regardless of whether or not any VFs are
currently enabled for the device.
NB: sriov_numvfs (the number of VFs currently active) is also
available in sysfs, but that value is implied by the number of items
in the list that is inside the capability element, so there is no
reason to explicitly provide it as an attribute.
sriov_totalvfs and sriov_numvfs are available in kernels at least as far
back as the 2.6.32 that is in RHEL6.7, but in the case that they
simply aren't there, libvirt will behave as it did prior to this patch
- no maxCount will be displayed, and the virtual_functions capability
will be absent from the device's XML when 0 VFs are enabled.
---
src/network/bridge_driver.c | 5 +++--
src/node_device/node_device_linux_sysfs.c | 7 +--
src/util/virnetdev.c | 9 ++---
src/util/virnetdev.h | 5 +++--
src/util/virpci.c | 30 ++
src/util/virpci.h | 5 +++--
6 files changed, 46 insertions(+), 15 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index f838671..a0f6494 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2334,6 +2334,7 @@ static int
networkCreateInterfacePool(virNetworkDefPtr netdef)
{
size_t numVirtFns = 0;
+unsigned int maxVirtFns = 0;
char **vfNames = NULL;
virPCIDeviceAddressPtr *virtFns;
@@ -2343,8 +2344,8 @@ networkCreateInterfacePool(virNetworkDefPtr netdef)
if (netdef->forward.npfs == 0 || netdef->forward.nifs > 0)
return 0;
-if ((virNetDevGetVirtualFunctions(netdef->forward.pfs->dev,
- &vfNames, &virtFns, &numVirtFns)) < 0) {
+if ((virNetDevGetVirtualFunctions(netdef->forward.pfs->dev, &vfNames,
+ &virtFns, &numVirtFns, &maxVirtFns)) <
0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not get Virtual functions on %s"),
netdef->forward.pfs->dev);
diff --git a/src/node_device/node_device_linux_sysfs.c
b/src/node_device/node_device_linux_sysfs.c
index 6d5a406..431f471 100644
--- a/src/node_device/node_device_linux_sysfs.c
+++ b/src/node_device/node_device_linux_sysfs.c
@@ -150,6 +150,7 @@ nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath,
VIR_FREE(data->pci_dev.virtual_functions[i]);
VIR_FREE(data->pci_dev.virtual_functions);
data->pci_dev.num_virtual_functions = 0;
+data->pci_dev.max_virtual_functions = 0;
data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
@@ -157,11 +158,13 @@ nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath,
data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
ret = virPCIGetVirtualFunctions(sysfsPath,
&data->pci_dev.virtual_functions,
-&data->pci_dev.num_virtual_functions);
+&data->pci_dev.num_virtual_functions,
+&data->pci_dev.max_virtual_functions);
if (ret < 0)
return ret;
-if (data->pci_dev.num_virtual_functions > 0)
+if (data->pci_dev.num_virtual_functions > 0 ||
+data->pci_dev.max_virtual_functions > 0)
data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
return ret;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index ade9afa..e7f44cd 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1802,7 +1802,8 @@ int
virNetDevGetVirtualFunctions(const char *pfname,
char ***vfname,
virPCIDeviceAddressPtr **virt_fns,
- size_t *n_vfname)
+ size_t *n_vfname,
+ unsigned int *max_vfs)
{
int ret = -1;
size_t i;
@@ -1812,12 +1813,13 @@ virNetDevGetVirtualFunctions(const char *pfname,
*virt_fns = NULL;
*n_vfname = 0;
+*max_vfs = 0;
if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") < 0)
return ret;
if (virPCIGetVirtualFunctions(pf_sysfs_device_link, virt_fns,
- n_vfname) < 0)
+ n_vfname, max_vfs) < 0)
goto cleanup;
if (VIR_ALLOC_N(*vfname, *n_vfname) < 0)
@@
Re: [libvirt] [PATCH 24/34] qemu: Refactor qemuDomainHotplugVcpus
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Refactor the code flow so that 'exit_monitor:' can be removed.
>
> This patch also moves the auditing and setting of the new vCPU count
> right to the place where the hotplug happens, since it's possible that
> the hotplug succeeds and adds a cpu while other stuff fails.
>
> Lastly, failures of qemuMonitorGetCPUInfo are now reported rather than
> ignored. The function retuns 0 if it "successfully" detected 0 threads.
> ---
> src/qemu/qemu_driver.c | 54
> +++---
> 1 file changed, 25 insertions(+), 29 deletions(-)
>
Damn - should have peeked ahead...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9f0e3a3..9e0e334 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4698,41 +4698,46 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> int ret = -1;
> +int rc;
> int oldvcpus = virDomainDefGetVCpus(vm->def);
> -int vcpus = oldvcpus;
> pid_t *cpupids = NULL;
> -int ncpupids;
> +int ncpupids = 0;
> virCgroupPtr cgroup_vcpu = NULL;
> char *mem_mask = NULL;
> virDomainNumatuneMemMode mem_mode;
>
> qemuDomainObjEnterMonitor(driver, vm);
>
> -if (qemuMonitorSetCPU(priv->mon, vcpu, true) < 0)
> -goto exit_monitor;
> -
> -vcpus++;
> +rc = qemuMonitorSetCPU(priv->mon, vcpu, true);
>
> -/* After hotplugging the CPUs we need to re-detect threads corresponding
> - * to the virtual CPUs. Some older versions don't provide the thread ID
> - * or don't have the "info cpus" command (and they don't support multiple
> - * CPUs anyways), so errors in the re-detection will not be treated
> - * fatal */
> -if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) {
> -virResetLastError();
> -ret = 0;
> -goto exit_monitor;
> -}
> +if (rc == 0)
> +ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids);
>
> if (qemuDomainObjExitMonitor(driver, vm) < 0)
This could overwrite a qemuMonitorGetCPUInfo error, but that's no
different than we current could do (though we don't ResetLast on
GetCPUInfo failure).
> goto cleanup;
But if we leave here, then we know we're still active thus the
adjustment from cleanup to call SetVcpus only if still active is
satisfied...
>
> -if (ncpupids != vcpus) {
> +virDomainAuditVcpu(vm, oldvcpus, oldvcpus + 1, "update", rc == 0);
If the ExitMonitor fails, then we won't Audit
> +
> +if (rc < 0)
> +goto cleanup;
> +
> +ignore_value(virDomainDefSetVCpus(vm->def, oldvcpus + 1));
Why not just :
if (virDomainDefSetVCpus(vm->def, oldvcpus + 1) < 0 ||
ncpupids < 0)
goto cleanup;
I would *hope* that we don't fail SetVcpus at this point - at least we
can avoid the pointless ignore_value
ACK - as long as we can audit on failure to ExitMonitor... Whether you
feel the GetCPUInfo error is worth saving/sending is up to you. The last
comment is purely an observation.
John
> +
> +if (ncpupids < 0)
> +goto cleanup;
> +
> +/* failure to re-detect vCPU pids after hotplug due to lack of support
> was
> + * historically deemed not fatal. We need to skip the rest of the steps
> though. */
> +if (ncpupids == 0) {
> +ret = 0;
> +goto cleanup;
> +}
> +
> +if (ncpupids != oldvcpus + 1) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("got wrong number of vCPU pids from QEMU monitor. "
> "got %d, wanted %d"),
> - ncpupids, vcpus);
> -vcpus = oldvcpus;
> + ncpupids, oldvcpus + 1);
> goto cleanup;
> }
>
> @@ -4781,17 +4786,8 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> cleanup:
> VIR_FREE(cpupids);
> VIR_FREE(mem_mask);
> -if (virDomainObjIsActive(vm) &&
> -virDomainDefSetVCpus(vm->def, vcpus) < 0)
> -ret = -1;
> -virDomainAuditVcpu(vm, oldvcpus, vcpus, "update", ret == 0);
> -if (cgroup_vcpu)
> -virCgroupFree(&cgroup_vcpu);
> +virCgroupFree(&cgroup_vcpu);
> return ret;
> -
> - exit_monitor:
> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
> -goto cleanup;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC] libxl: use libxl_event_wait to process libxl events
On 11/23/2015 04:24 AM, Ian Jackson wrote:
> Jim Fehlig writes ("[PATCH RFC] libxl: use libxl_event_wait to process libxl
> events"):
>> Prior to this patch, libxl events were delivered to libvirt via
>> the libxlDomainEventHandler callback registered with libxl.
>> Documenation in $xensrc/tools/libxl/libxl_event.h states that the
>> callback "may occur on any thread in which the application calls
>> libxl". This can result in deadlock since many of the libvirt
>> callees of libxl hold a lock on the virDomainObj they are working
>> on. When the callback is invoked, it attempts to find a virDomainObj
>> corresponding to the domain ID provided by libxl. Searching the
>> domain obj list results in locking each obj before checking if it is
>> active, and its ID equals the requested ID. Deadlock is possible
>> when attempting to lock an obj that is already locked further up
>> the call stack. Indeed, Max Ustermann recently reported an instance
>> of this deadlock
> This sounds like a very plausible failure mode.
>
>> https://www.redhat.com/archives/libvir-list/2015-November/msg00130.html
>>
>> This patch moves processing of libxl events to a thread, where
>> libxl_event_wait() is used to collect events. This allows processing
>> libxl events asynchronously in libvirt, avoiding the deadlock.
> The reasoning is sound and the remedy is IMO correct. (However, you
> mean "this allows processing libxl events _synchronously_", since what
> you are doing is serialising them all into your main loop.)
Ah yes, poor choice of words. The last sentence should read:
This approach gives libvirt more control over event processing, ensuring object
locking constraints can be met.
But your comment exposes a flaw in the patch, one that had already been fixed in
the event_occurs approach. Shutting down a large memory domain can take
considerable time due to memory scrubbing, meanwhile stalling reception of other
events. The patch was a bit aggressive in removing libxlDomainShutdownThread().
>
> So:
>
> Acked-by: Ian Jackson
>
> NB that I have not reviewed the patch in detail. I can do that if you
> like, although of course my knowledge of the innards of libvirt is not
> wonderful.
>
>> The only reservations I have about this patch come from comments
>> about libxl_event_wait in libxl_event.h
>>
>> Like libxl_event_check but blocks if no suitable events are
>> available, until some are. Uses libxl_osevent_beforepoll/
>> _afterpoll so may be inefficient if very many domains are being
>> handled by a single program.
> If this turns out to be a problem in practice, we will improve libxl's
> data structures to not involve so many linear searches. (In fact I
> think you are probably already calling synchronous libxl ao functions,
> which have the same performance properties, although this is not
> documented.)
>
> Given what you say above I don't think there is a reasonable
> alternative remedy. So you should go ahead with this patch.
Thanks for your comments and ACK'ing the change . I'll submit a V2 that retains
handling of shutdown event in a thread.
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 23/34] qemu: cpu hotplug: Move loops to qemuDomainSetVcpusFlags
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> qemuDomainHotplugVcpus/qemuDomainHotunplugVcpus are complex enough in
> regards of adding one CPU. Additionally it will be desired to reuse
> those functions later with specific vCPU hotplug.
>
> Move the loops for adding vCPUs into qemuDomainSetVcpusFlags so that the
> helpers can be made simpler and more straightforward.
> ---
> src/qemu/qemu_driver.c | 105
> ++---
> 1 file changed, 48 insertions(+), 57 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9011b2d..9f0e3a3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4694,10 +4694,9 @@ qemuDomainDelCgroupForThread(virCgroupPtr cgroup,
> static int
> qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> - unsigned int nvcpus)
> + unsigned int vcpu)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> -size_t i;
> int ret = -1;
> int oldvcpus = virDomainDefGetVCpus(vm->def);
> int vcpus = oldvcpus;
You could set this to oldvcpus + 1;...
> @@ -4709,13 +4708,10 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
>
> qemuDomainObjEnterMonitor(driver, vm);
>
> -for (i = vcpus; i < nvcpus; i++) {
> -/* Online new CPU */
> -if (qemuMonitorSetCPU(priv->mon, i, true) < 0)
> -goto exit_monitor;
> +if (qemuMonitorSetCPU(priv->mon, vcpu, true) < 0)
> +goto exit_monitor;
>
> -vcpus++;
> -}
> +vcpus++;
Thus removing the need for this... and an Audit message that doesn't
use the same value for oldvcpus and vcpus (although it could before
these changes).
>
> /* After hotplugging the CPUs we need to re-detect threads corresponding
> * to the virtual CPUs. Some older versions don't provide the thread ID
> @@ -4747,37 +4743,34 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> &mem_mask, -1) < 0)
> goto cleanup;
>
> -for (i = oldvcpus; i < nvcpus; i++) {
> -if (priv->cgroup) {
> -cgroup_vcpu =
> -qemuDomainAddCgroupForThread(priv->cgroup,
> - VIR_CGROUP_THREAD_VCPU,
> - i, mem_mask,
> - cpupids[i]);
> -if (!cgroup_vcpu)
> -goto cleanup;
> -}
> +if (priv->cgroup) {
> +cgroup_vcpu =
> +qemuDomainAddCgroupForThread(priv->cgroup,
> + VIR_CGROUP_THREAD_VCPU,
> + vcpu, mem_mask,
> + cpupids[vcpu]);
> +if (!cgroup_vcpu)
> +goto cleanup;
> +}
>
> -/* Inherit def->cpuset */
> -if (vm->def->cpumask) {
> -if (qemuDomainHotplugAddPin(vm->def->cpumask, i,
> -&vm->def->cputune.vcpupin,
> -&vm->def->cputune.nvcpupin) < 0) {
> -goto cleanup;
> -}
> -if (qemuDomainHotplugPinThread(vm->def->cpumask, i, cpupids[i],
> - cgroup_vcpu) < 0) {
> -goto cleanup;
> -}
> +/* Inherit def->cpuset */
> +if (vm->def->cpumask) {
> +if (qemuDomainHotplugAddPin(vm->def->cpumask, vcpu,
> +&vm->def->cputune.vcpupin,
> +&vm->def->cputune.nvcpupin) < 0) {
> +goto cleanup;
> }
> -virCgroupFree(&cgroup_vcpu);
Ahhh.. finally ;-)
> -
> -if (qemuProcessSetSchedParams(i, cpupids[i],
> - vm->def->cputune.nvcpusched,
> - vm->def->cputune.vcpusched) < 0)
> +if (qemuDomainHotplugPinThread(vm->def->cpumask, vcpu, cpupids[vcpu],
> + cgroup_vcpu) < 0) {
> goto cleanup;
> +}
> }
>
> +if (qemuProcessSetSchedParams(vcpu, cpupids[vcpu],
> + vm->def->cputune.nvcpusched,
> + vm->def->cputune.vcpusched) < 0)
> +goto cleanup;
> +
> priv->nvcpupids = ncpupids;
> VIR_FREE(priv->vcpupids);
> priv->vcpupids = cpupids;
> @@ -4791,7 +4784,7 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> if (virDomainObjIsActive(vm) &&
> virDomainDefSetVCpus(vm->def, vcpus) < 0)
> ret = -1;
> -virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", ret == 0);
> +virDomainAuditVcpu(vm, oldvcpus, vcpus, "update", ret == 0);
> if (cgroup_vcpu)
> virCgroupFree(&cgroup_vcpu);
> return ret;
> @@ -4805,10 +4798,9 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver
Re: [libvirt] [PATCH 22/34] qemu: monitor: Remove weird return values from qemuMonitorSetCPU
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Let the function report errors internally and change it to return
> standard return codes.
> ---
> src/qemu/qemu_driver.c | 22 --
> src/qemu/qemu_monitor_json.c | 4
> src/qemu/qemu_monitor_text.c | 22 +++---
> 3 files changed, 15 insertions(+), 33 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 49fdd63..9011b2d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4698,7 +4698,6 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> size_t i;
> -int rc = 1;
> int ret = -1;
> int oldvcpus = virDomainDefGetVCpus(vm->def);
> int vcpus = oldvcpus;
> @@ -4712,10 +4711,7 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
>
> for (i = vcpus; i < nvcpus; i++) {
> /* Online new CPU */
> -rc = qemuMonitorSetCPU(priv->mon, i, true);
> -if (rc == 0)
> -goto unsupported;
> -if (rc < 0)
> +if (qemuMonitorSetCPU(priv->mon, i, true) < 0)
> goto exit_monitor;
>
> vcpus++;
> @@ -4795,14 +4791,11 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> if (virDomainObjIsActive(vm) &&
> virDomainDefSetVCpus(vm->def, vcpus) < 0)
> ret = -1;
> -virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1);
> +virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", ret == 0);
> if (cgroup_vcpu)
> virCgroupFree(&cgroup_vcpu);
> return ret;
>
> - unsupported:
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("cannot change vcpu count of this domain"));
> exit_monitor:
> ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto cleanup;
> @@ -4816,7 +4809,6 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> size_t i;
> -int rc = 1;
> int ret = -1;
> int oldvcpus = virDomainDefGetVCpus(vm->def);
> int vcpus = oldvcpus;
> @@ -4827,10 +4819,7 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
>
> for (i = vcpus - 1; i >= nvcpus; i--) {
> /* Offline old CPU */
> -rc = qemuMonitorSetCPU(priv->mon, i, false);
> -if (rc == 0)
> -goto unsupported;
> -if (rc < 0)
> +if (qemuMonitorSetCPU(priv->mon, i, false) < 0)
> goto exit_monitor;
>
> vcpus--;
> @@ -4889,12 +4878,9 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
> if (virDomainObjIsActive(vm) &&
> virDomainDefSetVCpus(vm->def, vcpus) < 0)
> ret = -1;
> -virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1);
> +virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", ret == 0);
> return ret;
>
> - unsupported:
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("cannot change vcpu count of this domain"));
> exit_monitor:
> ignore_value(qemuDomainObjExitMonitor(driver, vm));
> goto cleanup;
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 86b8c7b..50d6f62 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
Need to adjust comments here... Probably could move the comments to
qemuMonitorSetCPU just so it doesn't cause chase into second level to
know what function returns.
> @@ -2188,10 +2188,6 @@ int qemuMonitorJSONSetCPU(qemuMonitorPtr mon,
> else
> ret = qemuMonitorJSONCheckError(cmd, reply);
>
> -/* this function has non-standard return values, so adapt it */
> -if (ret == 0)
> -ret = 1;
> -
> cleanup:
> virJSONValueFree(cmd);
> virJSONValueFree(reply);
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index f44da20..fd38d01 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -1137,8 +1137,7 @@ qemuMonitorTextSetBalloon(qemuMonitorPtr mon,
>
>
> /*
> - * Returns: 0 if CPU hotplug not supported, +1 if CPU hotplug worked
> - * or -1 on failure
> + * Returns: 0 if CPU modification was successful or -1 on failure
> */
Could copy/move the comment to qemuMonitorSetCPU
ACK - as long as JSON function comments modified.
John
> int qemuMonitorTextSetCPU(qemuMonitorPtr mon, int cpu, bool online)
> {
> @@ -1149,22 +1148,23 @@ int qemuMonitorTextSetCPU(qemuMonitorPtr mon, int
> cpu, bool online)
> if (virAsprintf(&cmd, "cpu_set %d %s", cpu, online ? "online" :
> "offline") < 0)
> return -1;
>
> -if (qemuMonitorHMPCommand(mon, cmd, &reply) < 0) {
> -VIR_FREE(cmd);
> -return -1;
> -}
> -VIR_FREE(cmd);
> +if (qemuMonitorHMPCommand(mon, cmd, &reply) < 0)
> +goto cleanup;
>
> /* If the command failed qemu prints: 'unknown command'
> * No message is printed on success it seems */
> if (strstr(reply, "unknown command:"
Re: [libvirt] [PATCH 21/34] qemu: monitor: Explain logic of qemuMonitorGetCPUInfo
On 11/20/2015 10:22 AM, Peter Krempa wrote: > The return value has non-obvious semantics. Document it. > --- > src/qemu/qemu_monitor.c | 9 + > 1 file changed, 9 insertions(+) > ACK - wish I'd peeked two ahead before I sent last one ;-) John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 20/34] qemu: cpu hotplug: Fix error handling logic
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> The cpu hotplug helper functions used negative error handling in a part
> of them, although some code that was added later didn't properly set the
> error codes in some cases. This would cause improper error messages in
> cases where we couldn't modify the numa cpu mask and a few other cases.
>
> Fix the logic by converting it to the regularly used pattern.
> ---
> src/qemu/qemu_driver.c | 26 +-
> 1 file changed, 9 insertions(+), 17 deletions(-)
>
ACK (could have removed a couple of open/close {} brackets [1]
One other "could do" thing since I peeked to the next patch -
qemuMonitorSetCPU could lift the comments from qemuMonitorJSONSetCPU or
qemuMonitorTextSetCPU...
John
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a483220..49fdd63 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4721,10 +4721,6 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> vcpus++;
> }
>
> -/* hotplug succeeded */
> -
> -ret = 0;
> -
> /* After hotplugging the CPUs we need to re-detect threads corresponding
> * to the virtual CPUs. Some older versions don't provide the thread ID
> * or don't have the "info cpus" command (and they don't support multiple
> @@ -4732,12 +4728,12 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> * fatal */
> if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) {
> virResetLastError();
> +ret = 0;
> goto exit_monitor;
> }
> -if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> -ret = -1;
> +
> +if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
> -}
>
> if (ncpupids != vcpus) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -4745,7 +4741,6 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> "got %d, wanted %d"),
> ncpupids, vcpus);
> vcpus = oldvcpus;
> -ret = -1;
> goto cleanup;
> }
>
> @@ -4772,12 +4767,10 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> if (qemuDomainHotplugAddPin(vm->def->cpumask, i,
> &vm->def->cputune.vcpupin,
> &vm->def->cputune.nvcpupin) < 0) {
> -ret = -1;
> goto cleanup;
> }
[1] {} not necessary
> if (qemuDomainHotplugPinThread(vm->def->cpumask, i, cpupids[i],
> cgroup_vcpu) < 0) {
> -ret = -1;
> goto cleanup;
> }
[1] {} not necessary
> }
> @@ -4794,6 +4787,8 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> priv->vcpupids = cpupids;
> cpupids = NULL;
>
> +ret = 0;
> +
> cleanup:
> VIR_FREE(cpupids);
> VIR_FREE(mem_mask);
> @@ -4841,8 +4836,6 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
> vcpus--;
> }
>
> -ret = 0;
> -
> /* After hotplugging the CPUs we need to re-detect threads corresponding
> * to the virtual CPUs. Some older versions don't provide the thread ID
> * or don't have the "info cpus" command (and they don't support multiple
> @@ -4850,19 +4843,17 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
> * fatal */
> if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) {
> virResetLastError();
> +ret = 0;
> goto exit_monitor;
> }
> -if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> -ret = -1;
> +if (qemuDomainObjExitMonitor(driver, vm) < 0)
> goto cleanup;
> -}
>
> /* check if hotunplug has failed */
> if (ncpupids == oldvcpus) {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> _("qemu didn't unplug the vCPUs properly"));
> vcpus = oldvcpus;
> -ret = -1;
> goto cleanup;
> }
>
> @@ -4872,7 +4863,6 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
> "got %d, wanted %d"),
> ncpupids, vcpus);
> vcpus = oldvcpus;
> -ret = -1;
> goto cleanup;
> }
>
> @@ -4892,6 +4882,8 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
> priv->vcpupids = cpupids;
> cpupids = NULL;
>
> +ret = 0;
> +
> cleanup:
> VIR_FREE(cpupids);
> if (virDomainObjIsActive(vm) &&
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 19/34] qemu: Split up vCPU hotplug and hotunplug
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> There's only very little common code among the two operations. Split the
> functions so that the internals are easier to understand and refactor
> later.
> ---
> src/qemu/qemu_driver.c | 210
> -
> 1 file changed, 136 insertions(+), 74 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 72879cf..a483220 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4710,31 +4710,15 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
>
> qemuDomainObjEnterMonitor(driver, vm);
>
> -/* We need different branches here, because we want to offline
> - * in reverse order to onlining, so any partial fail leaves us in a
> - * reasonably sensible state */
I originally though it might have be nice to carry this comment in the
unplug - just to understand why going in reverse order was chosen, but I
see eventually that becomes irrelevant.
> -if (nvcpus > vcpus) {
> -for (i = vcpus; i < nvcpus; i++) {
> -/* Online new CPU */
> -rc = qemuMonitorSetCPU(priv->mon, i, true);
> -if (rc == 0)
> -goto unsupported;
> -if (rc < 0)
> -goto exit_monitor;
> -
> -vcpus++;
> -}
> -} else {
> -for (i = vcpus - 1; i >= nvcpus; i--) {
> -/* Offline old CPU */
> -rc = qemuMonitorSetCPU(priv->mon, i, false);
> -if (rc == 0)
> -goto unsupported;
> -if (rc < 0)
> -goto exit_monitor;
> +for (i = vcpus; i < nvcpus; i++) {
> +/* Online new CPU */
> +rc = qemuMonitorSetCPU(priv->mon, i, true);
> +if (rc == 0)
> +goto unsupported;
> +if (rc < 0)
> +goto exit_monitor;
>
> -vcpus--;
> -}
> +vcpus++;
> }
>
> /* hotplug succeeded */
> @@ -4755,15 +4739,6 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> -/* check if hotplug has failed */
> -if (vcpus < oldvcpus && ncpupids == oldvcpus) {
> -virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> - _("qemu didn't unplug the vCPUs properly"));
> -vcpus = oldvcpus;
> -ret = -1;
> -goto cleanup;
> -}
> -
> if (ncpupids != vcpus) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("got wrong number of vCPU pids from QEMU monitor. "
> @@ -4781,50 +4756,37 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> &mem_mask, -1) < 0)
> goto cleanup;
>
> -if (nvcpus > oldvcpus) {
> -for (i = oldvcpus; i < nvcpus; i++) {
> -if (priv->cgroup) {
> -cgroup_vcpu =
> -qemuDomainAddCgroupForThread(priv->cgroup,
> - VIR_CGROUP_THREAD_VCPU,
> - i, mem_mask,
> - cpupids[i]);
> -if (!cgroup_vcpu)
> -goto cleanup;
> -}
Good thing I peeked ahead one patch ;-) Was going to make a comment
about the ret = -1; logic especially w/r/t how [n]vcpupids is handled.
> +for (i = oldvcpus; i < nvcpus; i++) {
> +if (priv->cgroup) {
> +cgroup_vcpu =
> +qemuDomainAddCgroupForThread(priv->cgroup,
> + VIR_CGROUP_THREAD_VCPU,
> + i, mem_mask,
> + cpupids[i]);
> +if (!cgroup_vcpu)
> +goto cleanup;
> +}
>
> -/* Inherit def->cpuset */
> -if (vm->def->cpumask) {
> -if (qemuDomainHotplugAddPin(vm->def->cpumask, i,
> -&vm->def->cputune.vcpupin,
> -&vm->def->cputune.nvcpupin) < 0)
> {
> -ret = -1;
> -goto cleanup;
> -}
> -if (qemuDomainHotplugPinThread(vm->def->cpumask, i,
> cpupids[i],
> - cgroup_vcpu) < 0) {
> -ret = -1;
> -goto cleanup;
> -}
> +/* Inherit def->cpuset */
> +if (vm->def->cpumask) {
> +if (qemuDomainHotplugAddPin(vm->def->cpumask, i,
> +&vm->def->cputune.vcpupin,
> +&vm->def->cputune.nvcpupin) < 0) {
> +ret = -1;
> +goto cleanup;
> }
> -virCgroupFree(&cgroup_vcpu);
> -
> -if (qemuProcessSetSchedParams(i, cpupids[i],
> -
[libvirt] [PATCH v2 1/2] libxl: rename libxlConsoleCallback
. to a more generic name i.e. libxlDomainStartCallback,
since it will now cover another case other than the console.
Signed-off-by: Joao Martins
---
src/libxl/libxl_domain.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 40dcea1..a7267b0 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -854,7 +854,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config
*d_config)
}
static void
-libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
+libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
{
virDomainObjPtr vm = for_callback;
size_t i;
@@ -988,7 +988,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
virObjectUnlock(vm);
aop_console_how.for_callback = vm;
-aop_console_how.callback = libxlConsoleCallback;
+aop_console_how.callback = libxlDomainStartCallback;
if (restore_fd < 0) {
ret = libxl_domain_create_new(cfg->ctx, &d_config,
&domid, NULL, &aop_console_how);
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/2] libxl: implement virDomainInterfaceStats
Introduce support for domainInterfaceStats API call for querying
network interface statistics. Consequently it also enables the
use of `virsh domifstat ` command plus
seeing the interfaces names instead of "-" when doing
`virsh domiflist `.
After successful guest creation we fill the network
interfaces names based on domain, device id and append suffix
if it's emulated in the following form: vif.[-emu].
We extract the network interfaces info from libxl in
libxlDomainStartCallback() and make ifname . On domain
cleanup we also clear ifname, in case it was set by libvirt (i.e.
being prefixed with "vif"). We also skip these two steps in case the name
of the interface was manually inserted by the adminstrator.
For getting the interface statistics we resort to virNetInterfaceStats
and let libvirt handle the platform specific nits. Note that the latter
is not yet supported in FreeBSD.
Signed-off-by: Joao Martins
---
Changes since v3:
- Use libxl_device_nic_list() for getting each network interface
devid in DomainStartCallback.
- Improve error reporting by appropriately setting the right error
when no interface is known.
- Do not unlock vm if libxlDomainObjEndJob() returns false
- Set vm->def->net[i]->ifname on DomainStartCallback instead of
DomainStart.
- Change commit message reflecting the changes on the previous
item and mention correct interface names when doing domiflist.
Changes since v2:
- Clear ifname if it's autogenerated, since otherwise will persist
on successive domain starts. Change commit message reflecting this
change.
Changes since v1:
- Fill .ifname after domain start with generated
name from libxl based on domain id and devid returned by libxl.
After that path validation don interfaceStats is enterily based
on ifname pretty much like the other drivers.
- Modify commit message reflecting the changes mentioned in
the previous item.
- Bump version to 1.2.22
---
src/libxl/libxl_domain.c | 29 +++
src/libxl/libxl_driver.c | 52
2 files changed, 81 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index a7267b0..141f241 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
}
}
+if ((vm->def->nnets)) {
+ssize_t i;
+
+for (i = 0; i < vm->def->nnets; i++) {
+virDomainNetDefPtr net = vm->def->nets[i];
+
+if (STRPREFIX(net->ifname, "vif"))
+VIR_FREE(net->ifname);
+}
+}
+
if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) {
if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name);
@@ -857,6 +868,8 @@ static void
libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
{
virDomainObjPtr vm = for_callback;
+libxl_device_nic *nics;
+int nnics;
size_t i;
virObjectLock(vm);
@@ -883,6 +896,22 @@ libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev,
void *for_callback)
VIR_FREE(console);
}
}
+
+if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL) {
+for (i = 0; i < vm->def->nnets && i < nnics; i++) {
+virDomainNetDefPtr net = vm->def->nets[i];
+libxl_device_nic *x_nic = &nics[i];
+const char *suffix =
+x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : "";
+
+if (net->ifname)
+continue;
+
+if (virAsprintf(&net->ifname, "vif%d.%d%s",
+ev->domid, x_nic->devid, suffix) < 0)
+continue;
+}
+}
virObjectUnlock(vm);
libxl_event_free(ctx, ev);
}
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d77a0e4..55d991b 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -58,6 +58,7 @@
#include "virhostdev.h"
#include "network/bridge_driver.h"
#include "locking/domain_lock.h"
+#include "virstats.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -4643,6 +4644,56 @@ libxlDomainIsUpdated(virDomainPtr dom)
}
static int
+libxlDomainInterfaceStats(virDomainPtr dom,
+ const char *path,
+ virDomainInterfaceStatsPtr stats)
+{
+libxlDriverPrivatePtr driver = dom->conn->privateData;
+virDomainObjPtr vm;
+ssize_t i;
+int ret = -1;
+
+if (!(vm = libxlDomObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainInterfaceStatsEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+goto endjob;
+}
+
+/* C
[libvirt] [PATCH v2 0/2] libxl: implement virDomainInterfaceStats
Hey, This patch series implements virDomainInterfaceStats but based on "ConsoleCallback" as opposed of doing it in libxlDomainStart. The series is divided as following: Patch 1 renames console callback to something more generic and Patch 2 implements virDomainInterfaceStats also taking the previous review (v3 statistics) comments. Changes since v1: - Improve error reporting in case interface is not known - Use libxl_device_nic_list as opposed to libxl_domain_config Regards, Joao Joao Martins (2): libxl: rename libxlConsoleCallback libxl: implement virDomainInterfaceStats src/libxl/libxl_domain.c | 33 -- src/libxl/libxl_driver.c | 52 2 files changed, 83 insertions(+), 2 deletions(-) -- 2.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/34] qemu: qemuDomainSetVcpusAgent: re-check agent before calling it the again
On 11/20/2015 10:22 AM, Peter Krempa wrote: > With a very unfortunate timing, the agent might vanish before we do the > second call while the locks were down. Re-check that the agent is > available before attempting it again. > --- > src/qemu/qemu_driver.c | 3 +++ > 1 file changed, 3 insertions(+) > ACK John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] document virCommandRunRegex function
On Mon, Nov 23, 2015 at 03:09:49PM +0100, Christian Loehle wrote: > >From 01a3ed1e6bacba8cd9f398e5233960714b2c4f49 Mon Sep 17 00:00:00 2001 > From: Christian Loehle > Date: Mon, 23 Nov 2015 15:06:37 +0100 > Subject: [PATCH] =?UTF-8?q?document=20virCommandRunRegex=20function=C3=84?= > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > --- > src/util/vircommand.c | 16 ++-- > 1 file changed, 14 insertions(+), 2 deletions(-) ACK, I will push this tomorrow with the following changes: > diff --git a/src/util/vircommand.c b/src/util/vircommand.c > index c7f1538..a88cc13 100644 > --- a/src/util/vircommand.c > +++ b/src/util/vircommand.c > @@ -2889,12 +2889,24 @@ virCommandSetDryRun(virBufferPtr buf, > } > > #ifndef WIN32 > -/* > +/** > + * virCommandRunRegex: > + * @cmd: command to run > + * @nregex: number of regexes to apply > + * @regex: array of regexes to apply > + * @nvars: array of numbers of variables each regex will produce > + * @func: callback function that is called for every line of output, > + * needs to return 0 on success > + * @data: additional data that will be passed to the callback function > + * @prefix: prefix that will be skipped at the beginning of each line > + * @exitstatus: 0 on success, -1 on memory allocation error, virCommandRun > + * error or callback function error > + * We use the format "Returns:" after the description of what the function does elswehere in the file. > * Run an external program. > * > * Read its output and apply a series of regexes to each line > * When the entire set of regexes has matched consecutively > - * then run a callback passing in all the matches > + * then run a callback passing in all the matches of the current line. s/of/on/ (Also, this does not seem very useful for multiple regexes, but we only call the function with one regex.) 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 5/6] RFC qemu: add spice opengl support
On Fri, Nov 20, 2015 at 04:30:44PM +0100, Marc-André Lureau wrote: > Add Spice graphics gl attribute. qemu 2.6 should have -spice gl=on > argument to enable opengl rendering context. This is necessary > to actually enable virgl rendering. > > Signed-off-by: Marc-André Lureau > --- > docs/formatdomain.html.in | 6 ++ > docs/schemas/domaincommon.rng | 5 + > src/conf/domain_conf.c | 11 +++ > src/conf/domain_conf.h | 1 + A qemuxml2xmltest would be nice to go with the parser. > src/qemu/qemu_capabilities.c| 2 ++ > src/qemu/qemu_capabilities.h| 1 + > src/qemu/qemu_command.c | 10 ++ And the qemuxml2argvtest can be squashed in with the cmd line formatter. > tests/qemucapabilitiesdata/caps_2.5.0-1.caps| 1 + > tests/qemucapabilitiesdata/caps_2.5.0-1.replies | 4 > 9 files changed, 41 insertions(+) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index df29fa1..d7e1d49 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -4979,6 +4979,12 @@ qemu-kvm -net nic,model=? /dev/null >0.8.8); and usbredir >(since 0.9.12). > > + > + Spice may provide accelerated server-side rendering with > + OpenGL. You can enable OpenGL support with > + gl attribute. It is disabled by default. > + (since 1.1.22). According to the plan: http://wiki.qemu.org/Planning/2.5 QEMU 2.5.0 is planned for 10th Dec. Only features merged on qemu master are accepted to libvirt, so this won't make the next 1.3.0 release at the beginning of December. > + > >> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 228f062..8f4d2ac 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -2711,6 +2711,11 @@ > > > > + > + > + This accepts "yes", "no" and "default", > + > + > > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 15413dc..c4bdd11 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -10952,6 +10953,12 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, > VIR_FREE(autoport); > } > > +if ((gl = virXMLPropString(node, "gl")) != NULL) { > +if (STREQ(gl, "yes")) > +def->data.spice.gl = true; > +VIR_FREE(gl); > +} but the parser only implements "yes" and "default". Overall, the XML looks good to me. Apart from the accel3d attribute, which is ugly, but less ugly than inveting a separate one. Anyone else with an opinion? 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 06/24] qemu: Always set async job when starting a domain
On Thu, Nov 12, 2015 at 07:37:08PM +0100, Jiri Denemark wrote: > We only started an async job for incoming migration from another host. > When we were starting a domain from scratch or restoring from a saved > state (migration from file) we didn't set any async job. Let's introduce > a new QEMU_ASYNC_JOB_START for these cases. > > Signed-off-by: Jiri Denemark After this patch was applied to get, libvirtd starting printing out warnings for me when booting guests 2015-11-23 17:55:49.878+: 32094: warning : qemuDomainObjEnterMonitorInternal:1750 : This thread seems to be the async job owner; entering monitor without asking for a nested job is dangerous 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
Re: [libvirt] [PATCH 2/6] qemu: query virtio-gpu for virgl support
On Fri, Nov 20, 2015 at 04:30:41PM +0100, Marc-André Lureau wrote: > Check if virtio-gpu provides virgl option. > The commit also formats qemu command line. > Signed-off-by: Marc-André Lureau > --- > src/qemu/qemu_capabilities.c | 8 ++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_command.c | 15 +++- The qemuxml2argvtest can be squashed together with the command line formatter. > tests/qemucapabilitiesdata/caps_1.2.2-1.replies | 22 -- > tests/qemucapabilitiesdata/caps_1.3.1-1.replies | 22 -- > tests/qemucapabilitiesdata/caps_1.4.2-1.replies | 22 -- > tests/qemucapabilitiesdata/caps_1.5.3-1.replies | 22 -- > tests/qemucapabilitiesdata/caps_1.6.0-1.replies | 22 -- > tests/qemucapabilitiesdata/caps_1.6.50-1.replies | 22 -- > tests/qemucapabilitiesdata/caps_2.1.1-1.replies | 22 -- > tests/qemucapabilitiesdata/caps_2.4.0-1.replies | 95 > ++-- > 11 files changed, 216 insertions(+), 57 deletions(-) > @@ -1886,6 +1893,7 @@ virQEMUCapsExtractDeviceStr(const char *qemu, > "-device", "vmware-svga,?", > "-device", "qxl,?", > "-device", "qxl-vga,?", > + "-device", "virtio-gpu-pci,?", > NULL); > /* qemu -help goes to stdout, but qemu -device ? goes to stderr. */ > virCommandSetErrorBuffer(cmd, &output); This hunk should not be needed; -help parsing is only used for QEMU older than 1.2.0. 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 1/6] qemu: add virtio video device
On Fri, Nov 20, 2015 at 04:30:40PM +0100, Marc-André Lureau wrote:
> qemu 2.5 provides virtio video device. Similarly to other devices,
> it can be used with -vga virtio or -device virtio-vga for primary
> devices, or -device virtio-gpu for non-vga devices.
>
This adds virtio-vga but not virtio-gpu. It would be nice to say
so in the commit message.
There is a public bug filed for this functionality that would also be
nice to include in the commit message.
https://bugzilla.redhat.com/show_bug.cgi?id=1195176
> Signed-off-by: Marc-André Lureau
> ---
> docs/formatdomain.html.in| 5 +++--
> docs/schemas/domaincommon.rng| 1 +
> src/conf/domain_conf.c | 3 ++-
> src/conf/domain_conf.h | 1 +
The .xml test file could be squashed in with the parser changes and
added to qemuxml2xmltest as well.
> src/qemu/qemu_capabilities.c | 11 +++
> src/qemu/qemu_capabilities.h | 3 +++
> src/qemu/qemu_command.c | 19 +++
> tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 3 +++
> 8 files changed, 39 insertions(+), 7 deletions(-)
Similarly to qemu command line builder and the qemuxml2argvtest
addition.
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index e5e0167..df29fa1 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -5153,8 +5153,9 @@ qemu-kvm -net nic,model=? /dev/null
>
>The model element has a mandatory type
>attribute which takes the value "vga", "cirrus", "vmvga", "xen",
> - "vbox", or "qxl" (since 0.8.6) depending
> - on the hypervisor features available.
> + "vbox", "qxl" (since 0.8.6) or
> + "virtio" (since 1.2.22)
> + depending on the hypervisor features available.
The next planned version is 1.3.0.
>
>
>You can provide the amount of video memory in kibibytes (blocks of
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 994face..228f062 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2921,6 +2921,7 @@
> vmvga
> xen
> vbox
> +virtio
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0ac7dbf..15413dc 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -532,7 +532,8 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
>"xen",
>"vbox",
>"qxl",
> - "parallels")
> + "parallels",
> + "virtio")
>
This is for QEMUs without -device support, I hope nobody would bother
backporting the feature to such ancient QEMUs.
> VIR_ENUM_IMPL(virDomainInput, VIR_DOMAIN_INPUT_TYPE_LAST,
>"mouse",
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8d43ee6..c26c56d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1374,6 +1374,7 @@ typedef enum {
> VIR_DOMAIN_VIDEO_TYPE_VBOX,
> VIR_DOMAIN_VIDEO_TYPE_QXL,
> VIR_DOMAIN_VIDEO_TYPE_PARALLELS, /* pseudo device for VNC in containers
> */
> +VIR_DOMAIN_VIDEO_TYPE_VIRTIO,
>
> VIR_DOMAIN_VIDEO_TYPE_LAST
> } virDomainVideoType;
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2813212..357980b 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -301,6 +301,9 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"gic-version",
>
>"incoming-defer", /* 200 */
> + "vga-virtio",
> + "virtio-gpu",
> + "virtio-vga",
> );
>
>
> @@ -1117,6 +1120,8 @@ virQEMUCapsComputeCmdFlags(const char *help,
> const char *nl = strstr(p, "\n");
> if (strstr(p, "|qxl"))
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_VGA_QXL);
> +if (strstr(p, "|virtio"))
> +virQEMUCapsSet(qemuCaps, QEMU_CAPS_VGA_VIRTIO);
The -help output is only parsed for QEMUs older than 1.2.0, this
capability can be dropped too.
> if ((p = strstr(p, "|none")) && p < nl)
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_VGA_NONE);
> }
> @@ -1543,6 +1548,9 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[]
> = {
> { "virtio-net-ccw", QEMU_CAPS_DEVICE_VIRTIO_NET },
> { "virtio-net-s390", QEMU_CAPS_DEVICE_VIRTIO_NET },
> { "virtio-net-device", QEMU_CAPS_DEVICE_VIRTIO_NET },
> +{ "virtio-gpu-pci", QEMU_CAPS_DEVICE_VIRTIO_GPU },
> +{ "virtio-gpu-device", QEMU_CAPS_DEVICE_VIRTIO_GPU },
> +{ "virtio-vga", QEMU_CAPS_DEVICE_VIRTIO_VGA },
> };
>
> static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = {
> @@ -2399,6 +2407,9 @@ virQEMUCapsProbeQMPObjects(virQEMUCaps
Re: [libvirt] [PATCH v4 02/10] Add iommu group number info to virPCIDevice
On 14.11.2015 09:35, Shivaprasad G Bhat wrote:
> The iommu group number need not be fetched from the sysfs
> everytime as it remains constant. Fetch it once during
> allocation.
>
> Signed-off-by: Shivaprasad G Bhat
> ---
> src/util/virpci.c |5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index bff37d7..89e69e2 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -75,6 +75,7 @@ struct _virPCIDevice {
> bool has_pm_reset;
> bool managed;
> char *stubDriver;
> +int iommuGroup;
>
> /* used by reattach function */
> bool unbind_from_stub;
> @@ -1564,6 +1565,8 @@ virPCIDeviceNew(unsigned int domain,
> virPCIDevicePtr dev;
> char *vendor = NULL;
> char *product = NULL;
> +virPCIDeviceAddress devAddr = { domain, bus,
> +slot, function };
I'd rather see this explicitly initialized {.domain = domain, .bus =
bus, ...} because if we ever change order of virPCIDeviceAddress struct
(no idea why we would do that right now, but we might) this will slip
silently.
>
> if (VIR_ALLOC(dev) < 0)
> return NULL;
> @@ -1611,6 +1614,8 @@ virPCIDeviceNew(unsigned int domain,
> goto error;
> }
>
> +dev->iommuGroup = virPCIDeviceAddressGetIOMMUGroupNum(&devAddr);
> +
> VIR_DEBUG("%s %s: initialized", dev->id, dev->name);
>
> cleanup:
>
ACK with that fixed.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 08/10] Add iommu info for pci on mocked sysfs
On 14.11.2015 09:38, Shivaprasad G Bhat wrote:
> The iommu group info can be used to check if the devices are bound/unbound
> from vfio at the group level granualrity. Add the info to mock as to help
> add test cases later.
>
> Signed-off-by: Shivaprasad G Bhat
> ---
> tests/virpcimock.c | 180
> +++-
> 1 file changed, 164 insertions(+), 16 deletions(-)
>
> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
> index 0724a36..837976a 100644
> --- a/tests/virpcimock.c
> +++ b/tests/virpcimock.c
> @@ -127,9 +127,15 @@ struct pciDevice {
> int vendor;
> int device;
> int class;
> +int iommu;
> struct pciDriver *driver; /* Driver attached. NULL if attached to no
> driver */
> };
>
> +struct pciIommuGroup {
> +int iommu;
> +size_t nDevicesBoundToVFIO;/* Indicates the devices in the group
> */
> +};
> +
> struct fdCallback {
> int fd;
> char *path;
> @@ -141,6 +147,9 @@ size_t nPCIDevices = 0;
> struct pciDriver **pciDrivers = NULL;
> size_t nPCIDrivers = 0;
>
> +struct pciIommuGroup **pciIommuGroups = NULL;
> +size_t npciIommuGroups = 0;
> +
> struct fdCallback *callbacks = NULL;
> size_t nCallbacks = 0;
>
> @@ -325,6 +334,7 @@ pci_device_new_from_stub(const struct pciDevice *data)
> char *configSrc;
> char tmp[32];
> struct stat sb;
> +char *iommugrouppath, *deviommupath, *iommugroupdevs = NULL;
>
> if (VIR_STRDUP_QUIET(id, data->id) < 0)
> ABORT_OOM();
> @@ -387,6 +397,25 @@ pci_device_new_from_stub(const struct pciDevice *data)
> ABORT("@tmp overflow");
> make_file(devpath, "class", tmp, -1);
>
> +if (virAsprintfQuiet(&deviommupath, "%s/iommu_group", devpath) < 0 ||
> +virAsprintfQuiet(&iommugrouppath, "%s/iommu_groups/%d",
> + fakesysfsdir, dev->iommu) < 0)
> +ABORT("@deviommupath overflow");
Technically this is not overflow rather than OOM. I guess you've just copied a
pattern just above these lines (not to be seen in this patch though), where we
really are snprintf()-ing into 32 bytes wide string. Here we have all the
memory, so this should be ABORT_OOM();
> +
> +if (symlink(iommugrouppath, deviommupath) < 0)
> +ABORT("Unable to link device to iommu group");
> +
> +VIR_FREE(deviommupath);
> +if (virAsprintfQuiet(&iommugroupdevs, "%s/devices/%s",
> + iommugrouppath, dev->id) < 0)
> +ABORT("@iommugroupdevs overflow");
Same here.
> +
> +if (symlink(devpath, iommugroupdevs) < 0)
> +ABORT("Unable to link iommu group devices to current device");
> +
> +VIR_FREE(iommugrouppath);
> +VIR_FREE(iommugroupdevs);
> +
> if (pci_device_autobind(dev) < 0)
> ABORT("Unable to bind: %s", data->id);
>
> @@ -435,7 +464,95 @@ pci_device_autobind(struct pciDevice *dev)
> return pci_driver_bind(driver, dev);
> }
>
> +static void
> +pci_iommu_new(int num)
> +{
> +char *iommupath;
> +struct pciIommuGroup *iommuGroup;
> +
> +if (VIR_ALLOC_QUIET(iommuGroup) < 0)
> +ABORT_OOM();
> +
> +iommuGroup->iommu = num;
> +iommuGroup->nDevicesBoundToVFIO = 0; /* No device bound to vfio by
> default */
> +
> +if (virAsprintfQuiet(&iommupath, "%s/iommu_groups/%d/devices",
> fakesysfsdir, num) < 0)
> +ABORT_OOM();
> +
> +if (virFileMakePath(iommupath) < 0)
> +ABORT("Unable to create: %s", iommupath);
> +
> +if (VIR_APPEND_ELEMENT_QUIET(pciIommuGroups, npciIommuGroups,
> iommuGroup) < 0)
> +ABORT_OOM();
@iommupath is no longer needed and leaked.
> +}
> +
> +static int
> +pci_vfio_release_iommu(struct pciDevice *device)
> +{
> +char *vfiopath = NULL;
> +int ret = -1;
> +size_t i = 0;
> +
> +for (i = 0; i < npciIommuGroups; i++) {
> +if (pciIommuGroups[i]->iommu == device->iommu)
> +break;
> +}
> +
> +if (i != npciIommuGroups) {
> +if (pciIommuGroups[i]->nDevicesBoundToVFIO == 0) {
> +ret = 0;
> +goto cleanup;
> +}
This is somewhat confusing to me. This can happen due to a bug in our code - in
that case I'd expect an error to be thrown.
> +pciIommuGroups[i]->nDevicesBoundToVFIO--;
> +if (!pciIommuGroups[i]->nDevicesBoundToVFIO) {
> +if (virAsprintfQuiet(&vfiopath, "%s/dev/vfio/%d",
> + fakesysfsdir, device->iommu) < 0) {
> +errno = ENOMEM;
> +goto cleanup;
> +}
> +if (unlink(vfiopath) < 0)
> +goto cleanup;
> +}
> +}
> +
> +ret = 0;
> + cleanup:
> +VIR_FREE(vfiopath);
> +return ret;
> +}
> +
> +static int
> +pci_vfio_lock_iommu(struct pciDevice *device)
> +{
> +char *vfiopath = NULL;
> +int ret = -1;
> +size_t i = 0;
> +int fd = -1;
> +
> +for (i = 0; i < npciIommuGroups; i++) {
> +if (pciIommuGroups[i]->iommu ==
Re: [libvirt] [PATCH v4 03/10] Refuse to reattach from vfio if the iommu group is in use by any domain
On 14.11.2015 09:36, Shivaprasad G Bhat wrote:
> It is incorrect to attempt the device reattach of a function,
> when some other domain is using some functions belonging to the same iommu
> group.
>
> Signed-off-by: Shivaprasad G Bhat
> ---
> src/util/virhostdev.c | 21 -
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index de029a0..f24ccd8 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -1590,18 +1590,35 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr
> hostdev_mgr,
> virPCIDevicePtr pci)
> {
> virPCIDeviceAddressPtr devAddr = NULL;
> +bool usesVfio = false;
> +char *drvPath = NULL;
> +char *drvName = NULL;
> struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL,
> false};
> int ret = -1;
>
> +if (virPCIDeviceGetDriverPathAndName(pci, &drvPath, &drvName) < 0)
> +goto out;
> +
> +if (STREQ_NULLABLE(drvName, "vfio-pci"))
> +usesVfio = true;
> +
> virObjectLock(hostdev_mgr->activePCIHostdevs);
> virObjectLock(hostdev_mgr->inactivePCIHostdevs);
>
> if (!(devAddr = virPCIDeviceGetAddress(pci)))
> goto out;
>
> -if (virHostdevIsPCINodeDeviceUsed(devAddr, &data))
> +if (usesVfio) {
> +/* Doesn't matter which device. If any domain is actively using the
> + * iommu group, refuse to reattach */
s/reattach/reattach./
And, does not matter right now, but maybe we will need to set
data.usesVfio = true; in this case. Currently, this will result in no-op
> +if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
> +
> virHostdevIsPCINodeDeviceUsed,
> + &data) < 0)
> +goto out;
> +} else if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) {
> goto out;
> +}
>
> virPCIDeviceReattachInit(pci);
>
> @@ -1614,6 +1631,8 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr
> hostdev_mgr,
> virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);
> virObjectUnlock(hostdev_mgr->activePCIHostdevs);
> VIR_FREE(devAddr);
> +VIR_FREE(drvPath);
> +VIR_FREE(drvName);
> return ret;
> }
>
And as you and Laine agreed, this is going to be turned into a list, so
this patch probably ends up being dropped anyway.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 06/10] Pass activeDevs and inactiveDevs to virPCIDeviceUnbindFromStub and virPCIDeviceBindToStub
On 14.11.2015 09:36, Shivaprasad G Bhat wrote:
> The inactiveDevs need to be selectively altered for more than one
> device in case of vfio devices. So, pass the whole list.
>
> Signed-off-by: Shivaprasad G Bhat
> ---
> src/util/virpci.c | 36 +++-
> 1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index febf100..a8a22d1 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1129,7 +1129,9 @@ virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev,
> }
>
> static int
> -virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> +virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
> + virPCIDeviceListPtr activeDevs ATTRIBUTE_UNUSED,
> + virPCIDeviceListPtr inactiveDevs)
> {
> int result = -1;
> char *drvdir = NULL;
> @@ -1186,6 +1188,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> result = 0;
>
> cleanup:
> +if ((result == 0) && inactiveDevs)
> +virPCIDeviceListDel(inactiveDevs, dev);
> +
> /* do not do it again */
> dev->unbind_from_stub = false;
> dev->remove_slot = false;
> @@ -1201,7 +1206,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
>
> static int
> virPCIDeviceBindToStub(virPCIDevicePtr dev,
> - const char *stubDriverName)
> + const char *stubDriverName,
> + virPCIDeviceListPtr activeDevs,
> + virPCIDeviceListPtr inactiveDevs)
> {
> int result = -1;
> bool reprobe = false;
> @@ -1328,9 +1335,15 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
> VIR_FREE(driverLink);
> VIR_FREE(path);
>
> +/* Add *a copy of* the dev into list inactiveDevs, if
> + * it's not already there. */
> +if ((result == 0) && inactiveDevs && !virPCIDeviceListFind(inactiveDevs,
> dev) &&
> +virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) {
> +result = -1;
> +}
> if (result < 0) {
> VIR_FREE(newDriverName);
> -virPCIDeviceUnbindFromStub(dev);
> +virPCIDeviceUnbindFromStub(dev, activeDevs, inactiveDevs);
> } else {
> VIR_FREE(dev->stubDriver);
> dev->stubDriver = newDriverName;
> @@ -1377,16 +1390,9 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
> return -1;
> }
>
> -if (virPCIDeviceBindToStub(dev, dev->stubDriver) < 0)
> -return -1;
> -
> -/* Add *a copy of* the dev into list inactiveDevs, if
> - * it's not already there.
> - */
> -if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) &&
> -virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) {
> +if (virPCIDeviceBindToStub(dev, dev->stubDriver,
> + activeDevs, inactiveDevs) < 0)
> return -1;
> -}
>
> return 0;
> }
> @@ -1402,13 +1408,9 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
> return -1;
> }
>
> -if (virPCIDeviceUnbindFromStub(dev) < 0)
> +if (virPCIDeviceUnbindFromStub(dev, activeDevs, inactiveDevs) < 0)
> return -1;
>
> -/* Steal the dev from list inactiveDevs */
> -if (inactiveDevs)
> -virPCIDeviceListDel(inactiveDevs, dev);
> -
> return 0;
> }
>
>
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 01/10] Implement virPCIIsKnownStub function
On 14.11.2015 09:34, Shivaprasad G Bhat wrote:
> The checks to known stubs can be easily done having this
> implementation.
>
> Signed-off-by: Shivaprasad G Bhat
> ---
> src/util/virpci.c | 28 ++--
> 1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 35b1459..bff37d7 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1080,6 +1080,23 @@ static const char *virPCIKnownStubs[] = {
> NULL
> };
>
> +static bool
> +virPCIIsKnownStub(char *driver)
> +{
> +const char **stubTest;
> +bool ret = false;
> +
> +for (stubTest = virPCIKnownStubs; *stubTest != NULL; stubTest++) {
> +if (STREQ_NULLABLE(driver, *stubTest)) {
> +ret = true;
> +VIR_DEBUG("Found stub driver %s", *stubTest);
> +break;
> +}
> +}
> +
> +return ret;
> +}
> +
> static int
> virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> {
> @@ -1087,8 +1104,6 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> char *drvdir = NULL;
> char *path = NULL;
> char *driver = NULL;
> -const char **stubTest;
> -bool isStub = false;
>
> /* If the device is currently bound to one of the "well known"
> * stub drivers, then unbind it, otherwise ignore it.
> @@ -1105,14 +1120,7 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> goto remove_slot;
>
> /* If the device isn't bound to a known stub, skip the unbind. */
> -for (stubTest = virPCIKnownStubs; *stubTest != NULL; stubTest++) {
> -if (STREQ(driver, *stubTest)) {
> -isStub = true;
> -VIR_DEBUG("Found stub driver %s", *stubTest);
> -break;
> -}
> -}
> -if (!isStub)
> +if (!virPCIIsKnownStub(driver))
> goto remove_slot;
>
> if (virPCIDeviceUnbind(dev, dev->reprobe) < 0)
>
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] libxl: add libxl_domain_config to libxlDomainObjPrivate
On 11/23/2015 03:35 PM, Jim Fehlig wrote: > On 11/20/2015 05:40 PM, Joao Martins wrote: >> >> On 11/20/2015 07:05 PM, Jim Fehlig wrote: >>> On 11/19/2015 04:45 PM, Joao Martins wrote: >>> >>> You're not going to be happy with me... >>> This new field in libxlDomainObjPrivate is named "config" and is kept while the domain is active. >>> While this sounded like a good idea when I mentioned it, I'm now worried >>> that >>> the config will quickly become stale and cause problems if used elsewhere >>> (e.g. >>> see my yet-to-be-written comment in 3/3). IIUC correctly, >>> libxl_domain_config >>> is only useful when creating the domain. Subsequently adding/removing >>> devices, >>> memory, vcpus, etc. would not be reflected in the libxl_domain_config >>> object. I >>> suppose it would useful (and still valid) in the start callback, but IMO >>> including it in the libxlDomainPrivate struct fools us into believing it >>> could >>> be used elsewhere throughout the life of the domain. I now have second >>> doubts >>> about this. What do you think? >> I agree with you, and since there a libxl_device_nic_list as you suggested, >> it >> would actually be much cleaner and safer compared to libxl_domain_config >> alternative (though with a small performance cost). And we would avoid end up >> having config just lying there with no additional use (besides StartCallback) >> and inconsistent info. >> >> The only thing that the libxlDomainObjPrivate approach is better than >> libxl_device_nic_list() would be that we don't need to refetch the devid, >> since >> the nics array has it correctly filled already when console callback is >> invoked. >> Whereas libxl_device_nic_list will refetch the same info (in additiona to all >> entries in the backend directory) from xenstore thus adding up overhead. But >> given that this is only once and in domain create I think it's not a big >> deal. > > Right. I think the extra overhead is in the noise relative to the other > activities involved in starting a domain. > >> Would you agree then to resend this series without this patch and using >> libxl_device_nic_list, as the final approach? Thanks for pointing out this >> issue! > > I think so. If you dislike the extra overhead of libxl_device_nic_list, > another > option would be something like a libxlDomainStartCallbackInfo struct that > contains the virDomainObj and libxl_domain_config, and is passed to the start > callback via for_callback of libxl_asyncop_how. That would allow us to use the > libxl_domain_config object in the callback, but still dispose it after the > start > completes. I did a quick measurement to double-check and have a rough idea of the "libxl_device_nic_list" cost. Each line is in the form of VIFs: / 1 VIF : 1066 us / 0.315 s 2 VIF : 1762 us / 0.375 s 4 VIF : 3528 us / 0.560 s 8 VIF : 6726 us / 0.953 s 16 VIF : 13378 us / 1.653 s It almost grows linearly with the number of NICs having ~1ms per NIC. And given the numbers above, I think the extra overhead is indeed small and neglible, so I'll be sending with the libxl_device_nic_list approach as also agreed in your previous comment. Regards, Joao > > Regards, > Jim > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 05/10] Split reprobe action from the virPCIUnbindFromStub into a new function
On 14.11.2015 09:36, Shivaprasad G Bhat wrote:
> The reprobe needs to be called multiple times for vfio devices for each
> device in the iommu group in future patch. So split the reprobe into a
> new function. No functional change.
>
> Signed-off-by: Shivaprasad G Bhat
> ---
> src/util/virpci.c | 47 +++
> 1 file changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 89e69e2..febf100 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1099,6 +1099,36 @@ virPCIIsKnownStub(char *driver)
> }
>
> static int
> +virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev,
> + const char *driver,
> + const char *drvdir)
> +{
> +char *path = NULL;
> +int ret = -1;
> +
> +/* Trigger a re-probe of the device is not in the stub's dynamic
> + * ID table. If the stub is available, but 'remove_id' isn't
> + * available, then re-probing would just cause the device to be
> + * re-bound to the stub.
> + */
> +if (driver && !(path = virPCIDriverFile(driver, "remove_id")))
> +goto cleanup;
> +
> +if (!driver || !virFileExists(drvdir) || virFileExists(path)) {
> +if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) {
> +virReportSystemError(errno,
> + _("Failed to trigger a re-probe for PCI
> device '%s'"),
> + dev->name);
> +goto cleanup;
> +}
> +}
> +ret = 0;
> + cleanup:
> +VIR_FREE(path);
> +return ret;
> +}
> +
> +static int
> virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> {
> int result = -1;
> @@ -1150,24 +1180,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> goto cleanup;
> }
>
> -/* Trigger a re-probe of the device is not in the stub's dynamic
> - * ID table. If the stub is available, but 'remove_id' isn't
> - * available, then re-probing would just cause the device to be
> - * re-bound to the stub.
> - */
> -VIR_FREE(path);
> -if (driver && !(path = virPCIDriverFile(driver, "remove_id")))
> +if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0)
> goto cleanup;
>
> -if (!driver || !virFileExists(drvdir) || virFileExists(path)) {
> -if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) {
> -virReportSystemError(errno,
> - _("Failed to trigger a re-probe for PCI
> device '%s'"),
> - dev->name);
> -goto cleanup;
> -}
> -}
> -
> result = 0;
>
> cleanup:
>
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/7] qemu: Introduce qemuProcessLaunch
On Thu, Nov 19, 2015 at 01:09:16PM +0100, Jiri Denemark wrote: > Once qemuProcessInit was called, qemuProcessLaunch will launch a new > QEMU process with stopped virtual CPUs. > > Signed-off-by: Jiri Denemark > --- > > Notes: > Version 2: > - save status before running VIR_HOOK_QEMU_OP_STARTED hook > - call virSecurityManagerSetAllLabel even if !incoming > - move qemuProcessStop out of qemuProcessLaunch to avoid it being called > twice > > src/qemu/qemu_process.c | 323 > > src/qemu/qemu_process.h | 9 ++ > 2 files changed, 199 insertions(+), 133 deletions(-) > ACK, Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 17/34] qemu: Extract vCPU onlining/offlining via agent into a separate function
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Separate the code so that qemuDomainSetVcpusFlags contains only code
> relevant to hardware hotplug/unplug.
> ---
> src/qemu/qemu_driver.c | 137
> +++--
> 1 file changed, 77 insertions(+), 60 deletions(-)
>
ACK 15, 16
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 95b9ede..ab22c65 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4853,6 +4853,59 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
>
>
This function relies on qemuAgentUpdateCPUInfo to perform the maxvcpus
checks that wouldn't be made by refactoring this code here. Perhaps
something worthy to note in the commit message (at least that's my
assumption based on reading the code).
ACK -
John
> static int
> +qemuDomainSetVcpusAgent(virDomainObjPtr vm,
> +unsigned int nvcpus)
> +{
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] how to know if PCI device has SR-IOV PF capability
On Mon, Nov 23, 2015 at 11:26:11AM -0500, Laine Stump wrote: > On 11/23/2015 10:40 AM, Daniel P. Berrange wrote: > >On Mon, Nov 23, 2015 at 10:34:43AM -0500, Laine Stump wrote: > > > >>If we're going to switch to emiting virt_functions whenever a device has the > >>potential to provide VFs, we may as well make it worthwhile and also emit > >>the maximum possible VFs for the device, maybe simply: > >> > >> > >> > >>(the current count is implicit in the number of entries in the list that > >>follows. I don't have an opinion on whether it is better to also include > >>explicitly with, e.g. "count='7'", or just leave it implicit). > >Is there any way for us to actually discover the max count ? If so, then > >it seems nice to include it. > > Yes. I don't know if it existed back when that code was originally added, > but at least RHEL6.7 (the oldest OS I have running on a machine with an > SRIOV-capable card) and later have two files in the device's sysfs, > sriov_numvfs and sriov_totalvfs. The former is the number that are currently > active, and the latter is the maximum possible for this PF. > > (On a related topic - you can change the number of currently active VFs by > writing "0" to sriov_numvfs then writing the desired number to it; this does > temporarily delete any VFs that are already active though, so it can only be > done if none are in use. I've planned to hook libvirt networks up to this so > that VFs can be enabled completely within libvirt (since the driver > commandline method isn't consistent between different vendors, and I believe > is now considered to be deprecated). Since Openstack doesn't use libvirt > networks but may want similar functionality, I'm wondering where would be a > good place to do that. We could provide something via the node-device API, > but that couldn't be automatically done by libvirtd at startup; alternately > Openstack could create a network but not use it, but that just seems > conceptually confusing even though it would work.) There is scope to extend node device APIs to allow definition of persistent config for virtual devices. We have similar scenario wrt NPIV devices which are dynamically allocatable 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
Re: [libvirt] how to know if PCI device has SR-IOV PF capability
On 11/23/2015 10:40 AM, Daniel P. Berrange wrote: On Mon, Nov 23, 2015 at 10:34:43AM -0500, Laine Stump wrote: If we're going to switch to emiting virt_functions whenever a device has the potential to provide VFs, we may as well make it worthwhile and also emit the maximum possible VFs for the device, maybe simply: (the current count is implicit in the number of entries in the list that follows. I don't have an opinion on whether it is better to also include explicitly with, e.g. "count='7'", or just leave it implicit). Is there any way for us to actually discover the max count ? If so, then it seems nice to include it. Yes. I don't know if it existed back when that code was originally added, but at least RHEL6.7 (the oldest OS I have running on a machine with an SRIOV-capable card) and later have two files in the device's sysfs, sriov_numvfs and sriov_totalvfs. The former is the number that are currently active, and the latter is the maximum possible for this PF. (On a related topic - you can change the number of currently active VFs by writing "0" to sriov_numvfs then writing the desired number to it; this does temporarily delete any VFs that are already active though, so it can only be done if none are in use. I've planned to hook libvirt networks up to this so that VFs can be enabled completely within libvirt (since the driver commandline method isn't consistent between different vendors, and I believe is now considered to be deprecated). Since Openstack doesn't use libvirt networks but may want similar functionality, I'm wondering where would be a good place to do that. We could provide something via the node-device API, but that couldn't be automatically done by libvirtd at startup; alternately Openstack could create a network but not use it, but that just seems conceptually confusing even though it would work.) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/8] perf: implement the public APIs for perf event
On Tue, Nov 17, 2015 at 16:00:43 +0800, Qiaowei Ren wrote: > * src/libvirt-domain.c: Implement virDomainGetPerfEvents and > virDomainSetPerfEvents. > > Signed-off-by: Qiaowei Ren > --- > src/libvirt-domain.c | 106 > +++ > 1 file changed, 106 insertions(+) > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > index de7eb04..e767606 100644 > --- a/src/libvirt-domain.c > +++ b/src/libvirt-domain.c > @@ -9572,6 +9572,112 @@ virDomainOpenChannel(virDomainPtr dom, > > > /** > + * virDomainGetPerfEvents: > + * @domain: pointer to domain object > + * @params: pointer to perf events parameter object > + * (return value, allocated by the caller) > + * @nparams: pointer to number of perf event parameters > + * > + * Get all perf events setting. On input, @nparams gives the size of the > + * @params array; on output, @nparams gives how many slots were filled > + * with parameter information, which might be less but will not exceed > + * the input value. > + * > + * As a special case, calling with @params as NULL and @nparams as 0 on > + * input will cause @nparams on output to contain the number of parameters > + * supported by the hypervisor. The caller should then allocate @params > + * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the > + * API again. > + * > + * See virDomainGetMemoryParameters() for an equivalent usage example. You took a wrong API to copy. Requiring the caller to run the API with @params == NULL to get the number of parameters and then pass the allocated @params to get the values is the original design which we do not use for new APIs. The API should just allocate @params, fill it with all the values it knows about, and return the count in @nparams. In other words, no caller allocated arrays, please. You can look at, e.g., virDomainGetJobStats to see how it works. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-glib v2 3/3] gobject: Port to GTask API
On Mon, Nov 23, 2015 at 4:08 PM, Christophe Fergeau wrote:
> On Fri, Nov 20, 2015 at 09:06:30PM +, Zeeshan Ali (Khattak) wrote:
>> Drop usage of deprecated GSimpleAsyncResult API.
>> ---
>
> A Changes since v1 section would be nice as this patch is a bit big.
>
>>
>> +typedef struct {
>> +guint flags;
>> +} DomainWakeupData;
>> +
>> +static void domain_wakeup_data_free(DomainWakeupData *data)
>> +{
>> +g_slice_free(DomainWakeupData, data);
>> +}
>> +
>
> Any reason for not using GINT_TO_POINTER? My feeling from the previous
> thread was that you were going to do that.
Oh, forgot.
--
Regards,
Zeeshan Ali (Khattak)
Befriend GNOME: http://www.gnome.org/friends/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Fw:[PATCH] fix configure for pcap
On Mon, Nov 23, 2015 at 03:58:25PM +0800, 程宝传 wrote: > when cross-compiling, pcap can not point to the correct toolchain. > > configure.ac | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/configure.ac b/configure.ac > index f481c50..d6d2cf4 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -1555,7 +1555,7 @@ if test "$with_numad" != "no" ; then >with_numad="yes" > fi >else > -test -z "$NUMAD" && > +test -z "$NUMAD" &&/ >AC_MSG_ERROR([You must install numad package to manage CPU and memory > placement dynamically]) > > test "$with_numactl" = "yes" || fail=1 > @@ -1588,8 +1588,13 @@ if test "$with_qemu" = "yes"; then > if ! $LIBPCAP_CONFIG --libs > /dev/null 2>&1 ; then >AC_MSG_RESULT(no) > else > - LIBPCAP_LIBS="`$LIBPCAP_CONFIG --libs`" > - LIBPCAP_CFLAGS="`$LIBPCAP_CONFIG --cflags`" > + if test -n $with_libtool_sysroot && test -n $lt_sysroot; then > + LIBPCAP_LIBS="-lpcap" > + LIBPCAP_CFLAGS="-I$lt_sysroot/usr/include" > + else > + LIBPCAP_LIBS="`$LIBPCAP_CONFIG --libs`" > +LIBPCAP_CFLAGS="`$LIBPCAP_CONFIG --cflags`" > + fi >LIBPCAP_FOUND="yes" >AC_MSG_RESULT(yes) > fi This doesn't really look very right. If you are cross compiling you need $PATH to include your sysroot, so that you find the libpcap-config program for your sysroot, instead of the native arch. 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
Re: [libvirt] [libvirt-glib v2 3/3] gobject: Port to GTask API
On Fri, Nov 20, 2015 at 09:06:30PM +, Zeeshan Ali (Khattak) wrote:
> Drop usage of deprecated GSimpleAsyncResult API.
> ---
A Changes since v1 section would be nice as this patch is a bit big.
>
> +typedef struct {
> +guint flags;
> +} DomainWakeupData;
> +
> +static void domain_wakeup_data_free(DomainWakeupData *data)
> +{
> +g_slice_free(DomainWakeupData, data);
> +}
> +
Any reason for not using GINT_TO_POINTER? My feeling from the previous
thread was that you were going to do that.
Christophe
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Fw:[PATCH] fix configure for pcap
when cross-compiling, pcap can not point to the correct toolchain. configure.ac | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index f481c50..d6d2cf4 100644 --- a/configure.ac +++ b/configure.ac @@ -1555,7 +1555,7 @@ if test "$with_numad" != "no" ; then with_numad="yes" fi else -test -z "$NUMAD" && +test -z "$NUMAD" &&/ AC_MSG_ERROR([You must install numad package to manage CPU and memory placement dynamically]) test "$with_numactl" = "yes" || fail=1 @@ -1588,8 +1588,13 @@ if test "$with_qemu" = "yes"; then if ! $LIBPCAP_CONFIG --libs > /dev/null 2>&1 ; then AC_MSG_RESULT(no) else - LIBPCAP_LIBS="`$LIBPCAP_CONFIG --libs`" - LIBPCAP_CFLAGS="`$LIBPCAP_CONFIG --cflags`" + if test -n $with_libtool_sysroot && test -n $lt_sysroot; then + LIBPCAP_LIBS="-lpcap" + LIBPCAP_CFLAGS="-I$lt_sysroot/usr/include" + else + LIBPCAP_LIBS="`$LIBPCAP_CONFIG --libs`" +LIBPCAP_CFLAGS="`$LIBPCAP_CONFIG --cflags`" + fi LIBPCAP_FOUND="yes" AC_MSG_RESULT(yes) fi -- 2.6.3.windows.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 14/34] conf: Replace read accesses to def->vcpus with accessor
On 11/20/2015 10:22 AM, Peter Krempa wrote: > --- > src/conf/domain_audit.c| 2 +- > src/conf/domain_conf.c | 19 +-- > src/conf/domain_conf.h | 1 + > src/libvirt_private.syms | 1 + > src/libxl/libxl_conf.c | 2 +- > src/libxl/libxl_driver.c | 8 > src/lxc/lxc_controller.c | 2 +- > src/lxc/lxc_driver.c | 2 +- > src/openvz/openvz_driver.c | 2 +- > src/phyp/phyp_driver.c | 5 +++-- > src/qemu/qemu_command.c| 2 +- > src/qemu/qemu_driver.c | 36 ++-- > src/qemu/qemu_process.c| 10 +- > src/test/test_driver.c | 14 +++--- > src/uml/uml_driver.c | 2 +- > src/vmware/vmware_driver.c | 2 +- > src/vmx/vmx.c | 14 -- > src/xen/xm_internal.c | 4 ++-- > src/xenapi/xenapi_utils.c | 2 +- > src/xenconfig/xen_common.c | 3 ++- > src/xenconfig/xen_sxpr.c | 5 +++-- > 21 files changed, 76 insertions(+), 62 deletions(-) > Again change the name from "VCpus" to "Vcpus" (or "VCPUs") Using cscope - after this patch the following still access ->vcpus virBhyveProcessBuildBhyveCmd bhyveDomainGetInfo virDomainDefSetVCpusMax virDomainDefHasVCpusOffline vzDomainGetInfo (twice) vzDomainGetVcpusFlags prlsdkCheckUnsupportedParams prlsdkDoApplyConfig Here too I'll try to remember to flag non accessor changes if I see them in future patches (although I suspect adjustment there could be ugly). Similar to comment earlier out maxvcpus, if there were some syntax-check rule that could be added (outside of domain_conf.c) - that would be great. ACK w/ the name adjustment and use of accessor in listed functions John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/7] qemu: Introduce qemuProcessInit
On Thu, Nov 19, 2015 at 01:09:15PM +0100, Jiri Denemark wrote: > qemuProcessStart is going to be split in three parts: qemuProcessInit, > qemuProcessLaunch, and qemuProcessFinish so that migration Prepare phase > can insert additional code in the process. qemuProcessStart will be a > small wrapper for all other callers. > > qemuProcessInit prepares the domain up to the point when priv->qemuCaps > is initialized. > > Signed-off-by: Jiri Denemark > --- > > Notes: > Version 2: > - avoid calling qemuProcessStop when starting fails with > "VM is already active" > > src/qemu/qemu_process.c | 133 > > src/qemu/qemu_process.h | 4 ++ > 2 files changed, 92 insertions(+), 45 deletions(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 2192ad8..784ae08 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -4476,6 +4476,86 @@ qemuProcessMakeDir(virQEMUDriverPtr driver, > } > > > +/** > + * qemuProcessInit: > + * > + * Prepares the domain up to the point when priv->qemuCaps is initialized. > + * > + * @returns 0 on success, > + * -1 on error requiring qemuProcessStop to be called, > + * -2 on error when qemuProcessStop must NOT be called. > + */ I don't like this as there is no benefit from letting the caller to decide whether qemuProcessStop needs to be called or not. Why don't just do it inside qemuProcessInit and add a comment that qemuProcessStop must not be called at all if this function fails? I know, that last patch updates qemuMigrationPrepareAny and that patch 5/7 joined 'stop' with 'endjob' but that's not necessary too. Just jump to 'stop' instead to 'stopjob'. Then it would be possible in case of qemuProcessInit fails jump to 'endjob' in qemuMigrationPrepareAny. Pavel [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] how to know if PCI device has SR-IOV PF capability
On Mon, Nov 23, 2015 at 10:34:43AM -0500, Laine Stump wrote: > On 11/23/2015 05:05 AM, Daniel P. Berrange wrote: > >On Sun, Nov 22, 2015 at 11:04:28AM +, Moshe Levi wrote: > >>Hi, > >> > >>I was looking on the output of virsh nodedev-dumpxml on a PCI to see if it > >>has SR-IOV PF capability. > >>It seem that if the virtual functions are enables the xml look like [1] but > >>if the PCI has no VFs enabled the > >>output is like in [2]. > >>As you can see for PCI which has no VFs the >>type='virt_functions'> tag doen't exist. > >>Is this by design? > >>I would except that tag with empty > >>elements will also be include in that case. > >That is an bug. The capability should be reported regardless of whether > >any are VFs currently enabled, so we should fix this. > > Prior to libvirt 1.0.4, "" was there for > every PCI device regardless of whether or not it had the ability to have > virtual functions. It looks like commit 9a3ff01d changed it to only emit > that element when there was at least one VF. So it used to be wrong, and now > it is wrong in a different way :-) Awesome. > If we're going to switch to emiting virt_functions whenever a device has the > potential to provide VFs, we may as well make it worthwhile and also emit > the maximum possible VFs for the device, maybe simply: > > > > (the current count is implicit in the number of entries in the list that > follows. I don't have an opinion on whether it is better to also include > explicitly with, e.g. "count='7'", or just leave it implicit). Is there any way for us to actually discover the max count ? If so, then it seems nice to include it. 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
Re: [libvirt] [PATCH 1/8] perf: add new public APIs for perf event
On Tue, Nov 17, 2015 at 16:00:41 +0800, Qiaowei Ren wrote:
> API agreed on in
> https://www.redhat.com/archives/libvir-list/2015-October/msg00872.html
>
> * include/libvirt/libvirt-domain.h (virDomainGetPerfEvents,
> virDomainSetPerfEvents): New declarations.
> * src/libvirt_public.syms: Export new symbols.
>
> Signed-off-by: Qiaowei Ren
> ---
> include/libvirt/libvirt-domain.h | 18 ++
> src/libvirt_public.syms | 6 ++
> 2 files changed, 24 insertions(+)
>
> diff --git a/include/libvirt/libvirt-domain.h
> b/include/libvirt/libvirt-domain.h
> index a1ea6a5..69965e6 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1802,6 +1802,24 @@ int virDomainListGetStats(virDomainPtr *doms,
> void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats);
>
> /*
> + * Perf Event API
> + */
> +
> +/**
> + * VIR_DOMAIN_PERF_CMT:
> + *
> + * Macro for typed parameter name that represents CMT perf event.
> + */
> +# define VIR_DOMAIN_PERF_CMT "cmt"
> +
> +int virDomainGetPerfEvents(virDomainPtr dom,
> + virTypedParameterPtr params,
> + int * nparams);
We don't put a space between * and the name.
> +int virDomainSetPerfEvents(virDomainPtr dom,
> + virTypedParameterPtr params,
> + int nparams);
> +
> +/*
> * BlockJob API
> */
>
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index dd94191..03206e7 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -725,4 +725,10 @@ LIBVIRT_1.2.19 {
> virDomainRename;
> } LIBVIRT_1.2.17;
>
> +LIBVIRT_1.2.23 {
> +global:
> +virDomainGetPerfEvents;
> +virDomainSetPerfEvents;
> +} LIBVIRT_1.2.19;
> +
> # define new API here using predicted next version number
This patch does not compile, you should squash patches 1, 2, and 3
together, since 'make all check syntax-check' should pass after each
patch in the series.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] how to know if PCI device has SR-IOV PF capability
On 11/23/2015 05:05 AM, Daniel P. Berrange wrote: On Sun, Nov 22, 2015 at 11:04:28AM +, Moshe Levi wrote: Hi, I was looking on the output of virsh nodedev-dumpxml on a PCI to see if it has SR-IOV PF capability. It seem that if the virtual functions are enables the xml look like [1] but if the PCI has no VFs enabled the output is like in [2]. As you can see for PCI which has no VFs the tag doen't exist. Is this by design? I would except that tag with empty elements will also be include in that case. That is an bug. The capability should be reported regardless of whether any are VFs currently enabled, so we should fix this. Prior to libvirt 1.0.4, "" was there for every PCI device regardless of whether or not it had the ability to have virtual functions. It looks like commit 9a3ff01d changed it to only emit that element when there was at least one VF. So it used to be wrong, and now it is wrong in a different way :-) If we're going to switch to emiting virt_functions whenever a device has the potential to provide VFs, we may as well make it worthwhile and also emit the maximum possible VFs for the device, maybe simply: (the current count is implicit in the number of entries in the list that follows. I don't have an opinion on whether it is better to also include explicitly with, e.g. "count='7'", or just leave it implicit). -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] libxl: add libxl_domain_config to libxlDomainObjPrivate
On 11/20/2015 05:40 PM, Joao Martins wrote: > > On 11/20/2015 07:05 PM, Jim Fehlig wrote: >> On 11/19/2015 04:45 PM, Joao Martins wrote: >> >> You're not going to be happy with me... >> >>> This new field in libxlDomainObjPrivate is named "config" >>> and is kept while the domain is active. >> While this sounded like a good idea when I mentioned it, I'm now worried that >> the config will quickly become stale and cause problems if used elsewhere >> (e.g. >> see my yet-to-be-written comment in 3/3). IIUC correctly, >> libxl_domain_config >> is only useful when creating the domain. Subsequently adding/removing >> devices, >> memory, vcpus, etc. would not be reflected in the libxl_domain_config >> object. I >> suppose it would useful (and still valid) in the start callback, but IMO >> including it in the libxlDomainPrivate struct fools us into believing it >> could >> be used elsewhere throughout the life of the domain. I now have second doubts >> about this. What do you think? > I agree with you, and since there a libxl_device_nic_list as you suggested, it > would actually be much cleaner and safer compared to libxl_domain_config > alternative (though with a small performance cost). And we would avoid end up > having config just lying there with no additional use (besides StartCallback) > and inconsistent info. > > The only thing that the libxlDomainObjPrivate approach is better than > libxl_device_nic_list() would be that we don't need to refetch the devid, > since > the nics array has it correctly filled already when console callback is > invoked. > Whereas libxl_device_nic_list will refetch the same info (in additiona to all > entries in the backend directory) from xenstore thus adding up overhead. But > given that this is only once and in domain create I think it's not a big deal. Right. I think the extra overhead is in the noise relative to the other activities involved in starting a domain. > Would you agree then to resend this series without this patch and using > libxl_device_nic_list, as the final approach? Thanks for pointing out this > issue! I think so. If you dislike the extra overhead of libxl_device_nic_list, another option would be something like a libxlDomainStartCallbackInfo struct that contains the virDomainObj and libxl_domain_config, and is passed to the start callback via for_callback of libxl_asyncop_how. That would allow us to use the libxl_domain_config object in the callback, but still dispose it after the start completes. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/34] conf: Move vcpu count check into helper
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> ---
> src/conf/domain_conf.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d8c1068..3062b3a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1461,6 +1461,13 @@ int
> virDomainDefSetVCpus(virDomainDefPtr def,
> unsigned int vcpus)
> {
> +if (vcpus > def->maxvcpus) {
Use accessor (virDomainDefGetVCpusMax)
FWIW: Just thinking about patch 12 where the code is reading off the
command line and setting counts, but not checking that vcpus <= maxvcpus
in qemuParseCommandLineSmp... Although I suspect that won't be a problem
since that code is reading qemu command line and I would hope we could
assume (haha) that qemu would have failed if vcpus > maxvcpus and of
course if cores/threads/sockets didn't add up properly as well.
ACK with the accessor change.
John
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("maxvcpus must not be less than current vcpus (%u <
> %u)"),
> + vcpus, def->maxvcpus);
> +return -1;
> +}
> +
> def->vcpus = vcpus;
>
> return 0;
> @@ -14723,13 +14730,6 @@ virDomainVcpuParse(virDomainDefPtr def,
> if (virDomainDefSetVCpus(def, vcpus) < 0)
> goto cleanup;
>
> -if (maxvcpus < def->vcpus) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("maxvcpus must not be less than current vcpus "
> - "(%u < %u)"), maxvcpus, def->vcpus);
> -goto cleanup;
> -}
> -
> tmp = virXPathString("string(./vcpu[1]/@placement)", ctxt);
> if (tmp) {
> if ((def->placement_mode =
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/34] conf: Replace writes to def->vcpus with accessor
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> ---
> src/conf/domain_conf.c | 25 +++--
> src/conf/domain_conf.h | 1 +
> src/hyperv/hyperv_driver.c | 5 -
> src/libvirt_private.syms | 1 +
> src/libxl/libxl_driver.c | 14 +-
> src/openvz/openvz_conf.c | 3 ++-
> src/openvz/openvz_driver.c | 4 +++-
> src/phyp/phyp_driver.c | 3 ++-
> src/qemu/qemu_command.c| 9 ++---
> src/qemu/qemu_driver.c | 8 +---
> src/test/test_driver.c | 8 +---
> src/vbox/vbox_common.c | 6 --
> src/vmx/vmx.c | 3 ++-
> src/vz/vz_sdk.c| 3 ++-
> src/xen/xm_internal.c | 3 ++-
> src/xenapi/xenapi_driver.c | 3 ++-
> src/xenconfig/xen_common.c | 5 -
> src/xenconfig/xen_sxpr.c | 10 +++---
> 18 files changed, 80 insertions(+), 34 deletions(-)
>
Still prefer to see "Vcpus" (or "VCPUs") rather than "VCpus"...
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b136314..4a67361 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -12543,6 +12543,7 @@ qemuParseCommandLineSmp(virDomainDefPtr dom,
> unsigned int cores = 0;
> unsigned int threads = 0;
> unsigned int maxcpus = 0;
> +unsigned int vcpus = 0;
> size_t i;
> int nkws;
> char **kws;
> @@ -12557,9 +12558,8 @@ qemuParseCommandLineSmp(virDomainDefPtr dom,
> for (i = 0; i < nkws; i++) {
> if (vals[i] == NULL) {
> if (i > 0 ||
> -virStrToLong_i(kws[i], &end, 10, &n) < 0 || *end != '\0')
> +virStrToLong_ui(kws[i], &end, 10, &vcpus) < 0 || *end !=
> '\0')
> goto syntax;
> -dom->vcpus = n;
> } else {
> if (virStrToLong_i(vals[i], &end, 10, &n) < 0 || *end != '\0')
> goto syntax;
A few lines down from here:
else if (STREQ(kws[i], "maxcpus"))
maxcpus = n;
Unrelated to this patch, but perhaps related to an earlier patch (at
least w/r/t to the Long_ui rather than Long_i change. Since all the
elements are unsigned int, then all could use _ui.
In any case, ACK w/ "VCpus" name adjustment
John
> @@ -12577,11 +12577,14 @@ qemuParseCommandLineSmp(virDomainDefPtr dom,
> }
>
> if (maxcpus == 0)
> -maxcpus = dom->vcpus;
> +maxcpus = vcpus;
>
> if (virDomainDefSetVCpusMax(dom, maxcpus) < 0)
> goto error;
>
> +if (virDomainDefSetVCpus(dom, vcpus) < 0)
> +goto error;
> +
> if (sockets && cores && threads) {
> virCPUDefPtr cpu;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/11] admin: Introduce virAdmConnectGetLibVersion
On Thu, Nov 19, 2015 at 10:52:35AM +0100, Erik Skultety wrote:
On 16/11/15 17:42, Martin Kletzander wrote:
const ADMIN_PROGRAM = 0x06900690;
const ADMIN_PROTOCOL_VERSION = 1;
@@ -71,5 +75,10 @@ enum admin_procedure {
/**
* @generate: none
*/
-ADMIN_PROC_CONNECT_CLOSE = 2
+ADMIN_PROC_CONNECT_CLOSE = 2,
+
+/**
+ * @generate: both
+ */
+ADMIN_PROC_CONNECT_GET_LIB_VERSION = 3
I was wondering why 'connect' is here, it does not necessarily relate
to connection and makes the name long, we could start using 'daemon'
instead as that is what we'll need to add anyway. Also 'lib' seems
unnecessary here.
Well, this is a matter of consistency with libvirt library. You also
mentioned the general API naming convention that should be met according
to the arguments the API takes in one of your earlier reviews [1]. The
'connect' is there because virAdmConnectGetLibVersion indeed takes
virAdmConnectPtr as its 1st argument and the remote version name of the
API imho should not differ that much.
We want to stay consistent with libvirt, but not with all the wrong
choices that were made along the way. I wish I named the initial
pointer the user gets with virAdmConnect() something like
virAdmDaemonPtr, everything you call with that would start with
virAdmDaemon and would make tad more sense. But I didn't know there
will be a "daemon" back then. Having said that, we *can* change that,
but I already confused myself as to what is the best naming convention
we should go with. Wider discussion about that would be fine and I
think we would achieve a conclusion in less than 5 minutes if there's
more than the two of us and it's not done through mail.
So for now keep it as it is, and we'll add the discussion about naming
to the TODO list of things needed to be done before enabling the admin
api in the daemon.
Erik
[1] https://www.redhat.com/archives/libvir-list/2015-September/msg00079.html
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/34] conf: Replace read access to def->maxvcpus with accessor
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Finalize the refactor by adding the 'virDomainDefGetVCpusMax' getter and
> reusing it accross libvirt.
> ---
> src/conf/domain_conf.c | 22 +++---
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> src/libxl/libxl_conf.c | 4 ++--
> src/libxl/libxl_driver.c | 9 ++---
> src/openvz/openvz_driver.c | 4 ++--
> src/qemu/qemu_command.c| 4 ++--
> src/qemu/qemu_driver.c | 10 +-
> src/qemu/qemu_process.c| 2 +-
> src/test/test_driver.c | 13 -
> src/vbox/vbox_common.c | 4 ++--
> src/vmx/vmx.c | 16 +---
> src/vz/vz_driver.c | 2 +-
> src/xen/xm_internal.c | 9 ++---
> src/xenapi/xenapi_utils.c | 6 ++
> src/xenconfig/xen_common.c | 4 ++--
> src/xenconfig/xen_sxpr.c | 8
> 17 files changed, 69 insertions(+), 50 deletions(-)
>
Even just after this patch, a cscope search on "->maxvcpus" returns:
libxlDomainGetPerCPUStats
virDomainDefHasVCpusOffline
Cannot recall for sure, but can there be some sort of syntax-check for
direct accesses (outside of domain_conf)? Just so there's no current
upstream patches that escape someone's review...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6b16430..4e5b7b6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1450,6 +1450,13 @@ virDomainDefHasVCpusOffline(const virDomainDef *def)
This API accesses maxvcpus directly still:
return def->vcpus < def->maxvcpus;
Although I do note by the end of the entire patch series a number of
these new API's access ->maxvcpus directly. Just seems 'safer' than any
access other the "Set" vcpusmax function uses the accessor. I'll try to
remember to point them out when I see them (let's see how well short
term memory is working today!).
> }
>
>
> +unsigned int
> +virDomainDefGetVCpusMax(const virDomainDef *def)
s/VCpus/Vcpus/g (or VCPUs - whatever had been chosen)
> +{
> +return def->maxvcpus;
> +}
> +
> +
[...]
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 0223e94..44f76f2 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -3066,6 +3066,7 @@ virVMXFormatConfig(virVMXContext *ctx,
> virDomainXMLOptionPtr xmlopt, virDomainDe
> bool scsi_present[4] = { false, false, false, false };
> int scsi_virtualDev[4] = { -1, -1, -1, -1 };
> bool floppy_present[2] = { false, false };
> +unsigned int maxvcpus;
>
> if (ctx->formatFileName == NULL) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -3181,15 +3182,16 @@ virVMXFormatConfig(virVMXContext *ctx,
> virDomainXMLOptionPtr xmlopt, virDomainDe
> "'current'"));
> goto cleanup;
> }
> -if ((def->maxvcpus % 2 != 0 && def->maxvcpus != 1)) {
> +maxvcpus = virDomainDefGetVCpusMax(def);
> +if (maxvcpus % 2 != 0 && maxvcpus != 1) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Expecting domain XML entry 'vcpu' to be 1 or a "
> - "multiple of 2 but found %d"),
> - def->maxvcpus);
> + _("Expecting domain XML entry 'vcpu' to be an
> unsigned "
> + "integer (1 or a multiple of 2) but found %d"),
> + maxvcpus);
Error message thrashing ... patch 10 changed this message one way and
then this patch changes it back. Doesn't really matter to me which way
it goes, but I actually liked the way patch 10 says it rather than going
back to this old format.
> goto cleanup;
> }
>
> -virBufferAsprintf(&buffer, "numvcpus = \"%d\"\n", def->maxvcpus);
> +virBufferAsprintf(&buffer, "numvcpus = \"%d\"\n", maxvcpus);
>
> /* def:cpumask -> vmx:sched.cpu.affinity */
> if (def->cpumask && virBitmapSize(def->cpumask) > 0) {
[...]
> index a80e084..d40f959 100644
> --- a/src/xenapi/xenapi_utils.c
> +++ b/src/xenapi/xenapi_utils.c
> @@ -504,10 +504,8 @@ createVMRecordFromXml(virConnectPtr conn,
> virDomainDefPtr def,
> else
> (*record)->memory_dynamic_max = (*record)->memory_static_max;
>
> -if (def->maxvcpus) {
> -(*record)->vcpus_max = (int64_t) def->maxvcpus;
> -(*record)->vcpus_at_startup = (int64_t) def->vcpus;
> -}
> +(*record)->vcpus_max = (int64_t) virDomainDefGetVCpusMax(def);
> +(*record)->vcpus_at_startup = (int64_t) def->vcpus;
Hmmm... is this yet another hypervisor that allowed maxvcpus == 0 to
mean get me the number of CPU's on the host? For which setting a 1 by
default will change expectations?
If patch 10 was where we forced maxvcpus to be at least 1, then perhaps
the "if (def->maxvcpus)" check removal needs to be there instead - just
so it's captured in the right place.
ACK - with at least function name adjustment and accessor for
libxlDomainGetPerCPUStats. Whether virDomainDefHasVCpusOffline changes
or not is l
Re: [libvirt] [PATCH] storage: Ignore block devices that fail format detection
On Fri, Oct 30, 2015 at 11:13:21AM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1276198
>
> Prior to commit id '98322052' failure to saferead the block device would
> cause an error to be logged and the device to be skipped while attempting
> to discover/create a stable target path for a new LUN (NPIV).
>
> This was because virStorageBackendSCSIFindLUs ignored errors from
> processLU and virStorageBackendSCSINewLun.
>
> Ignoring the failure allowed a multipath device with an "active" and
> "ghost" to be present on the host with the "ghost" block device being
> ignored. This patch will return a -2 to the caller indicating the desire
> to ignore the block device since it cannot be used directly rather than
> fail the pool startup.
>
> Additionally, it was found during some debugging that it was possible
> for the virStorageBackendDetectBlockVolFormatFD to not detect a format,
> which while not a probably - we probably should at least add some sort
> of warning message.
>
> Signed-off-by: John Ferlan
> ---
> src/storage/storage_backend.c | 4
> src/storage/storage_backend_scsi.c | 7 ++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index a375fe0..2de606f 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1393,6 +1393,10 @@
> virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target,
> }
> }
>
> +if (target->format == VIR_STORAGE_POOL_DISK_UNKNOWN)
> +VIR_WARN("cannot determine the target format for '%s'",
> + target->path);
> +
> return 0;
> }
>
This change is unrelated to the other one. Also, is this warning really
needed? I think leaving the format as 'unknown' is visible enough.
> diff --git a/src/storage/storage_backend_scsi.c
> b/src/storage/storage_backend_scsi.c
> index a593a2b..d60473d 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -224,9 +224,14 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
> goto cleanup;
> }
>
> +/* Failing to process the format is not fatal - we'll just skip
> + * this volume.
> + */
> if (virStorageBackendUpdateVolInfo(vol, true,
> - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0)
> + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) {
> +retval = -2;
> goto cleanup;
Adding the VIR_STORAGE_VOL_OPEN_NOERROR flag and propagating it down to
virStorageBackendDetectBlockVolFormatFD would be nicer.
That way it can return -2 on EIO if the flag is present without spamming
the logs. The other functions called by virStorageBackendUpdateVolInfo
can also return -1 on a fatal error that way (although there do not seem
to be any functions that could fail with NOERROR at the moment).
Jan
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] document virCommandRunRegex function
>From 01a3ed1e6bacba8cd9f398e5233960714b2c4f49 Mon Sep 17 00:00:00 2001 From: Christian Loehle Date: Mon, 23 Nov 2015 15:06:37 +0100 Subject: [PATCH] =?UTF-8?q?document=20virCommandRunRegex=20function=C3=84?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/util/vircommand.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index c7f1538..a88cc13 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2889,12 +2889,24 @@ virCommandSetDryRun(virBufferPtr buf, } #ifndef WIN32 -/* +/** + * virCommandRunRegex: + * @cmd: command to run + * @nregex: number of regexes to apply + * @regex: array of regexes to apply + * @nvars: array of numbers of variables each regex will produce + * @func: callback function that is called for every line of output, + * needs to return 0 on success + * @data: additional data that will be passed to the callback function + * @prefix: prefix that will be skipped at the beginning of each line + * @exitstatus: 0 on success, -1 on memory allocation error, virCommandRun + * error or callback function error + * * Run an external program. * * Read its output and apply a series of regexes to each line * When the entire set of regexes has matched consecutively - * then run a callback passing in all the matches + * then run a callback passing in all the matches of the current line. */ int virCommandRunRegex(virCommandPtr cmd, -- 2.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/34] conf: Assume at least 1 maximum and current vCPU for every conf
On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Set new domain configs to contain at least 1 vCPU add a check that
> maximum vCPU count isn't set to 0 and remove unnecesary checks.
>
> The openvz test suite change is necessary since the test case generates
> the config via virDomainDefNew but does not set the vCPU info. With the
> change to virDomainDefNew the expected output has changed.
> ---
> src/conf/domain_conf.c | 12
> src/lxc/lxc_native.c | 7 ---
> src/openvz/openvz_driver.c | 20
> src/qemu/qemu_command.c| 3 ---
> src/vmx/vmx.c | 6 +++---
> tests/openvzutilstest.c| 2 +-
> 6 files changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3a1dcc7..6b16430 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1428,6 +1428,12 @@ int
> virDomainDefSetVCpusMax(virDomainDefPtr def,
> unsigned int vcpus)
> {
> +if (vcpus == 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("domain config can't have 0 maximum vCPUs"));
"domain configuration requires at least 1 vCPU"
Shouldn't this be a post parse check rather than a parse check;
otherwise, couldn't a domain disappear? May also "solve" the openvz
usage pattern issue...
> +return -1;
> +}
> +
> if (vcpus < def->vcpus)
> def->vcpus = vcpus;
>
> @@ -2697,6 +2703,12 @@ virDomainDefNew(void)
> if (!(ret->numa = virDomainNumaNew()))
> goto error;
>
> +/* assume at least 1 cpu for every config */
> +if (virDomainDefSetVCpusMax(ret, 1) < 0)
> +goto error;
> +
> +ret->vcpus = 1;
> +
[1]
>From a quick read - this is what generates issues w/ openvz which seems
to allow a "0" to be interpreted as all CPU's on the host during parse.
> ret->mem.hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
> ret->mem.soft_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
> ret->mem.swap_hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index d4a72c1..a3fea0a 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -1017,13 +1017,6 @@ lxcParseConfigString(const char *config)
> vmdef->onPoweroff = VIR_DOMAIN_LIFECYCLE_DESTROY;
> vmdef->virtType = VIR_DOMAIN_VIRT_LXC;
>
> -/* Value not handled by the LXC driver, setting to
> - * minimum required to make XML parsing pass */
> -if (virDomainDefSetVCpusMax(vmdef, 1) < 0)
> -goto error;
> -
> -vmdef->vcpus = 1;
> -
> vmdef->nfss = 0;
> vmdef->os.type = VIR_DOMAIN_OSTYPE_EXE;
>
> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> index 1361432..53a2d57 100644
> --- a/src/openvz/openvz_driver.c
> +++ b/src/openvz/openvz_driver.c
> @@ -1035,12 +1035,10 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const
> char *xml, unsigned int fla
> _("current vcpu count must equal maximum"));
> goto cleanup;
> }
> -if (vm->def->maxvcpus > 0) {
> -if (openvzDomainSetVcpusInternal(vm, vm->def->maxvcpus) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Could not set number of vCPUs"));
> - goto cleanup;
> -}
> +if (openvzDomainSetVcpusInternal(vm, vm->def->maxvcpus) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not set number of vCPUs"));
> + goto cleanup;
> }
>
> if (vm->def->mem.cur_balloon > 0) {
> @@ -1133,12 +1131,10 @@ openvzDomainCreateXML(virConnectPtr conn, const char
> *xml,
> vm->def->id = vm->pid;
> virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED);
>
> -if (vm->def->maxvcpus > 0) {
> -if (openvzDomainSetVcpusInternal(vm, vm->def->maxvcpus) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Could not set number of vCPUs"));
> -goto cleanup;
> -}
> +if (openvzDomainSetVcpusInternal(vm, vm->def->maxvcpus) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not set number of vCPUs"));
> +goto cleanup;
> }
>
> dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ef44b8e..cc6785f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -12694,9 +12694,6 @@ qemuParseCommandLine(virCapsPtr qemuCaps,
> def->id = -1;
> def->mem.cur_balloon = 64 * 1024;
> virDomainDefSetMemoryTotal(def, def->mem.cur_balloon);
> -if (virDomainDefSetVCpusMax(def, 1) < 0)
> -goto error;
> -def->vcpus = 1;
> def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC;
>
> def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART;
> diff --git a/src/vmx/vm
Re: [libvirt] [PATCH 09/34] conf: Add helper to check whether domain has offline vCPUs
On 11/20/2015 10:21 AM, Peter Krempa wrote: > The new helper will simplify checking whether the domain config contains > inactive vCPUs. > --- > src/conf/domain_conf.c | 9 - > src/conf/domain_conf.h | 1 + > src/libvirt_private.syms | 1 + > src/openvz/openvz_driver.c | 2 +- > src/qemu/qemu_command.c| 4 ++-- > src/vbox/vbox_common.c | 2 +- > src/vmx/vmx.c | 2 +- > src/vz/vz_sdk.c| 2 +- > src/xenconfig/xen_common.c | 2 +- > src/xenconfig/xen_sxpr.c | 4 ++-- > 10 files changed, 19 insertions(+), 10 deletions(-) > Like Patch 7 - use "Vcpus" rather than "VCpus" John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/11] admin: Add support for URI aliases
On Thu, Nov 19, 2015 at 02:00:16PM +0100, Erik Skultety wrote: -if (!(conn->uri = virURIParse(uri ? uri : default_uri))) +if ((!(flags & VIR_CONNECT_NO_ALIASES) && + virURIResolveAlias(conf, uri ? uri : default_uri, &alias) < 0)) this should also be fixed (with what I mentioned in previous review). Fixed. +goto error; + +if (!(conn->uri = virURIParse(alias ? alias : (uri ? uri : default_uri Also, if you modify virURIResolveAlias() to simply copy the string over to @alias if the alias is not found, that would be pretty nice syntax-sugar and spare our souls from this ugly thing =) ACK with that fixed. Hmm, I think that in ideal world 1 API should do exactly 1 thing and 1 thing only, but we're not living in an ideal world, do we?! However, in this specific case I don't think it's worth changing alias resolving in a way that would copy the searched alias into URI string if the alias wasn't matched. I could do a wrapper encompassing the alias resolving and this proposed syntactic sugar, but it still wouldn't make it worth I guess, since the resolver isn't executed every time unless appropriate flags have been set. Moreover, since I fixed the ternary operator you suggested above, the other one also becomes more readable: virURIParse(alias ? alias : uri)what do you think? Well, one is better than two in this case, so at least one is fixed. You don't need to change that just for the purpose of creating another syntactic sugar, that was just a hint if you would like to do that. PS: thanks for the ACK though Erik signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Simplify qemuSetupChrSourceCgroup and its callers
On Fri, Nov 20, 2015 at 09:42:43AM -0500, John Ferlan wrote:
>
>
> On 11/20/2015 05:04 AM, Ján Tomko wrote:
> > The domain definition is not needed in any of these functions.
> > Only pass it to qemuSetupChardevCgroup, which is used as a callback
> > for virDomainChrDefForeach.
> >
> > Use the right type for passing virDomainObjPtr instead of
> > void* where possible.
> > ---
> > src/qemu/qemu_cgroup.c | 25 ++---
> > 1 file changed, 10 insertions(+), 15 deletions(-)
> >
...
> > @@ -585,10 +583,7 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver,
> > vm) < 0)
> > goto cleanup;
> >
> > -if (vm->def->tpm &&
> > -(qemuSetupTPMCgroup(vm->def,
> > -vm->def->tpm,
> > -vm) < 0))
> > +if (vm->def->tpm && qemuSetupTPMCgroup(vm, vm->def->tpm) < 0)
>
> Why not just pass "vm"... qemuSetupTPMCgroup could easily deref
> "->def->tpm)"
>
> ACK - your call if you want to keep/remove the 2nd parameter.
>
Thanks. I removed the second parameter as well and pushed the series.
Jan
> John
> > goto cleanup;
> >
> > for (i = 0; i < vm->def->nhostdevs; i++) {
> >
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 v2 1/3] gobject: Drop redundant glib compatibility code
ACK.
On Fri, Nov 20, 2015 at 09:06:28PM +, Zeeshan Ali (Khattak) wrote:
> We already require and use glib >= 2.36 so there is no reason to keep
> around code to ensure compatibility with glib older than that.
> ---
> libvirt-gobject/libvirt-gobject-compat.c | 87
>
> libvirt-gobject/libvirt-gobject-compat.h | 73 ---
> 2 files changed, 160 deletions(-)
>
> diff --git a/libvirt-gobject/libvirt-gobject-compat.c
> b/libvirt-gobject/libvirt-gobject-compat.c
> index 14b5eb3..e91b018 100644
> --- a/libvirt-gobject/libvirt-gobject-compat.c
> +++ b/libvirt-gobject/libvirt-gobject-compat.c
> @@ -17,93 +17,6 @@
>
> #include "libvirt-gobject-compat.h"
>
> -#if !GLIB_CHECK_VERSION(2,28,0)
> -/**
> - * g_simple_async_result_take_error: (skip)
> - * @simple: a #GSimpleAsyncResult
> - * @error: a #GError
> - *
> - * Sets the result from @error, and takes over the caller's ownership
> - * of @error, so the caller does not need to free it any more.
> - *
> - * Since: 2.28
> - **/
> -G_GNUC_INTERNAL void
> -g_simple_async_result_take_error (GSimpleAsyncResult *simple,
> - GError *error)
> -{
> -/* this code is different from upstream */
> -/* we can't avoid extra copy/free, since the simple struct is
> - opaque */
> -g_simple_async_result_set_from_error (simple, error);
> -g_error_free (error);
> -}
> -
> -/**
> - * g_simple_async_result_new_take_error: (skip)
> - * @source_object: (allow-none): a #GObject, or %NULL
> - * @callback: (scope async): a #GAsyncReadyCallback
> - * @user_data: (closure): user data passed to @callback
> - * @error: a #GError
> - *
> - * Creates a #GSimpleAsyncResult from an error condition, and takes over the
> - * caller's ownership of @error, so the caller does not need to free it
> anymore.
> - *
> - * Returns: a #GSimpleAsyncResult
> - *
> - * Since: 2.28
> - **/
> -GSimpleAsyncResult *
> -g_simple_async_result_new_take_error(GObject *source_object,
> - GAsyncReadyCallback callback,
> - gpointer user_data,
> - GError *error)
> -{
> -GSimpleAsyncResult *simple;
> -
> -g_return_val_if_fail(!source_object || G_IS_OBJECT(source_object), NULL);
> -
> -simple = g_simple_async_result_new(source_object,
> - callback,
> - user_data, NULL);
> -g_simple_async_result_take_error(simple, error);
> -
> -return simple;
> -}
> -
> -/**
> - * g_simple_async_report_take_gerror_in_idle: (skip)
> - * @object: (allow-none): a #GObject, or %NULL
> - * @callback: a #GAsyncReadyCallback.
> - * @user_data: user data passed to @callback.
> - * @error: the #GError to report
> - *
> - * Reports an error in an idle function. Similar to
> - * g_simple_async_report_gerror_in_idle(), but takes over the caller's
> - * ownership of @error, so the caller does not have to free it any more.
> - *
> - * Since: 2.28
> - **/
> -void
> -g_simple_async_report_take_gerror_in_idle(GObject *object,
> - GAsyncReadyCallback callback,
> - gpointer user_data,
> - GError *error)
> -{
> -GSimpleAsyncResult *simple;
> -
> -g_return_if_fail(!object || G_IS_OBJECT(object));
> -g_return_if_fail(error != NULL);
> -
> -simple = g_simple_async_result_new_take_error(object,
> - callback,
> - user_data,
> - error);
> -g_simple_async_result_complete_in_idle(simple);
> -g_object_unref(simple);
> -}
> -#endif
> -
> GMutex *gvir_mutex_new(void)
> {
> GMutex *mutex;
> diff --git a/libvirt-gobject/libvirt-gobject-compat.h
> b/libvirt-gobject/libvirt-gobject-compat.h
> index 2e87966..27fa305 100644
> --- a/libvirt-gobject/libvirt-gobject-compat.h
> +++ b/libvirt-gobject/libvirt-gobject-compat.h
> @@ -35,77 +35,4 @@ GMutex *gvir_mutex_new(void);
>
> #endif
>
> -#if !GLIB_CHECK_VERSION(2,26,0)
> -#define G_DEFINE_BOXED_TYPE(TypeName, type_name, copy_func, free_func)
> G_DEFINE_BOXED_TYPE_WITH_CODE (TypeName, type_name, copy_func, free_func, {})
> -#define G_DEFINE_BOXED_TYPE_WITH_CODE(TypeName, type_name, copy_func,
> free_func, _C_) _G_DEFINE_BOXED_TYPE_BEGIN (TypeName, type_name, copy_func,
> free_func) {_C_;} _G_DEFINE_TYPE_EXTENDED_END()
> -#if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 7)
> -#define _G_DEFINE_BOXED_TYPE_BEGIN(TypeName, type_name, copy_func,
> free_func) \
> -GType \
> -type_name##_get_type (void) \
> -{ \
> - static volatile gsize g_define_type_id__volatile = 0; \
> - if (g_once_init_enter (&g_define_type_id__volatile)) \
> -{ \
> - GType (* _g_register_boxed) \
> -(cons
Re: [libvirt] [libvirt-glib 2/3] gconfig: Drop redundant glib compatibility code
I don't know why this one is 2/3, I would squash it with the other
similar gobject patch. ACK.
Christophe
On Fri, Nov 20, 2015 at 09:05:23PM +, Zeeshan Ali (Khattak) wrote:
> We already require and use glib >= 2.36 so there is no reason to keep
> around code to ensure compatibility with glib older than that.
> ---
> libvirt-gconfig/libvirt-gconfig-compat.h | 20
> 1 file changed, 20 deletions(-)
>
> diff --git a/libvirt-gconfig/libvirt-gconfig-compat.h
> b/libvirt-gconfig/libvirt-gconfig-compat.h
> index fbf552c..c9ac645 100644
> --- a/libvirt-gconfig/libvirt-gconfig-compat.h
> +++ b/libvirt-gconfig/libvirt-gconfig-compat.h
> @@ -25,26 +25,6 @@
>
> #include
>
> -#if !GLIB_CHECK_VERSION(2,32,0)
> -
> -#if__GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 1)
> -#define G_DEPRECATED __attribute__((__deprecated__))
> -#elif defined(_MSC_VER) && (_MSC_VER >= 1300)
> -#define G_DEPRECATED __declspec(deprecated)
> -#else
> -#define G_DEPRECATED
> -#endif
> -
> -#if__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)
> -#define G_DEPRECATED_FOR(f) __attribute__((__deprecated__("Use '" #f "'
> instead")))
> -#elif defined(_MSC_FULL_VER) && (_MSC_FULL_VER > 140050320)
> -#define G_DEPRECATED_FOR(f) __declspec(deprecated("is deprecated. Use '" #f
> "' instead"))
> -#else
> -#define G_DEPRECATED_FOR(f) G_DEPRECATED
> -#endif
> -
> -#endif
> -
> #if GLIB_CHECK_VERSION(2, 35, 0)
> #define g_type_init()
> #endif
> --
> 2.5.0
>
> --
> 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] [v3] gobject: Add wrapper virDomainSetTime()
On Fri, Nov 20, 2015 at 03:27:22PM +, Zeeshan Ali (Khattak) wrote:
> ---
> libvirt-gobject/libvirt-gobject-domain.c | 134
> +++
> libvirt-gobject/libvirt-gobject-domain.h | 15 +++-
> libvirt-gobject/libvirt-gobject.sym | 9 +++
> 3 files changed, 157 insertions(+), 1 deletion(-)
>
> diff --git a/libvirt-gobject/libvirt-gobject-domain.c
> b/libvirt-gobject/libvirt-gobject-domain.c
> index 34eb7ca..cb8096b 100644
> --- a/libvirt-gobject/libvirt-gobject-domain.c
> +++ b/libvirt-gobject/libvirt-gobject-domain.c
> @@ -1886,3 +1886,137 @@ gboolean
> gvir_domain_get_has_current_snapshot(GVirDomain *dom,
>
> return TRUE;
> }
> +
> +/**
> + * gvir_domain_set_time:
> + * @dom: the domain
> + * @date_time: (allow-none)(transfer none): the time to set as #GDateTime.
> + * @flags: Unused, pass 0.
> + * @error: (allow-none): Place-holder for error or %NULL
> + *
> + * This function tries to set guest time to the given value. The passed
> + * time must in UTC.
> + *
> + * If @date_time is %NULL, the time is reset using the domain's RTC.
> + *
> + * Please note that some hypervisors may require guest agent to be configured
> + * and running in order for this function to work.
> + *
> + * Returns: %TRUE on success, %FALSE otherwise.
> + */
> +gboolean gvir_domain_set_time(GVirDomain *dom,
> + GDateTime *date_time,
> + guint flags G_GNUC_UNUSED,
It's not strictly true that it's unused now, so I'd remove this, it's ok
if you want to keep it.
> + GError **err)
> +{
> +int ret;
> +GTimeVal tv;
> +glong seconds;
> +glong nseconds;
> +guint settime_flags;
> +
> +g_return_val_if_fail(GVIR_IS_DOMAIN(dom), FALSE);
> +g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
> +g_return_val_if_fail(flags == 0, FALSE);
> +
> +if (date_time != NULL) {
> +if (!g_date_time_to_timeval(date_time, &tv)) {
> +g_set_error_literal(err, GVIR_DOMAIN_ERROR,
> +0,
> +"Failed to parse given time argument");
> +return FALSE;
> +}
> +
> +seconds = tv.tv_sec;
> +nseconds = tv.tv_usec * 1000;
> +settime_flags = 0;
> +} else {
> +seconds = 0;
> +nseconds = 0;
> +settime_flags = VIR_DOMAIN_TIME_SYNC;
> +}
> +
> +ret = virDomainSetTime(dom->priv->handle, seconds, nseconds,
> settime_flags);
> +if (ret < 0) {
> +gvir_set_error_literal(err, GVIR_DOMAIN_ERROR,
> + 0,
> + "Unable to set domain time");
> +return FALSE;
> +}
> +
> +return TRUE;
> +}
> +
> +static void
> +gvir_domain_set_time_helper(GTask *task,
> +gpointer object,
> +gpointer task_data,
> +GCancellable *cancellable G_GNUC_UNUSED)
> +{
> +GVirDomain *dom = GVIR_DOMAIN(object);
> +GDateTime *date_time = (GDateTime *) task_data;
> +GError *err = NULL;
> +
> +if (!gvir_domain_set_time(dom, date_time, 0, &err))
> +g_task_return_error(task, err);
> +else
> +g_task_return_boolean(task, TRUE);
> +}
> +
> +/**
> + * gvir_domain_set_time_async:
> + * @dom: the domain
> + * @date_time: (allow-none)(transfer none): the time to set as #GDateTime.
> + * @flags: bitwise-OR of #GVirDomainSetTimeFlags.
> + * @cancellable: (allow-none)(transfer none): cancellation object
> + * @callback: (scope async): completion callback
> + * @user_data: (closure): opaque data for callback
> + *
> + * Asynchronous variant of #gvir_domain_set_time.
> + */
> +void gvir_domain_set_time_async(GVirDomain *dom,
> +GDateTime *date_time,
> +guint flags G_GNUC_UNUSED,
> +GCancellable *cancellable,
> +GAsyncReadyCallback callback,
> +gpointer user_data)
> +{
> +GTask *task;
> +
> +g_return_if_fail(GVIR_IS_DOMAIN(dom));
> +g_return_if_fail((cancellable == NULL) || G_IS_CANCELLABLE(cancellable));
> +g_return_if_fail(flags == 0);
> +
> +task = g_task_new(G_OBJECT(dom),
> + cancellable,
> + callback,
> + user_data);
> +if (date_time != NULL)
> +g_task_set_task_data(task,
> + g_date_time_ref(date_time),
> + (GDestroyNotify)g_date_time_unref);
> +g_task_run_in_thread(task, gvir_domain_set_time_helper);
> +
> +g_object_unref(task);
> +}
> +
> +/**
> + * gvir_domain_set_time_finish:
> + * @dom: the domain
> + * @result: (transfer none): async method result
> + * @err: Place-holder for possible errors
> + *
> + * Finishes the operation started by #gvir_domain_set_time_async.
> + *
> + * Re
Re: [libvirt] [PATCH 07/34] conf: Replace writes to def->maxvcpus with accessor
On 11/20/2015 10:21 AM, Peter Krempa wrote: > To support further refactors replace all write access to def->maxvcpus > with a accessor function. > --- > src/conf/domain_conf.c | 18 -- > src/conf/domain_conf.h | 2 ++ > src/hyperv/hyperv_driver.c | 5 - > src/libvirt_private.syms | 1 + > src/libxl/libxl_driver.c | 8 ++-- > src/lxc/lxc_native.c | 4 +++- > src/openvz/openvz_conf.c | 4 +++- > src/openvz/openvz_driver.c | 5 - > src/phyp/phyp_driver.c | 4 +++- > src/qemu/qemu_command.c| 9 +++-- > src/qemu/qemu_driver.c | 4 +++- > src/test/test_driver.c | 4 +++- > src/vbox/vbox_common.c | 11 +-- > src/vmx/vmx.c | 5 - > src/vz/vz_sdk.c| 4 +++- > src/xen/xm_internal.c | 4 +++- > src/xenapi/xenapi_driver.c | 4 +++- > src/xenconfig/xen_common.c | 4 +++- > src/xenconfig/xen_sxpr.c | 3 ++- > 19 files changed, 82 insertions(+), 21 deletions(-) > To be consistent with other uses (e.g. drivers, remote, libvirt-api), I think it should be "Vcpus" rather than "VCpus". The other options are of course 'vCPUs' or "VCPUs", but they both look strange in/as API names. The consistency is more for searching for vCPU related functionality and since VCpu isn't used at all - introducing it just requires another way to look-up names John > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt external snapshot error
2015-11-23 15:13 GMT+03:00 Vasiliy Tolstov : > If file is not exists i get error: > error: internal error: unable to execute QEMU command 'transaction': > Could not open '/test.raw': Invalid argument Does libvirt/qemu supports external snapshots from raw source format? for example : virsh snapshot-create-as 40083 --no-metadata --disk-only --diskspec sda,driver=raw,file=/test.raw error: unsupported configuration: disk format 'raw' lacks backing file support -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt external snapshot error
2015-11-23 15:10 GMT+03:00 Peter Krempa : > The file should not exist prior to the snapshot. If you want to > pre-create it (with correct size and format), you need to specify > --reuse-external in that case. Virsh manual already documents that: > >If --reuse-external is specified, and the snapshot XML requests an >external snapshot with a destination of an existing file, then the >destination must exist and be pre-created with correct format and >metadata. The file is then reused; otherwise, a snapshot is refused >to avoid losing contents of the existing files. > If file is not exists i get error: error: internal error: unable to execute QEMU command 'transaction': Could not open '/test.raw': Invalid argument -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt external snapshot error
On Mon, Nov 23, 2015 at 15:04:55 +0300, Vasiliy Tolstov wrote: > 2015-11-23 14:43 GMT+03:00 Peter Krempa : > > If you insist on using --live you also have to specify --memspec to > > point to a location where the memory snapshot is saved. Btw, --live and > > --disk-only are basically mutually exclusive since --disk-only implies > > that the memory state should not be saved, whereas --live is used when > > the memory state is saved. > > > > Additionally a disk-only snapshot is always considered "live" since it > > usually doesn't require pausing of the VM. > > > So, i'm try to modify command to virsh snapshot-create-as 40219 > --no-metadata --disk-only --diskspec sda,file=/test.raw > but also have error: > error: unsupported configuration: external snapshot file for disk sda > already exists and is not a block device: /test.raw The file should not exist prior to the snapshot. If you want to pre-create it (with correct size and format), you need to specify --reuse-external in that case. Virsh manual already documents that: If --reuse-external is specified, and the snapshot XML requests an external snapshot with a destination of an existing file, then the destination must exist and be pre-created with correct format and metadata. The file is then reused; otherwise, a snapshot is refused to avoid losing contents of the existing files. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/34] conf: Use local copy of maxvcpus in virDomainVcpuParse
On 11/20/2015 10:21 AM, Peter Krempa wrote:
> Use the local variable rather than getting it all the time from the
> struct. This will simplify further refactors.
> ---
> src/conf/domain_conf.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0ac7dbf..3c8a926 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14664,13 +14664,13 @@ virDomainVcpuParse(virDomainDefPtr def,
> goto cleanup;
> }
>
> -def->vcpus = def->maxvcpus;
> +def->vcpus = maxvcpus;
There is no local maxvcpus (yet) and this breaks git bisect.
ACK 1-5 with this fixed
John
> }
>
> -if (def->maxvcpus < def->vcpus) {
> +if (maxvcpus < def->vcpus) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("maxvcpus must not be less than current vcpus "
> - "(%u < %u)"), def->maxvcpus, def->vcpus);
> + "(%u < %u)"), maxvcpus, def->vcpus);
> goto cleanup;
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt external snapshot error
2015-11-23 14:43 GMT+03:00 Peter Krempa : > If you insist on using --live you also have to specify --memspec to > point to a location where the memory snapshot is saved. Btw, --live and > --disk-only are basically mutually exclusive since --disk-only implies > that the memory state should not be saved, whereas --live is used when > the memory state is saved. > > Additionally a disk-only snapshot is always considered "live" since it > usually doesn't require pausing of the VM. So, i'm try to modify command to virsh snapshot-create-as 40219 --no-metadata --disk-only --diskspec sda,file=/test.raw but also have error: error: unsupported configuration: external snapshot file for disk sda already exists and is not a block device: /test.raw libvirt xml : 5000 -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt external snapshot error
On Mon, Nov 23, 2015 at 13:13:09 +0300, Vasiliy Tolstov wrote: > Hi, i'm using > virsh -c qemu+ssh://test.node/system snapshot-create-as 40083 > --no-metadata --disk-only --live –atomic --diskspec sda,file=/test > > outputs: > error: Operation not supported: live snapshot creation is supported > only with external checkpoints > > info: > Compiled against library: libvirt 1.2.16 > Using library: libvirt 1.2.16 > Using API: QEMU 1.2.16 > Running hypervisor: QEMU 2.4.1 > > > Does it supported on never libvirt? How can i solve the issue? I'm use > raw format for disk. If you insist on using --live you also have to specify --memspec to point to a location where the memory snapshot is saved. Btw, --live and --disk-only are basically mutually exclusive since --disk-only implies that the memory state should not be saved, whereas --live is used when the memory state is saved. Additionally a disk-only snapshot is always considered "live" since it usually doesn't require pausing of the VM. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] vz: implementation of boot device order
This patch implements functionality of boot device order
based on boot element from os section in XML.
Now we support boot from CDROM and HDD.
If we have several devices of the same type (for example hdd0 and hdd1),
than we mark the first one as bootable.
---
src/vz/vz_sdk.c | 111 +---
1 file changed, 81 insertions(+), 30 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 750133d..bebd3b8 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2198,6 +2198,78 @@ prlsdkAddDeviceToBootList(PRL_HANDLE sdkdom,
return -1;
}
+static int
+prlsdkSetBootDevices(PRL_HANDLE sdkdom,
+ virDomainDefPtr def)
+{
+size_t i;
+PRL_RESULT pret;
+PRL_HANDLE devList = PRL_INVALID_HANDLE;
+PRL_HANDLE dev = PRL_INVALID_HANDLE;
+PRL_DEVICE_TYPE currentDevType, bootDevType;
+PRL_UINT32 devIndex, devCount, j;
+PRL_UINT32 bootSequence = 0;
+bool rootMount = false;
+
+pret = PrlVmCfg_GetAllDevices(sdkdom, &devList);
+prlsdkCheckRetGoto(pret, error);
+
+pret = PrlHndlList_GetItemsCount(devList, &devCount);
+prlsdkCheckRetGoto(pret, error);
+
+for (i = 0; i < def->os.nBootDevs; i++) {
+switch (def->os.bootDevs[i]) {
+case VIR_DOMAIN_BOOT_CDROM:
+bootDevType = PDE_OPTICAL_DISK;
+break;
+case VIR_DOMAIN_BOOT_DISK:
+bootDevType = PDE_HARD_DISK;
+break;
+default:
+continue;
+}
+
+for (j = 0; j < devCount; j++) {
+pret = PrlHndlList_GetItem(devList, j, &dev);
+prlsdkCheckRetGoto(pret, error);
+
+pret = PrlVmDev_GetType(dev, ¤tDevType);
+prlsdkCheckRetGoto(pret, error);
+
+if (currentDevType == bootDevType) {
+pret = PrlVmDev_GetIndex(dev, &devIndex);
+prlsdkCheckRetGoto(pret, error);
+
+if (prlsdkAddDeviceToBootList(sdkdom, devIndex,
currentDevType, bootSequence) < 0)
+goto error;
+bootSequence++;
+
+if (IS_CT(def) && !rootMount) {
+/* If we add physical device as a boot disk to container
+ we have to specify mount point for it */
+pret = PrlVmDevHd_SetMountPoint(dev, "/");
+prlsdkCheckRetGoto(pret, error);
+rootMount = true;
+}
+}
+PrlHandle_Free(dev);
+dev = PRL_INVALID_HANDLE;
+}
+}
+
+PrlHandle_Free(devList);
+return 0;
+
+ error:
+if (dev != PRL_INVALID_HANDLE)
+PrlHandle_Free(dev);
+
+if (devList != PRL_INVALID_HANDLE)
+PrlHandle_Free(devList);
+
+return -1;
+}
+
static int prlsdkCheckGraphicsUnsupportedParams(virDomainDefPtr def)
{
virDomainGraphicsDefPtr gr;
@@ -3145,9 +3217,7 @@ static int prlsdkDelDisk(PRL_HANDLE sdkdom, int idx)
}
static int prlsdkAddDisk(PRL_HANDLE sdkdom,
- virDomainDiskDefPtr disk,
- bool bootDisk,
- bool isCt)
+ virDomainDiskDefPtr disk)
{
PRL_RESULT pret;
PRL_HANDLE sdkdisk = PRL_INVALID_HANDLE;
@@ -3156,7 +3226,6 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom,
PRL_MASS_STORAGE_INTERFACE_TYPE sdkbus;
int idx;
virDomainDeviceDriveAddressPtr drive;
-PRL_UINT32 devIndex;
PRL_DEVICE_TYPE devType;
char *dst = NULL;
@@ -3302,21 +3371,6 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom,
goto cleanup;
}
-if (bootDisk) {
-pret = PrlVmDev_GetIndex(sdkdisk, &devIndex);
-prlsdkCheckRetGoto(pret, cleanup);
-
-if (prlsdkAddDeviceToBootList(sdkdom, devIndex, devType, 0) < 0)
-goto cleanup;
-
-/* If we add physical device as a boot disk to container
- * we have to specify mount point for it */
-if (isCt) {
-pret = PrlVmDevHd_SetMountPoint(sdkdisk, "/");
-prlsdkCheckRetGoto(pret, cleanup);
-}
-}
-
return 0;
cleanup:
PrlHandle_Free(sdkdisk);
@@ -3335,7 +3389,7 @@ prlsdkAttachVolume(virDomainObjPtr dom,
virDomainDiskDefPtr disk)
if (PRL_FAILED(waitJob(job)))
goto cleanup;
-ret = prlsdkAddDisk(privdom->sdkdom, disk, false, IS_CT(dom->def));
+ret = prlsdkAddDisk(privdom->sdkdom, disk);
if (ret == 0) {
job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE);
if (PRL_FAILED(waitJob(job))) {
@@ -3472,7 +3526,7 @@ prlsdkDoApplyConfig(virConnectPtr conn,
PRL_RESULT pret;
size_t i;
char uuidstr[VIR_UUID_STRING_BUFLEN + 2];
-bool needBoot = true;
+bool rootfsFound = false;
char *mask = NULL;
if (prlsdkCheckUnsupportedParams(sdkdom, def) < 0)
@@ -3552,21 +3606,18 @@ prlsdkDoApplyConfig(virConnectPtr conn,
for (i = 0; i < def->nfss; i++) {
if (STREQ(def->fss[i]
Re: [libvirt] [PATCH RFC] libxl: use libxl_event_wait to process libxl events
Jim Fehlig writes ("[PATCH RFC] libxl: use libxl_event_wait to process libxl
events"):
> Prior to this patch, libxl events were delivered to libvirt via
> the libxlDomainEventHandler callback registered with libxl.
> Documenation in $xensrc/tools/libxl/libxl_event.h states that the
> callback "may occur on any thread in which the application calls
> libxl". This can result in deadlock since many of the libvirt
> callees of libxl hold a lock on the virDomainObj they are working
> on. When the callback is invoked, it attempts to find a virDomainObj
> corresponding to the domain ID provided by libxl. Searching the
> domain obj list results in locking each obj before checking if it is
> active, and its ID equals the requested ID. Deadlock is possible
> when attempting to lock an obj that is already locked further up
> the call stack. Indeed, Max Ustermann recently reported an instance
> of this deadlock
This sounds like a very plausible failure mode.
> https://www.redhat.com/archives/libvir-list/2015-November/msg00130.html
>
> This patch moves processing of libxl events to a thread, where
> libxl_event_wait() is used to collect events. This allows processing
> libxl events asynchronously in libvirt, avoiding the deadlock.
The reasoning is sound and the remedy is IMO correct. (However, you
mean "this allows processing libxl events _synchronously_", since what
you are doing is serialising them all into your main loop.)
So:
Acked-by: Ian Jackson
NB that I have not reviewed the patch in detail. I can do that if you
like, although of course my knowledge of the innards of libvirt is not
wonderful.
> The only reservations I have about this patch come from comments
> about libxl_event_wait in libxl_event.h
>
> Like libxl_event_check but blocks if no suitable events are
> available, until some are. Uses libxl_osevent_beforepoll/
> _afterpoll so may be inefficient if very many domains are being
> handled by a single program.
If this turns out to be a problem in practice, we will improve libxl's
data structures to not involve so many linear searches. (In fact I
think you are probably already calling synchronous libxl ao functions,
which have the same performance properties, although this is not
documented.)
Given what you say above I don't think there is a reasonable
alternative remedy. So you should go ahead with this patch.
Regards,
Ian.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] libvirt external snapshot error
Hi, i'm using virsh -c qemu+ssh://test.node/system snapshot-create-as 40083 --no-metadata --disk-only --live –atomic --diskspec sda,file=/test outputs: error: Operation not supported: live snapshot creation is supported only with external checkpoints info: Compiled against library: libvirt 1.2.16 Using library: libvirt 1.2.16 Using API: QEMU 1.2.16 Running hypervisor: QEMU 2.4.1 Does it supported on never libvirt? How can i solve the issue? I'm use raw format for disk. -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] how to know if PCI device has SR-IOV PF capability
On Sun, Nov 22, 2015 at 11:04:28AM +, Moshe Levi wrote: > Hi, > > I was looking on the output of virsh nodedev-dumpxml on a PCI to see if it > has SR-IOV PF capability. > It seem that if the virtual functions are enables the xml look like [1] but > if the PCI has no VFs enabled the > output is like in [2]. > As you can see for PCI which has no VFs the type='virt_functions'> tag doen't exist. > Is this by design? > I would except that tag with empty > elements will also be include in that case. That is an bug. The capability should be reported regardless of whether any are VFs currently enabled, so we should fix this. 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
Re: [libvirt] [libvirt-glib 3/3] gobject: Port to GTask API
On Fri, Nov 20, 2015 at 03:57:08PM -0500, Zeeshan Ali (Khattak) wrote:
> >> diff --git a/libvirt-gobject/libvirt-gobject-stream.c
> >> b/libvirt-gobject/libvirt-gobject-stream.c
> >> index 46dbd9a..f8a1a8c 100644
> >> --- a/libvirt-gobject/libvirt-gobject-stream.c
> >> +++ b/libvirt-gobject/libvirt-gobject-stream.c
> >> @@ -129,14 +129,13 @@ static gboolean gvir_stream_close(GIOStream
> >> *io_stream,
> >> return (i_ret && o_ret);
> >> }
> >>
> >> -
> >> -static void gvir_stream_close_async(GIOStream *stream,
> >> -int io_priority G_GNUC_UNUSED,
> >> -GCancellable *cancellable,
> >> -GAsyncReadyCallback callback,
> >> -gpointer user_data)
> >> +static void
> >> +gvir_stream_close_helper(GTask *task,
> >> + gpointer source_object,
> >> + gpointer task_data G_GNUC_UNUSED,
> >> + GCancellable *cancellable)
> >> {
> >> -GSimpleAsyncResult *res;
> >> +GIOStream *stream = G_IO_STREAM(source_object);
> >> GIOStreamClass *class;
> >> GError *error;
> >>
> >> @@ -146,27 +145,36 @@ static void gvir_stream_close_async(GIOStream
> >> *stream,
> >> error = NULL;
> >> if (class->close_fn &&
> >> !class->close_fn(stream, cancellable, &error)) {
> >> -g_simple_async_report_take_gerror_in_idle(G_OBJECT (stream),
> >> - callback, user_data,
> >> - error);
> >> +g_task_return_error(task, error);
> >> return;
> >> }
> >>
> >> -res = g_simple_async_result_new(G_OBJECT (stream),
> >> -callback,
> >> -user_data,
> >> -gvir_stream_close_async);
> >> -g_simple_async_result_complete_in_idle(res);
> >> -g_object_unref (res);
> >> +g_task_return_boolean(task, TRUE);
> >> +}
> >> +
> >> +static void gvir_stream_close_async(GIOStream *stream,
> >> +int io_priority G_GNUC_UNUSED,
> >> +GCancellable *cancellable,
> >> +GAsyncReadyCallback callback,
> >> +gpointer user_data)
> >> +{
> >> +GTask *task;
> >> +
> >> +task = g_task_new(G_OBJECT(stream),
> >> + cancellable,
> >> + callback,
> >> + user_data);
> >> +g_task_run_in_thread(task, gvir_stream_close_helper);
> >> +g_object_unref(task);
> >> }
> >
> > Was this one creating a GSimpleAsyncResult and returning immediately
> > without doing anything?
>
> I doubt so. AFAICT, it was scheduling the result to be returned in the
> idle using 'g_simple_async_result_complete_in_idle'.
Sorry, I was not explicit, but this is what I meant by "returning
immediately", as opposed to creating a thread to handle the call (as all
the other methods).
>
> > The helper does not seem to be doing anything more, in which case I'd
> > suggest not creating an intermediate helper thread, and to do something
> > similar to what was done before. If this is a bug and more work should
> > be done, I'd prefer if this was done in a separate patch.
>
> I just failed to find a straight GTask replacement API for
> 'g_simple_async_result_complete_in_idle'. The docs for
> 'g_task_return_pointer' say:
>
> ""Completes the task" means that for an ordinary asynchronous task it
> will either invoke the task's callback, or else queue that callback to
> be invoked in the proper GMainContext, or in the next iteration of the
> current GMainContext."
>
> So I'm not sure if simply calling g_task_return_*() wouldn't change
> the behaviour and call the callback immediately.
For me, the main point of _complete_in_idle() was to make sure that the
callback gets called in the main thread rather than in a random thread.
Since g_task_return_*() handles this properly, there is indeed no
_in_idle() variant.
In my opinion, it's no big deal whether the callback gets run from an
idle or directly as long as this happens in the 'right' thread. If you
want to keep the 'run in idle' behaviour, I guess you could run
g_task_return_*() in a g_idle_add() callback, and avoid the spawning of
a thread.
Christophe
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: handle more machines with a single builtin IDE controller
On Sat, Nov 21, 2015 at 02:36:44PM -0500, Laine Stump wrote: > On 11/21/2015 02:00 PM, Guido Günther wrote: > >like I440FX by moving the condition into qemuDomainMachineHasBuiltinIDE > >and adding more machines. > > Nice! I like this better than the original 3 patch series, since it allows > recognizing any new machinetypes with a single line change. > > ACK, and thanks again! Pushed now. Thanks! -- Guido -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
