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.

diff --git a/cfg.mk b/cfg.mk
index daaa867960e15076cbe34e1968bded784804b09d..37f51a6858123f9338044c11399189023fbd4029 100644 (file)
--- 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 d273c25bf9fafe33b67627c501086bec1bc249ab..5226bd8515603b9c631fd4b1650660615927efc5 100644 (file)
--- 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 = &params[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 = &params[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 = &params[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 = &params[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 = &params[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 = &params[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 = &params[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(&params[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(&params[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(&params[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(&params[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(&params[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(&params[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;
 }
 
 /*