On Mittwoch, 27. Oktober 2021 18:48:10 CEST Philippe Mathieu-Daudé wrote: > On 10/27/21 18:21, Christian Schoenebeck wrote: > > On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote: > >> Hi Christian, > >> > >> On 10/27/21 16:05, Christian Schoenebeck wrote: > >>> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote: > >>>> The following changes since commit > > > > 931ce30859176f0f7daac6bac255dae5eb21284e: > >>>> Merge remote-tracking branch > >>>> 'remotes/dagrh/tags/pull-virtiofs-20211026' > >>>> > >>>> into staging (2021-10-26 07:38:41 -0700) > >>>> > >>>> are available in the Git repository at: > >>>> https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 > >>>> > >>>> for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e: > >>>> 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) > >>>> > >>>> ---------------------------------------------------------------- > >>>> 9pfs: performance fix and cleanup > >>>> > >>>> * First patch fixes suboptimal I/O performance on guest due to > >>>> previously > >>>> > >>>> incorrect block size being transmitted to 9p client. > >>>> > >>>> * Subsequent patches are cleanup ones intended to reduce code > >>>> complexity. > >>>> > >>>> ---------------------------------------------------------------- > >>>> > >>>> Christian Schoenebeck (8): > >>>> 9pfs: fix wrong I/O block size in Rgetattr > >>>> 9pfs: deduplicate iounit code > >>>> 9pfs: simplify blksize_to_iounit() > >>>> 9pfs: introduce P9Array > >>>> fsdev/p9array.h: check scalar type in P9ARRAY_NEW() > >>>> 9pfs: make V9fsString usable via P9Array API > >>>> 9pfs: make V9fsPath usable via P9Array API > >>>> 9pfs: use P9Array in v9fs_walk() > >>>> > >>>> fsdev/9p-marshal.c | 2 + > >>>> fsdev/9p-marshal.h | 3 + > >>>> fsdev/file-op-9p.h | 2 + > >>>> fsdev/p9array.h | 160 > >>>> > >>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c > >>>> > >>>> 70 +++++++++++++---------- > >>>> > >>>> 5 files changed, 208 insertions(+), 29 deletions(-) > >>>> create mode 100644 fsdev/p9array.h > >>> > >>> Regarding last 5 patches: Daniel raised a concern that not using > >>> g_autoptr > >>> would deviate from current QEMU coding patterns: > >>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html > >>> > >>> Unfortunately I saw no way to address his concern without adding > >>> unnecessary code complexity, so I decided to make this a 9p local type > >>> (QArray -> P9Array) for now, which can easily be replaced in future > >>> (e.g. > >>> when there will be something appropriate on glib side). > >> > >> Hmm various patches aren't reviewed yet... In particular > >> patch #5 has a Suggested-by tag without Reviewed-by, this > >> looks odd. > >> > >> See https://wiki.qemu.org/Contribute/SubmitAPullRequest: > >> Don't send pull requests for code that hasn't passed review. > >> A pull request says these patches are ready to go into QEMU now, > >> so they must have passed the standard code review processes. In > >> particular if you've corrected issues in one round of code review, > >> you need to send your fixed patch series as normal to the list; > >> you can't put it in a pull request until it's gone through. > >> (Extremely trivial fixes may be OK to just fix in passing, but > >> if in doubt err on the side of not.) > > > > There are in general exactly two persons adding their RBs to 9p patches, > > which is either Greg or me, and Greg made it already clear that he barely > > has time for anything above trivial set. > > > > So what do you suggest? You want to participate and review 9p patches? > > Well I am a bit surprised... > > $ git log --oneline \ > --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l > 18 > > I also reviewed patch #3 if this pull request... > > > Now I see you posted this 4 times in 2 months, so indeed eventual > reviewers had plenty of time to look at your patches. > > Note I haven't said I'd NAck your pull request, I noticed your own > concern wrt Daniel comment, so I looked at the patch and realized > it was not reviewed, and simply said this is this is odd. > > Regards, > > Phil.
Philippe, of course I understand why this looks odd to you, and yes you reviewed that particular patch. But the situation on the 9p front is like this for >2 years now: people quickly come by to nack patches, but even after having addressed their concerns they barely add their RBs afterwards. That means I am currently forced to send out PRs without RBs once in a while. The mentioned 5 patches look like overkill on first sight, but they are just intended as preparatory ones. I actually should fix a protocol implementation deficit in "Twalk" request handling, and that in turn means I will have to add complexity to function v9fs_walk(). But before even daring to do so, I should get rid of as much of complexity as possible. Because we already had a bunch of issues in that function before. I believe these are trivial 5 patches. But I can also accompany them with test cases if somebody is worried. Greg, I could also drop them now, but the general issue will retain: Reality is that I am the only person working on 9p right now and a fact that I cannot change. The rest is for other people to decide. Best regards, Christian Schoenebeck
