On 02/23/2017 01:21 PM, Peter Krempa wrote:
The function has very specific semantics. Split out the part that parses the backing store specification string into a separate helper so that it can be reused later while keeping the wrapper with existing semantics.
Note that virStorageFileParseChainIndex is pretty well covered by the test suite. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 68 +++++++++++++++++++++++++++++++++++++++-------- src/util/virstoragefile.h | 5 ++++ 3 files changed, 63 insertions(+), 11 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 07a35333b..69d1bc860 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2462,6 +2462,7 @@ virStorageFileGetMetadataInternal; virStorageFileGetRelativeBackingPath; virStorageFileGetSCSIKey; virStorageFileIsClusterFS; +virStorageFileParseBackingStoreStr; virStorageFileParseChainIndex; virStorageFileProbeFormat; virStorageFileResize; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c9420fdb7..3e711228b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1442,32 +1442,78 @@ int virStorageFileGetSCSIKey(const char *path, } #endif
+ +/** + * virStorageFileParseBackingStoreStr: + * @str: backing store specifier string to parse + * @target: returns target device portion of the string + * @chainIndex: returns the backing store portion of the string + * + * Parses the backing store specifier string such as vda[1], or sda into + * components and returns them via arguments. If the string did not specify an + * index 0 is assumed.
grammar nit: s/index/index,/
+ * + * Returns 0 on success -1 on error + */ +int +virStorageFileParseBackingStoreStr(const char *str, + char **target, + unsigned int *chainIndex) +{ + char **strings = NULL; + size_t nstrings; + unsigned int idx = 0; + char *suffix; + int ret = -1; + + *chainIndex = 0; + + if (!(strings = virStringSplitCount(str, "[", 2, &nstrings))) + return -1; + + if (nstrings == 2) { + if (virStrToLong_uip(strings[1], &suffix, 10, &idx) < 0 || + STRNEQ(suffix, "]")) + goto cleanup; + } + + if (target && + VIR_STRDUP(*target, strings[0]) < 0) + goto cleanup; + + *chainIndex = idx; + ret = 0; + + cleanup: + virStringListFreeCount(strings, nstrings); + return ret; +}
I might have gone for a simpler implementation (no need to malloc and throw away a full-blown string split, when it is easy to do by hand): char *p = strchr(str, '['); char *suffix; int ret = 0; *chainIndex = 0; if (!p) { if (target) ret = VIR_STRDUP(*target, str); } else if (virStrToLong_uip(p + 1, &suffix, 10, chainIndex) < 0 || STRNEQ(suffix, "]")) { return -1; } else if (target) { ret = VIR_STRNDUP(*target, str, p - str); } return ret; (well, modulo a tweak to the return value if returning 0 is more important than returning 1 on success) I see that you were just moving the pre-existing complexity, though. At any rate, it's a nice refactoring, whether or not you also improve the new helper function to not be so roundabout at computing its results. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org