[libvirt] [PATCH v3 2/9] remote: implement virDomainGetGuestInfo

[Michal Privoznik]

On 8/23/19 6:31 PM, Jonathon Jongsma wrote:
> Add daemon and client code to serialize/deserialize
> virDomainGetGuestInfo().
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>   src/remote/remote_daemon_dispatch.c | 41 ++++++++++++++++++++++
>   src/remote/remote_driver.c          | 53 +++++++++++++++++++++++++++++
>   src/remote/remote_protocol.x        | 21 +++++++++++-
>   src/remote_protocol-structs         | 12 +++++++
>   4 files changed, 126 insertions(+), 1 deletion(-)
> 
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index 1bd281dd6d..665d938a99 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -7650,3 +7650,44 @@ remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors,
>       }
>       return -1;
>   }
> +
> +static int
> +remoteDispatchDomainGetGuestInfo(virNetServerPtr server ATTRIBUTE_UNUSED,
> +                                 virNetServerClientPtr client,
> +                                 virNetMessagePtr msg ATTRIBUTE_UNUSED,
> +                                 virNetMessageErrorPtr rerr,
> +                                 remote_domain_get_guest_info_args *args,
> +                                 remote_domain_get_guest_info_ret *ret)
> +{
> +    int rv = -1;
> +    struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);

We prefer remoteGetHypervisorConn() ...


> +    virDomainPtr dom = NULL;
> +    virTypedParameterPtr params = NULL;
> +    int nparams = 0;
> +
> +    if (!priv->conn) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));

.. which also reports an error. So no need to invent a new one.

> +        goto cleanup;
> +    }
> +
> +    if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
> +        goto cleanup;
> +
> +    if (virDomainGetGuestInfo(dom, args->types, &params, &nparams, args->flags) < 0)
> +        goto cleanup;


1: here

> +
> +    if (virTypedParamsSerialize(params, nparams,
> +                                (virTypedParameterRemotePtr *) &ret->params.params_val,
> +                                &ret->params.params_len,
> +                                VIR_TYPED_PARAM_STRING_OKAY) < 0)
> +        goto cleanup;
> +
> +    rv = 0;
> +
> + cleanup:
> +    if (rv < 0)
> +        virNetMessageSaveError(rerr);
> +    virObjectUnref(dom);

You need to free @params here.

> +
> +    return rv;
> +}
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index daac506672..5ba144648a 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -8306,6 +8306,58 @@ remoteNetworkPortGetParameters(virNetworkPortPtr port,
>       return rv;
>   }
>   
> +static int
> +remoteDomainGetGuestInfo(virDomainPtr dom,
> +                         unsigned int types,
> +                         virTypedParameterPtr *params,
> +                         int *nparams,
> +                         unsigned int flags)
> +{
> +    int rv = -1;
> +    struct private_data *priv = dom->conn->privateData;
> +    remote_domain_get_guest_info_args args;
> +    remote_domain_get_guest_info_ret ret;
> +
> +    remoteDriverLock(priv);
> +
> +    make_nonnull_domain(&args.dom, dom);
> +
> +    args.types = types;
> +    args.flags = flags;
> +
> +    memset(&ret, 0, sizeof(ret));
> +
> +    if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_GUEST_INFO,
> +             (xdrproc_t)xdr_remote_domain_get_guest_info_args, (char *)&args,
> +             (xdrproc_t)xdr_remote_domain_get_guest_info_ret, (char *)&ret) == -1)
> +        goto done;
> +
> +    if (ret.params.params_len > REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Too many params in guestinfo: %d for limit %d"),
> +                       ret.params.params_len, REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX);
> +        goto cleanup;
> +    }

No need to check for this here. It's checked for in 
virTypedParamsDeserialize(). That's why you need to pass _MAX argument. 
In fact, the check should go to [1].

> +
> +    if (params) {

The @params can't be NULL here, can they? I mean, in the public API you 
check for:

   virCheckNonNullArgGoto(params, error);

But I agree that our RPC infrastructure is complicated and it's not easy 
to understand it at the first glance. Basically, it works like this: at 
client side on virConnectOpen() the conn->driver is initialized to 
remote driver (@hypervisor_driver from remote_driver.c). So calling any 
public API ends up calling the callback from remote_driver.c 
(remoteDomainGetGuestInfo() in this case). That is the reason why these 
callbacks in remote driver need to look exactly like the public APIs 
(and hence you need to create typedef for every public API).
In the end, these callbacks do mere arg serialization and reply 
deserialization.

On server, things are a bit different. We have a code which deserializes 
incoming RPC packet and then we have this huge array of pairs PROC_NR + 
dispatch callback (look at the end of 
src/remote/remote_daemon_dispatch_stubs.h you'll find the array there). 
Yeah, yeah, it's not list of pairs, rather than tuples, but for our case 
we can safely ignore the other members of the tuple.

So, after server has decoded the incoming RPC it learned what procedure 
the client wants to call and thus it calls the associated callback 
(remoteDispatchDomainGetGuestInfoHelper() in this case). And this 
callback will then call the public API again, but this time conn->driver 
points to actual driver (qemu driver from instance). It is only here 
where all the interesting work is done.

It's more or less documented here:

https://libvirt.org/api.html
https://libvirt.org/internals/rpc.html
https://libvirt.org/api_extension.html (looks like you've followed this one)

Please let me know if you want me to expand on something.

> +        if (virTypedParamsDeserialize((virTypedParameterRemotePtr) ret.params.params_val,
> +                                      ret.params.params_len,
> +                                      REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX,
> +                                      params,
> +                                      nparams) < 0)
> +            goto cleanup;
> +    }
> +
> +    rv = 0;
> +
> + cleanup:
> +    xdr_free((xdrproc_t)xdr_remote_domain_get_guest_info_ret,
> +             (char *) &ret);
> +
> + done:
> +    remoteDriverUnlock(priv);
> +    return rv;
> +}
>   
>   /* get_nonnull_domain and get_nonnull_network turn an on-wire
>    * (name, uuid) pair into virDomainPtr or virNetworkPtr object.
> @@ -8733,6 +8785,7 @@ static virHypervisorDriver hypervisor_driver = {
>       .domainCheckpointLookupByName = remoteDomainCheckpointLookupByName, /* 5.6.0 */
>       .domainCheckpointGetParent = remoteDomainCheckpointGetParent, /* 5.6.0 */
>       .domainCheckpointDelete = remoteDomainCheckpointDelete, /* 5.6.0 */
> +    .domainGetGuestInfo = remoteDomainGetGuestInfo, /* 5.6.0 */

Ooops, s/6/7/ :D

Michal