On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
> - Defined the wire protocol format for virNodeGetCPUMapFlags and its
> arguments
> - Implemented remote method invocation (remoteNodeGetCPUMapFlags)
> - Implemented method dispacher (remoteDispatchNodeGetCPUMapFlags)
s/dispacher/dispatcher/
and of course, s/Flags//g :)
>
> Signed-off-by: Viktor Mihajlovski <mihajlov@xxxxxxxxxxxxxxxxxx>
> ---
> daemon/remote.c | 44 +++++++++++++++++++++++++++++++++++++
> src/remote/remote_driver.c | 49 ++++++++++++++++++++++++++++++++++++++++++
> src/remote/remote_protocol.x | 13 ++++++++++-
> src/remote_protocol-structs | 12 ++++++++++
> 4 files changed, 117 insertions(+), 1 deletions(-)
>
I'm reviewing a bit out-of-order here:
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index b0b530c..bc5e70f 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -2666,6 +2666,16 @@ struct remote_node_get_memory_parameters_ret {
> int nparams;
> };
>
> +struct remote_node_get_cpu_map_flags_args {
> + unsigned int flags;
> +};
This is insufficient for the caller to know if the map and/or online
parameters were non-NULL. No need to make the remote side allocate
something in reply if the local side doesn't want them. 'online' is
cheap enough to always provide, but skipping out on 'cpumap' will make
the return more efficient. So we need an additional member in this
struct, stating whether cpumap started life NULL.
> +
> +struct remote_node_get_cpu_map_flags_ret {
> + opaque cpumap<REMOTE_CPUMAP_MAX>;
> + unsigned int online;
> + int ret;
> +};
> +
This looks reasonable, modulo s/_flags//.
> /*----- Protocol. -----*/
>
> /* Define the program number, protocol version and procedure numbers
here. */
> @@ -3008,7 +3018,8 @@ enum remote_procedure {
> REMOTE_PROC_NODE_GET_MEMORY_PARAMETERS = 289, /* skipgen skipgen */
Hmm - pre-existing, but I think node memory parameters should be
priority:high, since obtaining that information doesn't block. I'll
think about that a bit more, and maybe submit a patch.
> REMOTE_PROC_DOMAIN_BLOCK_COMMIT = 290, /* autogen autogen */
>
> - REMOTE_PROC_NETWORK_UPDATE = 291 /* autogen autogen priority:high */
> + REMOTE_PROC_NETWORK_UPDATE = 291, /* autogen autogen priority:high */
> + REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS = 292 /* skipgen skipgen */
292 was already claimed. But I think you are right that we can't use
skipgen, due to special-case handling of cpumap allocation.
Also, this can be priority:high, since there's nothing we do in this API
that blocks.
> diff --git a/daemon/remote.c b/daemon/remote.c
> index e7fe128..36291d1 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -4544,6 +4544,50 @@ cleanup:
> return rv;
> }
>
> +static int
> +remoteDispatchNodeGetCPUMapFlags(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client ATTRIBUTE_UNUSED,
> + virNetMessagePtr msg ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr,
> + remote_node_get_cpu_map_flags_args *args,
> + remote_node_get_cpu_map_flags_ret *ret)
> +{
> + unsigned char *cpumap;
Uninitialized...
> + unsigned int online;
> + unsigned int flags;
> + int cpunum;
> + int rv = -1;
> + struct daemonClientPrivate *priv =
> + virNetServerClientGetPrivateData(client);
> +
> + if (!priv->conn) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> + goto cleanup;
...and goto cleanup...
> + }
> +
> + flags = args->flags;
> +
> + cpunum = virNodeGetCPUMapFlags(priv->conn, &cpumap, &online, flags);
Here, we should pass NULL instead of &cpumap if the user on the client
side passed NULL.
> + if (cpunum < 0)
> + goto cleanup;
> +
> + /* 'serialize' return cpumap */
> + ret->cpumap.cpumap_len = VIR_CPU_MAPLEN(cpunum);
> + ret->cpumap.cpumap_val = (char *) cpumap;
> + cpumap = NULL;
Again, if the user didn't ask for a map, then we have nothing to
transfer here.
> +
> + ret->online = online;
> + ret->ret = cpunum;
> +
> + rv = 0;
> +
> +cleanup:
> + if (rv < 0)
> + virNetMessageSaveError(rerr);
> + VIR_FREE(cpumap);
...equals boom.
> + return rv;
> +}
> +
> /*----- Helpers. -----*/
>
> /* get_nonnull_domain and get_nonnull_network turn an on-wire
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index fc4c696..7c89477 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -5749,6 +5749,54 @@ done:
> return rv;
> }
>
> +int
Needs to be static.
> +remoteNodeGetCPUMapFlags(virConnectPtr conn,
> + unsigned char **cpumap,
> + unsigned int *online,
> + unsigned int flags)
> +{
> + int rv = -1;
> + remote_node_get_cpu_map_flags_args args;
> + remote_node_get_cpu_map_flags_ret ret;
> + struct private_data *priv = conn->privateData;
> +
> + remoteDriverLock(priv);
> +
> + /* IMPROVEME: send info about optional args */
Aha - you were even thinking about sending information about optional
arguments!
> + args.flags = flags;
> +
> + memset (&ret, 0, sizeof(ret));
> + if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS,
We aren't consistent about space after 'call' here, so not your fault on
this one.
> +++ b/src/remote_protocol-structs
> @@ -2123,6 +2123,17 @@ struct remote_node_get_memory_parameters_ret {
> } params;
> int nparams;
> };
> +struct remote_node_get_cpu_map_flags_args {
> + u_int flags;
> +};
And this needs to match whatever I changed in the .x.
Here's what I'm squashing in:
diff --git i/daemon/remote.c w/daemon/remote.c
index 9482b86..7a9df60 100644
--- i/daemon/remote.c
+++ w/daemon/remote.c
@@ -4570,14 +4570,14 @@ cleanup:
}
static int
-remoteDispatchNodeGetCPUMapFlags(virNetServerPtr server ATTRIBUTE_UNUSED,
- virNetServerClientPtr client
ATTRIBUTE_UNUSED,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr,
- remote_node_get_cpu_map_flags_args *args,
- remote_node_get_cpu_map_flags_ret *ret)
+remoteDispatchNodeGetCPUMap(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
+ remote_node_get_cpu_map_args *args,
+ remote_node_get_cpu_map_ret *ret)
{
- unsigned char *cpumap;
+ unsigned char *cpumap = NULL;
unsigned int online;
unsigned int flags;
int cpunum;
@@ -4592,14 +4592,17 @@ remoteDispatchNodeGetCPUMapFlags(virNetServerPtr
server ATTRIBUTE_UNUSED,
flags = args->flags;
- cpunum = virNodeGetCPUMapFlags(priv->conn, &cpumap, &online, flags);
+ cpunum = virNodeGetCPUMap(priv->conn, args->need_results ? &cpumap
: NULL,
+ &online, flags);
if (cpunum < 0)
goto cleanup;
/* 'serialize' return cpumap */
- ret->cpumap.cpumap_len = VIR_CPU_MAPLEN(cpunum);
- ret->cpumap.cpumap_val = (char *) cpumap;
- cpumap = NULL;
+ if (args->need_results) {
+ ret->cpumap.cpumap_len = VIR_CPU_MAPLEN(cpunum);
+ ret->cpumap.cpumap_val = (char *) cpumap;
+ cpumap = NULL;
+ }
ret->online = online;
ret->ret = cpunum;
diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c
index 48affac..71218f0 100644
--- i/src/remote/remote_driver.c
+++ w/src/remote/remote_driver.c
@@ -5780,28 +5780,28 @@ done:
return rv;
}
-int
-remoteNodeGetCPUMapFlags(virConnectPtr conn,
- unsigned char **cpumap,
- unsigned int *online,
- unsigned int flags)
+static int
+remoteNodeGetCPUMap(virConnectPtr conn,
+ unsigned char **cpumap,
+ unsigned int *online,
+ unsigned int flags)
{
int rv = -1;
- remote_node_get_cpu_map_flags_args args;
- remote_node_get_cpu_map_flags_ret ret;
+ remote_node_get_cpu_map_args args;
+ remote_node_get_cpu_map_ret ret;
struct private_data *priv = conn->privateData;
remoteDriverLock(priv);
- /* IMPROVEME: send info about optional args */
+ args.need_results = !!cpumap;
args.flags = flags;
memset (&ret, 0, sizeof(ret));
- if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS,
- (xdrproc_t) xdr_remote_node_get_cpu_map_flags_args,
- (char *) &args,
- (xdrproc_t) xdr_remote_node_get_cpu_map_flags_ret,
- (char *) &ret) == -1)
+ if (call(conn, priv, 0, REMOTE_PROC_NODE_GET_CPU_MAP,
+ (xdrproc_t) xdr_remote_node_get_cpu_map_args,
+ (char *) &args,
+ (xdrproc_t) xdr_remote_node_get_cpu_map_ret,
+ (char *) &ret) == -1)
goto done;
if (ret.ret < 0)
@@ -5821,8 +5821,7 @@ remoteNodeGetCPUMapFlags(virConnectPtr conn,
rv = ret.ret;
cleanup:
- xdr_free ((xdrproc_t) xdr_remote_node_get_cpu_map_flags_ret,
- (char *) &ret);
+ xdr_free((xdrproc_t) xdr_remote_node_get_cpu_map_ret, (char *) &ret);
done:
remoteDriverUnlock(priv);
return rv;
@@ -6142,7 +6141,7 @@ static virDriver remote_driver = {
.domainGetHostname = remoteDomainGetHostname, /* 0.10.0 */
.nodeSetMemoryParameters = remoteNodeSetMemoryParameters, /* 0.10.2 */
.nodeGetMemoryParameters = remoteNodeGetMemoryParameters, /* 0.10.2 */
- .nodeGetCPUMapFlags = remoteNodeGetCPUMapFlags, /* 1.0.0 */
+ .nodeGetCPUMap = remoteNodeGetCPUMap, /* 1.0.0 */
};
static virNetworkDriver network_driver = {
diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x
index ebfaa6a..765ffcd 100644
--- i/src/remote/remote_protocol.x
+++ w/src/remote/remote_protocol.x
@@ -2670,11 +2670,12 @@ struct remote_node_get_memory_parameters_ret {
int nparams;
};
-struct remote_node_get_cpu_map_flags_args {
+struct remote_node_get_cpu_map_args {
+ int need_results;
unsigned int flags;
};
-struct remote_node_get_cpu_map_flags_ret {
+struct remote_node_get_cpu_map_ret {
opaque cpumap<REMOTE_CPUMAP_MAX>;
unsigned int online;
int ret;
@@ -3024,7 +3025,7 @@ enum remote_procedure {
REMOTE_PROC_NETWORK_UPDATE = 291, /* autogen autogen priority:high */
REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND_DISK = 292, /* autogen autogen */
- REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS = 293 /* skipgen skipgen */
+ REMOTE_PROC_NODE_GET_CPU_MAP = 293 /* skipgen skipgen */
/*
* Notice how the entries are grouped in sets of 10 ?
diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs
index 894fd1d..567864a 100644
--- i/src/remote_protocol-structs
+++ w/src/remote_protocol-structs
@@ -2126,10 +2126,11 @@ struct remote_node_get_memory_parameters_ret {
} params;
int nparams;
};
-struct remote_node_get_cpu_map_flags_args {
+struct remote_node_get_cpu_map_args {
+ int need_results;
u_int flags;
};
-struct remote_node_get_cpu_map_flags_ret {
+struct remote_node_get_cpu_map_ret {
struct {
u_int cpumap_len;
char * cpumap_val;
@@ -2430,5 +2431,5 @@ enum remote_procedure {
REMOTE_PROC_DOMAIN_BLOCK_COMMIT = 290,
REMOTE_PROC_NETWORK_UPDATE = 291,
REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND_DISK = 292,
- REMOTE_PROC_NODE_GET_CPU_MAP_FLAGS = 293,
+ REMOTE_PROC_NODE_GET_CPU_MAP = 293,
};
--
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