On Tue, Jun 07, 2016 at 08:54:56PM +0000, Ashish Mittal wrote:
> Hi,
>
> This is a patch review request in response to the following bugzilla:
> Bug 1341866 RFE: Request to upstream libvirt dependencies for qemu based network block driver from Veritas
Great, thanks for moving the discussion to this upstream mailing
list instead of BZ.
>
> (1) This patch is required to enable Veritas OpenFlame
> functionality on RedHat OSP8 platform.
> (2) Patch has been created on top of libvirt-1.2.17-13.el7.x86_64.
> Please let me know in case patch has to be rebased on top of the
> latest libvirt git master branch instead.
Yep, patches sent for libvirt should always be rebased on top of the
most recent git master available at time of posting.
BTW, it is preferrable to send patches inline to the mail, as a formal
git commit, rather than as an atachment. The easiest way todo this is
to just use 'git send-email'.
None the less I'll do a quick look at your patch here;
> --- a/docs/schemas/domaincommon.rng.orig 2016-01-04 17:00:06.332000000 -0800
> +++ b/docs/schemas/domaincommon.rng 2016-01-07 18:53:02.889000000 -0800
> @@ -1369,6 +1369,7 @@
> <value>ftp</value>
> <value>ftps</value>
> <value>tftp</value>
> + <value>oflame</value>
> </choice>
> </attribute>
> <optional>
Ok, this is adding a new network based disk protocol for guests.
Any change to the 'rng' file should also update the docs/formatdomain.html.in
with documentation.
As a general policy, we'd suggest having multiple patches. One that
does the RNG schema, docs and XML parser additions, and then a separate
patch that does the QEMU driver implementation.
> --- a/src/storage/storage_driver.c.orig 2016-01-04 17:00:01.149000000 -0800
> +++ b/src/storage/storage_driver.c 2016-01-07 18:53:02.890000000 -0800
> @@ -1534,6 +1534,7 @@
> case VIR_STORAGE_POOL_RBD:
> case VIR_STORAGE_POOL_SHEEPDOG:
> case VIR_STORAGE_POOL_ZFS:
> + case VIR_STORAGE_POOL_OFLAME:
> case VIR_STORAGE_POOL_LAST:
> if (VIR_STRDUP(stable_path, path) < 0) {
> virStoragePoolObjUnlock(pool);
> @@ -3345,6 +3346,7 @@
> case VIR_STORAGE_POOL_RBD:
> case VIR_STORAGE_POOL_SHEEPDOG:
> case VIR_STORAGE_POOL_GLUSTER:
> + case VIR_STORAGE_POOL_OFLAME:
> case VIR_STORAGE_POOL_LAST:
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("using '%s' pools for backing 'volume' disks "
Here you're defining a new type of storage pool, but you're not
actually implementing any driver for this storage pool later on,
so this seems rather pointless. Likewise all the other places in
your patch related to VIR_STORAGE_POOL_OFLAME.
> --- a/src/util/virstoragefile.h.orig 2016-01-04 16:59:52.034000000 -0800
> +++ b/src/util/virstoragefile.h 2016-01-07 18:53:02.895000000 -0800
> @@ -130,6 +130,7 @@
> VIR_STORAGE_NET_PROTOCOL_FTP,
> VIR_STORAGE_NET_PROTOCOL_FTPS,
> VIR_STORAGE_NET_PROTOCOL_TFTP,
> + VIR_STORAGE_NET_PROTOCOL_OFLAME,
>
> VIR_STORAGE_NET_PROTOCOL_LAST
> } virStorageNetProtocol;
Ok, matches the rng schema addition.
> --- a/include/libvirt/libvirt-storage.h.orig 2015-02-23 22:04:12.000000000 -0800
> +++ b/include/libvirt/libvirt-storage.h 2016-01-07 19:27:37.983000000 -0800
> @@ -211,6 +211,7 @@
> VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG = 1 << 15,
> VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER = 1 << 16,
> VIR_CONNECT_LIST_STORAGE_POOLS_ZFS = 1 << 17,
> + VIR_CONNECT_LIST_STORAGE_POOLS_OFLAME = 1 << 18,
> } virConnectListAllStoragePoolsFlags;
>
This is also pointless since you've not implemented any OFLAME
storage pool driver.
> int virConnectListAllStoragePools(virConnectPtr conn,
> --- a/src/conf/storage_conf.c.orig 2015-06-30 23:09:47.000000000 -0700
> +++ b/src/conf/storage_conf.c 2016-01-08 13:31:59.350000000 -0800
> @@ -59,7 +59,7 @@
> "dir", "fs", "netfs",
> "logical", "disk", "iscsi",
> "scsi", "mpath", "rbd",
> - "sheepdog", "gluster", "zfs")
> + "sheepdog", "gluster", "zfs", "oflame")
>
> VIR_ENUM_IMPL(virStoragePoolFormatFileSystem,
> VIR_STORAGE_POOL_FS_LAST,
> @@ -272,6 +272,19 @@
> .defaultFormat = VIR_STORAGE_FILE_RAW,
> },
> },
> + {.poolType = VIR_STORAGE_POOL_OFLAME,
> + .poolOptions = {
> + .flags = (VIR_STORAGE_POOL_SOURCE_HOST |
> + VIR_STORAGE_POOL_SOURCE_NETWORK |
> + VIR_STORAGE_POOL_SOURCE_NAME |
> + VIR_STORAGE_POOL_SOURCE_DIR),
> + },
> + .volOptions = {
> + .defaultFormat = VIR_STORAGE_FILE_RAW,
> + .formatFromString = virStorageVolumeFormatFromString,
> + .formatToString = virStorageFileFormatTypeToString,
> + }
> + },
> };
>
>
> @@ -1173,6 +1186,7 @@
> * files, so they don't have a target */
> if (def->type != VIR_STORAGE_POOL_RBD &&
> def->type != VIR_STORAGE_POOL_SHEEPDOG &&
> + def->type != VIR_STORAGE_POOL_OFLAME &&
> def->type != VIR_STORAGE_POOL_GLUSTER) {
> virBufferAddLit(buf, "<target>\n");
> virBufferAdjustIndent(buf, 2);
> @@ -2572,6 +2586,13 @@
> /* Only one mpath pool is valid per host */
> matchpool = pool;
> break;
> + case VIR_STORAGE_POOL_OFLAME:
> + if (STREQ(pool->def->source.name, def->source.name) &&
> + STREQ_NULLABLE(pool->def->source.dir, def->source.dir) &&
> + virStoragePoolSourceMatchSingleHost(&pool->def->source,
> + &def->source))
> + matchpool = pool;
> + break;
> case VIR_STORAGE_POOL_RBD:
> case VIR_STORAGE_POOL_LAST:
> break;
> @@ -2655,7 +2676,9 @@
> (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG) &&
> (poolobj->def->type == VIR_STORAGE_POOL_SHEEPDOG)) ||
> (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER) &&
> - (poolobj->def->type == VIR_STORAGE_POOL_GLUSTER))))
> + (poolobj->def->type == VIR_STORAGE_POOL_GLUSTER)) ||
> + (MATCH(VIR_CONNECT_LIST_STORAGE_POOLS_OFLAME) &&
> + (poolobj->def->type == VIR_STORAGE_POOL_OFLAME))))
> return false;
> }
>
Everything in this file is pointless again.
> --- a/src/qemu/qemu_command.c.orig 2016-01-04 17:00:15.978000000 -0800
> +++ b/src/qemu/qemu_command.c 2016-04-05 14:30:15.033000000 -0700
> @@ -3074,6 +3074,16 @@
> return -1;
> }
>
> +static int
> +qemuParseOflameString(virDomainDiskDefPtr def)
> +{
> + virURIPtr uri = NULL;
> +
> + if (!(uri = virURIParse(def->src->path)))
> + return -1;
> +
> + return qemuParseDriveURIString(def, uri, "oflame");
> +}
>
> static int
> qemuNetworkDriveGetPort(int protocol,
> @@ -3122,6 +3132,7 @@
> case VIR_STORAGE_NET_PROTOCOL_RBD:
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> case VIR_STORAGE_NET_PROTOCOL_NONE:
> + case VIR_STORAGE_NET_PROTOCOL_OFLAME:
> /* not applicable */
> return -1;
> }
> @@ -3335,6 +3346,67 @@
> ret = virBufferContentAndReset(&buf);
> break;
>
> + case VIR_STORAGE_NET_PROTOCOL_OFLAME:
> + if (src->nhosts != 1)
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("protocol '%s' accepts more than one host, %ld, hostname %s"),
> + virStorageNetProtocolTypeToString(src->protocol), src->nhosts, src->hosts->name);
> + 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 (username) {
> + if (secret) {
> + if (virAsprintf(&uri->user, "%s:%s", username, secret) < 0)
> + goto cleanup;
This is passing the secret password / authentication key to QEMU directly on
the command line where it is visible to all users on the host. I realize you
probably just copied historical QEMU (bad) practice, but we should not
perpetuate this insecure design.
As of QEMU 2.6 codebase, there is a new facility for securely passing secrets
over to QEMU. eg you use the "-object secret" argument to load the secret and
then in the -driver arg simply refer to the ID of the secret. With RBD this
looks like
echo "QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=" > poolkey.b64
$QEMU -object secret,id=secret0,file=poolkey.b64,format=base64 \
-drive driver=rbd,filename=rbd:pool/image:id=myname:\
auth_supported=cephx,password-secret=secret0
Libvirt just gained support for using "-object secret" too, so we should be
making use of that straight away.
> + } else {
> + if (VIR_STRDUP(uri->user, username) < 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,
> + _("qemuBuildOFlameString builturi '%s'"), ret);
> + }
> +
> + ret = virBufferContentAndReset(&buf);
> + break;
>
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> case VIR_STORAGE_NET_PROTOCOL_NONE:
Any change to the qemu_command.c file should also be accompanied by
additions to tests/qemuxml2argvtest.c with example XML files and
ARGV files to provide the conversion is working.
BTW, I've not seen the corresponding patch to QEMU posted to the
qemu-devel mailing list yet. Our general policy for libvirt is
that while we're happy to review proposed patches, we won't actually
accept patches for merge, until the corresponding patch is merged into
QEMU upstream GIT repo (or at least the block maintainers tree in this
case)
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list