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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
