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

On 09/23/2014 03:26 PM, Pavel Hrdina wrote: >>> + >>> + VIR_DEBUG("Relaying domain tunable event %s %d, callback %d", >>> + dom->name, dom->id, callback->callbackID); >>> + >> >> Might also be nice to log "%p %n", params, nparams > > Yes, that would be probably nice, but since I've pushed this patch > already I can create a following patch with this small update? > Yes, a followup is fine. >> >> >>> +++ b/src/remote/remote_protocol.x >>> @@ -247,6 +247,9 @@ const REMOTE_NETWORK_DHCP_LEASES_MAX = 65536; >>> /* Upper limit on count of parameters returned via bulk stats API */ >>> const REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX = 4096; >>> >>> +/* Upper limit of message size for tunable event. */ >>> +const REMOTE_DOMAIN_EVENT_TUNABLE_MAX = 8388608; >> >> That feels excessive... >> >>> + >>> /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ >>> typedef opaque remote_uuid[VIR_UUID_BUFLEN]; >>> >>> @@ -2990,6 +2993,12 @@ struct remote_domain_event_block_job_2_msg { >>> int status; >>> }; >>> >>> +struct remote_domain_event_callback_tunable_msg { >>> + int callbackID; >>> + remote_nonnull_domain dom; >>> + remote_typed_param params<REMOTE_DOMAIN_EVENT_TUNABLE_MAX>; >> >> ...each param in the array will occupy multiple bytes. I think that >> something as low as 2048 for REMOTE_DOMAIN_EVENT_TUNABLE_MAX is still >> plenty (we don't have that many tunables yet); even if each tunable >> requires 64 bytes to transmit (mostly in the name of the parameter, but >> also in the type and value), that's still well under a megabyte limit of >> information passed on an instance of the event. >> > > Well, yes and no :). Let's say, that we will add in the future (and I'm > planning to do it) blkiotune where you can update at the same time all > of the tunables for all host's disks where all params for now will be > only VIR_TYPED_PARAM_STRING and that could consume a lot of memory. I > know that it will probably never be that much, but I wanted to be sure > that we will have enough space for all possible tunable events. Still, are you going to return 8 million separate strings? Or just 8 million bytes but still contained within 2000 strings? Seriously, I think 2048 is a perfectly LARGE limit - there are not THAT many tunables per domain. The params<LIMIT> is not the overall size of the command, but the number of parameters (each of which can be quite large if they are type string)