Re: [libvirt] [PATCH 2/7] Add virDomainBlockPull support to the remote driver. The generator can handle DomainBlockPullAll and DomainBlockPullAbort. DomainBlockPull and DomainBlockPullInfo must be written by hand.
2011/6/1 Adam Litke <agl(a)us.ibm.com>:
On 06/01/2011 12:27 PM, Matthias Bolte wrote:
> 2011/6/1 Adam Litke <agl(a)us.ibm.com>:
>> * src/remote/remote_protocol.x: provide defines for the new entry points
>> * src/remote/remote_driver.c daemon/remote.c: implement the client and
>> server side
>> * daemon/remote_generator.pl: Specify the manually-written functions
>>
>> Signed-off-by: Adam Litke <agl(a)us.ibm.com>
>> ---
>
> +static int remoteDomainBlockPull(virDomainPtr domain,
> + const char *path,
> + virDomainBlockPullInfoPtr info,
> + unsigned int flags)
> +{
> + int rv = -1;
> + remote_domain_block_pull_args args;
> + remote_domain_block_pull_ret ret;
> + struct private_data *priv = domain->conn->privateData;
> +
> + remoteDriverLock(priv);
> +
> + make_nonnull_domain(&args.dom, domain);
> + args.path = (char *)path;
> + args.flags = flags;
> +
> + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_BLOCK_PULL,
> + (xdrproc_t)xdr_remote_domain_block_pull_args, (char *)&args,
> + (xdrproc_t)xdr_remote_domain_block_pull_ret, (char *)&ret) == -1)
> + goto done;
> +
> + if (info) {
> + info->cur = ret.info.cur;
> + info->end = ret.info.end;
> + }
> + rv = 0;
> +
> +done:
> + remoteDriverUnlock(priv);
> + return rv;
> +}
>
> From the generator point-of-view I would want to avoid having the info
> parameter being NULLable, as this differs from the common pattern and
> results in special case code in the generator.
From an API user's point of view, I think it's nice to allow NULL for
the info struct. The user may not care about the current progress
information. I could fix this at the global libvirt level by allocating
a dummy struct to pass down to the driver implementations if the user
passed NULL. Would that be acceptable?
No, don't make runtime compromises to simplify the generator, we can
special case this later.
>> +struct remote_domain_block_pull_info {
>> + unsigned hyper cur;
>> + unsigned hyper end;
>> +};
>
>> +struct remote_domain_block_pull_ret {
>> + remote_domain_block_pull_info info;
>> +};
>
>> +struct remote_domain_get_block_pull_info_ret {
>> + remote_domain_block_pull_info info;
>> +};
>
> From the generator point-of-view I would avoid this approach of
> putting a struct in a struct because that differs from the common
> approach and results in special case code in the generator. It should
> look like this
>
> struct remote_domain_block_pull_ret {
> unsigned hyper cur;
> unsigned hyper end;
> };
>
> struct remote_domain_get_block_pull_info_ret {
> unsigned hyper cur;
> unsigned hyper end;
> };
Ok, I will make this change.
The patch from my other response require this change and makes the
generator deal with the virDomainGetBlockPullInfo. virDomainBlockPull
requires some additional special case for the optional struct so you
should just keep that one manually written and we'll deal with that
later.
Matthias