[libvirt] [PATCH v4 2/4] event: introduce new event for tunable values

[John Ferlan]

On 09/23/2014 05:13 AM, Pavel Hrdina wrote:
> On 09/22/2014 05:50 PM, John Ferlan wrote:
>>
>>
>> On 09/18/2014 04:39 AM, Pavel Hrdina wrote:
>>> This new event will use typedParameters to expose what has been actually
>>> updated and the reason is that we can in the future extend any tunable
>>> values or add new tunable values. With typedParameters we don't have to
>>> worry about creating some other events, we will just use this universal
>>> event to inform user about updates.
>>>
>>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>>> ---
>>>   daemon/remote.c              | 45 +++++++++++++++++++++
>>>   include/libvirt/libvirt.h.in | 22 +++++++++++
>>>   src/conf/domain_event.c      | 93 ++++++++++++++++++++++++++++++++++++++++++++
>>>   src/conf/domain_event.h      |  9 +++++
>>>   src/libvirt_private.syms     |  2 +
>>>   src/remote/remote_driver.c   | 42 ++++++++++++++++++++
>>>   src/remote/remote_protocol.x | 14 ++++++-
>>>   src/remote_protocol-structs  |  9 +++++
>>>   tools/virsh-domain.c         | 33 ++++++++++++++++
>>>   9 files changed, 268 insertions(+), 1 deletion(-)
>>>
>>
>> Seems you covered previous code review comments just fine, although I
>> have a couple of questions
>>
>> #1. Is virsh-domain.c relative here or perhaps later?  Or should the
>> verb/action be changed from "cpu-tune" to something more generic?
>> "tunable"?
>>
> 
> That should be definitely changed to "tunable". Thanks for catching it.
> 
>> #2. Should the REMOTE_CPUMAPS_MAX be replaced by something more name
>> (and size) relative to these events?  And secondarily, since event max
>> params is relevant to the size of the params buffer allocated for each
>> event series shouldn't that be passed along and eventually used as the
>> limit for the remoteDeserializeTypedParameters?  Although I'm not quite
>> clear/sure how that works with the protocol definition file.
>>
> 
> And this one is also wrong and should definitely be 
> REMOTE_DOMAIN_EVENT_TUNABLE_MAX and that means create a new const. About 
> the size of the rpc message I should extend the size as in the future 
> there could be some tunable staff that will require more memory than the 
> cputune event.
> 
> Currently maximal size of any rpc message is 16MiB and I think that 8MiB 
> should by more than enough limit for the tunable event, what do you think?

The "right size" is always a bit tough to judge on these generic events.
I'm fine with the 8MiB value - just seems large though.  I see why
cpumaps max is large since it's a text mapping of the 'arbitrary' number
of cpus (on different archs)... Given the recent add of the all domain
stats, my mind went to how much more data than that could be necessary,
but in this case I guess substance also matters.

John

> 
> Pavel
> 
>> I think this is ACK-able - just want to make sure of the above - #1
>> should be 'simple' right?  #2 is just one of those size things - the
>> current usage goes on a byte max, but is that relative to how this has
>> changed/evolved.
>>
>> John
>>
> 
> [...]
>