[libvirt] Patch review request for Red Hat Bugzilla Bug 1341866
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 (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. Regards, Ashish
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 :|
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@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 :|
On Wed, Jun 08, 2016 at 02:55:24PM +0000, Ashish Mittal wrote:
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.
Ah, that sounds great.
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.
Hmm, I don't appear to have received that other patch and there is only this first patch visible in the mailing list archives https://www.redhat.com/archives/libvir-list/2016-June/author.html I guess its possible there's a delay in our mailman delivery, but worth checking you used the right git send-email config too, in case of typos/etc 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 :|
Hi Daniel, I am sending an updated patch with the following changes: (1) Removed code pertaining to storage pools as we do not do use it at present. (2) Rebased patch to latest libvirt master (could be a day old as I cannot do a git pull locally) (3) Updated docs/formatdomain.html.in (4) Modified code to securely pass secrets to libvirt. (5) Added test case for openflame with new argv and sample xml files. Please try to take a quick look at the patch to see if you approve of the changes. 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? (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. 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 Regards, Ashish On 6/8/16, 8:05 AM, "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Jun 08, 2016 at 02:55:24PM +0000, Ashish Mittal wrote:
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.
Ah, that sounds great.
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.
Hmm, I don't appear to have received that other patch and there is only this first patch visible in the mailing list archives
https://www.redhat.com/archives/libvir-list/2016-June/author.html
I guess its possible there's a delay in our mailman delivery, but worth checking you used the right git send-email config too, in case of typos/etc
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 :|
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@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 :|
participants (2)
-
Ashish Mittal -
Daniel P. Berrange