On 11/12/2014 08:47 AM, Peter Krempa wrote:
> To allow reuse this non-trivial parser code in the backing store parser
> this part of the command line parser needs to be split out into a
> separate funciton.
> ---
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_command.c | 133 +++---------------------------------------
> src/util/virstoragefile.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virstoragefile.h | 3 +
> 4 files changed, 156 insertions(+), 125 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b8f35e8..b33722e 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1991,6 +1991,7 @@ virStorageSourceInitChainElement;
> virStorageSourceIsEmpty;
> virStorageSourceIsLocalStorage;
> virStorageSourceNewFromBacking;
> +virStorageSourceParseRBDColonString;
> virStorageSourcePoolDefFree;
> virStorageSourcePoolModeTypeFromString;
> virStorageSourcePoolModeTypeToString;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 19e8f9d..021ec07 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2540,137 +2540,20 @@ qemuGetSecretString(virConnectPtr conn,
> }
>
>
> -static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
> +static int
> +qemuParseRBDString(virDomainDiskDefPtr disk)
> {
> - char *port;
> - size_t skip;
> - char **parts;
> -
> - if (VIR_EXPAND_N(disk->src->hosts, disk->src->nhosts, 1) < 0)
> - return -1;
> -
> - if ((port = strchr(hostport, ']'))) {
> - /* ipv6, strip brackets */
> - hostport += 1;
> - skip = 3;
> - } else {
> - port = strstr(hostport, "\\:");
> - skip = 2;
> - }
> -
> - if (port) {
> - *port = '\0';
> - port += skip;
> - if (VIR_STRDUP(disk->src->hosts[disk->src->nhosts - 1].port, port) < 0)
> - goto error;
> - } else {
> - if (VIR_STRDUP(disk->src->hosts[disk->src->nhosts - 1].port, "6789") < 0)
> - goto error;
> - }
> -
> - parts = virStringSplit(hostport, "\\:", 0);
> - if (!parts)
> - goto error;
> - disk->src->hosts[disk->src->nhosts-1].name = virStringJoin((const char **)parts, ":");
> - virStringFreeList(parts);
> - if (!disk->src->hosts[disk->src->nhosts-1].name)
> - goto error;
> + char *source = disk->src->path;
> + int ret;
>
> - disk->src->hosts[disk->src->nhosts-1].transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> - disk->src->hosts[disk->src->nhosts-1].socket = NULL;
> + disk->src->path = NULL;
>
> - return 0;
> + ret = virStorageSourceParseRBDColonString(source, disk->src);
>
> - error:
> - VIR_FREE(disk->src->hosts[disk->src->nhosts-1].port);
> - VIR_FREE(disk->src->hosts[disk->src->nhosts-1].name);
> - return -1;
> + VIR_FREE(source);
> + return ret;
> }
>
> -/* disk->src initially has everything after the rbd: prefix */
> -static int qemuParseRBDString(virDomainDiskDefPtr disk)
> -{
> - char *options = NULL;
> - char *p, *e, *next;
> - virStorageAuthDefPtr authdef = NULL;
> -
> - p = strchr(disk->src->path, ':');
> - if (p) {
> - if (VIR_STRDUP(options, p + 1) < 0)
> - goto error;
> - *p = '\0';
> - }
> -
> - /* options */
> - if (!options)
> - return 0; /* all done */
> -
> - p = options;
> - while (*p) {
> - /* find : delimiter or end of string */
> - for (e = p; *e && *e != ':'; ++e) {
> - if (*e == '\\') {
> - e++;
> - if (*e == '\0')
> - break;
> - }
> - }
> - if (*e == '\0') {
> - next = e; /* last kv pair */
> - } else {
> - next = e + 1;
> - *e = '\0';
> - }
> -
> - if (STRPREFIX(p, "id=")) {
> - const char *secrettype;
> - /* formulate authdef for disk->src->auth */
> - if (VIR_ALLOC(authdef) < 0)
> - goto error;
> -
> - if (VIR_STRDUP(authdef->username, p + strlen("id=")) < 0)
> - goto error;
> - secrettype = virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH);
> - if (VIR_STRDUP(authdef->secrettype, secrettype) < 0)
> - goto error;
> - disk->src->auth = authdef;
> - authdef = NULL;
> -
> - /* Cannot formulate a secretType (eg, usage or uuid) given
> - * what is provided.
> - */
> - }
> - if (STRPREFIX(p, "mon_host=")) {
> - char *h, *sep;
> -
> - h = p + strlen("mon_host=");
> - while (h < e) {
> - for (sep = h; sep < e; ++sep) {
> - if (*sep == '\\' && (sep[1] == ',' ||
> - sep[1] == ';' ||
> - sep[1] == ' ')) {
> - *sep = '\0';
> - sep += 2;
> - break;
> - }
> - }
> - if (qemuAddRBDHost(disk, h) < 0)
> - goto error;
> -
> - h = sep;
> - }
> - }
> -
> - p = next;
> - }
> - VIR_FREE(options);
> - return 0;
> -
> - error:
> - VIR_FREE(options);
> - virStorageAuthDefFree(authdef);
> - return -1;
> -}
>
> static int
> qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri,
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index c2d5b46..f267d1a 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2209,6 +2209,150 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src,
>
>
> static int
> +virStorageSourceRBDAddHost(virStorageSourcePtr src,
> + char *hostport)
> +{
> + char *port;
> + size_t skip;
> + char **parts;
> +
> + if (VIR_EXPAND_N(src->hosts, src->nhosts, 1) < 0)
> + return -1;
> +
> + if ((port = strchr(hostport, ']'))) {
> + /* ipv6, strip brackets */
> + hostport += 1;
> + skip = 3;
> + } else {
> + port = strstr(hostport, "\\:");
> + skip = 2;
> + }
> +
> + if (port) {
> + *port = '\0';
> + port += skip;
> + if (VIR_STRDUP(src->hosts[src->nhosts - 1].port, port) < 0)
> + goto error;
> + } else {
> + if (VIR_STRDUP(src->hosts[src->nhosts - 1].port, "6789") < 0)
> + goto error;
> + }
> +
> + parts = virStringSplit(hostport, "\\:", 0);
> + if (!parts)
> + goto error;
> + src->hosts[src->nhosts-1].name = virStringJoin((const char **)parts, ":");
> + virStringFreeList(parts);
> + if (!src->hosts[src->nhosts-1].name)
> + goto error;
> +
> + src->hosts[src->nhosts-1].transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> + src->hosts[src->nhosts-1].socket = NULL;
> +
> + return 0;
> +
> + error:
> + VIR_FREE(src->hosts[src->nhosts-1].port);
> + VIR_FREE(src->hosts[src->nhosts-1].name);
> + return -1;
> +}
> +
> +
> +int
> +virStorageSourceParseRBDColonString(const char *rbdstr,
> + virStorageSourcePtr src)
> +{
> + char *options = NULL;
> + char *p, *e, *next;
> + virStorageAuthDefPtr authdef = NULL;
> +
> + /* optionally skip the "rbd:" prefix if provided */
> + if (STRPREFIX(rbdstr, "rbd:"))
> + rbdstr += strlen("rbd:");
> +
> + if (VIR_STRDUP(src->path, rbdstr) < 0)
> + goto error;
> +
> + p = strchr(src->path, ':');
> + if (p) {
> + if (VIR_STRDUP(options, p + 1) < 0)
> + goto error;
> + *p = '\0';
> + }
> +
> + /* options */
> + if (!options)
> + return 0; /* all done */
> +
> + p = options;
> + while (*p) {
> + /* find : delimiter or end of string */
> + for (e = p; *e && *e != ':'; ++e) {
> + if (*e == '\\') {
> + e++;
> + if (*e == '\0')
> + break;
> + }
> + }
> + if (*e == '\0') {
> + next = e; /* last kv pair */
> + } else {
> + next = e + 1;
> + *e = '\0';
> + }
> +
> + if (STRPREFIX(p, "id=")) {
> + /* formulate authdef for src->auth */
> + if (VIR_ALLOC(authdef) < 0)
> + goto error;
> +
> + if (VIR_STRDUP(authdef->username, p + strlen("id=")) < 0)
> + goto error;
> +
> + if (VIR_STRDUP(authdef->secrettype, "ceph") < 0)
> + goto error;
Mostly code motion, but I see here in order to avoid the memory leak of
'secrettype' in the old code, you're just using "ceph" here (or perhaps
dragging in the former definition...)
Not that it could ever change, but you could use
virStorageAuthTypeFromString(VIR_STORAGE_AUTH_TYPE_CEPHX) since it's
defined in here... Of course the secrettype would need to be promoted to
the top of the function and then VIR_FREE()'d appropriately
> + src->auth = authdef;
> + authdef = NULL;
> +
> + /* Cannot formulate a secretType (eg, usage or uuid) given
> + * what is provided.
> + */
> + }
> + if (STRPREFIX(p, "mon_host=")) {
> + char *h, *sep;
> +
> + h = p + strlen("mon_host=");
> + while (h < e) {
> + for (sep = h; sep < e; ++sep) {
> + if (*sep == '\\' && (sep[1] == ',' ||
> + sep[1] == ';' ||
> + sep[1] == ' ')) {
> + *sep = '\0';
> + sep += 2;
> + break;
> + }
> + }
> +
> + if (virStorageSourceRBDAddHost(src, h) < 0)
> + goto error;
> +
> + h = sep;
> + }
> + }
> +
> + p = next;
> + }
> + VIR_FREE(options);
> + return 0;
> +
> + error:
> + VIR_FREE(options);
> + virStorageAuthDefFree(authdef);
> + return -1;
> +}
> +
> +
> +static int
> virStorageSourceParseBackingColon(virStorageSourcePtr src,
> const char *path)
> {
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 7f3f353..b1ba73a 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -363,6 +363,9 @@ virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src,
> bool backingChain)
> ATTRIBUTE_NONNULL(1);
>
> +int virStorageSourceParseRBDColonString(const char *rbdstr,
> + virStorageSourcePtr src);
> +
I assume "ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
ACK
John
> typedef int
> (*virStorageFileSimplifyPathReadlinkCallback)(const char *path,
> char **link,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list