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

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 :|