Hello Michael, André, Could you do a quick review before a final submission ?
http://paste.ubuntu.com/23446279/ - I split the commits into 1) bugfix, 2) new util with test, 3) vhostlog The unit test is testing passing fds between 2 processes and asserting contents of mmap buffer coming from the "vhostlog" util (mmap-file). Your final comment on the "vhostlog" was: >> Argv examples: >> >> -netdev tap,id=net0,vhost=on >> -netdev tap,id=net0,vhost=on,vhostlog=/tmp/guest.log >> -netdev tap,id=net0,vhost=on,vhostlog=/tmp (André) > Could it be only a filename? This would simplify testing. (Michael) > When vhostlog is not specified, can we just use memfd as we did? I'm going to change this to: 1 - if vhostlog is not provided shared log can't be used. Use memfd. 2 - for shared logs, vhostlog has to be provided as a "file" ? Should i keep vhostlog being a directory also ? (i know we are unlinking the file so might not be needed BUT a static file might have a race condition in between different instances and providing a directory - that creates random files on it - might be better approach). Is there anything else ? Thank you Rafael Tinoco On Mon, Oct 31, 2016 at 8:30 PM, Michael S. Tsirkin <m...@redhat.com> wrote: > On Mon, Oct 31, 2016 at 08:35:33AM -0200, Rafael David Tinoco wrote: >> On Sun, Oct 30, 2016 at 5:26 PM, Michael S. Tsirkin <m...@redhat.com> wrote: >> > >> > On Sat, Oct 22, 2016 at 07:00:41AM +0000, Rafael David Tinoco wrote: >> > > Commit 31190ed7 added a migration blocker in vhost_dev_init() to >> > > check if memfd would succeed. It is better if this blocker first >> > > checks if vhost backend requires shared log. This will avoid a >> > > situation where a blocker is added inappropriately (e.g. shared >> > > log allocation fails when vhost backend doesn't support it). >> > >> > Sounds like a bugfix but I'm not sure. Can this part be split >> > out in a patch by itself? >> >> Already sent some days ago (and pointed by Marc today). >> >> > > Commit: 35f9b6e added a fallback mechanism for systems not supporting >> > > memfd_create syscall (started being supported since 3.17). >> > > >> > > Backporting memfd_create might not be accepted for distros relying >> > > on older kernels. Nowadays there is no way for security driver >> > > to discover memfd filename to be created: <tmpdir>/memfd-XXXXXX. >> > > >> > > Also, because vhost log file descriptors can be passed to other >> > > processes, after discussion, we thought it is best to back mmap by >> > > using files that can be placed into a specific directory: this commit >> > > creates "vhostlog" argv parameter for such purpose. This will allow >> > > security drivers to operate on those files appropriately. >> > > >> > > Argv examples: >> > > >> > > -netdev tap,id=net0,vhost=on >> > > -netdev tap,id=net0,vhost=on,vhostlog=/tmp/guest.log >> > > -netdev tap,id=net0,vhost=on,vhostlog=/tmp >> > > >> > > For vhost backends supporting shared logs, if vhostlog is non-existent, >> > > or a directory, random files are going to be created in the specified >> > > directory (or, for non-existent, in tmpdir). If vhostlog is specified, >> > > the filepath is always used when allocating vhost log files. >> > >> > When vhostlog is not specified, can we just use memfd as we did? >> > >> >> This was my approach on a "pastebin" example before this patch (in the >> discussion thread we had). Problem goes back to when vhost log file >> descriptor is shared with some vhost-user implementation - like the >> interface allows to - and the security driver labelling issue. IMO, >> yes, we could let vhostlog to specify a log file, and, if not >> specified, assume memfd is ok to be used. >> >> Please let me know if you - and Marc - want me to keep using memfd. >> I'll create the mmap-file tests and files in a different commit, like >> Marc has asked for, and will propose the patch again by the end of >> this week. > > I think that the best approach is to allow passing in the fd, > not the file path. If not passed, use memfd. > > -- > MST
