Hi Richard, On 10/27/21 19:29, 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: >>>> On 10/27/21 16:05, Christian Schoenebeck wrote:
>>>>> 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. To be explicit, I just asked clarifications to Christian. I understand the 9pfs subsystem situation, and I am not against (Nacking) this pull request being merged. Thanks both, Phil.
