From dc4301c7260b75856c389e51a8b0fb611900d0d2 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 29 May 2012 08:34:56 -0600 Subject: [PATCH] virsh: avoid strncpy strncpy is generally evil - it runs the risk of missing NUL termination, and more often than not wastes time zeroing way more bytes than strictly necessary. We've avoided this evil in our virStrncpy wrapper, except for places where we forgot to use the wrapper; meanwhile, we have also added an even higher layer wrapper for setting virTypedParameter values. * tools/virsh.c (cmdMemtune, cmdBlkdeviotune): Use modern API. * cfg.mk (exclude_file_name_regexp--sc_prohibit_strncpy): Tighten. --- cfg.mk | 3 +- tools/virsh.c | 149 +++++++++++++++++++++++++------------------------- 2 files changed, 76 insertions(+), 76 deletions(-) diff --git a/cfg.mk b/cfg.mk index daaa86796..37f51a685 100644 --- a/cfg.mk +++ b/cfg.mk @@ -808,8 +808,7 @@ exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/util\.c$$ exclude_file_name_regexp--sc_prohibit_sprintf = \ ^(docs/hacking\.html\.in)|(examples/systemtap/.*stp)|(src/dtrace2systemtap\.pl)|(src/rpc/gensystemtap\.pl)$$ -exclude_file_name_regexp--sc_prohibit_strncpy = \ - ^(src/util/util|tools/virsh)\.c$$ +exclude_file_name_regexp--sc_prohibit_strncpy = ^src/util/util\.c$$ exclude_file_name_regexp--sc_prohibit_strtol = \ ^src/(util/sexpr|(vbox|xen|xenxs)/.*)\.c$$ diff --git a/tools/virsh.c b/tools/virsh.c index d273c25bf..5226bd851 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6283,7 +6283,6 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) for (i = 0; i < nparams; i++) { temp = ¶ms[i]; - temp->type = VIR_TYPED_PARAM_ULLONG; /* * Some magic here, this is used to fill the params structure with @@ -6292,24 +6291,32 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) * to the next valid argument and so on. */ if (soft_limit) { - temp->value.ul = soft_limit; - strncpy(temp->field, VIR_DOMAIN_MEMORY_SOFT_LIMIT, - sizeof(temp->field)); + if (virTypedParameterAssign(temp, + VIR_DOMAIN_MEMORY_SOFT_LIMIT, + VIR_TYPED_PARAM_ULLONG, + soft_limit) < 0) + goto error; soft_limit = 0; } else if (hard_limit) { - temp->value.ul = hard_limit; - strncpy(temp->field, VIR_DOMAIN_MEMORY_HARD_LIMIT, - sizeof(temp->field)); + if (virTypedParameterAssign(temp, + VIR_DOMAIN_MEMORY_HARD_LIMIT, + VIR_TYPED_PARAM_ULLONG, + hard_limit) < 0) + goto error; hard_limit = 0; } else if (swap_hard_limit) { - temp->value.ul = swap_hard_limit; - strncpy(temp->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, - sizeof(temp->field)); + if (virTypedParameterAssign(temp, + VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, + VIR_TYPED_PARAM_ULLONG, + swap_hard_limit) < 0) + goto error; swap_hard_limit = 0; } else if (min_guarantee) { - temp->value.ul = min_guarantee; - strncpy(temp->field, VIR_DOMAIN_MEMORY_MIN_GUARANTEE, - sizeof(temp->field)); + if (virTypedParameterAssign(temp, + VIR_DOMAIN_MEMORY_MIN_GUARANTEE, + VIR_TYPED_PARAM_ULLONG, + min_guarantee) < 0) + goto error; min_guarantee = 0; } @@ -6318,15 +6325,19 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) temp->value.ul = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; } if (virDomainSetMemoryParameters(dom, params, nparams, flags) != 0) - vshError(ctl, "%s", _("Unable to change memory parameters")); + goto error; else ret = true; } - cleanup: +cleanup: VIR_FREE(params); virDomainFree(dom); return ret; + +error: + vshError(ctl, "%s", _("Unable to change memory parameters")); + goto cleanup; } /* @@ -8106,7 +8117,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) unsigned long long total_bytes_sec = 0, read_bytes_sec = 0, write_bytes_sec = 0; unsigned long long total_iops_sec = 0, read_iops_sec = 0, write_iops_sec = 0; int nparams = 0; - virTypedParameterPtr params = NULL, temp = NULL; + virTypedParameterPtr params = NULL; unsigned int flags = 0, i = 0; int rv = 0; bool current = vshCommandOptBool(cmd, "current"); @@ -8224,64 +8235,50 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) params = vshCalloc(ctl, nparams, sizeof(*params)); i = 0; - if (i < nparams && vshCommandOptBool(cmd, "total-bytes-sec")) { - temp = ¶ms[i]; - temp->type = VIR_TYPED_PARAM_ULLONG; - strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, - sizeof(temp->field)); - temp->value.ul = total_bytes_sec; - i++; - } - - if (i < nparams && vshCommandOptBool(cmd, "read-bytes-sec")) { - temp = ¶ms[i]; - temp->type = VIR_TYPED_PARAM_ULLONG; - strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, - sizeof(temp->field)); - temp->value.ul = read_bytes_sec; - i++; - } - - if (i < nparams && vshCommandOptBool(cmd, "write-bytes-sec")) { - temp = ¶ms[i]; - temp->type = VIR_TYPED_PARAM_ULLONG; - strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, - sizeof(temp->field)); - temp->value.ul = write_bytes_sec; - i++; - } - - if (i < nparams && vshCommandOptBool(cmd, "total-iops-sec")) { - temp = ¶ms[i]; - temp->type = VIR_TYPED_PARAM_ULLONG; - strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, - sizeof(temp->field)); - temp->value.ul = total_iops_sec; - i++; - } - - if (i < nparams && vshCommandOptBool(cmd, "read-iops-sec")) { - temp = ¶ms[i]; - temp->type = VIR_TYPED_PARAM_ULLONG; - strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, - sizeof(temp->field)); - temp->value.ul = read_iops_sec; - i++; - } - - if (i < nparams && vshCommandOptBool(cmd, "write-iops-sec")) { - temp = ¶ms[i]; - temp->type = VIR_TYPED_PARAM_ULLONG; - strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, - sizeof(temp->field)); - temp->value.ul = write_iops_sec; - } - - if (virDomainSetBlockIoTune(dom, disk, params, nparams, flags) < 0) { - vshError(ctl, "%s", - _("Unable to change block I/O throttle")); - goto cleanup; - } + if (i < nparams && vshCommandOptBool(cmd, "total-bytes-sec") && + virTypedParameterAssign(¶ms[i++], + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, + VIR_TYPED_PARAM_ULLONG, + total_bytes_sec) < 0) + goto error; + + if (i < nparams && vshCommandOptBool(cmd, "read-bytes-sec") && + virTypedParameterAssign(¶ms[i++], + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, + VIR_TYPED_PARAM_ULLONG, + read_bytes_sec) < 0) + goto error; + + if (i < nparams && vshCommandOptBool(cmd, "write-bytes-sec") && + virTypedParameterAssign(¶ms[i++], + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, + VIR_TYPED_PARAM_ULLONG, + write_bytes_sec) < 0) + goto error; + + if (i < nparams && vshCommandOptBool(cmd, "total-iops-sec") && + virTypedParameterAssign(¶ms[i++], + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, + VIR_TYPED_PARAM_ULLONG, + total_iops_sec) < 0) + goto error; + + if (i < nparams && vshCommandOptBool(cmd, "read-iops-sec") && + virTypedParameterAssign(¶ms[i++], + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, + VIR_TYPED_PARAM_ULLONG, + read_iops_sec) < 0) + goto error; + + if (i < nparams && vshCommandOptBool(cmd, "write-iops-sec") && + virTypedParameterAssign(¶ms[i++], + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, + VIR_TYPED_PARAM_ULLONG, + write_iops_sec) < 0) + goto error; + + if (virDomainSetBlockIoTune(dom, disk, params, nparams, flags) < 0) + goto error; } ret = true; @@ -8290,6 +8287,10 @@ cleanup: VIR_FREE(params); virDomainFree(dom); return ret; + +error: + vshError(ctl, "%s", _("Unable to change block I/O throttle")); + goto cleanup; } /* -- 2.39.5