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

[John Ferlan]

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