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