On 08/26/2014 08:14 AM, Peter Krempa wrote:
> Implement the remote driver support for shuffling the domain stats
> around.
> ---
> daemon/remote.c | 91 ++++++++++++++++++++++++++++++++++++++++++++
> src/remote/remote_driver.c | 88 ++++++++++++++++++++++++++++++++++++++++++
> src/remote/remote_protocol.x | 26 ++++++++++++-
> src/remote_protocol-structs | 23 +++++++++++
> 4 files changed, 227 insertions(+), 1 deletion(-)
Rearranging things in my review (man, I wish git format-patch -Ofilename
could be offloaded into git config by more than just an alias - it's
nice to have a filename that lists .x and .h to show before .c).
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index 5c316fb..9729392 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -241,6 +241,9 @@ const REMOTE_DOMAIN_FSFREEZE_MOUNTPOINTS_MAX = 256;
> /* Upper limit on the maximum number of leases in one lease file */
> const REMOTE_NETWORK_DHCP_LEASES_MAX = 65536;
>
> +/* Upper limit on count of parameters returned via bulk stats API */
> +const REMOTE_DOMAIN_STATS_PARAMS_MAX = 2048;
so up to 2048 stats per domain...
> +
> /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */
> typedef opaque remote_uuid[VIR_UUID_BUFLEN];
>
> @@ -3057,6 +3060,21 @@ struct remote_network_get_dhcp_leases_ret {
> unsigned int ret;
> };
>
> +struct remote_domain_stats_record {
> + remote_nonnull_domain dom;
> + remote_typed_param params<REMOTE_DOMAIN_STATS_PARAMS_MAX>;
> +};
> +
> +struct remote_domain_list_get_stats_args {
> + remote_nonnull_domain doms<REMOTE_DOMAIN_LIST_MAX>;
...for up to 16k domains, for as big as a worst-case 32M list of
parameters (and each parameter is non-trivial in size). Wow, that can
add up to a lot of RPC :)
I think your limits are okay, but do worry a little bit that this RPC
may bump against limits elsewhere (max RPC size, for starters) if we
ever have that many stats per domain.
More interesting is how many stats can we return on one domain - a
domain with 32 cpus and 32 <disk> elements can generate LOTS of stats.
Will we find the 2048 limit too tight?
> + unsigned int stats;
> + unsigned int flags;
> +};
Going back to a point I made on 1/5: Your RPC call does NOT special case
a user passing a NULL retStats variable, which means the remote side
will ALWAYS be populating an array and sending it back, even if the user
didn't care about the stats themselves but just the count of structs
that would be returned. I'm okay if you require the user to always pass
retStats, but if so, enforce that in libvirt.c; if not, then look at
remote_connect_list_all_domains_args for what you have to do to signal
to the remote side that you are just collecting a count instead of
stats, to avoid sending lots of return data just to throw it away.
> +
> +struct remote_domain_list_get_stats_ret {
> + remote_domain_stats_record retStats<REMOTE_DOMAIN_LIST_MAX>;
> + int ret;
> +};
Looks sufficient to cover both public APIs.
> /*----- Protocol. -----*/
>
> /* Define the program number, protocol version and procedure numbers here. */
> @@ -5420,5 +5438,11 @@ enum remote_procedure {
> * @generate: both
> * @acl: connect:write
> */
> - REMOTE_PROC_CONNECT_GET_DOMAIN_CAPABILITIES = 342
> + REMOTE_PROC_CONNECT_GET_DOMAIN_CAPABILITIES = 342,
> +
> + /**
> + * @generate: none
> + * @acl: domain:read
> + */
> + REMOTE_PROC_DOMAIN_LIST_GET_STATS = 343
Merge conflicts; trivial to resolve :)
Did you even try generating the functions, or just assumed it was
complex enough to need special handling? At any rate, I'm fine with a
manual implementation (I was actually fairly shocked that my
virDomainBlockRebase worked with the generated code).
So on to that:
>
> diff --git a/daemon/remote.c b/daemon/remote.c
> index ea16789..d488c58 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -6337,6 +6337,97 @@
remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server ATTRIBUTE_UNUSED,
> }
>
>
> +static int
> +remoteDispatchDomainListGetStats(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client,
> + virNetMessagePtr msg ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr,
> + remote_domain_list_get_stats_args *args,
> + remote_domain_list_get_stats_ret *ret)
> +{
> +
> + if (retStats && nrecords) {
Again, if you are going to special-case the user passing a NULL
retStats, then this makes sense; but if you are going to require the
user to pass a non-NULL retStats, then it is sufficient to check
nrecords being non-negative here, without also having to check for
non-NULL retStats.
> + } else {
> + ret->retStats.retStats_len = 0;
> + ret->retStats.retStats_val = NULL;
> + }
> +
> + ret->ret = nrecords;
Hmm, ret->ret is redundant if you are going to enforce the user passing
non-NULL retStats - just use ret->retStats.retStats_len. The redundancy
is necessary only if you are going to support NULL for gathering a count
independently from populating the array.
> +++ b/src/remote/remote_driver.c
> @@ -7670,6 +7670,93 @@ remoteNetworkGetDHCPLeases(virNetworkPtr net,
> }
>
>
> +static int
> +remoteDomainListGetStats(virConnectPtr conn,
> + virDomainPtr *doms,
> + unsigned int ndoms,
> + unsigned int stats,
> + virDomainStatsRecordPtr **retStats,
> + unsigned int flags)
> +{
> +
> + if (ret.retStats.retStats_len) {
> + if (VIR_ALLOC_N(tmpret, ret.retStats.retStats_len + 1) < 0)
Oops. If the return value is 0, you STILL want to alloc a 1-element
array with NULL as its only entry, rather than bypassing allocation
altogether (unless you are supporting NULL retStats)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list