On Fri, Jun 30, 2017 at 1:07 PM, John Ferlan <jferlan@redhat.com> wrote:
On 06/30/2017 11:30 AM, ashish mittal wrote:
Hi,
Thanks for the review!
On Fri, Jun 30, 2017 at 12:41 AM, Peter Krempa <pkrempa@redhat.com> wrote:
On Thu, Jun 29, 2017 at 19:02:38 -0700, Ashish Mittal wrote:
From: Ashish Mittal <ashish.mittal@veritas.com>
QEMU changes for VxHS (including TLS support) are already upstream. This series of patches adds support for VxHS block devices in libvirt.
Patch 1 adds the base functionality for supporting VxHS protocol.
Patch 2 adds two new configuration options in qemu.conf to enable TLS for VxHS devices.
Patch 3 implements the main TLS functionality.
This ordering is wrong and also your patches have a lot of stuff going on at once.
Recall what I pointed out last week when you were @ Westford. Try to find a "happy medium" between throwing everything into a few patches and over creating patches for the sake of having really small compileable and testable patches. It is a delicate balance...
I suggest you add the TLS support (Which should be designed to be generic with other protocols which may start using TLS in mind) first and then start adding the rest of the stuff.
I'm not sure I understand this point. libvirt currently does not have support of VxHS devices. So I cannot add TLS support for VxHS before base VxHS functionality. If you mean that I should add generic TLS handling functionality for block device protocols, then it would probably just be some new variables in structures and bare-bone functions (1 or 2) that don't do much. None of the block devices at present use TLS, so even if I add generic TLS code, how would I test it in the same patch unit?
I'm not so sure we could have generic block TLS env. IIUC, the _tls_x509_cert_dir (or "s:dir" to the tls-creds-x509 object) is the location for the server certificate that QEMU would present to say VxHS server. Whereas, NBD which could use the migrate TLS environment (or it's own I suppose, but we use it for migration). If Gluster, iSCSI, RBD, etc. allowed the ability to use TLS instead of <auth ...> secrets, then they too could conceivably have their own environment.
This isn't a QEMU to QEMU communication, right? It's QEMU to some server where the storage is located from whence you'll get your storage.
That is correct! In this case qemu is the client and the actual VxHS storage server is remote. The use of TLS/SSL here is to secure this communication.
John
I'm sure if I have this wrong someone will correct me...
I'll reply to some parts of the patches separately, but in this state it's kind of a mess to go through, so please re-send the patch split into reasonable chunks.
Note that each patch needs to make sense and can be compiled and tested individually.
Regards, Ashish