On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- daemon/remote.c | 43 +++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 57 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 +++++++++++++++- src/remote_protocol-structs | 11 +++++++++ 4 files changed, 130 insertions(+), 1 deletion(-)
static int +remoteDispatchConnectGetCPUModelNames(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_get_cpu_model_names_args *args, + remote_connect_get_cpu_model_names_ret *ret) +{ + int len, rv = -1; + char **models; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + len = virConnectGetCPUModelNames(priv->conn, args->arch, &models, + args->flags);
Needs a tweak to pass NULL on to the driver.
+ if (len < 0) + goto cleanup; + + if (ret->models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) {
Wrong variable; here, you want to check if we can encode 'len' into ret.
+ virReportError(VIR_ERR_RPC, + _("Too many CPU models '%d' for limit '%d'"), + ret->models.models_len, REMOTE_CONNECT_CPU_MODELS_MAX); + virStringFreeList(models); + goto cleanup; + } + + ret->models.models_val = models; + ret->models.models_len = len;
Doesn't work when len is 0 - in that case, models is non-NULL (space to a lone NULL pointer terminating the empty array), but xdr wants us to pass NULL, and we must still free models in that case.
+++ b/src/remote/remote_driver.c @@ -5541,6 +5541,62 @@ done:
static int +remoteConnectGetCPUModelNames(virConnectPtr conn, + const char *arch, + char ***models, + unsigned int flags) +{ + int rv = -1; + size_t i; + char **retmodels; + remote_connect_get_cpu_model_names_args args; + remote_connect_get_cpu_model_names_ret ret; + struct private_data *priv = conn->privateData; + remoteDriverLock(priv); + + memset(&args, 0, sizeof(args));
Pointless, since you end up assigning all members of args below.
+ memset(&ret, 0, sizeof(ret)); + + args.arch = (char *) arch; + args.flags = flags; + + if (call(conn, priv, 0, REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES, + (xdrproc_t) xdr_remote_connect_get_cpu_model_names_args, + (char *) &args, + (xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret, + (char *) &ret) < 0) + goto error; + + /* Check the length of the returned list carefully. */ + if (ret.models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) { + virReportError(VIR_ERR_RPC, "%s", + _("remoteConnectGetCPUModelNames: " + "returned number of CPU models exceeds limit"));
What limit? Other functions tell us the limit being violated.
+ goto error; + } + + if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0) + goto error; + + for (i = 0; i < ret.models.models_len; i++) + retmodels[i] = ret.models.models_val[i]; + + /* Caller frees MODELS. */ + *models = retmodels;
Needs a tweak to avoid a blind NULL dereference.
+ rv = ret.models.models_len; + +done: + remoteDriverUnlock(priv); + return rv; + +error: + rv = -1;
Dead assignment; rv is already -1 if we get here.
+ virStringFreeList(ret.models.models_val);
Hmm. The moment we support a NULL models argument, we have to be careful that we free models_val always (not just on error); otherwise, a malicious program on the other end of the rpc pipe could ignore our request for not needing a result and still cause ret to contain an allocated list. I think a cleanup/done pair works for this better than a done/error pair, modelling after the ListAll methods.
+ goto done; +} + + +static int remoteDomainOpenGraphics(virDomainPtr dom, unsigned int idx, int fd, @@ -6933,6 +6989,7 @@ static virDriver remote_driver = { .domainMigratePerform3Params = remoteDomainMigratePerform3Params, /* 1.1.0 */ .domainMigrateFinish3Params = remoteDomainMigrateFinish3Params, /* 1.1.0 */ .domainMigrateConfirm3Params = remoteDomainMigrateConfirm3Params, /* 1.1.0 */ + .connectGetCPUModelNames = remoteConnectGetCPUModelNames, /* 1.1.3 */ };
static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index a1c23da..a1d90ad 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -232,6 +232,9 @@ const REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX = 64; /* Upper limit on number of job stats */ const REMOTE_DOMAIN_JOB_STATS_MAX = 16;
+/* Upper limit on number of CPU models */ +const REMOTE_CONNECT_CPU_MODELS_MAX = 8192; +
Seems a bit large given that we return just 2 for i686/x86_64 setups, but no real issue in keeping it that big.
/* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN];
@@ -2835,6 +2838,15 @@ struct remote_domain_event_device_removed_msg { remote_nonnull_string devAlias; };
+struct remote_connect_get_cpu_model_names_args { + remote_nonnull_string arch; + unsigned int flags; +}; + +struct remote_connect_get_cpu_model_names_ret { + remote_nonnull_string models<REMOTE_CONNECT_CPU_MODELS_MAX>;
Needs a tweak to support the NULL models. ACK with this squashed in: diff --git i/daemon/remote.c w/daemon/remote.c index d357016..c8afa8f 100644 --- i/daemon/remote.c +++ w/daemon/remote.c @@ -5045,7 +5045,7 @@ remoteDispatchConnectGetCPUModelNames(virNetServerPtr server ATTRIBUTE_UNUSED, remote_connect_get_cpu_model_names_ret *ret) { int len, rv = -1; - char **models; + char **models = NULL; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); @@ -5054,27 +5054,36 @@ remoteDispatchConnectGetCPUModelNames(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - len = virConnectGetCPUModelNames(priv->conn, args->arch, &models, + len = virConnectGetCPUModelNames(priv->conn, args->arch, + args->need_results ? &models : NULL, args->flags); if (len < 0) goto cleanup; - if (ret->models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) { + if (len > REMOTE_CONNECT_CPU_MODELS_MAX) { virReportError(VIR_ERR_RPC, _("Too many CPU models '%d' for limit '%d'"), - ret->models.models_len, REMOTE_CONNECT_CPU_MODELS_MAX); - virStringFreeList(models); + len, REMOTE_CONNECT_CPU_MODELS_MAX); goto cleanup; } - ret->models.models_val = models; - ret->models.models_len = len; + if (len && models) { + ret->models.models_val = models; + ret->models.models_len = len; + models = NULL; + } else { + ret->models.models_val = NULL; + ret->models.models_len = 0; + } + + ret->ret = len; rv = 0; cleanup: if (rv < 0) virNetMessageSaveError(rerr); + virStringFreeList(models); return rv; } diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c index aa3762a..96ccb99 100644 --- i/src/remote/remote_driver.c +++ w/src/remote/remote_driver.c @@ -5548,51 +5548,57 @@ remoteConnectGetCPUModelNames(virConnectPtr conn, { int rv = -1; size_t i; - char **retmodels; + char **retmodels = NULL; remote_connect_get_cpu_model_names_args args; remote_connect_get_cpu_model_names_ret ret; + struct private_data *priv = conn->privateData; - remoteDriverLock(priv); - memset(&args, 0, sizeof(args)); - memset(&ret, 0, sizeof(ret)); + remoteDriverLock(priv); args.arch = (char *) arch; + args.need_results = !!models; args.flags = flags; + memset(&ret, 0, sizeof(ret)); if (call(conn, priv, 0, REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES, (xdrproc_t) xdr_remote_connect_get_cpu_model_names_args, (char *) &args, (xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret, (char *) &ret) < 0) - goto error; + goto done; /* Check the length of the returned list carefully. */ if (ret.models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) { - virReportError(VIR_ERR_RPC, "%s", - _("remoteConnectGetCPUModelNames: " - "returned number of CPU models exceeds limit")); - goto error; + virReportError(VIR_ERR_RPC, + _("Too many model names '%d' for limit '%d'"), + ret.models.models_len, + REMOTE_CONNECT_CPU_MODELS_MAX); + goto cleanup; } - if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0) - goto error; + if (models) { + if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0) + goto cleanup; - for (i = 0; i < ret.models.models_len; i++) - retmodels[i] = ret.models.models_val[i]; + for (i = 0; i < ret.models.models_len; i++) { + retmodels[i] = ret.models.models_val[i]; + ret.models.models_val[i] = NULL; + } + *models = retmodels; + retmodels = NULL; + } - /* Caller frees MODELS. */ - *models = retmodels; - rv = ret.models.models_len; + rv = ret.ret; + +cleanup: + virStringFreeList(retmodels); + + xdr_free((xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret, (char *) &ret); done: remoteDriverUnlock(priv); return rv; - -error: - rv = -1; - virStringFreeList(ret.models.models_val); - goto done; } diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x index bb8b0ce..8d17bad 100644 --- i/src/remote/remote_protocol.x +++ w/src/remote/remote_protocol.x @@ -2840,11 +2840,13 @@ struct remote_domain_event_device_removed_msg { struct remote_connect_get_cpu_model_names_args { remote_nonnull_string arch; + int need_results; unsigned int flags; }; struct remote_connect_get_cpu_model_names_ret { remote_nonnull_string models<REMOTE_CONNECT_CPU_MODELS_MAX>; + int ret; }; /*----- Protocol. -----*/ diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs index 9148202..98d2d5b 100644 --- i/src/remote_protocol-structs +++ w/src/remote_protocol-structs @@ -2318,6 +2318,7 @@ struct remote_domain_event_device_removed_msg { }; struct remote_connect_get_cpu_model_names_args { remote_nonnull_string arch; + int need_results; u_int flags; }; struct remote_connect_get_cpu_model_names_ret { @@ -2325,6 +2326,7 @@ struct remote_connect_get_cpu_model_names_ret { u_int models_len; remote_nonnull_string * models_val; } models; + int ret; }; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org