On 06/05/2012 07:19 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.
We might end up teaching the generator how to do this, as we add more
ListAll* commands (for example, I'm in the middle of writing the series
for virDomainListAllSnapshots, using this series as my template), but
updating the generator is a story for another day.
>
> The new api is handled by REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS, with
> number 271 and marked with high priority.
> ---
> Diff to v1:
> Add the NULL element at the end of the list.
> ---
> daemon/remote.c | 52 ++++++++++++++++++++++++++++++++++
> src/remote/remote_driver.c | 63 ++++++++++++++++++++++++++++++++++++++++++
> src/remote/remote_protocol.x | 14 ++++++++-
> src/remote_protocol-structs | 12 ++++++++
> 4 files changed, 140 insertions(+), 1 deletions(-)
> +++ b/daemon/remote.c
> @@ -996,6 +996,58 @@ no_memory:
> }
>
> static int
> +remoteDispatchConnectListAllDomains(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client,
> + virNetMessagePtr msg ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr,
> + remote_connect_list_all_domains_args *args,
> + remote_connect_list_all_domains_ret *ret)
> +{
> + if ((ndomains = virConnectListAllDomains(priv->conn,
> + args->need_results ? &doms : NULL,
> + args->flags)) < 0)
> + goto cleanup;
> +
> + if (doms) {
Use 'if (doms && ndomains)' to avoid a pointless 0-length array allocation.
> + if (VIR_ALLOC_N(ret->domains.domains_val, ndomains) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + ret->domains.domains_len = ndomains;
> +
> + for (i = 0; i < ndomains; i++)
> + make_nonnull_domain(ret->domains.domains_val+i, doms[i]);
Style nit - spaces around '+'.
Ouch - we have a latent bug. make_nonnull_domain and friends do
strdup() without checking for NULL. Not your fault, but we should
really clean that up to trigger the OOM handler instead.
> + } else {
> + ret->domains.domains_len = 0;
> + ret->domains.domains_val = NULL;
> + }
> +
> + rv = ndomains;
> +
> +cleanup:
> + if (rv < 0)
> + virNetMessageSaveError(rerr);
> + if (doms) {
> + for (i = 0; i < ndomains;i++)
Style nit - space after second ';'.
> +++ 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 {
> + bool need_results;
s/bool/int/ - I'm not sure that rpcgen will gracefully handle 'bool' on
all platforms (just because glibc supports it doesn't make it valid XDR
notation everywhere else).
> +++ b/src/remote_protocol-structs
> @@ -1921,6 +1921,17 @@ struct remote_domain_get_disk_errors_ret {
> } errors;
> int nerrors;
> };
> +struct remote_connect_list_all_domains_args {
> + bool need_results;
If you have dwarves-1.9 installed, it's broken, hence you were not
actually testing this file. Upgrade to dwarves-1.10.
If you have dwarves-1.10 installed, you will notice that this shows up
as 'bool_t' and not bool (don't know why the debug information encodes
things that way, but that's what we have to live with). All the more
reason to use int, not bool, in the .x file.
I'll go ahead and prepare a patch for the OOM bug in daemon/remote.c,
but meanwhile, I think I can ACK this patch with nits fixed, without
having to see a v3, since the changes are pretty small.
--
Eric Blake eblake@xxxxxxxxxx +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