Re: [libvirt] [PATCH v4 0/3] Add support for Veritas HyperScale (VxHS) block device protocol

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(a)redhat.com&gt; wrote: >> On Thu, Jun 29, 2017 at 19:02:38 -0700, Ashish Mittal wrote: >>> From: Ashish Mittal <ashish.mittal(a)veritas.com&gt; >>> >>> 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.

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 >