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.
+ *
+ * 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;
+}
+
+
 int
 virStorageFileParseChainIndex(const char *diskTarget,
                               const char *name,
                               unsigned int *chainIndex)
 {
-    char **strings = NULL;
     unsigned int idx = 0;
-    char *suffix;
+    char *target = NULL;
     int ret = 0;

     *chainIndex = 0;

-    if (name && diskTarget)
-        strings = virStringSplit(name, "[", 2);
+    if (!name || !diskTarget)
+        return 0;

-    if (virStringListLength((const char * const *)strings) != 2)
-        goto cleanup;
+    if (virStorageFileParseBackingStoreStr(name, &target, &idx) < 0)
+        return 0;

-    if (virStrToLong_uip(strings[1], &suffix, 10, &idx) < 0 ||
-        STRNEQ(suffix, "]"))
+    if (idx == 0)
         goto cleanup;

-    if (STRNEQ(diskTarget, strings[0])) {
+    if (STRNEQ(diskTarget, target)) {
         virReportError(VIR_ERR_INVALID_ARG,
                        _("requested target '%s' does not match target '%s'"),
-                       strings[0], diskTarget);
+                       target, diskTarget);
         ret = -1;
         goto cleanup;
     }
@@ -1475,7 +1521,7 @@ virStorageFileParseChainIndex(const char *diskTarget,
     *chainIndex = idx;

  cleanup:
-    virStringListFree(strings);
+    VIR_FREE(target);
     return ret;
 }

diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index bea1c35a2..5f6e41911 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -309,6 +309,11 @@ int virStorageFileParseChainIndex(const char *diskTarget,
                                   unsigned int *chainIndex)
     ATTRIBUTE_NONNULL(3);

+int virStorageFileParseBackingStoreStr(const char *str,
+                                       char **target,
+                                       unsigned int *chainIndex)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+
 virStorageSourcePtr virStorageFileChainLookup(virStorageSourcePtr chain,
                                               virStorageSourcePtr startFrom,
                                               const char *name,
-- 
2.11.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list