[libvirt] [PATCH 1/2] virStrndup: Accept negative values as string length
It may shorten the code a bit as the following pattern: VIR_STRNDUP(dst, src, cond ? n : strlen(src)) is used on several places among our code. However, we can move the strlen into virStrndup and thus write just: VIR_STRNDUP(dst, src, cond ? n : -1) --- src/util/virstring.c | 7 ++++++- src/util/virstring.h | 11 ++++++++--- tests/virstringtest.c | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index fcbb375..b244e6c 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -568,12 +568,15 @@ virStrdup(char **dest, * caller's body where virStrndup is called from. Consider * using VIR_STRNDUP which sets these automatically. * + * In case @n is smaller than zero, the whole @src string is + * copied. + * * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise. */ int virStrndup(char **dest, const char *src, - size_t n, + ssize_t n, bool report, int domcode, const char *filename, @@ -582,6 +585,8 @@ virStrndup(char **dest, { if (!src) return 0; + if (n < 0) + n = strlen(src); if (!(*dest = strndup(src, n))) { if (report) virReportOOMErrorFull(domcode, filename, funcname, linenr); diff --git a/src/util/virstring.h b/src/util/virstring.h index 534ce91..7063fe4 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -93,7 +93,7 @@ int virStrdup(char **dest, const char *src, bool report, int domcode, const char *filename, const char *funcname, size_t linenr) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); -int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, +int virStrndup(char **dest, const char *src, ssize_t n, bool report, int domcode, const char *filename, const char *funcname, size_t linenr) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); @@ -132,7 +132,9 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * @n: the maximum number of bytes to copy * * Duplicate @src string and store it into @dst. If @src is longer than @n, - * only @n bytes are copied and terminating null byte '\0' is added. + * only @n bytes are copied and terminating null byte '\0' is added. If @n + * is a negative number, then the whole @src string is copied. That is + * VIR_STRDUP(dst, src) and VIR_STRNDUP(dst, src, -1) are equal. * * This macro is safe to use on arguments with side effects. * @@ -150,7 +152,10 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * @n: the maximum number of bytes to copy * * Duplicate @src string and store it into @dst. If @src is longer than @n, - * only @n bytes are copied and terminating null byte '\0' is added. + * only @n bytes are copied and terminating null byte '\0' is added. If @n + * is a negative number, then the whole @src string is copied. That is + * VIR_STRDUP_QUIET(dst, src) and VIR_STRNDUP_QUIET(dst, src, -1) are + * equal. * * This macro is safe to use on arguments with side effects. * diff --git a/tests/virstringtest.c b/tests/virstringtest.c index da06c0f..3d0b55b 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -196,6 +196,39 @@ cleanup: return ret; } +static int +testStrndupNegative(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + char *dst; + const char *src = "Hello world"; + int value; + + if ((value = VIR_STRNDUP(dst, src, 5)) != 1) { + fprintf(stderr, "unexpected virStrndup result %d, expected 1\n", value); + goto cleanup; + } + + if (STRNEQ_NULLABLE(dst, "Hello")) { + fprintf(stderr, "unexpected content '%s'", dst); + goto cleanup; + } + + if ((value = VIR_STRNDUP(dst, src, -1)) != 1) { + fprintf(stderr, "unexpected virStrndup result %d, expected 1\n", value); + goto cleanup; + } + + if (STRNEQ_NULLABLE(dst, src)) { + fprintf(stderr, "unexpected content '%s'", dst); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(dst); + return ret; +} static int mymain(void) @@ -245,6 +278,9 @@ mymain(void) if (virtTestRun("strdup", 1, testStrdup, NULL) < 0) ret = -1; + if (virtTestRun("strdup", 1, testStrndupNegative, NULL) < 0) + ret = -1; + return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.8.2.1
With previous patch, we accept negative value as length of string to duplicate. So there is no need to pass strlen(src) in case we want to do duplicate the whole string. --- src/conf/domain_conf.c | 6 ++---- src/qemu/qemu_command.c | 14 ++++++-------- src/util/virsexpr.c | 2 +- src/xenxs/xen_sxpr.c | 4 ++-- 4 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ad5550c..a9656af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17370,8 +17370,7 @@ virDomainGraphicsListenSetAddress(virDomainGraphicsDefPtr def, return 0; } - if (VIR_STRNDUP(listenInfo->address, address, - len == -1 ? strlen(address) : len) < 0) + if (VIR_STRNDUP(listenInfo->address, address, len) < 0) return -1; return 0; } @@ -17409,8 +17408,7 @@ virDomainGraphicsListenSetNetwork(virDomainGraphicsDefPtr def, return 0; } - if (VIR_STRNDUP(listenInfo->network, network, - len == -1 ? strlen(network) : len) < 0) + if (VIR_STRNDUP(listenInfo->network, network, len) < 0) return -1; return 0; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 434f5a7..0373626 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8674,7 +8674,7 @@ static int qemuStringToArgvEnv(const char *args, if (!next) next = strchr(curr, '\n'); - if (VIR_STRNDUP(arg, curr, next ? next - curr : strlen(curr)) < 0) + if (VIR_STRNDUP(arg, curr, next ? next - curr : -1) < 0) goto error; if (next && (*next == '\'' || *next == '"')) @@ -9566,16 +9566,14 @@ qemuParseCommandLineChr(virDomainChrSourceDefPtr source, if (VIR_STRNDUP(source->data.tcp.host, val, svc - val) < 0) goto error; svc++; - if (VIR_STRNDUP(source->data.tcp.service, svc, - opt ? opt - svc : strlen(svc)) < 0) + if (VIR_STRNDUP(source->data.tcp.service, svc, opt ? opt - svc : -1) < 0) goto error; } else if (STRPREFIX(val, "unix:")) { const char *opt; val += strlen("unix:"); opt = strchr(val, ','); source->type = VIR_DOMAIN_CHR_TYPE_UNIX; - if (VIR_STRNDUP(source->data.nix.path, val, - opt ? opt - val : strlen(val)) < 0) + if (VIR_STRNDUP(source->data.nix.path, val, opt ? opt - val : -1) < 0) goto error; } else if (STRPREFIX(val, "/dev")) { @@ -9634,7 +9632,7 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, next++; if (p == val) { - if (VIR_STRNDUP(model, p, next ? next - p - 1 : strlen(p)) < 0) + if (VIR_STRNDUP(model, p, next ? next - p - 1 : -1) < 0) goto error; if (!STREQ(model, "qemu32") && !STREQ(model, "qemu64")) { @@ -9658,7 +9656,7 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, if (*p == '\0' || *p == ',') goto syntax; - if (VIR_STRNDUP(feature, p, next ? next - p - 1 : strlen(p)) < 0) + if (VIR_STRNDUP(feature, p, next ? next - p - 1 : -1) < 0) goto error; if (STREQ(feature, "kvmclock")) { @@ -9717,7 +9715,7 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, if (*p == '\0' || *p == ',') goto syntax; - if (VIR_STRNDUP(feature, p, next ? next - p - 1 : strlen(p)) < 0) + if (VIR_STRNDUP(feature, p, next ? next - p - 1 : -1) < 0) goto error; dom->features |= (1 << VIR_DOMAIN_FEATURE_HYPERV); diff --git a/src/util/virsexpr.c b/src/util/virsexpr.c index c75dfd9..7db215a 100644 --- a/src/util/virsexpr.c +++ b/src/util/virsexpr.c @@ -120,7 +120,7 @@ sexpr_string(const char *str, ssize_t len) return ret; ret->kind = SEXPR_VALUE; - if (VIR_STRNDUP(ret->u.value, str, len > 0 ? len : strlen(str)) < 0) + if (VIR_STRNDUP(ret->u.value, str, len) < 0) VIR_FREE(ret); return ret; diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index c17aa5d..394b814 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -242,7 +242,7 @@ xenParseSxprChar(const char *value, offset2 = strchr(offset, ','); offset++; if (VIR_STRNDUP(def->source.data.tcp.service, offset, - offset2 ? offset2 - offset : strlen(offset)) < 0) + offset2 ? offset2 - offset : -1) < 0) goto error; if (offset2 && strstr(offset2, ",server")) @@ -296,7 +296,7 @@ xenParseSxprChar(const char *value, { const char *offset = strchr(value, ','); if (VIR_STRNDUP(def->source.data.nix.path, value, - offset ? offset - value : strlen(value)) < 0) + offset ? offset - value : -1) < 0) goto error; if (offset != NULL && -- 1.8.2.1
On 24.05.2013 11:53, Michal Privoznik wrote:
With previous patch, we accept negative value as length of string to duplicate. So there is no need to pass strlen(src) in case we want to do duplicate the whole string. --- src/conf/domain_conf.c | 6 ++---- src/qemu/qemu_command.c | 14 ++++++-------- src/util/virsexpr.c | 2 +- src/xenxs/xen_sxpr.c | 4 ++-- 4 files changed, 11 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 434f5a7..0373626 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8674,7 +8674,7 @@ static int qemuStringToArgvEnv(const char *args, if (!next) next = strchr(curr, '\n');
- if (VIR_STRNDUP(arg, curr, next ? next - curr : strlen(curr)) < 0) + if (VIR_STRNDUP(arg, curr, next ? next - curr : -1) < 0)
Or we can even go with 'VIR_STRNDUP(arg, curr, next - curr) < 0' but that's not so easy to read. The rationale behind is: I intentionally made VIR_STRNDUP to accept *any* negative value, not just -1. Because if strrchr(cur, '\n') just a few lines above fails, next is just NULL. Deducting from NULL will get a negative value. Which will make VIR_STRNDUP duplicate the while string. But I worry that it would be an ugly code, wouldn't it? Michal
On 05/24/2013 06:57 AM, Michal Privoznik wrote:
On 24.05.2013 11:53, Michal Privoznik wrote:
With previous patch, we accept negative value as length of string to duplicate. So there is no need to pass strlen(src) in case we want to do duplicate the whole string. ---
- if (VIR_STRNDUP(arg, curr, next ? next - curr : strlen(curr)) < 0) + if (VIR_STRNDUP(arg, curr, next ? next - curr : -1) < 0)
Or we can even go with 'VIR_STRNDUP(arg, curr, next - curr) < 0' but that's not so easy to read. The rationale behind is: I intentionally made VIR_STRNDUP to accept *any* negative value, not just -1. Because if strrchr(cur, '\n') just a few lines above fails, next is just NULL. Deducting from NULL will get a negative value. Which will make VIR_STRNDUP duplicate the while string. But I worry that it would be an ugly code, wouldn't it?
It would be undefined behavior according to the C standard. Pointer subtraction is only well-defined within the bounds of a single object; your object (in C terminology) starts at 'curr' and ends at the NUL byte that terminates 'curr'. NULL falls outside that bounds. 'next - curr' is not guaranteed to be negative, since C says the behavior is undefined. Stick with the long form. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/24/2013 03:53 AM, Michal Privoznik wrote:
With previous patch, we accept negative value as length of string to duplicate. So there is no need to pass strlen(src) in case we want to do duplicate the whole string. --- src/conf/domain_conf.c | 6 ++---- src/qemu/qemu_command.c | 14 ++++++-------- src/util/virsexpr.c | 2 +- src/xenxs/xen_sxpr.c | 4 ++-- 4 files changed, 11 insertions(+), 15 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/24/2013 03:53 AM, Michal Privoznik wrote:
It may shorten the code a bit as the following pattern:
VIR_STRNDUP(dst, src, cond ? n : strlen(src))
is used on several places among our code. However, we can move the strlen into virStrndup and thus write just:
VIR_STRNDUP(dst, src, cond ? n : -1) --- src/util/virstring.c | 7 ++++++- src/util/virstring.h | 11 ++++++++--- tests/virstringtest.c | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 4 deletions(-)
@@ -132,7 +132,9 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * @n: the maximum number of bytes to copy * * Duplicate @src string and store it into @dst. If @src is longer than @n, - * only @n bytes are copied and terminating null byte '\0' is added. + * only @n bytes are copied and terminating null byte '\0' is added. If @n + * is a negative number, then the whole @src string is copied. That is
s/That is/That is,/
+ * VIR_STRDUP(dst, src) and VIR_STRNDUP(dst, src, -1) are equal. * * This macro is safe to use on arguments with side effects. * @@ -150,7 +152,10 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * @n: the maximum number of bytes to copy * * Duplicate @src string and store it into @dst. If @src is longer than @n, - * only @n bytes are copied and terminating null byte '\0' is added. + * only @n bytes are copied and terminating null byte '\0' is added. If @n + * is a negative number, then the whole @src string is copied. That is
and again.
+static int +testStrndupNegative(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + char *dst; + const char *src = "Hello world"; + int value; + + if ((value = VIR_STRNDUP(dst, src, 5)) != 1) { + fprintf(stderr, "unexpected virStrndup result %d, expected 1\n", value); + goto cleanup; + } + + if (STRNEQ_NULLABLE(dst, "Hello")) { + fprintf(stderr, "unexpected content '%s'", dst); + goto cleanup; + } + + if ((value = VIR_STRNDUP(dst, src, -1)) != 1) {
Memory leak. VIR_FREE(dst) before doing another dup into it. ACK with those fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michal Privoznik