On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote:
> Signed-off-by: Giuseppe Scrivano <gscrivan@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list