[libvirt] [PATCH 3/5] remote: implement remote protocol for virConnectListAllDomains()

[Eric Blake]

On 05/20/2012 09:56 AM, Peter Krempa wrote:
> This patch wires up the RPC protocol handlers for
> virConnectListAllDomains(). The RPC generator has no support for the way
> how virConnectListAllDomains() returns the results so the handler code
> had to be done manually.
> 
> The new api is handled by REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS, with
> number 271 and marked with high priority.
> ---
>  daemon/remote.c              |   50 +++++++++++++++++++++++++++++++
>  src/remote/remote_driver.c   |   66 ++++++++++++++++++++++++++++++++++++++++++
>  src/remote/remote_protocol.x |   14 ++++++++-
>  src/remote_protocol-structs  |   12 +++++++
>  4 files changed, 141 insertions(+), 1 deletions(-)
> 

> diff --git a/daemon/remote.c b/daemon/remote.c
> index 16a8a05..8ac0568 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c


> +
> +    if ((ndomains = virConnectListAllDomains(priv->conn,
> +                                             &doms,
> +                                             args->ndomains,

No need for args->ndomains.

> +                                             args->flags)) < 0)
> +        goto cleanup;
> +
> +    if (doms) {

My proposal is that we _guarantee_ a terminating NULL entry.  Which
means if we get this far, doms will always be non-NULL.

> +        if (VIR_ALLOC_N(ret->domains.domains_val, ndomains) < 0) {

However, this is correct.  We do not want to transmit the trailing NULL
over the wire.

That said, you proposed that passing NULL doms could get just a count.
As written, you have no way to flag that the client passed in NULL, so
the client is _always_ requesting a list, and the server is always
returning a list, for the client to then discard that list.  I'm
wondering if struct remote_connect_list_all_domains_args needs an extra
field that flags whether the client is interested in a return list or
just a count.

> +++ b/src/remote/remote_driver.c
> @@ -1265,6 +1265,71 @@ done:
>      return rv;
>  }
> 
> +static int
> +remoteConnectListAllDomains(virConnectPtr conn,
> +                            virDomainPtr **domains,
> +                            int ndomains,
> +                            unsigned int flags)
> +{
> +    int rv = -1;
> +    int i;
> +    virDomainPtr *doms = NULL;
> +    remote_connect_list_all_domains_args args;
> +    remote_connect_list_all_domains_ret ret;
> +
> +    struct private_data *priv = conn->privateData;
> +
> +    remoteDriverLock(priv);
> +
> +    if (domains)
> +        *domains = NULL;

I'm debating whether we should guaranteed that *domains is set to NULL
on error (in which case, this should probably be hoisted out of here and
put in libvirt.c in patch 1/5 instead), or if we should instead
guarantee that *domains is untouched on error and only modified on
success.  Either way, the RPC driver shouldn't need to touch *domains
except on success.

> +
> +    args.ndomains = ndomains;

Again, drop this.

> +    args.flags = flags;

And as my counterpart to the daemon side, if the args struct has a field
that flags whether the user passed in NULL for domains, it would save
effort over the wire to return a 0-size array and just a return count.


> +
> +    if (domains) {
> +        if (VIR_ALLOC_N(doms, ret.domains.domains_len) < 0)

This must be VIR_ALLOC_N(doms, ret.domains.domains_len + 1) in order to
match my proposed semantics of guaranteeing a terminating NULL.


> +++ b/src/remote/remote_protocol.x
> @@ -2463,6 +2463,16 @@ struct remote_domain_get_disk_errors_ret {
>      int nerrors;
>  };
> 
> +struct remote_connect_list_all_domains_args {
> +    int ndomains;

Drop ndomains.

> +    unsigned int flags;
> +};
> +
> +struct remote_connect_list_all_domains_ret {
> +    remote_nonnull_domain domains<>;

This is an unbounded array; we aren't using any of these anywhere else.
 I wonder if reusing REMOTE_DOMAIN_ID_LIST_MAX would be reasonable
instead.  Then again, we are returning more information per domain: a
name, uuid, and id, so maybe REMOTE_DOMAIN_NAME_LIST_MAX is the best we
can do (currently 1024, but then again there is the pending patch to
raise that limit).  The name element may make us run into RPC limits
even if we don't otherwise enforce a limit.

> +    unsigned int ret;

Either this ret value is redundant (you can get it from
domains.domains_len), or it makes sense (because you added a field to
the args, and return a 0-length domains but a non-zero ret when the
caller passed in a NULL domains).

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120521/657d26d0/attachment-0001.sig>