[libvirt] Patch review request for Red Hat Bugzilla Bug 1341866

Wed Jun 15 17:34:15 UTC 2016

On Fri, Jun 10, 2016 at 03:14:52AM +0000, Ashish Mittal wrote:
> I tried using “git send-email”, but unfortunately:
> (a) Internal firewall blocks git communication to outside world. I was
> thinking of using a http based read-only repo, but I guess that would be a
> bit older code. Do you know of a http based git repo mirror that I can
> reliably use for "git pull” etc?

We have automatically updated mirrors:

    https://gitlab.com/libvirt/libvirt
    https://github.com/libvirt/libvirt

they should never be more than 60 minutes old

> (b) git send-email is not working for me. I am having trouble setting it
> up to forward emails via my official MS Exchange server account.

Ok, if that doesn't work, the usual tip is to use

  'git format-patch -1'

which generates a suitable formatted patch file you can copy and
paste into the *body* of the email - just be sure that your email
client doesn't mangle whitespace.

If you must use an attachment, please make sure it gets "text/plain"
not "application/octet-stream".

BTW, we usually recommend people start a new top level mail thread
for each new version of a patch that is posted, and use the git
commit message first line, as the subject of the mail, and include
the version. eg this would be suitable

   "[PATCH v2] Enable Veritas OpenFlame functionality"

> Here’s the same output from the new test:
> 127) QEMU XML-2-ARGV disk-drive-network-rbd-no-colon                   ...
> Got expected error:
> 2016-06-10 00:39:27.543+0000: 607: error : qemuBuildNetworkDriveURI:859 :
> unsupported configuration: ':' not allowed in RBD source volume name
> 'poolname/imagename:rbd_cache=1:rbd_cache_size=67108864:rbd_cache_max_dirty
> =0'
> OK
> 128) QEMU XML-2-ARGV disk-drive-network-oflame                         ...
> OK
> 129) QEMU XML-2-ARGV disk-drive-no-boot                                ...
> OK

> From 069fe0bafd526a20c1630d32ff481e33acf0781c Mon Sep 17 00:00:00 2001
> From: Ashish Mittal <ashish.mittal at veritas.com>
> Date: Thu, 9 Jun 2016 19:52:12 -0700
> Subject: [PATCH] Bug 1341866 Enable Veritas OpenFlame functionality on RedHat
>  OSP8 platform
> 
> Request to upstream libvirt dependencies for qemu based network block driver from Veritas.

No need to talk about Red Hat / OSP or the bug number in this
commit message. Just describe what it is that the patch is
doing from a purely technical POV.

Since you're enabling use of new XML,it would be helpful to
include the example <disk> XML snippet in the commit message
for clarity.

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b5d84e6..162f807 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -497,6 +497,7 @@ qemuNetworkDriveGetPort(int protocol,
>              return 0;
>  
>          case VIR_STORAGE_NET_PROTOCOL_RBD:
> +        case VIR_STORAGE_NET_PROTOCOL_OFLAME:
>          case VIR_STORAGE_NET_PROTOCOL_LAST:
>          case VIR_STORAGE_NET_PROTOCOL_NONE:
>              /* not applicable */
> @@ -894,6 +895,56 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
>              ret = virBufferContentAndReset(&buf);
>              break;
>  
> +        case VIR_STORAGE_NET_PROTOCOL_OFLAME:
> +            if (VIR_ALLOC(uri) < 0)
> +                goto cleanup;
> +
> +            if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
> +                if (VIR_STRDUP(uri->scheme,
> +                               virStorageNetProtocolTypeToString(src->protocol)) < 0)
> +                    goto cleanup;
> +            } else {
> +                if (virAsprintf(&uri->scheme, "%s+%s",
> +                                virStorageNetProtocolTypeToString(src->protocol),
> +                                virStorageNetHostTransportTypeToString(src->hosts->transport)) < 0)
> +                    goto cleanup;
> +            }
> +
> +            if (src->path) {
> +                if (src->volume) {
> +                    if (virAsprintf(&uri->path, "/%s%s",
> +                                    src->volume, src->path) < 0)
> +                        goto cleanup;
> +                } else {
> +                    if (virAsprintf(&uri->path, "%s%s",
> +                                    src->path[0] == '/' ? "" : "/",
> +                                    src->path) < 0)
> +                        goto cleanup;
> +                }
> +            }
> +
> +            if (src->hosts->socket &&
> +                virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0)
> +                goto cleanup;
> +
> +            if (qemuBuildGeneralSecinfoURI(uri, secinfo) < 0)
> +                goto cleanup;
> +
> +            for (i = 0; i < src->nhosts; i++) {
> +                if ((uri->port = qemuNetworkDriveGetPort(src->protocol, src->hosts[i].port)) < 0)
> +                    goto cleanup;
> +
> +                if (VIR_STRDUP(uri->server, src->hosts[i].name) < 0)
> +                    goto cleanup;
> +
> +            ret = virURIFormat(uri);
> +            virBufferEscape(&buf, ',', ",", "%s", ret);
> +            virReportError(VIR_ERR_INTERNAL_ERROR,

Your indentation got a little messed up here.

> +                _("qemuBuildOFlameString builturi '%s'"), ret);
> +            }
> +
> +            ret = virBufferContentAndReset(&buf);
> +            break;
>  
>          case VIR_STORAGE_NET_PROTOCOL_LAST:
>          case VIR_STORAGE_NET_PROTOCOL_NONE:

> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-oflame.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-oflame.args
> new file mode 100644
> index 0000000..5083cae
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-oflame.args
> @@ -0,0 +1,24 @@

> +-drive file=oflame://192.168.0.1:9999/%7Beb90327c-8302-4725-9e1b-4e85ed4dc251%7D\
> +oflame://172.172.17.56:9999/%7Beb90327c-8302-4725-9e1b-4e85ed4dc251%7D,\

Hmm, unless I'm mis-reading this, there is no separator between
the two URIs you're providing - is there a missing ',' or something
similar.

Slightly off topic for the libvirt list, but I would be pretty surprised
if the QEMU developers accepted patches using this URI syntax for providing
multiple servers.

A similar approach was originally proposed for the glusterfs driver and
the submitter was told to write it a different way, not using the legacy
URI syntax at all, but instead a QAPI based syntax:

  https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg04126.html

So the sooner you can submit your QEMU patches to the qemu-devel mailing
list the better, as we'll need to get clarity on the accepted QEMU syntax
in order to proceed further with libvirt patch review.

Broadly speaking I think the patch you've proposed looks pretty much
fine now. I'd have a slight preference for it to be done as two patches.
One patch adds the XML schema, parser changes, etc. The second patch
just does the QEMU driver integration & unit tests.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

[libvirt] Patch review request for Red Hat Bugzilla ­ Bug 1341866

[libvirt] Patch review request for Red Hat Bugzilla Bug 1341866