On 09/04/2014 01:49 AM, John Ferlan wrote:
>
>
> On 08/28/2014 02:38 PM, Pavel Hrdina wrote:
>> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
>> ---
>> daemon/remote.c | 87 +++++++++++++++++++++++++++++++
>> include/libvirt/libvirt.h.in | 62 ++++++++++++++++++++++
>> src/conf/domain_event.c | 120 +++++++++++++++++++++++++++++++++++++++++++
>> src/conf/domain_event.h | 7 +++
>> src/libvirt_private.syms | 2 +
>> src/remote/remote_driver.c | 110 +++++++++++++++++++++++++++++++++++++++
>> src/remote/remote_protocol.x | 39 +++++++++++++-
>> src/remote_protocol-structs | 32 ++++++++++++
>> tools/virsh-domain.c | 49 ++++++++++++++++++
>> 9 files changed, 507 insertions(+), 1 deletion(-)
>>
>
> Should it be noted in any documentation (docs/*.html.in) about the new
> event being triggered? My quick scan didn't find anything, but figured
> I'd ask to be sure.
>
That's a good point and maybe it would be worth it to create a
documentation about events at all.
>> diff --git a/daemon/remote.c b/daemon/remote.c
>> index 89714ca..ae42c4d 100644
>> --- a/daemon/remote.c
>> +++ b/daemon/remote.c
>> @@ -969,6 +969,92 @@ remoteRelayDomainEventBlockJob2(virConnectPtr conn,
>> }
>>
>>
>> +static int
>> +remoteRelayDomainEventCputune(virConnectPtr conn,
>> + virDomainPtr dom,
>> + virDomainCputuneInfoPtr cputune,
>> + void *opaque)
>> +{
>> + daemonClientEventCallbackPtr callback = opaque;
>> + remote_domain_event_cputune_msg data;
>> + size_t i;
>> +
>> + if (callback->callbackID < 0 ||
>> + !remoteRelayDomainEventCheckACL(callback->client, conn, dom))
>> + return -1;
>> +
>> + VIR_DEBUG("Relaying domain cputune event %s %d, callback %d",
>> + dom->name, dom->id, callback->callbackID);
>> +
>> + /* build return data */
>> + memset(&data, 0, sizeof(data));
>> + make_nonnull_domain(&data.dom, dom);
>> +
>> + data.shares = cputune->shares;
>> + data.sharesSpecified = cputune->sharesSpecified;
>> + data.period = cputune->period;
>> + data.quota = cputune->quota;
>> + data.emulatorPeriod = cputune->emulatorPeriod;
>> + data.emulatorQuota = cputune->emulatorQuota;
>> + data.nvcpupin = cputune->nvcpupin;
>> +
>> + if (cputune->emulatorpin.map) {
>> + if (VIR_ALLOC_N(data.emulatorpin.map.map_val,
>> + cputune->emulatorpin.mapLen) < 0)
>> + goto error;
>> + memcpy(data.emulatorpin.map.map_val, cputune->emulatorpin.map,
>> + cputune->emulatorpin.mapLen);
>> + data.emulatorpin.map.map_len = cputune->emulatorpin.mapLen;
>> + data.emulatorpin.mapLen = cputune->emulatorpin.mapLen;
>> + }
>> + if (cputune->vcpupin) {
>> + if (VIR_ALLOC_N(data.vcpupin.vcpupin_val, data.nvcpupin) < 0)
>> + goto error;
>> + data.vcpupin.vcpupin_len = data.nvcpupin;
>> +
>> + for (i = 0; i < data.nvcpupin; i++) {
>> + data.vcpupin.vcpupin_val[i].vcpuid = cputune->vcpupin[i].vcpuid;
>> + if (VIR_ALLOC_N(data.vcpupin.vcpupin_val[i].cpumask.map.map_val,
>> + cputune->vcpupin[i].cpumask.mapLen) < 0)
>> + goto error;
>> + memcpy(data.vcpupin.vcpupin_val[i].cpumask.map.map_val,
>> + cputune->vcpupin[i].cpumask.map,
>> + cputune->vcpupin[i].cpumask.mapLen);
>
> Was this more or less what the unused VIR_COPY_CPUMAP was supposed to
> be? Nothing you created, but something I found during my recent look
> into the code.
Thanks for pointing this out, I've missed that macro. It could be used here.
>
>> + data.vcpupin.vcpupin_val[i].cpumask.map.map_len =
>> + cputune->vcpupin[i].cpumask.mapLen;
>> + data.vcpupin.vcpupin_val[i].cpumask.mapLen =
>> + cputune->vcpupin[i].cpumask.mapLen;
>> + }
>> + }
>> +
>> + if (callback->legacy) {
>> + remoteDispatchObjectEventSend(callback->client, remoteProgram,
>> + REMOTE_PROC_DOMAIN_EVENT_CPUTUNE,
>> + (xdrproc_t)xdr_remote_domain_event_cputune_msg,
>> + &data);
>> + } else {
>> + remote_domain_event_callback_cputune_msg msg = { callback->callbackID,
>> + data };
>> +
>> + remoteDispatchObjectEventSend(callback->client, remoteProgram,
>> + REMOTE_PROC_DOMAIN_EVENT_CALLBACK_CPUTUNE,
>> + (xdrproc_t)xdr_remote_domain_event_callback_cputune_msg,
>> + &msg);
>> + }
>> +
>> + return 0;
>> +
>> + error:
>> + VIR_FREE(data.emulatorpin.map.map_val);
>> + if (data.vcpupin.vcpupin_val) {
>> + for (i = 0; i < data.nvcpupin; i++)
>> + VIR_FREE(data.vcpupin.vcpupin_val[i].cpumask.map.map_val);
>> + VIR_FREE(data.vcpupin.vcpupin_val);
>> + }
>> + return -1;
>> +}
>> +
>> +
>> static virConnectDomainEventGenericCallback domainEventCallbacks[] = {
>> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle),
>> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventReboot),
>> @@ -987,6 +1073,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = {
>> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventPMSuspendDisk),
>> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceRemoved),
>> VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockJob2),
>> + VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventCputune),
>> };
>>
>> verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST);
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index 9358314..636b89b 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -5127,7 +5127,68 @@ typedef void (*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn,
>> virDomainPtr dom,
>> const char *devAlias,
>> void *opaque);
>> +/**
>> + * _virDomainCpumaksInfo
>
> s/maks/mask/
>
>> + *
>> + * This structure stores a mask with pinning information for emulator
>> + * or vcpus.
>
> And of course I'm muddying things up by adding a iothreadpin
>
>> + */
>> +struct _virDomainCpumaskInfo {
>> + int mapLen;
>> + unsigned char *map;
>> +};
>> +typedef struct _virDomainCpumaskInfo virDomainCpumaskInfo;
>> +typedef virDomainCpumaskInfo *virDomainCpumaskInfoPtr;
>> +
>> +/**
>> + * _virDomainVcpupinInfo
>> + *
>> + * This structure stores cpumask with pinning information
>> + * for each vcpu where the pinning has been set.
>> + */
>> +struct _virDomainVcpupinInfo {
>> + int vcpuid;
>
> Looking at this makes me think I should change the 'int vcpuid' to just
> 'int id' for my series (in _virDomainVcpuPinDef) - makes reusing this so
> much easier when it comes to the difference between vcpuid and
> iothreadid. Course that probably also means a structure name change from
> Vcpu to something more generic like Array.
>
>> + virDomainCpumaskInfo cpumask;
>> +};
>> +typedef struct _virDomainVcpupinInfo virDomainVcpupinInfo;
>> +typedef virDomainVcpupinInfo *virDomainVcpupinInfoPtr;
>> +
>> +/**
>> + * _virDomainCputuneInfo
>> + *
>> + * Structure containing all infromation about cputune for
>> + * specific domain.
>
> s/infromation/information
> s/for specific domain/for a specific domain/
>
> Your call - add something about this is for events or event mgmt API's.
> Basically this is the user view right?
Yes, this is only for user view.
>
>> + */
>> +struct _virDomainCputuneInfo {
>> + unsigned long long shares;
>> + int sharesSpecified;
>> + unsigned long long period;
>> + long long quota;
>> + unsigned long long emulatorPeriod;
>> + long long emulatorQuota;
>> + size_t nvcpupin;
>> + virDomainVcpupinInfoPtr vcpupin;
>> + virDomainCpumaskInfo emulatorpin;
>> +};
>> +typedef struct _virDomainCputuneInfo virDomainCputuneInfo;
>> +typedef virDomainCputuneInfo *virDomainCputuneInfoPtr;
>>
>> +/**
>> + * virConnectDomainEventCputuneCallback:
>> + * @conn: connection object
>> + * @dom: domain on which the event occurred
>> + * @cputune: cputune informations
>
> s/informations/information/
>
>> + * @opaque: application specified data
>> + *
>> + * This callback occurs when cpu tune is updated.
>
> s/cpu tune/guest cputune data/
>
> (or something similar)
>
> The remainder of this seems OK. Although I'm curious to know if there's
> been any thought as to whether we bump up against any limits if there
> are a lot of remote_domain_vcpupin's (e.g. vcpupin_val) within the
> remote_domain_event_cputune_msg? Perhaps 1 or 2 for testing work, but
> what if someone has whatever the maximum is?
The max size of rpc message is 16MB so there is a plenty space for the
vcpupin staff.
>
> My other question or concern would be if/when the iothreadpin series is
> accepted - obviously another patch would be required to add iothreadpin
> data in. Would this need to be done within the same release version? And
> if it wasn't would there have to be a v2 type event? IOW, can event data
> be extended? If not, then I just have to be sure to set aside the time
> within the release scope.
>
This is actually an issue of the current design for cputune event as it
wouldn't be possible to extend it after release and as you said there
will have to be probably second version of the cputune event. I think
that I'll have to rewrite the patches and use typed parameters for this
event as they are easily expandable.
Pavel
> Hopefully someone with a bit more knowledge of events can double check
> that you've touched everything you're supposed to touch
>
> John
[..]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list