[virt-tools-list] vhostmd - virtio channel support
michael.trapp at sap.com
Thu Jun 7 15:42:51 UTC 2018
On 06.06.18, 20:16, "Jim Fehlig" <jfehlig at suse.com> wrote:
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:
>>> 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
>>> >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'>
>>> <channel type='unix'>
>>> <source mode='bind' path='/var/lib/libvirt/qemu/channels/cf335144-
>> 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
>>> <target type='virtio' name='vhostmd'/>
>> We'd generally recomend reverse domain name for channel names, along
>> a version number in case protocol needs to change. eg perhaps
> 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
I didn't do a detailed review, but here are some initial comments from my
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
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.
I fully understand the requirements regarding coding style and the way the changes should be split and discussed in detail,
but may I ask you to provide some general feedback about the basic concept and if this an option at all before we start discussing the details.
More information about the virt-tools-list