Re: [Qemu-devel] [PATCH v7 0/3] 9p: Fix file ID collisions

[Greg Kurz <groug@kaod.org>]

On Mon, 23 Sep 2019 16:06:13 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 23. September 2019 14:56:11 CEST Greg Kurz wrote:
> > On Mon, 23 Sep 2019 11:50:46 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > On Freitag, 13. September 2019 19:01:57 CEST Greg Kurz wrote:
> > > > So I did some changes in 1/3 and pushed everything to 9p-next.
> > > 
> > > I've reviewed your changes. Some notes:
> > > 
> > > Patch 1:
> > > https://github.com/gkurz/qemu/commit/9295011c5a961603959b966c8aa6ad9840fe6
> > > db2> 
> > > * Typo 1:
> > > 	error_append_hint(&local_err, "Valide options are: multidevs="
> > > 	
> > > 	Valide -> Valid
> > > 
> > > * Typo 2 in log comment:
> > > 	[groug: - Moved "multidevs" parsing the local backend.
> > > 	->
> > > 	[groug: - Moved "multidevs" parsing to the local backend.
> > 
> > Fixed.
> 
> Thanks! Saw it, looks fine now.
> 
> > > > I'll do some
> > > > more manual testing and issue a PR when I'm confident enough.
> > > 
> > > That would be highly appreciated! So far I am the only one ever having
> > > tested this patch set at all!
> > 
> > Just to clarify, I won't thoroughly test it. My main concern is that it
> > doesn't break things. 
> 
> So in other words you are only going to test the default behaviour
> --multidevs=warn?
> 

This I've already done, along with multidevs=forbid.

Now I plan to run the PJD test suite from Tuxera with a simple
cross-device setup and --multidevs=remap. And that's it.

> If yes, and since that would mean I was the only person ever having tested the 
> actual fix, shouldn't --multidevs=remap|forbid better be marked as 
> experimental (docs and runtime warning) for now? Maybe that would also 
> anticipate receiving feedback from people actually using it later on.
> 

Makes sense. I don't think it is worth having a runtime warning,
but I'll turn remap to x-remap and amend the docs.

> > I usually rely on this:
> > 
> > https://www.tuxera.com/community/posix-test-suite/
> 
> Well, that website is far from being too verbose of what that test suite 
> actually encompasses.
> 

Yeah... this does a variety of tests I've never had time to
audit, but it exercices many paths of the 9pfs code.

> > > > It would be nice to have some sort of automated test for that in 'make
> > > > check'. My first thought is to simulate a cross-device setup with the
> > > > synth
> > > > backend, because it might be difficult to do this on a real filesystem
> > > > without requiring elevated privileges.
> > > 
> > > Hmm, since I neither haven't used the synth backend before, nor added qemu
> > > test cases so far, I am yet missing the complete picture here. My initial
> > > suggested approach would have been using loopback devices for simulating
> > > two file systems, but yes that's probably not viable due to required
> > > permissions. How would the synth backend help here? I mean you would need
> > > to simulate specific inode numbers and device numbers in some way for the
> > > test cases.
> > The synth backend allows to simulate anything you want, provided you
> > code it of course :)
> > 
> > It is currently used to run some 9p protocol conformance tests. Have a
> > look at the backend code to get the idea.
> > 
> > hw/9pfs/9p-synth.h
> > hw/9pfs/9p-synth.c
> > 
> > and the test program:
> > 
> > tests/virtio-9p-test.c
> > 
> > It currently doesn't care for st_dev/st_ino at all, but I guess
> > it shouldn't be that hard to add the necessary bits.
> 
> I see. Well, I will look at it, but that will definitely not be one of my 
> current high priority tasks that would happen in the next few weeks (simply 
> due to my work load).
> 

Same here :)