On Mittwoch, 27. Oktober 2021 20:44:52 CEST Richard Henderson wrote: > On 10/27/21 10:29 AM, Christian Schoenebeck wrote: > > 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. > In know the feeling -- it takes quite some prodding to get tcg patches > reviewed, and I have also sent out PRs that are incompletely reviewed. > > I see that patch 5 was something I suggested myself, which I then failed to > review afterward. In recompense, I have reviewed the whole patch set: > > Reviewed-by: Richard Henderson <richard.hender...@linaro.org> > > I think the P9Array is fairly clever, and I do prefer it over glib ugliness. > I can imagine only C++ being an improvement over what you've created. > > Rather than force you to re-create the PR, I'll simply apply this along with > the S-o-b, to the merge object. > > > r~
Thanks Richard, I highly appreciate that! Best regards, Christian Schoenebeck
