On 04/29/2013 07:50 AM, Michal Privoznik wrote:
> ---
> daemon/libvirtd-config.c | 37 ++++++++++---------
> daemon/libvirtd.c | 17 +++++----
> daemon/remote.c | 94 ++++++++++++++++++++----------------------------
> 3 files changed, 66 insertions(+), 82 deletions(-)
>
> diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
> index efb564e..df2bc7b 100644
> --- a/daemon/libvirtd-config.c
> +++ b/daemon/libvirtd-config.c
> @@ -59,15 +59,14 @@ remoteConfigGetStringList(virConfPtr conf, const char *key, char ***list_arg,
> key);
> return -1;
> }
> - list[0] = strdup(p->str);
> - list[1] = NULL;
> - if (list[0] == NULL) {
> + if (VIR_STRDUP(list[0], p->str) < 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("failed to allocate memory for %s config list value"),
> key);
We should just nuke this error, and let VIR_STRDUP's OOM error suffice.
This whole function could probably use a cleanup pass to use
virReportOOMError() as appropriate.
> VIR_FREE(list);
> return -1;
> }
> + list[1] = NULL;
> break;
>
> case VIR_CONF_LIST: {
> @@ -90,8 +89,7 @@ remoteConfigGetStringList(virConfPtr conf, const char *key, char ***list_arg,
> VIR_FREE(list);
> return -1;
> }
> - list[i] = strdup(pp->str);
> - if (list[i] == NULL) {
> + if (VIR_STRDUP(list[i], pp->str) < 0) {
> int j;
> for (j = 0 ; j < i ; j++)
> VIR_FREE(list[j]);
Yep, here's another case where we should nuke the
virReportError(VIR_ERR_CONFIG_UNSUPPORTED) and rely on VIR_STRDUP's OOM
error instead.
> @@ -136,8 +134,8 @@ checkType(virConfValuePtr p, const char *filename,
> }
>
> /* If there is no config data for the key, #var_name, then do nothing.
> - If there is valid data of type VIR_CONF_STRING, and strdup succeeds,
> - store the result in var_name. Otherwise, (i.e. invalid type, or strdup
> + If there is valid data of type VIR_CONF_STRING, and VIR_STRDUP succeeds,
> + store the result in var_name. Otherwise, (i.e. invalid type, or VIR_STRDUP
> failure), give a diagnostic and "goto" the cleanup-and-fail label. */
> #define GET_CONF_STR(conf, filename, var_name) \
> do { \
> @@ -146,7 +144,7 @@ checkType(virConfValuePtr p, const char *filename,
> if (checkType(p, filename, #var_name, VIR_CONF_STRING) < 0) \
> goto error; \
> VIR_FREE(data->var_name); \
> - if (!(data->var_name = strdup(p->str))) { \
> + if (VIR_STRDUP(data->var_name, p->str) < 0) { \
> virReportOOMError(); \
Drop the virReportOOMError here; since VIR_STRDUP is documented to do
the work for you.
> goto error; \
> } \
> @@ -200,7 +198,7 @@ int
> daemonConfigFilePath(bool privileged, char **configfile)
> {
> if (privileged) {
> - if (!(*configfile = strdup(SYSCONFDIR "/libvirt/libvirtd.conf")))
> + if (VIR_STRDUP(*configfile, SYSCONFDIR "/libvirt/libvirtd.conf") < 0)
> goto no_memory;
You can drop the no_memory label and just use the error label.
> } else {
> char *configdir = NULL;
> @@ -238,9 +236,9 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED)
> data->listen_tls = 1;
> data->listen_tcp = 0;
>
> - if (!(data->tls_port = strdup(LIBVIRTD_TLS_PORT)))
> + if (VIR_STRDUP(data->tls_port, LIBVIRTD_TLS_PORT) < 0)
> goto no_memory;
> - if (!(data->tcp_port = strdup(LIBVIRTD_TCP_PORT)))
> + if (VIR_STRDUP(data->tcp_port, LIBVIRTD_TCP_PORT) < 0)
> goto no_memory;
Hmm. Here, the no_memory label does additional cleanup...
>
> /* Only default to PolicyKit if running as root */
> @@ -256,13 +254,14 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED)
> }
> #endif
>
> - if (data->auth_unix_rw == REMOTE_AUTH_POLKIT)
> - data->unix_sock_rw_perms = strdup("0777"); /* Allow world */
> - else
> - data->unix_sock_rw_perms = strdup("0700"); /* Allow user only */
> - data->unix_sock_ro_perms = strdup("0777"); /* Always allow world */
> - if (!data->unix_sock_ro_perms ||
> - !data->unix_sock_rw_perms)
> + if (data->auth_unix_rw == REMOTE_AUTH_POLKIT) {
> + if (VIR_STRDUP(data->unix_sock_rw_perms, "0777") < 0)
> + goto no_memory;
> + } else {
> + if (VIR_STRDUP(data->unix_sock_rw_perms, "0700") < 0)
> + goto no_memory;
> + }
> + if (VIR_STRDUP(data->unix_sock_ro_perms, "0777") < 0)
> goto no_memory;
...and one of the paths into the no_memory label is via virAsprintf(),
which has not yet been converted to report OOM. So for this function,
you'll need to add a label (and later delete no_memory when we do a
second pass over the code to scrub virAsprintf usage):
no_memory:
virReportOOMError();
cleanup:
daemonConfigFree(data);
return NULL;
>
> #if WITH_SASL
> @@ -382,7 +381,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
> */
> if (data->auth_unix_rw == REMOTE_AUTH_POLKIT) {
> VIR_FREE(data->unix_sock_rw_perms);
> - if (!(data->unix_sock_rw_perms = strdup("0777"))) {
> + if (VIR_STRDUP(data->unix_sock_rw_perms, "0777") < 0) {
> virReportOOMError();
Drop the duplicate error.
> goto error;
> }
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 9f81a0f..07c064d 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -241,7 +241,7 @@ daemonPidFilePath(bool privileged,
> char **pidfile)
> {
> if (privileged) {
> - if (!(*pidfile = strdup(LOCALSTATEDIR "/run/libvirtd.pid")))
> + if (VIR_STRDUP(*pidfile, LOCALSTATEDIR "/run/libvirtd.pid") < 0)
> goto no_memory;
Change to goto error.
> } else {
> char *rundir = NULL;
> @@ -287,9 +287,9 @@ daemonUnixSocketPaths(struct daemonConfig *config,
> goto no_memory;
> } else {
> if (privileged) {
> - if (!(*sockfile = strdup(LOCALSTATEDIR "/run/libvirt/libvirt-sock")))
> + if (VIR_STRDUP(*sockfile, LOCALSTATEDIR "/run/libvirt/libvirt-sock") < 0)
> goto no_memory;
> - if (!(*rosockfile = strdup(LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro")))
> + if (VIR_STRDUP(*rosockfile, LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro") < 0)
> goto no_memory;
Change both to goto error
> } else {
> char *rundir = NULL;
> @@ -961,7 +961,10 @@ static int migrateProfile(void)
>
> config_home = getenv("XDG_CONFIG_HOME");
> if (config_home && config_home[0] != '\0') {
> - xdg_dir = strdup(config_home);
> + if (VIR_STRDUP(xdg_dir, config_home) < 0) {
> + virReportOOMError();
Don't report the error twice.
> + goto cleanup;
> + }
> } else {
> if (virAsprintf(&xdg_dir, "%s/.config", home) < 0) {
> goto cleanup;
> @@ -1172,7 +1175,7 @@ int main(int argc, char **argv) {
>
> case 'p':
> VIR_FREE(pid_file);
> - if (!(pid_file = strdup(optarg))) {
> + if (VIR_STRDUP(pid_file, optarg) < 0) {
> VIR_ERROR(_("Can't allocate memory"));
_This_ instance might need to use VIR_STRDUP_QUIET and keep the
VIR_ERROR() report; main() is early enough that virReportOOMError()
might not go anywhere useful. Probably will never be triggered in
practice, though, so I'd also be okay with regular VIR_STRDUP and
dropping the VIR_ERROR.
> exit(EXIT_FAILURE);
> }
> @@ -1180,7 +1183,7 @@ int main(int argc, char **argv) {
>
> case 'f':
> VIR_FREE(remote_config_file);
> - if (!(remote_config_file = strdup(optarg))) {
> + if (VIR_STRDUP(remote_config_file, optarg) < 0) {
> VIR_ERROR(_("Can't allocate memory"));
Same story.
> exit(EXIT_FAILURE);
> }
> @@ -1287,7 +1290,7 @@ int main(int argc, char **argv) {
>
> /* Ensure the rundir exists (on tmpfs on some systems) */
> if (privileged) {
> - run_dir = strdup(LOCALSTATEDIR "/run/libvirt");
> + ignore_value(VIR_STRDUP(run_dir, LOCALSTATEDIR "/run/libvirt"));
Here, I'd hoist the OOM error that occurs several lines later into this
if branch (since it can't be reached on the else branch) rather than
using ignore_value just to then check for NULL ourselves a few lines
later. Which turns this into:
if (privileged) {
if (VIR_STRDUP(run_dir, LOCALSTATEDIR "/run/libvirt") < 0)
goto cleanup;
} else {
run_dir = virGetUserRuntimeDirectory();
> } else {
> run_dir = virGetUserRuntimeDirectory();
>
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 8c79680..fa8dedd 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -234,11 +234,9 @@ static int remoteRelayDomainEventIOError(virConnectPtr conn ATTRIBUTE_UNUSED,
>
> /* build return data */
> memset(&data, 0, sizeof(data));
> - data.srcPath = strdup(srcPath);
> - if (data.srcPath == NULL)
> + if (VIR_STRDUP(data.srcPath, srcPath) < 0)
> goto mem_error;
Drop the virReportOOMError() from mem_error(); at which point the label
should be renamed to plain 'error'.
> - data.devAlias = strdup(devAlias);
> - if (data.devAlias == NULL)
> + if (VIR_STRDUP(data.devAlias, devAlias) < 0)
> goto mem_error;
> make_nonnull_domain(&data.dom, dom);
> data.action = action;
> @@ -275,15 +273,12 @@ static int remoteRelayDomainEventIOErrorReason(virConnectPtr conn ATTRIBUTE_UNUS
>
> /* build return data */
> memset(&data, 0, sizeof(data));
> - data.srcPath = strdup(srcPath);
> - if (data.srcPath == NULL)
> + if (VIR_STRDUP(data.srcPath, srcPath) < 0)
> goto mem_error;
> - data.devAlias = strdup(devAlias);
> - if (data.devAlias == NULL)
> + if (VIR_STRDUP(data.devAlias, devAlias) < 0)
> goto mem_error;
> data.action = action;
> - data.reason = strdup(reason);
> - if (data.reason == NULL)
> + if (VIR_STRDUP(data.reason, reason) < 0)
> goto mem_error;
Same story (in fact, true for much of this file, so I'll quit pointing
it out).
> @@ -473,17 +460,17 @@ static int remoteRelayDomainEventDiskChange(virConnectPtr conn ATTRIBUTE_UNUSED,
> memset(&data, 0, sizeof(data));
> if (oldSrcPath &&
> ((VIR_ALLOC(oldSrcPath_p) < 0) ||
> - !(*oldSrcPath_p = strdup(oldSrcPath))))
> + VIR_STRDUP(*oldSrcPath_p, oldSrcPath) < 0))
> goto mem_error;
Ah, now here's where we have to choose to either report OOM twice on
VIR_STRDUP failure until a second pass of patches cleans up VIR_ALLOC to
report OOM, or where we temporarily have to make the code ugly so that
VIR_ALLOC uses goto mem_error while VIR_STRDUP uses goto error. I guess
I can live with either version, as long as we do get the VIR_ALLOC usage
scrubbed before 1.0.6.
> @@ -520,7 +507,7 @@ static int remoteRelayDomainEventTrayChange(virConnectPtr conn ATTRIBUTE_UNUSED,
> /* build return data */
> memset(&data, 0, sizeof(data));
>
> - if (!(data.devAlias = strdup(devAlias))) {
> + if (VIR_STRDUP(data.devAlias, devAlias) < 0) {
> virReportOOMError();
Drop the error report.
> return -1;
> }
> @@ -850,8 +837,7 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
> }
>
> /* remoteDispatchClientRequest will free this: */
> - val[j].field = strdup(params[i].field);
> - if (val[j].field == NULL) {
> + if (VIR_STRDUP(val[j].field, params[i].field) < 0) {
> virReportOOMError();
And again.
> goto cleanup;
> }
> @@ -876,9 +862,8 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
> val[j].value.remote_typed_param_value_u.b = params[i].value.b;
> break;
> case VIR_TYPED_PARAM_STRING:
> - val[j].value.remote_typed_param_value_u.s =
> - strdup(params[i].value.s);
> - if (val[j].value.remote_typed_param_value_u.s == NULL) {
> + if (VIR_STRDUP(val[j].value.remote_typed_param_value_u.s,
> + params[i].value.s) < 0) {
> virReportOOMError();
And again.
> goto cleanup;
> }
> @@ -966,9 +951,8 @@ remoteDeserializeTypedParameters(remote_typed_param *args_params_val,
> args_params_val[i].value.remote_typed_param_value_u.b;
> break;
> case VIR_TYPED_PARAM_STRING:
> - params[i].value.s =
> - strdup(args_params_val[i].value.remote_typed_param_value_u.s);
> - if (params[i].value.s == NULL) {
> + if (VIR_STRDUP(params[i].value.s,
> + args_params_val[i].value.remote_typed_param_value_u.s) < 0) {
> virReportOOMError();
Here too.
> goto cleanup;
> }
> @@ -2145,8 +2129,7 @@ remoteDispatchNodeGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED,
>
> for (i = 0; i < nparams; ++i) {
> /* remoteDispatchClientRequest will free this: */
> - ret->params.params_val[i].field = strdup(params[i].field);
> - if (ret->params.params_val[i].field == NULL)
> + if (VIR_STRDUP(ret->params.params_val[i].field, params[i].field) < 0)
> goto no_memory;
Here, goto cleanup.
>
> ret->params.params_val[i].value = params[i].value;
> @@ -2223,8 +2206,7 @@ remoteDispatchNodeGetMemoryStats(virNetServerPtr server ATTRIBUTE_UNUSED,
>
> for (i = 0; i < nparams; ++i) {
> /* remoteDispatchClientRequest will free this: */
> - ret->params.params_val[i].field = strdup(params[i].field);
> - if (ret->params.params_val[i].field == NULL)
> + if (VIR_STRDUP(ret->params.params_val[i].field, params[i].field) < 0)
> goto no_memory;
goto cleanup
>
> ret->params.params_val[i].value = params[i].value;
> @@ -3100,7 +3082,7 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server ATTRIBUTE_UNUSED,
> virReportOOMError();
> goto cleanup;
> }
> - if (!(*parent_p = strdup(parent))) {
> + if (VIR_STRDUP(*parent_p, parent) < 0) {
> VIR_FREE(parent_p);
> virReportOOMError();
drop the duplicate error report
> goto cleanup;
> @@ -4769,14 +4751,14 @@ static void
> make_nonnull_domain(remote_nonnull_domain *dom_dst, virDomainPtr dom_src)
> {
> dom_dst->id = dom_src->id;
> - dom_dst->name = strdup(dom_src->name);
> + ignore_value(VIR_STRDUP_QUIET(dom_dst->name , dom_src->name));
> memcpy(dom_dst->uuid, dom_src->uuid, VIR_UUID_BUFLEN);
Ouch. Pre-existing bug that we leave dom_dst->name as NULL on OOM. It
means that we could segfault instead of gracefully returning an error to
the user when we ran out of memory. We probably ought to clean that up
first (in fact, it would be appropriate to even clean it up before 1.0.5
- I might tackle that if I have time later today).
You should NOT have to wrap ignore_value() around every caller of
VIR_STRDUP_QUIET. What that says to me is that the macro you write in
patch 2/37 needs to inline the ignore_value() as part of the macro.
> }
>
> static void
> make_nonnull_network(remote_nonnull_network *net_dst, virNetworkPtr net_src)
> {
> - net_dst->name = strdup(net_src->name);
> + ignore_value(VIR_STRDUP_QUIET(net_dst->name, net_src->name));
> memcpy(net_dst->uuid, net_src->uuid, VIR_UUID_BUFLEN);
Hmm. All of our make_nonnull_* functions share that bug.
>
> @@ -4844,7 +4826,7 @@ remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors,
> goto no_memory;
>
> for (i = 0; i < nerrors; i++) {
> - if (!(val[i].disk = strdup(errors[i].disk)))
> + if (VIR_STRDUP(val[i].disk, errors[i].disk) < 0)
> goto no_memory;
This one's another case of VIR_ALLOC_N and VIR_STRDUP both feeding into
the same label; up to you whether to temporarily leave this one as
double-reporting (until the VIR_ALLOC audit), or whether to re-arrange
the label to have no_memory report OOM first instead of last, as well as
add a new error: label for the VIR_STRDUP to jump to.
It looks like we still have a long ways to go before this series is
ready - I don't see any point in churning the code to add a silent
VIR_STRDUP and then make a second pass that turns it to be noisy - it's
simpler in the git history if we just start life with VIR_STRDUP as
noisy and make the cleanups as we go as part of converting to use
VIR_STRDUP.
--
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