[virt-tools-list] vhostmd - virtio channel support

[Jim Fehlig]

On 05/22/2018 05:14 AM, Trapp, Michael wrote:
> On 17.05.18, 14:23, "Daniel P. Berrangé" <berrange at redhat.com> wrote:
>> On Thu, May 17, 2018 at 12:13:58PM +0000, Trapp, Michael wrote:
>>> Hi
>>>
>>> I would like to add virtio based communication to vhostmd.
>>>
>>> The current vhostmd implementation writes the metric data of all VMs and
>> the host to a single file. This file is mapped as a disk to all VMs and due to that
>> every VM can see all VMs and also has access to the whole data set of all
>> VMs.
>>> >From security perspective this could be more restrictive and a ‘per  VM’
>> view on the data would help to improve the situation a bit.
>>>
>>>
>>> So far I have implemented the virtio channel based communication
>> between VMs and vhostmd and tested the feature in a local setup.
>>>
>>> Let's start with the relevant VM config:
>>> <domain type='kvm'>
>>>    <name>vm_015</name>
>>>    <uuid>cf335144-567d-11e7-000f-0000594d2d82</uuid>
>>> ...
>>>      <channel type='unix'>
>>>        <source mode='bind' path='/var/lib/libvirt/qemu/channels/cf335144-
>> 567d-11e7-000f-0000594d2d82'/>
>>
>> Ewww, that is a global namespace you're using there - you can't assume
>> this is the only channel using this directory. It needs to include the
>> channel target name in the path as a prefix, as well a unique per-VM
>> identifier of some kind

As an example, the guest agent path is

/var/lib/libvirt/qemu/channel/target/domain-<id>-<name>/org.qemu.guest_agent.0

>>
>>>        <target type='virtio' name='vhostmd'/>
>>
>> We'd generally recomend reverse domain name for channel names, along
>> with
>> a version number in case protocol needs to change. eg perhaps
>>
>>     "org.github.vhostmd.1"
> Okay, I've changed the expected channel name to 'org.github.vhostmd.1.<UUID>'

I don't think the '.<UUID>' suffix is needed. 'org.github.vhostmd.1' should be 
sufficient.

 >>> 
https://github.com/TrappM/vhostmd/commit/4e33175cd403bc1c4f5725b5fe68c74dc209e30a

I didn't do a detailed review, but here are some initial comments from my 
cursory scan:

Please split the patch for easier review. There are several whitespace changes, 
error message improvements, etc that should be broken out in separate patch(es) 
explaining the change. Perhaps another patch to introduce new utility function 
vu_get_vm_by_uuidstr(), another introducing virtio.[ch], and finally a patch 
that changes vhostmd to use the virtio channel.

I think the use of virtio channel should be specified in the vhostmd config file 
($vhostmdsrc/vhostmd.xml) instead of command line argument. The <transport> 
element is already used to specify 'vbd' or 'xenstore'.

I'm not really fond of some style used throughout the vhostmd code, e.g. 'if 
(foo) bar;', but I suppose it is best to strive for consistency. Ensure any new 
code is consistent with the existing style. E.g. I noticed some cases of a space 
between function name and the parens of its parameter list

     vu_log (VHOSTMD_ERR...)

Send patches to this list instead of using github PR workflow (see "Contact" in 
README). I recently broke this rule and merged a trivial PR, and even have a few 
pending PR of my own :-/. I'll revoke those and send proper patches to the list.

Regards,
Jim