Re: [PULL 0/8] 9p queue 2021-10-27

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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.