On 05/22/2018 05:14 AM, Trapp, Michael wrote:
On 17.05.18, 14:23, "Daniel P. Berrangé" <berra...@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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
