Re: [PATCH 00/30] virtiofs daemon (base)

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Thu, Oct 24, 2019 at 12:14:39PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > On Thu, Oct 24, 2019 at 06:59:33AM -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Oct 21, 2019 at 03:33:57PM +0100, Dr. David Alan Gilbert wrote:
> > > > > * no-reply@patchew.org (no-reply@patchew.org) wrote:
> > > > > > Patchew URL: https://patchew.org/QEMU/20191021105832.36574-1-dgilbert@redhat.com/
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > This series seems to have some coding style problems. See output below for
> > > > > > more information:
> > > > > > 
> > > > > > Subject: [PATCH 00/30] virtiofs daemon (base)
> > > > > > Type: series
> > > > > > Message-id: 20191021105832.36574-1-dgilbert@redhat.com
> > > > > > 
> > > > > > === TEST SCRIPT BEGIN ===
> > > > > > #!/bin/bash
> > > > > > git rev-parse base > /dev/null || exit 0
> > > > > > git config --local diff.renamelimit 0
> > > > > > git config --local diff.renames True
> > > > > > git config --local diff.algorithm histogram
> > > > > > ./scripts/checkpatch.pl --mailback base..
> > > > > 
> > > > > Expecting checkpatch to be broken here; most of the files
> > > > > follow FUSE's formatting.
> > > > > 
> > > > > Dave
> > > > 
> > > > I wonder what do others think about this.
> > > > One problem with such inconsistencies is that people tend to copy code
> > > > around, which tends to result in a mess.
> > > 
> > > IIUC, most of this code is simpy copied as-is from the fuse or linux
> > > git repos. I'm wondering what the intention is for it long term ?
> > > 
> > > For header files, I would have expected us to be able to compile against
> > > the -devel package provided by the kernel or fuse packages. I can
> > > understand if we want to import the headers if the VSOCK additions to
> > > them are not yet widely available in distros though. If this is the case
> > > we should put a time limit on how long we'd keep these copied headers
> > > around for before dropping them. It would be fine to violate QEMU coding
> > > style in this case as its not code QEMU would "maintain" long term - just
> > > a read-only import.
> > 
> > The headers are really two types;  one are external definitions, the
> > other are internal parts of libfuse.  I'd expect to keep the internal
> > parts long term; teh external parts hmm; where would we pull them in
> > externally from?
> > 
> > > The source files though, we appear to then be modifying locally, which
> > > suggests they'll live in our repo forever. In this case I'd expect to
> > > have compliance with QEMU coding standards.
> > 
> > OK.
> > 
> > > I'm surprised we need to copy so much in from fuse though. Is there a
> > > case to be made for fuse to provide a library of APIs for people who
> > > are building fuse daemons to link against, instead of copy & fork ?
> > 
> > libfuse *is* such a library; but it preserves ABI compliance; it's
> > intention is to be used to build filesystem implementations on top of -
> > and that's got a fairly good separation;  however changing the fuse
> > transport, and security models is much more invasive than it was
> > designed for.
> > 
> > Dave
> 
> 
> I guess you did try to propose adding the functionality to the libfuse
> maintainer and got rejected? If not it's worth asking.

I looked at it; we started off thinking we'd merge much of it up there -
but there's at least two problems:

  a) Our libvhost-user isn't a separable library either - so we'd need
to copy it into libfuse which is just as big a problem.
  b) It's not additional interfaces, it's changes to existing APIs that
break A*B*I compatibility that are very hard to avoid when you're
changing the transport.

Also, we're not just changing the transport - if you look at the
security patchset we're inverting the security model and there's a bunch
of threading changes that you haven't got yet.

Dave

> 
> 
> > > Regards,
> > > Daniel
> > > -- 
> > > |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> > > |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> > > |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK