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

[Christian Schoenebeck]

On Montag, 23. September 2019 14:56:11 CEST Greg Kurz wrote:
> On Mon, 23 Sep 2019 11:50:46 +0200
> 
> Christian Schoenebeck <address@hidden> 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?

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.

> 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.

> > > 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).