[libvirt] [PATCH v5 1/4] Add Gluster protocol as supported network disk backend

[Jiri Denemark]

On Thu, Nov 22, 2012 at 23:40:38 +0530, Harsh Prateek Bora wrote:
> This patch introduces the RNG schema and updates necessary data strucutures
> to allow various hypervisors to make use of Gluster protocol as one of the
> supported network disk backend. Next patch will add support to make use of
> this feature in Qemu since it now supports Gluster protocol as one of the
> network based storage backend.
> 
> Two new optional attributes for <host> element are introduced - 'transport'
> and 'socket'. Valid transport values are tcp, unix or rdma. If none specified,
> tcp is assumed. If transport is unix, socket specifies path to unix socket.

I think the XML examples from 2/4 should be moved here.

> Signed-off-by: Harsh Prateek Bora <harsh at linux.vnet.ibm.com>
> ---
>  docs/formatdomain.html.in     | 24 ++++++++++----
>  docs/schemas/domaincommon.rng | 33 +++++++++++++++----
>  src/conf/domain_conf.c        | 77 ++++++++++++++++++++++++++++++++++---------
>  src/conf/domain_conf.h        | 12 +++++++
>  src/libvirt_private.syms      |  2 ++
>  5 files changed, 120 insertions(+), 28 deletions(-)
...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 33e1e7f..56986d6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
...
> @@ -3460,6 +3467,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>      char *source = NULL;
>      char *target = NULL;
>      char *protocol = NULL;
> +    char *protocol_transport;
>      char *trans = NULL;
>      virDomainDiskHostDefPtr hosts = NULL;
>      int nhosts = 0;
> @@ -3566,19 +3574,46 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>                              }
>                              hosts[nhosts].name = NULL;
>                              hosts[nhosts].port = NULL;
> +                            hosts[nhosts].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP;
> +                            hosts[nhosts].socket = NULL;
>                              nhosts++;
>  
> -                            hosts[nhosts - 1].name = virXMLPropString(child, "name");
> -                            if (!hosts[nhosts - 1].name) {
> -                                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                               "%s", _("missing name for host"));
> -                                goto error;
> +                            /* transport can be tcp (default), unix or rdma.  */
> +                            protocol_transport = virXMLPropString(child, "transport");
> +                            if (protocol_transport != NULL) {
> +                                hosts[nhosts - 1].transport = virDomainDiskProtocolTransportTypeFromString(protocol_transport);
> +                                if (hosts[nhosts - 1].transport < 0) {
> +                                    virReportError(VIR_ERR_XML_ERROR,
> +                                                   _("unknown protocol transport type '%s'"),
> +                                                   protocol_transport);
> +                                    goto error;
> +                                }
>                              }

This code would leak protocol_transport.

> -                            hosts[nhosts - 1].port = virXMLPropString(child, "port");
> -                            if (!hosts[nhosts - 1].port) {
> -                                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                               "%s", _("missing port for host"));
> -                                goto error;
> +                            hosts[nhosts - 1].socket = virXMLPropString(child, "socket");
> +                            if (hosts[nhosts - 1].transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX &&
> +                                hosts[nhosts - 1].socket == NULL) {
> +                                virReportError(VIR_ERR_XML_ERROR,
> +                                               "%s", _("missing socket for unix transport"));
> +                            }
> +                            if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX &&
> +                                hosts[nhosts - 1].socket != NULL) {
> +                                virReportError(VIR_ERR_XML_ERROR,
> +                                               _("transport %s do not support socket attribute"),

s/do/does/

> +                                               protocol_transport);
> +                            }

goto error is missing in the two error cases above.

> +                            if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
> +                                hosts[nhosts - 1].name = virXMLPropString(child, "name");
> +                                if (!hosts[nhosts - 1].name) {
> +                                    virReportError(VIR_ERR_XML_ERROR,
> +                                                   "%s", _("missing name for host"));
> +                                    goto error;
> +                                }
> +                                hosts[nhosts - 1].port = virXMLPropString(child, "port");
> +                                if (!hosts[nhosts - 1].port) {
> +                                    virReportError(VIR_ERR_XML_ERROR,
> +                                                   "%s", _("missing port for host"));
> +                                    goto error;
> +                                }
>                              }
>                          }
>                          child = child->next;
...

I'm intentionally ignoring long lines in your patch since it would be quite
hard to get rid of them due to the horrible code structure in
virDomainDiskDefParseXML. The function should rather be refactored but that's
a separate low-priority thing.

ACK with the following patch squashed in:

    diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
    index 57c167e..2ca608f 100644
    --- a/src/conf/domain_conf.c
    +++ b/src/conf/domain_conf.c
    @@ -3527,7 +3527,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
         char *source = NULL;
         char *target = NULL;
         char *protocol = NULL;
    -    char *protocol_transport;
    +    char *protocol_transport = NULL;
         char *trans = NULL;
         virDomainDiskHostDefPtr hosts = NULL;
         int nhosts = 0;
    @@ -3654,13 +3654,16 @@ virDomainDiskDefParseXML(virCapsPtr caps,
                                     hosts[nhosts - 1].socket == NULL) {
                                     virReportError(VIR_ERR_XML_ERROR,
                                                    "%s", _("missing socket for unix transport"));
    +                                goto error;
                                 }
                                 if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX &&
                                     hosts[nhosts - 1].socket != NULL) {
                                     virReportError(VIR_ERR_XML_ERROR,
    -                                               _("transport %s do not support socket attribute"),
    +                                               _("transport %s does not support socket attribute"),
                                                    protocol_transport);
    +                                goto error;
                                 }
    +                            VIR_FREE(protocol_transport);
                                 if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) {
                                     hosts[nhosts - 1].name = virXMLPropString(child, "name");
                                     if (!hosts[nhosts - 1].name) {
    @@ -4265,6 +4268,7 @@ cleanup:
         }
         VIR_FREE(hosts);
         VIR_FREE(protocol);
    +    VIR_FREE(protocol_transport);
         VIR_FREE(device);
         VIR_FREE(authUsername);
         VIR_FREE(usageType);