[libvirt] [PATCHv2 0/2] delete unnecessary white space of sysinfo.
sysinfo: delete unnecessary white space of sysinfo. This is v2 of delete unnecessary white space of sysinfo. Changes v1->v2: - Rebase latest libvirt GIT tree. - Use c_isspace() in virSkipSpacesBackwards(). Minoru Usui (2): sysinfo: add virSkipSpacesBackwards() sysinfo: delete unnecessary white space of sysinfo. src/util/sysinfo.c | 21 +++++++++++++++++++++ src/util/util.c | 25 +++++++++++++++++++++++++ src/util/util.h | 1 + 3 files changed, 47 insertions(+), 0 deletions(-) -- Minoru Usui <usui@mxm.nes.nec.co.jp>
sysinfo: add virSkipSpacesBackwards() * Add virSkipSpacesBackwards() to src/util/util.[ch] Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- src/util/util.c | 25 +++++++++++++++++++++++++ src/util/util.h | 1 + 2 files changed, 26 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 463d2b8..1cf4de8 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1550,6 +1550,31 @@ virSkipSpaces(const char **str) } /** + * virSkipSpacesBackwards: + * @str : pointer to the target strings + * @endp: pointer to the end of @str + * + * Skip potential blanks backwards. + */ +void +virSkipSpacesBackwards(const char *str, char **endp) +{ + char *cur; + + if (!endp || !*endp) + return; + + cur = *endp - 1; + while (cur >= str) { + if (!c_isspace(*cur)) + break; + cur--; + } + + *endp = (cur >= str) ? cur + 1 : NULL; +} + +/** * virParseNumber: * @str: pointer to the char pointer used * diff --git a/src/util/util.h b/src/util/util.h index 0c43f7a..ae74c30 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -167,6 +167,7 @@ int virHexToBin(unsigned char c); int virMacAddrCompare (const char *mac1, const char *mac2); void virSkipSpaces(const char **str); +void virSkipSpacesBackwards(const char *str, char **endp); int virParseNumber(const char **str); int virParseVersionString(const char *str, unsigned long *version); int virAsprintf(char **strp, const char *fmt, ...) -- 1.7.1
On 06/28/2011 10:42 PM, Minoru Usui wrote:
sysinfo: add virSkipSpacesBackwards()
* Add virSkipSpacesBackwards() to src/util/util.[ch]
Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- src/util/util.c | 25 +++++++++++++++++++++++++ src/util/util.h | 1 + 2 files changed, 26 insertions(+), 0 deletions(-)
+ * virSkipSpacesBackwards: + * @str : pointer to the target strings + * @endp: pointer to the end of @str + * + * Skip potential blanks backwards. + */ +void +virSkipSpacesBackwards(const char *str, char **endp) +{ + char *cur; + + if (!endp || !*endp) + return;
This interface is rather limited, because it cannot modify str in-place. I would find it more flexible if we could allow modifications or a way to let the interface do strlen() for me instead of me having to always find the string end first, so I wonder if we should instead add two interfaces: /** * virTrimSpaces: * @str: string to modify to remove all trailing spaces * @endp: track the end of the string * * If @endp is NULL on entry, then all spaces prior to the trailing * NUL in @str are removed, by writing NUL into the appropriate * location. If @endp is non-NULL but points to a NULL pointer, * then all spaces prior to the trailing NUL in @str are removed, * NUL is written to the new string end, and endp is set to the * location of the (new) string end. If @endp is non-NULL and * points to a non-NULL pointer, then that pointer is used as * the end of the string, endp is set to the (new) location, but * no NUL pointer is written into the string. */ void virTrimSpaces(char *str, char **endp) { char *end; if (!endp || !*endp) end = str + strlen(str); while (end > str && c_isspace(end[-1])) end--; if (endp) { if (!*endp) *end = '\0'; *endp = end; } else { *end = '\0'; } } /** * virSkipSpacesBackwards: * @str: start of string * @endp: on entry, must point to a location with @str, on exit, * will be adjusted to skip trailing spaces * * Returns 0 on success, -1 on error */ int virSkipSpacesBackwards(const char *str, char **endp) { if (!endp || !*endp) return -1; /* Casting away const is safe, since virTrimSpaces does not * modify string with this particular usage. */ virTrimSpaces((char *)str), endp); return 0; } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/29/2011 11:04 AM, Eric Blake wrote:
void virTrimSpaces(char *str, char **endp) { char *end;
if (!endp || !*endp) end = str + strlen(str);
else end = *endp;
while (end > str && c_isspace(end[-1])) end--; if (endp) { if (!*endp) *end = '\0'; *endp = end; } else { *end = '\0'; } } /** * virSkipSpacesBackwards: * @str: start of string * @endp: on entry, must point to a location with @str, on exit, * will be adjusted to skip trailing spaces * * Returns 0 on success, -1 on error */ int virSkipSpacesBackwards(const char *str, char **endp)
Actually, looking at how you used this in patch 2/2, you want to trim all spaces up to either the newline (*endp is non-NULL) or the end of the string (*endp is NULL), all without modifying str, so this would need a modification:
{ if (!endp || !*endp) return -1;
if (!endp) return -1; if (!*endp) *endp = (char*)str + strlen(str);
/* Casting away const is safe, since virTrimSpaces does not * modify string with this particular usage. */ virTrimSpaces((char *)str), endp); return 0; }
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
sysinfo: delete unnecessary white space of sysinfo. * Trim each element and delete null entry of sysinfo by virSkipSpacesBackwards(). Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- src/util/sysinfo.c | 21 +++++++++++++++++++++ 1 files changed, 21 insertions(+), 0 deletions(-) diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index a6e525b..d84b973 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -246,6 +246,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Socket Designation: ")) != NULL) { cur += 20; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_socket_destination = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -253,6 +254,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Type: ")) != NULL) { cur += 6; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_type = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -260,6 +262,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Family: ")) != NULL) { cur += 8; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_family = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -267,6 +270,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Manufacturer: ")) != NULL) { cur += 14; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_manufacturer = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -274,6 +278,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Signature: ")) != NULL) { cur += 11; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_signature = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -281,6 +286,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Version: ")) != NULL) { cur += 9; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_version = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -288,6 +294,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "External Clock: ")) != NULL) { cur += 16; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_external_clock = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -295,6 +302,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Max Speed: ")) != NULL) { cur += 11; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_max_speed = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -302,6 +310,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Status: ")) != NULL) { cur += 8; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_status = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -309,6 +318,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Serial Number: ")) != NULL) { cur += 15; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_serial_number = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -316,6 +326,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Part Number: ")) != NULL) { cur += 13; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((processor->processor_part_number = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -351,6 +362,7 @@ virSysinfoParseMemory(char *base, virSysinfoDefPtr ret) if (STREQLEN(cur, "No Module Installed", eol - cur)) goto next; + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((memory->memory_size = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -358,6 +370,7 @@ virSysinfoParseMemory(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Form Factor: ")) != NULL) { cur += 13; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((memory->memory_form_factor = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -365,6 +378,7 @@ virSysinfoParseMemory(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Locator: ")) != NULL) { cur += 9; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((memory->memory_locator = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -372,6 +386,7 @@ virSysinfoParseMemory(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Bank Locator: ")) != NULL) { cur += 14; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((memory->memory_bank_locator = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -379,6 +394,7 @@ virSysinfoParseMemory(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Type: ")) != NULL) { cur += 6; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((memory->memory_type = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -386,6 +402,7 @@ virSysinfoParseMemory(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Type Detail: ")) != NULL) { cur += 13; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((memory->memory_type_detail = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -393,6 +410,7 @@ virSysinfoParseMemory(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Speed: ")) != NULL) { cur += 7; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((memory->memory_speed = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -400,6 +418,7 @@ virSysinfoParseMemory(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Manufacturer: ")) != NULL) { cur += 14; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((memory->memory_manufacturer = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -407,6 +426,7 @@ virSysinfoParseMemory(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Serial Number: ")) != NULL) { cur += 15; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((memory->memory_serial_number = strndup(cur, eol - cur)) == NULL)) goto no_memory; @@ -414,6 +434,7 @@ virSysinfoParseMemory(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Part Number: ")) != NULL) { cur += 13; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) && ((memory->memory_part_number = strndup(cur, eol - cur)) == NULL)) goto no_memory; -- 1.7.1
On 06/28/2011 10:43 PM, Minoru Usui wrote:
sysinfo: delete unnecessary white space of sysinfo.
* Trim each element and delete null entry of sysinfo by virSkipSpacesBackwards().
Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- src/util/sysinfo.c | 21 +++++++++++++++++++++ 1 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index a6e525b..d84b973 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -246,6 +246,7 @@ virSysinfoParseProcessor(char *base, virSysinfoDefPtr ret) if ((cur = strstr(base, "Socket Designation: ")) != NULL) { cur += 20; eol = strchr(cur, '\n'); + virSkipSpacesBackwards(cur, &eol); if ((eol) &&
ACK and pushed; this version works (rather than needing to use v4) once I fixed my patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Incorporating my own suggestions from earlier rounds of review. Minoru's original 2/2 patch is unchanged. I think this series is probably worth including in 0.9.3, since it is fixing a just-added feature, but won't push without feedback. Eric Blake (2): util: fix virSkipSpaces util: add virTrimSpaces Minoru Usui (1): sysinfo: delete unnecessary white space of sysinfo. src/libvirt_private.syms | 3 ++ src/util/sysinfo.c | 21 +++++++++++++ src/util/util.c | 76 +++++++++++++++++++++++++++++++++++++++++++-- src/util/util.h | 5 +++ src/xen/xend_internal.c | 4 +- 5 files changed, 103 insertions(+), 6 deletions(-) -- 1.7.4.4
Most clients of virSkipSpaces don't want to omit backslashes. Also, open-coding the list of spaces is not as nice as using c_isspace. * src/util/util.c (virSkipSpaces): Use c_isspace. (virSkipSpacesAndBackslash): New function. * src/util/util.h (virSkipSpacesAndBackslash): New prototype. * src/xen/xend_internal.c (sexpr_to_xend_topology): Update caller. * src/libvirt_private.syms (util.h): Export new function. --- src/libvirt_private.syms | 1 + src/util/util.c | 23 +++++++++++++++++++---- src/util/util.h | 1 + src/xen/xend_internal.c | 4 ++-- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 626ac6c..024b3f1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1030,6 +1030,7 @@ virSetInherit; virSetNonBlock; virSetUIDGID; virSkipSpaces; +virSkipSpacesAndBackslash; virStrToDouble; virStrToLong_i; virStrToLong_l; diff --git a/src/util/util.c b/src/util/util.c index 463d2b8..27eefb2 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1535,16 +1535,31 @@ virHexToBin(unsigned char c) * @str: pointer to the char pointer used * * Skip potential blanks, this includes space tabs, line feed, - * carriage returns and also '\\' which can be erronously emitted - * by xend + * carriage returns. */ void virSkipSpaces(const char **str) { const char *cur = *str; - while ((*cur == ' ') || (*cur == '\t') || (*cur == '\n') || - (*cur == '\r') || (*cur == '\\')) + while (c_isspace(*cur)) + cur++; + *str = cur; +} + +/** + * virSkipSpacesAndBackslash: + * @str: pointer to the char pointer used + * + * Like virSkipSpaces, but also skip backslashes erroneously emitted + * by xend + */ +void +virSkipSpacesAndBackslash(const char **str) +{ + const char *cur = *str; + + while (c_isspace(*cur) || *cur == '\\') cur++; *str = cur; } diff --git a/src/util/util.h b/src/util/util.h index 0c43f7a..8dec78a 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -167,6 +167,7 @@ int virHexToBin(unsigned char c); int virMacAddrCompare (const char *mac1, const char *mac2); void virSkipSpaces(const char **str); +void virSkipSpacesAndBackslash(const char **str); int virParseNumber(const char **str); int virParseVersionString(const char *str, unsigned long *version); int virAsprintf(char **strp, const char *fmt, ...) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index d418847..d0eb32a0 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1199,11 +1199,11 @@ sexpr_to_xend_topology(const struct sexpr *root, cell = virParseNumber(&cur); if (cell < 0) goto parse_error; - virSkipSpaces(&cur); + virSkipSpacesAndBackslash(&cur); if (*cur != ':') goto parse_error; cur++; - virSkipSpaces(&cur); + virSkipSpacesAndBackslash(&cur); if (STRPREFIX(cur, "no cpus")) { nb_cpus = 0; for (cpu = 0; cpu < numCpus; cpu++) -- 1.7.4.4
The next patch wants to adjust an end pointer to trim trailing spaces but without modifying the underlying string, but a more generally useful ability to trim trailing spaces in place is also worth providing. * src/util/util.h (virTrimSpaces, virSkipSpacesBackwards): New prototypes. * src/util/util.c (virTrimSpaces, virSkipSpacesBackwards): New functions. * src/libvirt_private.syms (util.h): Export new functions. Inspired by a patch by Minoru Usui. --- src/libvirt_private.syms | 2 + src/util/util.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 4 +++ 3 files changed, 59 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 024b3f1..a444a40 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1031,6 +1031,7 @@ virSetNonBlock; virSetUIDGID; virSkipSpaces; virSkipSpacesAndBackslash; +virSkipSpacesBackwards; virStrToDouble; virStrToLong_i; virStrToLong_l; @@ -1042,6 +1043,7 @@ virStrcpy; virStrncpy; virTimeMs; virTimestamp; +virTrimSpaces; virVasprintf; diff --git a/src/util/util.c b/src/util/util.c index 27eefb2..f6601b9 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1565,6 +1565,59 @@ virSkipSpacesAndBackslash(const char **str) } /** + * virTrimSpaces: + * @str: string to modify to remove all trailing spaces + * @endp: track the end of the string + * + * If @endp is NULL on entry, then all spaces prior to the trailing + * NUL in @str are removed, by writing NUL into the appropriate + * location. If @endp is non-NULL but points to a NULL pointer, + * then all spaces prior to the trailing NUL in @str are removed, + * NUL is written to the new string end, and endp is set to the + * location of the (new) string end. If @endp is non-NULL and + * points to a non-NULL pointer, then that pointer is used as + * the end of the string, endp is set to the (new) location, but + * no NUL pointer is written into the string. + */ +void +virTrimSpaces(char *str, char **endp) +{ + char *end; + + if (!endp || !*endp) + end = str + strlen(str); + else + end = *endp; + while (end > str && c_isspace(end[-1])) + end--; + if (endp) { + if (!*endp) + *end = '\0'; + *endp = end; + } else { + *end = '\0'; + } +} + +/** + * virSkipSpacesBackwards: + * @str: start of string + * @endp: on entry, *endp must be NULL or a location within @str, on exit, + * will be adjusted to skip trailing spaces + */ +void +virSkipSpacesBackwards(const char *str, char **endp) +{ + /* Casting away const is safe, since virTrimSpaces does not + * modify string with this particular usage. */ + char *s = (char*) str; + + if (!*endp) + *endp = s + strlen(s); + virTrimSpaces(s, endp); +} + +/** * virParseNumber: * @str: pointer to the char pointer used * diff --git a/src/util/util.h b/src/util/util.h index 8dec78a..2ca592b 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -168,6 +168,10 @@ int virMacAddrCompare (const char *mac1, const char *mac2); void virSkipSpaces(const char **str); void virSkipSpacesAndBackslash(const char **str); +void virTrimSpaces(char *str, char **endp) ATTRIBUTE_NONNULL(1); +void virSkipSpacesBackwards(const char *str, char **endp) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int virParseNumber(const char **str); int virParseVersionString(const char *str, unsigned long *version); int virAsprintf(char **strp, const char *fmt, ...) -- 1.7.4.4
Hi, Eric On Wed, 29 Jun 2011 11:52:41 -0600 Eric Blake <eblake@redhat.com> wrote:
Incorporating my own suggestions from earlier rounds of review. Minoru's original 2/2 patch is unchanged.
I think this series is probably worth including in 0.9.3, since it is fixing a just-added feature, but won't push without feedback.
Thank you for your patches. I think these are good itself. But my 2/2 patch depends on prior behaviour of virSkipSpacesBackwards(). In case of @str consists of only spaces, prior virSkipSpacesBackwards() set NULL pointer to @endp. Your version set non-NULL but points to a NULL pointer. So, the caller can't delete NULL element line like folloing. <entry name='serial_number'></entry> <entry name='part_number'></entry> So I'll change 2/2 patch soon.
Eric Blake (2): util: fix virSkipSpaces util: add virTrimSpaces
Minoru Usui (1): sysinfo: delete unnecessary white space of sysinfo.
src/libvirt_private.syms | 3 ++ src/util/sysinfo.c | 21 +++++++++++++ src/util/util.c | 76 +++++++++++++++++++++++++++++++++++++++++++-- src/util/util.h | 5 +++ src/xen/xend_internal.c | 4 +- 5 files changed, 103 insertions(+), 6 deletions(-)
-- 1.7.4.4
-- Minoru Usui <usui@mxm.nes.nec.co.jp>
On 06/30/2011 02:32 AM, Minoru Usui wrote:
Hi, Eric
On Wed, 29 Jun 2011 11:52:41 -0600 Eric Blake <eblake@redhat.com> wrote:
Incorporating my own suggestions from earlier rounds of review. Minoru's original 2/2 patch is unchanged.
I think this series is probably worth including in 0.9.3, since it is fixing a just-added feature, but won't push without feedback.
Thank you for your patches. I think these are good itself.
With that, I'll push my patch 1 and 2, then review your patch 3 v4.
But my 2/2 patch depends on prior behaviour of virSkipSpacesBackwards(). In case of @str consists of only spaces, prior virSkipSpacesBackwards() set NULL pointer to @endp. Your version set non-NULL but points to a NULL pointer.
That's easy enough to fix - I could make virSkipSpacesBackwards set @endp to NULL if nothing remains. In fact, if I do that, then your v2 patch applies. So here's what I squashed in: diff --git i/src/util/util.c w/src/util/util.c index 457c7fa..5542557 100644 --- i/src/util/util.c +++ w/src/util/util.c @@ -1600,7 +1600,8 @@ virTrimSpaces(char *str, char **endp) * virSkipSpacesBackwards: * @str: start of string * @endp: on entry, *endp must be NULL or a location within @str, on exit, - * will be adjusted to skip trailing spaces + * will be adjusted to skip trailing spaces, or to NULL if @str had nothing + * but spaces. */ void virSkipSpacesBackwards(const char *str, char **endp) @@ -1612,6 +1613,8 @@ virSkipSpacesBackwards(const char *str, char **endp) if (!*endp) *endp = s + strlen(s); virTrimSpaces(s, endp); + if (s == *endp) + *endp = NULL; } /** diff --git i/src/util/util.h w/src/util/util.h index 3972fa5..7a1eb11 100644 --- i/src/util/util.h +++ w/src/util/util.h @@ -166,8 +166,8 @@ int virHexToBin(unsigned char c); int virMacAddrCompare (const char *mac1, const char *mac2); -void virSkipSpaces(const char **str); -void virSkipSpacesAndBackslash(const char **str); +void virSkipSpaces(const char **str) ATTRIBUTE_NONNULL(1); +void virSkipSpacesAndBackslash(const char **str) ATTRIBUTE_NONNULL(1); void virTrimSpaces(char *str, char **endp) ATTRIBUTE_NONNULL(1); void virSkipSpacesBackwards(const char *str, char **endp) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Minoru Usui