[libvirt] Patch review request for Red Hat Bugzilla Bug 1341866
Ashish Mittal
Ashish.Mittal at veritas.com
Wed Jun 8 14:55:24 UTC 2016
Thanks Daniel!
I also sent a path on top of libvirt master using "git send-email" as
directed on libvirt.org. That patch is much more cleaner. (1) does not
have storage pool changes since OpenFlame does not use storage pools
(yet). (2) Uses the new encrypted secret passing as the others have done.
Some of your suggestions would still be valid on top of the master patch.
I will make those changes and resubmit.
Can you please confirm if you¹ve received the other patch on top of
master? I will continue to make all future changes on top of master using
git send-email directly, if you so suggest.
Thanks,
Ashish
On 6/8/16, 7:32 AM, "Daniel P. Berrange" <berrange at redhat.com> wrote:
>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 :|
More information about the libvir-list
mailing list