[PATCH 00/16] Improve virStrcpy and remove virStrncpy
Peter Krempa (16): util: virstring: Always copy string in virStrcpy virProcessRunInForkHelper: Use virStrcpyStatic for static buffers sanlock: Use virStrcpy instead of virStrncpy virDevMapperGetTargetsImpl: Use virStrcpy instead of virStrncpy virFileLoopDeviceAssociate: Use virStrcpy instead of virStrncpy xenParseXMDisk: Replace g_new + virStrncpy by g_strndup test_driver: Rewrite testBuildFilename xenParseVif: Refactor parser xenParseSxprSound: Refactor parsing of model list openvzReadNetworkConf: Rework parser xenParseXLChannel: Use g_strndup instead of virStrndup xenParseXLUSBController: Avoid use of virStrndup xenParseXLUSB: Rewrite to avoid virStrncpy virNetLibsshAuthenticatePrivkeyCb: Use g_autofree for 'actual_prompt' virNetLibsshAuthenticatePrivkeyCb: Use virStrcpy instead of virStrncpy util: virstring: Remove virStrncpy docs/coding-style.rst | 13 --- src/libvirt_private.syms | 1 - src/libxl/xen_common.c | 182 +++++++++++------------------- src/libxl/xen_xl.c | 84 ++++---------- src/libxl/xen_xm.c | 9 +- src/locking/lock_driver_sanlock.c | 4 +- src/openvz/openvz_conf.c | 77 +++++-------- src/rpc/virnetlibsshsession.c | 17 +-- src/test/test_driver.c | 35 +----- src/util/virdevmapper.c | 2 +- src/util/virfile.c | 3 +- src/util/virprocess.c | 8 +- src/util/virstring.c | 54 +-------- src/util/virstring.h | 5 +- 14 files changed, 137 insertions(+), 357 deletions(-) -- 2.29.2
15 out of 72 invocations of virStrcpy(Static) ignore the return value as it's either impossible to fail or in certain cases a truncated copy is still good enough. Unfortunately virStrcpy doesn't copy anything in such case as the checks are done first. Fix this by using g_strlcpy for the implementation and removing G_GNUC_WARN_UNUSED_RESULT from the function so that callers can decide when it's okay. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstring.c | 12 +++++++----- src/util/virstring.h | 3 +-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index c3e64007fe..a35cd8ba76 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -503,16 +503,18 @@ virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) * @src: source buffer * @destbytes: number of bytes the destination can accommodate * - * Copies @src to @dest. + * Copies @src to @dest. @dest is guaranteed to be 'nul' terminated if + * destbytes is 1 or more. * - * See virStrncpy() for more information. - * - * Returns: 0 on success, <0 on failure. + * Returns: 0 on success, -1 if @src doesn't fit into @dest and was truncated. */ int virStrcpy(char *dest, const char *src, size_t destbytes) { - return virStrncpy(dest, src, -1, destbytes); + if (g_strlcpy(dest, src, destbytes) >= destbytes) + return -1; + + return 0; } /** diff --git a/src/util/virstring.h b/src/util/virstring.h index 45aead1838..da1fe86ffc 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -99,8 +99,7 @@ bool virStringIsEmpty(const char *str); int virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) G_GNUC_WARN_UNUSED_RESULT; -int virStrcpy(char *dest, const char *src, size_t destbytes) - G_GNUC_WARN_UNUSED_RESULT; +int virStrcpy(char *dest, const char *src, size_t destbytes); #define virStrcpyStatic(dest, src) virStrcpy((dest), (src), sizeof(dest)) int virStringSortCompare(const void *a, const void *b); -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
15 out of 72 invocations of virStrcpy(Static)
That's almost 21%
ignore the return value as it's either impossible to fail or in certain cases a truncated copy is still good enough.Unfortunately virStrcpy doesn't copy anything in such case as the checks are done first.
Fix this by using g_strlcpy for the implementation and removing G_GNUC_WARN_UNUSED_RESULT from the function so that callers can decide when it's okay.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstring.c | 12 +++++++----- src/util/virstring.h | 3 +-- 2 files changed, 8 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virprocess.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 69d64e9466..7a0960e337 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1168,14 +1168,14 @@ virProcessRunInForkHelper(int errfd, bin->data.code = err->code; bin->data.domain = err->domain; - ignore_value(virStrcpy(bin->data.message, err->message, sizeof(bin->data.message))); + virStrcpyStatic(bin->data.message, err->message); bin->data.level = err->level; if (err->str1) - ignore_value(virStrcpy(bin->data.str1, err->str1, sizeof(bin->data.str1))); + virStrcpyStatic(bin->data.str1, err->str1); if (err->str2) - ignore_value(virStrcpy(bin->data.str2, err->str2, sizeof(bin->data.str2))); + virStrcpyStatic(bin->data.str2, err->str2); if (err->str3) - ignore_value(virStrcpy(bin->data.str3, err->str3, sizeof(bin->data.str3))); + virStrcpyStatic(bin->data.str3, err->str3); bin->data.int1 = err->int1; bin->data.int2 = err->int2; -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virprocess.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We want a (possibly truncated) copy of the full source string so virStrcpy is a better fit. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/locking/lock_driver_sanlock.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index aaffe30e6f..1d8f109375 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -924,9 +924,7 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, /* sanlock doesn't use owner_name for anything, so it's safe to take just * the first SANLK_NAME_LEN - 1 characters from vm_name */ - ignore_value(virStrncpy(opt->owner_name, priv->vm_name, - MIN(strlen(priv->vm_name), SANLK_NAME_LEN - 1), - SANLK_NAME_LEN)); + virStrcpy(opt->owner_name, priv->vm_name, SANLK_NAME_LEN); if (state && STRNEQ(state, "")) { if ((rv = sanlock_state_to_args((char *)state, -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
We want a (possibly truncated) copy of the full source string so virStrcpy is a better fit.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/locking/lock_driver_sanlock.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virStrncpy was called with -1 for lenght of the copied source which is equivalent to virStrcpy. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virdevmapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index d97334bf67..fcb11e954f 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -240,7 +240,7 @@ virDevMapperGetTargetsImpl(int controlFD, if (!(sanitizedPath = virDMSanitizepath(path))) return 0; - if (virStrncpy(dm.name, sanitizedPath, -1, DM_TABLE_DEPS) < 0) { + if (virStrcpy(dm.name, sanitizedPath, DM_TABLE_DEPS) < 0) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("Resolved device mapper name too long")); return -1; -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
virStrncpy was called with -1 for lenght of the copied source which is
length
equivalent to virStrcpy.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virdevmapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Passing 'strlen(src)' for length makes it equivalent to virStrcpy. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 841a0f4e00..29445fc0ff 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -810,8 +810,7 @@ int virFileLoopDeviceAssociate(const char *file, lo.lo_flags = LO_FLAGS_AUTOCLEAR; /* Set backing file name for LOOP_GET_STATUS64 queries */ - if (virStrncpy((char *) lo.lo_file_name, file, - strlen(file), LO_NAME_SIZE) < 0) { + if (virStrcpy((char *) lo.lo_file_name, file, LO_NAME_SIZE) < 0) { virReportSystemError(errno, _("Unable to set backing file %s"), file); goto cleanup; -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Passing 'strlen(src)' for length makes it equivalent to virStrcpy.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xm.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/libxl/xen_xm.c b/src/libxl/xen_xm.c index 3e81c9ef0d..cc24317a76 100644 --- a/src/libxl/xen_xm.c +++ b/src/libxl/xen_xm.c @@ -146,14 +146,7 @@ xenParseXMDisk(char *entry, int hvm) if (!(offset = strchr(head, ','))) goto error; - disk->dst = g_new0(char, (offset - head) + 1); - - if (virStrncpy(disk->dst, head, offset - head, - (offset - head) + 1) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Dest file %s too big for destination"), head); - goto error; - } + disk->dst = g_strndup(head, offset - head); head = offset + 1; /* Extract source driver type */ -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xm.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use glib functions to do the relative name lookup instead of manual assembly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/test/test_driver.c | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 71ab04aa1a..30a1959589 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -739,32 +739,14 @@ testDomainStartState(testDriverPtr privconn, static char *testBuildFilename(const char *relativeTo, const char *filename) { - char *offset; - int baseLen; - char *ret; + g_autofree char *basename = NULL; - if (!filename || filename[0] == '\0') - return NULL; - if (filename[0] == '/') { - ret = g_strdup(filename); - return ret; - } + if (g_path_is_absolute(filename)) + return g_strdup(filename); - offset = strrchr(relativeTo, '/'); - if ((baseLen = (offset-relativeTo+1))) { - char *absFile; - int totalLen = baseLen + strlen(filename) + 1; - absFile = g_new0(char, totalLen); - if (virStrncpy(absFile, relativeTo, baseLen, totalLen) < 0) { - VIR_FREE(absFile); - return NULL; - } - strcat(absFile, filename); - return absFile; - } else { - ret = g_strdup(filename); - return ret; - } + basename = g_path_get_dirname(relativeTo); + + return g_strdup_printf("%s/%s", basename, filename); } static xmlNodePtr @@ -777,11 +759,6 @@ testParseXMLDocFromFile(xmlNodePtr node, const char *file, const char *type) if ((relFile = virXMLPropString(node, "file"))) { absFile = testBuildFilename(file, relFile); - if (!absFile) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("resolving %s filename"), type); - return NULL; - } if (!(doc = virXMLParse(absFile, NULL, type))) goto error; -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Use glib functions to do the relative name lookup instead of manual assembly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/test/test_driver.c | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use g_strsplit to split the string and avoid use of stack'd strings. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_common.c | 136 ++++++++++++++--------------------------- 1 file changed, 46 insertions(+), 90 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 781483f496..09c74dcb53 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -1141,104 +1141,61 @@ xenParseVif(char *entry, const char *vif_typename) { virDomainNetDefPtr net = NULL; virDomainNetDefPtr ret = NULL; - char *script = NULL; - char model[10]; - char type[10]; - char ip[128]; - char mac[18]; - char bridge[50]; - char vifname[50]; - char rate[50]; - char *key; - - bridge[0] = '\0'; - mac[0] = '\0'; - ip[0] = '\0'; - model[0] = '\0'; - type[0] = '\0'; - vifname[0] = '\0'; - rate[0] = '\0'; - - key = entry; - while (key) { - char *data; - char *nextkey = strchr(key, ','); - - if (!(data = strchr(key, '='))) + g_auto(GStrv) keyvals = NULL; + GStrv keyval; + g_autofree char *script = NULL; + g_autofree char *model = NULL; + g_autofree char *type = NULL; + g_autofree char *ip = NULL; + g_autofree char *mac = NULL; + g_autofree char *bridge = NULL; + g_autofree char *vifname = NULL; + g_autofree char *rate = NULL; + + keyvals = g_strsplit(entry, ",", 0); + + for (keyval = keyvals; keyval && *keyval; keyval++) { + const char *key = *keyval; + char *val = strchr(key, '='); + + virSkipSpaces(&key); + + if (!val) return NULL; - data++; + + val++; if (STRPREFIX(key, "mac=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(mac, data, len, sizeof(mac)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("MAC address %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&mac, g_free); + mac = g_strdup(val); } else if (STRPREFIX(key, "bridge=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(bridge, data, len, sizeof(bridge)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Bridge %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&bridge, g_free); + bridge = g_strdup(val); } else if (STRPREFIX(key, "script=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - VIR_FREE(script); - script = g_strndup(data, len); + g_clear_pointer(&script, g_free); + script = g_strdup(val); } else if (STRPREFIX(key, "model=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(model, data, len, sizeof(model)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Model %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&model, g_free); + model = g_strdup(val); } else if (STRPREFIX(key, "type=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(type, data, len, sizeof(type)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Type %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&type, g_free); + type = g_strdup(val); } else if (STRPREFIX(key, "vifname=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(vifname, data, len, sizeof(vifname)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Vifname %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&vifname, g_free); + vifname = g_strdup(val); } else if (STRPREFIX(key, "ip=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(ip, data, len, sizeof(ip)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("IP %s too big for destination"), data); - return NULL; - } + g_clear_pointer(&ip, g_free); + ip = g_strdup(val); } else if (STRPREFIX(key, "rate=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(rate, data, len, sizeof(rate)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("rate %s too big for destination"), data); - return NULL; - } + g_clear_pointer(&rate, g_free); + rate = g_strdup(val); } - - while (nextkey && (nextkey[0] == ',' || - nextkey[0] == ' ' || - nextkey[0] == '\t')) - nextkey++; - key = nextkey; } if (!(net = virDomainNetDefNew(NULL))) goto cleanup; - if (mac[0]) { + if (mac) { if (virMacAddrParse(mac, &net->mac) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("malformed mac address '%s'"), mac); @@ -1246,18 +1203,18 @@ xenParseVif(char *entry, const char *vif_typename) } } - if (bridge[0] || STREQ_NULLABLE(script, "vif-bridge") || + if (bridge || STREQ_NULLABLE(script, "vif-bridge") || STREQ_NULLABLE(script, "vif-vnic")) { net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; } else { net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; } - if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE && bridge[0]) { + if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE && bridge) { if (xenParseVifBridge(net, bridge) < 0) goto cleanup; } - if (ip[0]) { + if (ip) { char **ip_list = g_strsplit(ip, " ", 0); size_t i; @@ -1276,18 +1233,18 @@ xenParseVif(char *entry, const char *vif_typename) if (script && script[0]) net->script = g_strdup(script); - if (model[0]) { + if (model) { if (virDomainNetSetModelString(net, model) < 0) goto cleanup; } else { - if (type[0] && STREQ(type, vif_typename)) + if (type && STREQ(type, vif_typename)) net->model = VIR_DOMAIN_NET_MODEL_NETFRONT; } - if (vifname[0]) + if (vifname && vifname[0]) net->ifname = g_strdup(vifname); - if (rate[0]) { + if (rate) { virNetDevBandwidthPtr bandwidth; unsigned long long kbytes_per_sec; @@ -1304,7 +1261,6 @@ xenParseVif(char *entry, const char *vif_typename) cleanup: virDomainNetDefFree(net); - VIR_FREE(script); return ret; } -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Use g_strsplit to split the string and avoid use of stack'd strings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_common.c | 136 ++++++++++++++--------------------------- 1 file changed, 46 insertions(+), 90 deletions(-)
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 781483f496..09c74dcb53 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -1141,104 +1141,61 @@ xenParseVif(char *entry, const char *vif_typename) { virDomainNetDefPtr net = NULL; virDomainNetDefPtr ret = NULL; - char *script = NULL; - char model[10]; - char type[10]; - char ip[128]; - char mac[18]; - char bridge[50]; - char vifname[50]; - char rate[50]; - char *key; - - bridge[0] = '\0'; - mac[0] = '\0'; - ip[0] = '\0'; - model[0] = '\0'; - type[0] = '\0'; - vifname[0] = '\0'; - rate[0] = '\0'; - - key = entry; - while (key) { - char *data; - char *nextkey = strchr(key, ','); - - if (!(data = strchr(key, '='))) + g_auto(GStrv) keyvals = NULL; + GStrv keyval; + g_autofree char *script = NULL; + g_autofree char *model = NULL; + g_autofree char *type = NULL; + g_autofree char *ip = NULL; + g_autofree char *mac = NULL; + g_autofree char *bridge = NULL; + g_autofree char *vifname = NULL; + g_autofree char *rate = NULL; +
These can be marked as const instead of autofree. Their values are processed before 'keyvals' gets freed at the end of the function. It would result in less lines and no g_free/g_autofree mixing. Jano
e keyvals = g_strsplit(entry, ",", 0); + + for (keyval = keyvals; keyval && *keyval; keyval++) { + const char *key = *keyval; + char *val = strchr(key, '='); + + virSkipSpaces(&key); + + if (!val) return NULL; - data++; + + val++;
if (STRPREFIX(key, "mac=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(mac, data, len, sizeof(mac)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("MAC address %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&mac, g_free); + mac = g_strdup(val); } else if (STRPREFIX(key, "bridge=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(bridge, data, len, sizeof(bridge)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Bridge %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&bridge, g_free); + bridge = g_strdup(val); } else if (STRPREFIX(key, "script=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - VIR_FREE(script); - script = g_strndup(data, len); + g_clear_pointer(&script, g_free); + script = g_strdup(val); } else if (STRPREFIX(key, "model=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(model, data, len, sizeof(model)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Model %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&model, g_free); + model = g_strdup(val); } else if (STRPREFIX(key, "type=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(type, data, len, sizeof(type)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Type %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&type, g_free); + type = g_strdup(val); } else if (STRPREFIX(key, "vifname=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(vifname, data, len, sizeof(vifname)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Vifname %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&vifname, g_free); + vifname = g_strdup(val); } else if (STRPREFIX(key, "ip=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(ip, data, len, sizeof(ip)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("IP %s too big for destination"), data); - return NULL; - } + g_clear_pointer(&ip, g_free); + ip = g_strdup(val); } else if (STRPREFIX(key, "rate=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(rate, data, len, sizeof(rate)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("rate %s too big for destination"), data); - return NULL; - } + g_clear_pointer(&rate, g_free); + rate = g_strdup(val); } - - while (nextkey && (nextkey[0] == ',' || - nextkey[0] == ' ' || - nextkey[0] == '\t')) - nextkey++; - key = nextkey; }
if (!(net = virDomainNetDefNew(NULL))) goto cleanup;
- if (mac[0]) { + if (mac) { if (virMacAddrParse(mac, &net->mac) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("malformed mac address '%s'"), mac); @@ -1246,18 +1203,18 @@ xenParseVif(char *entry, const char *vif_typename) } }
- if (bridge[0] || STREQ_NULLABLE(script, "vif-bridge") || + if (bridge || STREQ_NULLABLE(script, "vif-bridge") || STREQ_NULLABLE(script, "vif-vnic")) { net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; } else { net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; }
- if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE && bridge[0]) { + if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE && bridge) { if (xenParseVifBridge(net, bridge) < 0) goto cleanup; } - if (ip[0]) { + if (ip) { char **ip_list = g_strsplit(ip, " ", 0); size_t i;
@@ -1276,18 +1233,18 @@ xenParseVif(char *entry, const char *vif_typename) if (script && script[0]) net->script = g_strdup(script);
- if (model[0]) { + if (model) { if (virDomainNetSetModelString(net, model) < 0) goto cleanup; } else { - if (type[0] && STREQ(type, vif_typename)) + if (type && STREQ(type, vif_typename)) net->model = VIR_DOMAIN_NET_MODEL_NETFRONT; }
- if (vifname[0]) + if (vifname && vifname[0]) net->ifname = g_strdup(vifname);
- if (rate[0]) { + if (rate) { virNetDevBandwidthPtr bandwidth; unsigned long long kbytes_per_sec;
@@ -1304,7 +1261,6 @@ xenParseVif(char *entry, const char *vif_typename)
cleanup: virDomainNetDefFree(net); - VIR_FREE(script); return ret; }
-- 2.29.2
Copy the input string so that we don't have to use a static buffer and virStrncpy. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_common.c | 46 +++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 09c74dcb53..1c71f69209 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -1332,38 +1332,30 @@ xenParseSxprSound(virDomainDefPtr def, def->sounds[def->nsounds++] = sound; } } else { - char model[10]; - const char *offset = str, *offset2; - - do { - int len; - virDomainSoundDefPtr sound; - offset2 = strchr(offset, ','); - if (offset2) - len = (offset2 - offset); - else - len = strlen(offset); - if (virStrncpy(model, offset, len, sizeof(model)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Sound model %s too big for destination"), - offset); - return -1; - } + g_autofree char *sounds = g_strdup(str); + char *sound = sounds; + int model; - sound = g_new0(virDomainSoundDef, 1); + while (*sound != '\0') { + char *next = strchr(sound, ','); + virDomainSoundDefPtr snddef; - if ((sound->model = virDomainSoundModelTypeFromString(model)) < 0) { - VIR_FREE(sound); - return -1; - } + if (next) + *next = '\0'; - if (VIR_APPEND_ELEMENT(def->sounds, def->nsounds, sound) < 0) { - virDomainSoundDefFree(sound); + if ((model = virDomainSoundModelTypeFromString(sound)) < 0) return -1; - } - offset = offset2 ? offset2 + 1 : NULL; - } while (offset); + snddef = g_new0(virDomainSoundDef, 1); + snddef->model = model; + + ignore_value(VIR_APPEND_ELEMENT(def->sounds, def->nsounds, snddef)); + + if (!next) + break; + + sound = next + 1; + } } return 0; -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Copy the input string so that we don't have to use a static buffer and virStrncpy.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_common.c | 46 +++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 27 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Rewrite so that the parser doesn't use virStrncpy by employing g_strsplit. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/openvz/openvz_conf.c | 77 ++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 51 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 041a031a3a..836885af18 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -230,82 +230,57 @@ openvzReadNetworkConf(virDomainDefPtr def, veid); goto error; } else if (ret > 0) { - token = strtok_r(temp, ";", &saveptr); - while (token != NULL) { - char *p = token; - char cpy_temp[32]; - int len; + g_auto(GStrv) devices = g_strsplit(temp, ";", 0); + GStrv device; - /* add new device to list */ - net = g_new0(virDomainNetDef, 1); + for (device = devices; device && *device; device++) { + g_auto(GStrv) keyvals = g_strsplit(*device, ",", 0); + GStrv keyval; + net = g_new0(virDomainNetDef, 1); net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; - /* parse string */ - do { - char *next = strchr(p, ','); - if (!next) - next = strchr(p, '\0'); - if (STRPREFIX(p, "ifname=")) { + for (keyval = keyvals; keyval && *keyval; keyval++) { + char *val = strchr(*keyval, '='); + + if (!val) + continue; + + val++; + + if (STRPREFIX(*keyval, "ifname=")) { /* skip in libvirt */ - } else if (STRPREFIX(p, "host_ifname=")) { - p += 12; - len = next - p; - if (len > 16) { + } else if (STRPREFIX(*keyval, "host_ifname=")) { + if (strlen(val) > 16) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Too long network device name")); goto error; } - net->ifname = g_new0(char, len+1); - - if (virStrncpy(net->ifname, p, len, len+1) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network ifname %s too long for destination"), p); - goto error; - } - } else if (STRPREFIX(p, "bridge=")) { - p += 7; - len = next - p; - if (len > 16) { + net->ifname = g_strdup(val); + } else if (STRPREFIX(*keyval, "bridge=")) { + if (strlen(val) > 16) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Too long bridge device name")); goto error; } - net->data.bridge.brname = g_new0(char, len+1); - - if (virStrncpy(net->data.bridge.brname, p, len, len+1) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Bridge name %s too long for destination"), p); - goto error; - } - } else if (STRPREFIX(p, "mac=")) { - p += 4; - len = next - p; - if (len != 17) { /* should be 17 */ + net->data.bridge.brname = g_strdup(val); + } else if (STRPREFIX(*keyval, "mac=")) { + if (strlen(val) != 17) { /* should be 17 */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Wrong length MAC address")); goto error; } - if (virStrncpy(cpy_temp, p, len, sizeof(cpy_temp)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("MAC address %s too long for destination"), p); - goto error; - } - if (virMacAddrParse(cpy_temp, &net->mac) < 0) { + if (virMacAddrParse(val, &net->mac) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Wrong MAC address")); goto error; } } - p = ++next; - } while (p < token + strlen(token)); - - if (VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, net) < 0) - goto error; + } - token = strtok_r(NULL, ";", &saveptr); + ignore_value(VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, net)); } } -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Rewrite so that the parser doesn't use virStrncpy by employing g_strsplit.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/openvz/openvz_conf.c | 77 ++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 51 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 041a031a3a..836885af18 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -230,82 +230,57 @@ openvzReadNetworkConf(virDomainDefPtr def, veid); goto error; } else if (ret > 0) { - token = strtok_r(temp, ";", &saveptr); - while (token != NULL) { - char *p = token; - char cpy_temp[32]; - int len; + g_auto(GStrv) devices = g_strsplit(temp, ";", 0); + GStrv device;
- /* add new device to list */ - net = g_new0(virDomainNetDef, 1); + for (device = devices; device && *device; device++) { + g_auto(GStrv) keyvals = g_strsplit(*device, ",", 0); + GStrv keyval;
+ net = g_new0(virDomainNetDef, 1); net->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
- /* parse string */ - do { - char *next = strchr(p, ','); - if (!next) - next = strchr(p, '\0'); - if (STRPREFIX(p, "ifname=")) { + for (keyval = keyvals; keyval && *keyval; keyval++) { + char *val = strchr(*keyval, '='); + + if (!val) + continue; + + val++; + + if (STRPREFIX(*keyval, "ifname=")) { /* skip in libvirt */ - } else if (STRPREFIX(p, "host_ifname=")) { - p += 12; - len = next - p; - if (len > 16) { + } else if (STRPREFIX(*keyval, "host_ifname=")) { + if (strlen(val) > 16) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Too long network device name")); goto error; }
- net->ifname = g_new0(char, len+1); - - if (virStrncpy(net->ifname, p, len, len+1) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network ifname %s too long for destination"), p); - goto error; - } - } else if (STRPREFIX(p, "bridge=")) { - p += 7; - len = next - p; - if (len > 16) { + net->ifname = g_strdup(val); + } else if (STRPREFIX(*keyval, "bridge=")) { + if (strlen(val) > 16) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Too long bridge device name")); goto error; }
- net->data.bridge.brname = g_new0(char, len+1); - - if (virStrncpy(net->data.bridge.brname, p, len, len+1) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Bridge name %s too long for destination"), p); - goto error; - } - } else if (STRPREFIX(p, "mac=")) { - p += 4; - len = next - p; - if (len != 17) { /* should be 17 */ + net->data.bridge.brname = g_strdup(val); + } else if (STRPREFIX(*keyval, "mac=")) { + if (strlen(val) != 17) { /* should be 17 */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Wrong length MAC address")); goto error; } - if (virStrncpy(cpy_temp, p, len, sizeof(cpy_temp)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("MAC address %s too long for destination"), p); - goto error; - } - if (virMacAddrParse(cpy_temp, &net->mac) < 0) { + if (virMacAddrParse(val, &net->mac) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Wrong MAC address")); goto error; } } - p = ++next; - } while (p < token + strlen(token)); - - if (VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, net) < 0) - goto error; + }
- token = strtok_r(NULL, ";", &saveptr); + ignore_value(VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, net));
Was this ignore_value needed because of some compiler warning? Even though it only returns 0 now, I'd rather not add ignore_value anywhere unless necessary. (Looking at viralloc.h, the INPLACE macro version which do do any allocation already contain ignore_value) Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
} }
-- 2.29.2
Make the temporary string a autofree-ing pointer and copy the contents. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xl.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 4113be8cd1..9b2a2fe292 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -1070,7 +1070,7 @@ xenParseXLChannel(virConfPtr conf, virDomainDefPtr def) if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { - char type[10]; + g_autofree char *type = NULL; char *key; if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) @@ -1087,11 +1087,8 @@ xenParseXLChannel(virConfPtr conf, virDomainDefPtr def) if (STRPREFIX(key, "connection=")) { int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(type, data, len, sizeof(type)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("connection %s too big"), data); - goto skipchannel; - } + g_clear_pointer(&type, g_free); + type = g_strndup(data, len); } else if (STRPREFIX(key, "name=")) { int len = nextkey ? (nextkey - data) : strlen(data); VIR_FREE(name); -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Make the temporary string a autofree-ing pointer and copy the contents.
an
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xl.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
s/virStrndup/virStrncpy/ On a Wednesday in 2021, Ján Tomko wrote:
On a Tuesday in 2021, Peter Krempa wrote:
Make the temporary string a autofree-ing pointer and copy the contents.
an
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xl.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
Use g_strndup with a freed buffer instead of the more complex approach using virStrndup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xl.c | 51 ++++++++++------------------------------------ 1 file changed, 11 insertions(+), 40 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 9b2a2fe292..1bce31a549 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -885,16 +885,11 @@ xenParseXLUSBController(virConfPtr conf, virDomainDefPtr def) if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { - char type[8]; - char version[4]; - char ports[4]; char *key; int usbctrl_version = 2; /* by default USB 2.0 */ int usbctrl_ports = 8; /* by default 8 ports */ int usbctrl_type = -1; - type[0] = version[0] = ports[0] = '\0'; - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) goto skipusbctrl; /* usbctrl=['type=pv,version=2,ports=8'] */ @@ -908,32 +903,19 @@ xenParseXLUSBController(virConfPtr conf, virDomainDefPtr def) data++; if (STRPREFIX(key, "type=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(type, data, len, sizeof(type)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("type %s invalid"), - data); + if (!STRPREFIX(data, "qusb")) goto skipusbctrl; - } } else if (STRPREFIX(key, "version=")) { int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(version, data, len, sizeof(version)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("version %s invalid"), - data); - goto skipusbctrl; - } - if (virStrToLong_i(version, NULL, 16, &usbctrl_version) < 0) + g_autofree char *tmp = g_strndup(data, len); + + if (virStrToLong_i(tmp, NULL, 16, &usbctrl_version) < 0) goto skipusbctrl; } else if (STRPREFIX(key, "ports=")) { int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(ports, data, len, sizeof(ports)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("version %s invalid"), - data); - goto skipusbctrl; - } - if (virStrToLong_i(ports, NULL, 16, &usbctrl_ports) < 0) + g_autofree char *tmp = g_strndup(data, len); + + if (virStrToLong_i(tmp, NULL, 16, &usbctrl_ports) < 0) goto skipusbctrl; } @@ -944,21 +926,10 @@ xenParseXLUSBController(virConfPtr conf, virDomainDefPtr def) key = nextkey; } - if (type[0] == '\0') { - if (usbctrl_version == 1) - usbctrl_type = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1; - else - usbctrl_type = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2; - } else { - if (STRPREFIX(type, "qusb")) { - if (usbctrl_version == 1) - usbctrl_type = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1; - else - usbctrl_type = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2; - } else { - goto skipusbctrl; - } - } + if (usbctrl_version == 1) + usbctrl_type = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1; + else + usbctrl_type = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2; if (!(controller = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB))) return -1; -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Use g_strndup with a freed buffer instead of the more complex approach using virStrndup.
s/virStrndup/virStrncpy/ here and in the commit summary.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xl.c | 51 ++++++++++------------------------------------ 1 file changed, 11 insertions(+), 40 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xl.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 1bce31a549..29c145851e 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -960,14 +960,10 @@ xenParseXLUSB(virConfPtr conf, virDomainDefPtr def) if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { - char bus[3]; - char device[3]; char *key; int busNum; int devNum; - bus[0] = device[0] = '\0'; - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) goto skipusb; /* usbdev=['hostbus=1,hostaddr=3'] */ @@ -982,20 +978,16 @@ xenParseXLUSB(virConfPtr conf, virDomainDefPtr def) if (STRPREFIX(key, "hostbus=")) { int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(bus, data, len, sizeof(bus)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("bus %s too big for destination"), - data); + g_autofree char *tmp = g_strndup(data, len); + + if (virStrToLong_i(tmp, NULL, 16, &busNum) < 0) goto skipusb; - } } else if (STRPREFIX(key, "hostaddr=")) { int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(device, data, len, sizeof(device)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("device %s too big for destination"), - data); + g_autofree char *tmp = g_strndup(data, len); + + if (virStrToLong_i(tmp, NULL, 16, &devNum) < 0) goto skipusb; - } } while (nextkey && (nextkey[0] == ',' || @@ -1005,10 +997,6 @@ xenParseXLUSB(virConfPtr conf, virDomainDefPtr def) key = nextkey; } - if (virStrToLong_i(bus, NULL, 16, &busNum) < 0) - goto skipusb; - if (virStrToLong_i(device, NULL, 16, &devNum) < 0) - goto skipusb; if (!(hostdev = virDomainHostdevDefNew())) return -1; -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xl.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
So that the 'error' label can be removed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 8814487557..0fc1f66706 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -406,7 +406,7 @@ virNetLibsshAuthenticatePrivkeyCb(const char *prompt, virNetLibsshSessionPtr sess = userdata; virConnectCredential retr_passphrase; int cred_type; - char *actual_prompt = NULL; + g_autofree char *actual_prompt = NULL; int p; /* request user's key password */ @@ -421,7 +421,7 @@ virNetLibsshAuthenticatePrivkeyCb(const char *prompt, if (cred_type == -1) { virReportError(VIR_ERR_LIBSSH, "%s", _("no suitable callback for input of key passphrase")); - goto error; + return -1; } actual_prompt = g_strndup(prompt, virLengthForPromptString(prompt)); @@ -434,7 +434,7 @@ virNetLibsshAuthenticatePrivkeyCb(const char *prompt, virReportError(VIR_ERR_LIBSSH, "%s", _("failed to retrieve private key passphrase: " "callback has failed")); - goto error; + return -1; } p = virStrncpy(buf, retr_passphrase.result, @@ -444,16 +444,10 @@ virNetLibsshAuthenticatePrivkeyCb(const char *prompt, if (p < 0) { virReportError(VIR_ERR_LIBSSH, "%s", _("passphrase is too long for the buffer")); - goto error; + return -1; } - VIR_FREE(actual_prompt); - return 0; - - error: - VIR_FREE(actual_prompt); - return -1; } static int -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
So that the 'error' label can be removed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We already assume that 'retr_passphrase.result' is a string, thus we can use virStrcpy instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 0fc1f66706..2dc8cc8911 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -437,8 +437,7 @@ virNetLibsshAuthenticatePrivkeyCb(const char *prompt, return -1; } - p = virStrncpy(buf, retr_passphrase.result, - retr_passphrase.resultlen, len); + p = virStrcpy(buf, retr_passphrase.result, len); virSecureEraseString(retr_passphrase.result); g_free(retr_passphrase.result); if (p < 0) { -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
We already assume that 'retr_passphrase.result' is a string, thus we can use virStrcpy instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function is now unused and motivated users to write crazy parsers which were hard to understand, had pointless error paths just to avoid few memory allocations. Remove the function as we're fine with g_strndup and virStrcpy. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/coding-style.rst | 13 ------------ src/libvirt_private.syms | 1 - src/util/virstring.c | 44 ---------------------------------------- src/util/virstring.h | 2 -- 4 files changed, 60 deletions(-) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 55dfa196a2..c43d20c7b2 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -714,19 +714,6 @@ does **not** guarantee a NULL-terminated buffer, which makes it extremely dangerous to use. Instead, use one of the replacement functions provided by libvirt: -:: - - virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) - -The first two arguments have the same meaning as for strncpy, -namely the destination and source of the copy operation. Unlike -strncpy, the function will always copy exactly the number of bytes -requested and make sure the destination is NULL-terminated, as the -source is required to be; sanity checks are performed to ensure -the size of the destination, as specified by the last argument, is -sufficient for the operation to succeed. On success, 0 is -returned; on failure, a value <0 is returned instead. - :: virStrcpy(char *dest, const char *src, size_t destbytes) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2a3bbdc577..eb121032d5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3264,7 +3264,6 @@ virStringStripIPv6Brackets; virStringStripSuffix; virStringToUpper; virStringTrimOptionalNewline; -virStrncpy; virStrToDouble; virStrToLong_i; virStrToLong_l; diff --git a/src/util/virstring.c b/src/util/virstring.c index a35cd8ba76..1d6141b5c1 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -452,50 +452,6 @@ virDoubleToStr(char **strp, double number) } -/** - * virStrncpy: - * - * @dest: destination buffer - * @src: source buffer - * @n: number of bytes to copy - * @destbytes: number of bytes the destination can accommodate - * - * Copies the first @n bytes of @src to @dest. - * - * @src must be NULL-terminated; if successful, @dest is guaranteed to - * be NULL-terminated as well. - * - * @n must be a reasonable value, that is, it must not exceed either - * the length of @src or the size of @dest. For the latter constraint, - * the fact that @dest needs to accommodate a NULL byte in addition to - * the bytes copied from @src must be taken into account. - * - * If you want to copy *all* of @src to @dest, use virStrcpy() or - * virStrcpyStatic() instead. - * - * Returns: 0 on success, <0 on failure. - */ -int -virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) -{ - size_t src_len = strlen(src); - - /* As a special case, -1 means "copy the entire string". - * - * This is to avoid calling strlen() twice, once in the virStrcpy() - * wrapper and once here for bound checking purposes. */ - if (n == -1) - n = src_len; - - if (n > src_len || n > (destbytes - 1)) - return -1; - - memcpy(dest, src, n); - dest[n] = '\0'; - - return 0; -} - /** * virStrcpy: * diff --git a/src/util/virstring.h b/src/util/virstring.h index da1fe86ffc..e688495574 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -97,8 +97,6 @@ void virSkipSpacesBackwards(const char *str, char **endp) bool virStringIsEmpty(const char *str); -int virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) - G_GNUC_WARN_UNUSED_RESULT; int virStrcpy(char *dest, const char *src, size_t destbytes); #define virStrcpyStatic(dest, src) virStrcpy((dest), (src), sizeof(dest)) -- 2.29.2
On a Tuesday in 2021, Peter Krempa wrote:
The function is now unused and motivated users to write crazy parsers which were hard to understand, had pointless error paths just to avoid few memory allocations.
Remove the function as we're fine with g_strndup and virStrcpy.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/coding-style.rst | 13 ------------ src/libvirt_private.syms | 1 - src/util/virstring.c | 44 ---------------------------------------- src/util/virstring.h | 2 -- 4 files changed, 60 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa