Re: [Qemu-devel] [PATCH 00/22] 9pfs: disentangling virtio and generic code

[Stefano Stabellini]

On Thu, 7 Jan 2016, Greg Kurz wrote:
> On Thu, 7 Jan 2016 13:32:43 +0100
> Greg Kurz <address@hidden> wrote:
> 
> > On Thu, 7 Jan 2016 11:59:22 +0000
> > Stefano Stabellini <address@hidden> wrote:
> > 
> > > On Tue, 5 Jan 2016, Wei Liu wrote:
> > > > Hi all
> > > > 
> > > > Back in 2015 summer one of our OPW interns Linda Jacobson explored the
> > > > possibility of making 9pfs work on Xen. It turned out lots of code in 
> > > > QEMU can
> > > > be reused.
> > > > 
> > > > This series refactors 9pfs related code:
> > > > 
> > > > 1. Rename a bunch of files and functions to make clear they are generic.
> > > > 2. Only export two functions (marshal and unmarshal) from transport to 
> > > > generic
> > > >    code.
> > > > 3. disentangle virtio transport code and generic 9pfs code.
> > > > 4. Some function name clean-up.
> > > 
> > > The series looks pretty good and it looks like a clean improvement over
> > > the current code base.
> > > 
> 
> Stefano,
> 
> Can I add your Reviewed-by to the patches ?

Yes, go ahead, thanks.


> > I agree Wei did a great job. I'll fix the title in patch 3 and push it to
> > my tree next week.
> > 
> > Thanks.
> > 
> > --
> > Greg
> > 
> > > 
> > > 
> > > > To make sure this series doesn't break compilation a rune is use to 
> > > > compile
> > > > every commit.
> > > > 
> > > > $ git rebase -i origin/master --exec "make -j16 clean; ./configure 
> > > > --target-list=x86_64-softmmu --enable-virtfs --enable-kvm; make -j16 && 
> > > > echo ok."
> > > > 
> > > > Three use cases are tested:
> > > > 
> > > > 1. Local file system driver with passthrough security policy
> > > > ./qemu-system-x86_64 -L .  -hda /dev/DATA/jessie   -vnc 0.0.0.0:0,to=99 
> > > >  -fsdev local,path=/root/qemu,security_model=passthrough,id=fs1  
> > > > -device virtio-9p-pci,fsdev=fs1,mount_tag=qemu  &
> > > > 
> > > > 2. Local file system driver with mapped security policy
> > > > ./qemu-system-x86_64 -L . -hda /dev/DATA/jessie -vnc 0.0.0.0:0,to=99 
> > > > -fsdev local,path=/root/qemu,security_model=mapped,id=fs1 -device 
> > > > virtio-9p-pci,fsdev=fs1,mount_tag=qemu &
> > > > 
> > > > 3. Proxy file system driver
> > > > ./virtfs-proxy-helper -p /root/qemu -n -s virtfs-helper-sock -u 0 -g 0 &
> > > > ./qemu-system-x86_64 -L .  -hda /dev/DATA/jessie   -vnc 0.0.0.0:0,to=99 
> > > >  -fsdev proxy,socket=virtfs-helper-sock,id=fs1  -device 
> > > > virtio-9p-pci,fsdev=fs1,mount_tag=qemu &
> > > > 
> > > > During each of the tests, mounting, unmounting, read and write 
> > > > operations are
> > > > performed. In "mapped" test, getfattr in host was used to inspect the
> > > > attributes.
> > > > 
> > > > Let me know if you would like to see more tests.
> > > > 
> > > > Xen transport is still under development. I figure it would be better 
> > > > to post
> > > > this series as soon as possible because rebasing such huge series from 
> > > > time to
> > > > time is prone to error.
> > > > 
> > > > Comments are welcome. Thanks!
> > > > 
> > > > Wei.
> > > > ---
> > > > Cc: "Aneesh Kumar K.V" <address@hidden>
> > > > Cc: Greg Kurz <address@hidden>
> > > > Cc: "Michael S. Tsirkin" <address@hidden>
> > > > Cc: Stefano Stabellini <address@hidden>
> > > > ---
> > > > 
> > > > Wei Liu (22):
> > > >   9pfs: rename virtio-9p-coth.{c,h} to coth.{c,h}
> > > >   9pfs: rename virtio-9p-handle.c to 9p-handle.c
> > > >   9pfs: rename virtio-9p-handle.c to 9p-handle.c
> > > >   9pfs: rename virtio-9p-posix-acl.c to 9p-posix-acl.c
> > > >   9pfs: rename virtio-9p-proxy.{c,h} to 9p-proxy.{c,h}
> > > >   9pfs: rename virtio-9p-synth.{c,h} to 9p-synth.{c,h}
> > > >   9pfs: rename virtio-9p-xattr{,-user}.{c,h} to 9p-xattr{,-user}.{c,h}
> > > >   9pfs: merge hw/virtio/virtio-9p.h into hw/9pfs/virtio-9p.h
> > > >   9pfs: remove dead code
> > > >   fsdev: break out 9p-marshal.{c,h} from virtio-9p-marshal.{c,h}
> > > >   fsdev: 9p-marshal: introduce V9fsBlob
> > > >   9pfs: use V9fsBlob to transmit xattr
> > > >   fsdev: rename virtio-9p-marshal.{c,h} to 9p-iov-marshal.{c,h}
> > > >   9pfs: PDU processing functions don't need to take V9fsState as
> > > >     argument
> > > >   9pfs: PDU processing functions should start pdu_ prefix
> > > >   9pfs: make pdu_{,un}marshal proper functions
> > > >   9pfs: factor out virtio_pdu_{,un}marshal
> > > >   9pfs: factor out pdu_push_and_notify
> > > >   9pfs: break out virtio_init_iov_from_pdu
> > > >   9pfs: break out generic code from virtio-9p.{c,h}
> > > >   9pfs: factor out v9fs_device_{,un}realize_common
> > > >   9pfs: disentangle V9fsState
> > > > 
> > > >  Makefile                                           |   2 +-
> > > >  fsdev/{virtio-9p-marshal.c => 9p-iov-marshal.c}    | 205 ++++++-----
> > > >  fsdev/9p-iov-marshal.h                             |  15 +
> > > >  fsdev/9p-marshal.c                                 |  64 ++++
> > > >  fsdev/{virtio-9p-marshal.h => 9p-marshal.h}        |  26 +-
> > > >  fsdev/Makefile.objs                                |   2 +-
> > > >  fsdev/virtfs-proxy-helper.c                        |   6 +-
> > > >  hw/9pfs/{virtio-9p-handle.c => 9p-handle.c}        |   7 +-
> > > >  hw/9pfs/{virtio-9p-local.c => 9p-local.c}          |   7 +-
> > > >  hw/9pfs/{virtio-9p-posix-acl.c => 9p-posix-acl.c}  |   7 +-
> > > >  hw/9pfs/{virtio-9p-proxy.c => 9p-proxy.c}          |   7 +-
> > > >  hw/9pfs/{virtio-9p-proxy.h => 9p-proxy.h}          |  10 +-
> > > >  hw/9pfs/{virtio-9p-synth.c => 9p-synth.c}          |   6 +-
> > > >  hw/9pfs/{virtio-9p-synth.h => 9p-synth.h}          |   6 +-
> > > >  .../{virtio-9p-xattr-user.c => 9p-xattr-user.c}    |   7 +-
> > > >  hw/9pfs/{virtio-9p-xattr.c => 9p-xattr.c}          |   7 +-
> > > >  hw/9pfs/{virtio-9p-xattr.h => 9p-xattr.h}          |   6 +-
> > > >  hw/9pfs/{virtio-9p.c => 9p.c}                      | 301 
> > > > ++++++++++------
> > > >  hw/9pfs/9p.h                                       | 335 
> > > > ++++++++++++++++++
> > > >  hw/9pfs/Makefile.objs                              |  14 +-
> > > >  hw/9pfs/codir.c                                    |   2 +-
> > > >  hw/9pfs/cofile.c                                   |   2 +-
> > > >  hw/9pfs/cofs.c                                     |   2 +-
> > > >  hw/9pfs/{virtio-9p-coth.c => coth.c}               |   4 +-
> > > >  hw/9pfs/{virtio-9p-coth.h => coth.h}               |  15 +-
> > > >  hw/9pfs/coxattr.c                                  |   2 +-
> > > >  hw/9pfs/virtio-9p-device.c                         | 196 ++++++-----
> > > >  hw/9pfs/virtio-9p.h                                | 391 
> > > > +--------------------
> > > >  hw/s390x/virtio-ccw.h                              |   2 +-
> > > >  hw/virtio/virtio-pci.h                             |   4 +-
> > > >  include/hw/virtio/virtio-9p.h                      |  24 --
> > > >  31 files changed, 927 insertions(+), 757 deletions(-)
> > > >  rename fsdev/{virtio-9p-marshal.c => 9p-iov-marshal.c} (53%)
> > > >  create mode 100644 fsdev/9p-iov-marshal.h
> > > >  create mode 100644 fsdev/9p-marshal.c
> > > >  rename fsdev/{virtio-9p-marshal.h => 9p-marshal.h} (78%)
> > > >  rename hw/9pfs/{virtio-9p-handle.c => 9p-handle.c} (99%)
> > > >  rename hw/9pfs/{virtio-9p-local.c => 9p-local.c} (99%)
> > > >  rename hw/9pfs/{virtio-9p-posix-acl.c => 9p-posix-acl.c} (97%)
> > > >  rename hw/9pfs/{virtio-9p-proxy.c => 9p-proxy.c} (99%)
> > > >  rename hw/9pfs/{virtio-9p-proxy.h => 9p-proxy.h} (89%)
> > > >  rename hw/9pfs/{virtio-9p-synth.c => 9p-synth.c} (99%)
> > > >  rename hw/9pfs/{virtio-9p-synth.h => 9p-synth.h} (94%)
> > > >  rename hw/9pfs/{virtio-9p-xattr-user.c => 9p-xattr-user.c} (96%)
> > > >  rename hw/9pfs/{virtio-9p-xattr.c => 9p-xattr.c} (97%)
> > > >  rename hw/9pfs/{virtio-9p-xattr.h => 9p-xattr.h} (97%)
> > > >  rename hw/9pfs/{virtio-9p.c => 9p.c} (93%)
> > > >  create mode 100644 hw/9pfs/9p.h
> > > >  rename hw/9pfs/{virtio-9p-coth.c => coth.c} (95%)
> > > >  rename hw/9pfs/{virtio-9p-coth.h => coth.h} (96%)
> > > >  delete mode 100644 include/hw/virtio/virtio-9p.h
> > > > 
> > > > -- 
> > > > 2.1.4
> > > > 
> > > 
> > 
> > 
>