Re: [libvirt] [PATCH V4 2/5] remote handler for virDomainGetCPUStats()

On 01/27/2012 11:21 PM, KAMEZAWA Hiroyuki wrote: s/dimentional/dimensional/ Odd that the diffstat listed twice. I'm going to reorder my review, to start with the .x file. We should enforce the two limits independently, which means we also need two smaller constants (it's a shame that rpcgen doesn't allow products when computing a constant, but we can document how we arrive at various numbers). We can reuse VIR_NODE_CPU_STATS_MAX for one of the two limits. Looks good. autogen */ skipgen */ Trivial conflict resolution. Use the constants from the .x file, rather than open-coding things. Oops, I just realized that I mentioned that libvirt.c should not have a length check, but then I forgot to delete it. Meanwhile, even if libvirt.c does not have a length check, it still needs to have a multiplication overflow check. s/callar/caller/ We can still use remoteSerializeTypedParameters; we just have to teach it to skip zero entries like it already can skip string entries. Oh, I just realized that to support string entries, we have to support a flags of VIR_TYPED_PARAM_STRING_OKAY. Cleanup is a lot simpler if we let the existing serialization skip zero entries. This side needs to validate incoming parameters before sending over the wire. Hmm, remoteNodeGetCPUStats needs to do likewise. I prefer sizeof(expr) over sizeof(type), in case expr ever changes types. Again, open coding things is not nice. If we write the server to never send zero entries (which is a good thing - less data over the wire), then the client needs to re-create zero entries by calling deserialization in a loop - deserialize ret.nparams entries at every nparams slot. Potential free of bogus memory if anything can reach cleanup with rv < 0 but before we sanitized the contents of params. Here's what I'm squashing in: diff --git i/daemon/remote.c w/daemon/remote.c index 15857a8..cb8423a 100644 --- i/daemon/remote.c +++ w/daemon/remote.c @@ -704,8 +704,11 @@ remoteSerializeTypedParameters(virTypedParameterPtr params, } for (i = 0, j = 0; i < nparams; ++i) { - if (!(flags & VIR_TYPED_PARAM_STRING_OKAY) && - params[i].type == VIR_TYPED_PARAM_STRING) { + /* virDomainGetCPUStats can return a sparse array; also, we + * can't pass back strings to older clients. */ + if (!params[i].type || + (!(flags & VIR_TYPED_PARAM_STRING_OKAY) && + params[i].type == VIR_TYPED_PARAM_STRING)) { --*ret_params_len; continue; } @@ -3534,10 +3537,7 @@ remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, virDomainPtr dom = NULL; struct daemonClientPrivate *priv; virTypedParameterPtr params = NULL; - remote_typed_param *info = NULL; - int cpu, idx, src, dest; int rv = -1; - unsigned int return_size = 0; int percpu_len = 0; priv = virNetServerClientGetPrivateData(client); @@ -3546,128 +3546,53 @@ remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - /* - * nparams * ncpus should be smaller than - * REMOTE_DOMAIN_GET_CPU_STATS_MAX but nparams - * and ncpus are limited by API. check it. - */ - if (args->nparams > 16) { + if (args->nparams > REMOTE_NODE_CPU_STATS_MAX) { virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (args->ncpus > 128) { + if (args->ncpus > REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX) { virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("ncpus too large")); goto cleanup; } - if (args->nparams > 0) { - if (VIR_ALLOC_N(params, args->ncpus * args->nparams) < 0) - goto no_memory; + if (args->nparams > 0 && + VIR_ALLOC_N(params, args->ncpus * args->nparams) < 0) { + virReportOOMError(); + goto cleanup; } if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; percpu_len = virDomainGetCPUStats(dom, params, args->nparams, - args->start_cpu, args->ncpus, args->flags); + args->start_cpu, args->ncpus, + args->flags); if (percpu_len < 0) goto cleanup; - /* If nparams == 0, the function returns a sigle value */ + /* If nparams == 0, the function returns a single value */ if (args->nparams == 0) goto success; - /* - * Here, we return values based on the real param size returned by - * a driver rather than the passed one. RPC Client stub should decode - * it to fit callar's expectation. - * - * If cpu ID is sparse, The whole array for some cpu IDs will be - * zero-cleared. This doesn't meet to remoteSerializeTypedParameters() - * and we do serialization by ourselves. - */ - return_size = percpu_len * args->ncpus; - if (VIR_ALLOC_N(info, return_size) < 0) - goto no_memory; + if (remoteSerializeTypedParameters(params, args->nparams * args->ncpus, + &ret->params.params_val, + &ret->params.params_len, + args->flags) < 0) + goto cleanup; + + percpu_len = ret->params.params_len / args->ncpus; - for (cpu = 0; cpu < args->ncpus; ++cpu) { - for (idx = 0; idx < percpu_len; ++idx) { - src = cpu * args->nparams + idx; - dest = cpu * percpu_len + idx; - - /* If CPU ID is discontiguous, this can happen */ - if (params[src].type == 0) - continue; - - info[dest].field = strdup(params[src].field); - if (info[dest].field == NULL) - goto no_memory; - - info[dest].value.type = params[src].type; - - switch (params[src].type) { - case VIR_TYPED_PARAM_INT: - info[dest].value.remote_typed_param_value_u.i = - params[src].value.i; - break; - case VIR_TYPED_PARAM_UINT: - info[dest].value.remote_typed_param_value_u.ui = - params[src].value.ui; - break; - case VIR_TYPED_PARAM_LLONG: - info[dest].value.remote_typed_param_value_u.l = - params[src].value.l; - break; - case VIR_TYPED_PARAM_ULLONG: - info[dest].value.remote_typed_param_value_u.ul = - params[src].value.ul; - break; - case VIR_TYPED_PARAM_DOUBLE: - info[dest].value.remote_typed_param_value_u.d = - params[src].value.d; - break; - case VIR_TYPED_PARAM_BOOLEAN: - info[dest].value.remote_typed_param_value_u.b = - params[src].value.b; - break; - case VIR_TYPED_PARAM_STRING: - info[dest].value.remote_typed_param_value_u.s = - strdup(params[src].value.s); - if (info[dest].value.remote_typed_param_value_u.s == NULL) - goto no_memory; - break; - default: - virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"), - params[src].type); - goto cleanup; - } - } - } success: rv = 0; - ret->params.params_len = return_size; - ret->params.params_val = info; ret->nparams = percpu_len; + cleanup: - if (rv < 0) { + if (rv < 0) virNetMessageSaveError(rerr); - if (info) { - int i; - for (i = 0; i < return_size; i++) { - VIR_FREE(info[i].field); - if (info[i].value.type == VIR_TYPED_PARAM_STRING) - VIR_FREE(info[i].value.remote_typed_param_value_u.s); - } - VIR_FREE(info); - } - } virTypedParameterArrayClear(params, args->ncpus * args->nparams); VIR_FREE(params); if (dom) virDomainFree(dom); return rv; -no_memory: - virReportOOMError(); - goto cleanup; } /*----- Helpers. -----*/ diff --git i/src/libvirt.c w/src/libvirt.c index 7060f5a..e702a34 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -18162,7 +18162,7 @@ error: * @nparams: number of parameters per cpu * @start_cpu: which cpu to start with, or -1 for summary * @ncpus: how many cpus to query - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of virTypedParameterFlags * * Get statistics relating to CPU usage attributable to a single * domain (in contrast to the statistics returned by @@ -18251,20 +18251,19 @@ int virDomainGetCPUStats(virDomainPtr domain, * if start_cpu is -1, ncpus must be 1 * params == NULL must match nparams == 0 * ncpus must be non-zero unless params == NULL + * nparams * ncpus must not overflow (RPC may restrict it even more) */ if (start_cpu < -1 || (start_cpu == -1 && ncpus != 1) || ((params == NULL) != (nparams == 0)) || - (ncpus == 0 && params != NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } - - /* remote protocol doesn't welcome big args in one shot */ - if ((nparams > 16) || (ncpus > 128)) { + (ncpus == 0 && params != NULL) || + ncpus < UINT_MAX / nparams) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + flags |= VIR_TYPED_PARAM_STRING_OKAY; if (conn->driver->domainGetCPUStats) { int ret; diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c index a3ca63e..2312cf9 100644 --- i/src/remote/remote_driver.c +++ w/src/remote/remote_driver.c @@ -2,7 +2,7 @@ * remote_driver.c: driver to provide access to libvirtd running * on a remote machine * - * Copyright (C) 2007-2011 Red Hat, Inc. + * Copyright (C) 2007-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -2315,12 +2315,24 @@ static int remoteDomainGetCPUStats(virDomainPtr domain, struct private_data *priv = domain->conn->privateData; remote_domain_get_cpu_stats_args args; remote_domain_get_cpu_stats_ret ret; - remote_typed_param *info; int rv = -1; - int cpu, idx, src, dest; + int cpu; remoteDriverLock(priv); + if (nparams > REMOTE_NODE_CPU_STATS_MAX) { + remoteError(VIR_ERR_RPC, + _("nparams count exceeds maximum: %u > %u"), + nparams, REMOTE_NODE_CPU_STATS_MAX); + goto done; + } + if (ncpus > REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX) { + remoteError(VIR_ERR_RPC, + _("ncpus count exceeds maximum: %u > %u"), + ncpus, REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX); + goto done; + } + make_nonnull_domain(&args.dom, domain); args.nparams = nparams; args.start_cpu = start_cpu; @@ -2336,67 +2348,38 @@ static int remoteDomainGetCPUStats(virDomainPtr domain, (char *) &ret) == -1) goto done; + /* Check the length of the returned list carefully. */ + if (ret.params.params_len > nparams * ncpus || + (ret.params.params_len && + ret.nparams * ncpus != ret.params.params_len)) { + remoteError(VIR_ERR_RPC, "%s", + _("remoteDomainGetCPUStats: " + "returned number of stats exceeds limit")); + memset(params, 0, sizeof(*params) * nparams * ncpus); + goto cleanup; + } + + /* Handle the case when the caller does not know the number of stats + * and is asking for the number of stats supported + */ if (nparams == 0) { rv = ret.nparams; goto cleanup; } - /* - * the returned arrray's size is not same to nparams * ncpus. And - * if cpu ID is not contiguous, all-zero entries can be found. + + /* The remote side did not send back any zero entries, so we have + * to expand things back into a possibly sparse array. */ - memset(params, 0, sizeof(virTypedParameter) * nparams * ncpus); - - /* Here, ret.nparams is always smaller than nparams */ - info = ret.params.params_val; - - for (cpu = 0; cpu < ncpus; ++cpu) { - for (idx = 0; idx < ret.nparams; ++idx) { - src = cpu * ret.nparams + idx; - dest = cpu * nparams + idx; - - if (info[src].value.type == 0) /* skip zeroed ones */ - continue; - - params[dest].type = info[src].value.type; - strcpy(params[dest].field, info[src].field); - - switch (params[dest].type) { - case VIR_TYPED_PARAM_INT: - params[dest].value.i = - info[src].value.remote_typed_param_value_u.i; - break; - case VIR_TYPED_PARAM_UINT: - params[dest].value.ui = - info[src].value.remote_typed_param_value_u.ui; - break; - case VIR_TYPED_PARAM_LLONG: - params[dest].value.l = - info[src].value.remote_typed_param_value_u.l; - break; - case VIR_TYPED_PARAM_ULLONG: - params[dest].value.ul = - info[src].value.remote_typed_param_value_u.ul; - break; - case VIR_TYPED_PARAM_DOUBLE: - params[dest].value.d = - info[src].value.remote_typed_param_value_u.d; - break; - case VIR_TYPED_PARAM_BOOLEAN: - params[dest].value.b = - info[src].value.remote_typed_param_value_u.b; - break; - case VIR_TYPED_PARAM_STRING: - params[dest].value.s = - strdup(info[src].value.remote_typed_param_value_u.s); - if (params[dest].value.s == NULL) - goto out_of_memory; - break; - default: - remoteError(VIR_ERR_RPC, _("unknown parameter type: %d"), - params[dest].type); - goto cleanup; - } - } + memset(params, 0, sizeof(*params) * nparams * ncpus); + for (cpu = 0; cpu < ncpus; cpu++) { + int tmp = nparams; + remote_typed_param *stride = &ret.params.params_val[cpu * ret.nparams]; + + if (remoteDeserializeTypedParameters(stride, ret.nparams, + REMOTE_NODE_CPU_STATS_MAX, + &params[cpu * nparams], + &tmp) < 0) + goto cleanup; } rv = ret.nparams; @@ -2415,9 +2398,6 @@ cleanup: done: remoteDriverUnlock(priv); return rv; -out_of_memory: - virReportOOMError(); - goto cleanup; } diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x index 0bd399c..b58925a 100644 --- i/src/remote/remote_protocol.x +++ w/src/remote/remote_protocol.x @@ -209,8 +209,13 @@ const REMOTE_DOMAIN_SEND_KEY_MAX = 16; const REMOTE_DOMAIN_INTERFACE_PARAMETERS_MAX = 16; /* - * Upper limit on list of (real) cpu parameters - * nparams(<=16) * ncpus(<=128) <= 2048. + * Upper limit on cpus involved in per-cpu stats + */ +const REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX = 128; + +/* + * Upper limit on list of per-cpu stats: + * REMOTE_NODE_CPU_STATS_MAX * REMOTE_DOMAIN_GET_CPU_STATS_MAX */ const REMOTE_DOMAIN_GET_CPU_STATS_MAX = 2048; -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org