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

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(a)redhat.com&gt; >> --- >> 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?

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 > [...]