Re: [libvirt] [PATCHv2 4/9] virNodeGetCPUMapFlags: Implement wire protocol.

On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote: s/dispacher/dispatcher/ and of course, s/Flags//g :) I'm reviewing a bit out-of-order here: 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. This looks reasonable, modulo s/_flags//. here. */ 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. 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. Uninitialized... ...and goto cleanup... Here, we should pass NULL instead of &cpumap if the user on the client side passed NULL. Again, if the user didn't ask for a map, then we have nothing to transfer here. ...equals boom. Needs to be static. Aha - you were even thinking about sending information about optional arguments! We aren't consistent about space after 'call' here, so not your fault on this one. 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org