Message-ID: <8b4106e0-8523-56b6-94ef-b7add020497a@users.sourceforge.net>
Date:   Wed, 14 Sep 2016 08:56:35 +0200
From:   SF Markus Elfring <elfring@...rs.sourceforge.net>
To:     Christian Bornträger <borntraeger@...ibm.com>,
        virtualization@...ts.linux-foundation.org,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Minfei Huang <minfei.hmf@...baba-inc.com>,
        Cornelia Huck <cornelia.huck@...ibm.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Julia Lawall <julia.lawall@...6.fr>,
        kernel-janitors@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        Chao Fan <fanc.fnst@...fujitsu.com>
Subject: Re: virtio_blk: Less function calls in init_vq() after error
 detection

>>    How much will it matter in general that two function calls are performed
>>    in this use case without checking their return values immediately?
>>    https://cwe.mitre.org/data/definitions/252.html
>>
>> 	if (!names || !callbacks || !vqs) { …
>>
>>    https://cwe.mitre.org/data/definitions/754.html
> 
> The return values are checked,

I agree to this information.


> just a bit later.

I suggest to reconsider this design detail if it is really acceptable
for the safe implementation of such a software module.


> Markus, kernel patches are not about be formally correct vs. CodingStyle and/or checkpatch

I guess that some of the involved technical advices will also matter here, don't they?


> or against code guidelines from sombody else.

Some ideas or advices are integrated from other information sources also into various
Linux software.


> You will find many people consider gotos an no-go,

I became trained also in this design direction for a while.


> still it is accepted in the kernel for good reasons.

I can follow such reasons to some degree.


> You have to think about each change and its tradeoffs (e.g simplicity vs. performance) 
> for each code part again.

This can happen as usual for an ordinary source code review process.
I would be interested to clarify if items could be optimised for repeated checks
in this work flow.


> Here we have slow path error handling, so given the low coverage of these code parts,

Would you like to add any other background information for this aspect?


> simplicity is important.

I can follow this opinion to some degree.


> Yes, your code makes an unlikely error case bail out faster,

Is this kind of functionality desired finally?


> but is the cost of your patch (review time, danger of adding bugs, danger of merge conflicts,
> making git blame less useful) in sync with the expected win?

I hope so.

Do the mentioned aspects express a fear or other general concerns which hinder changes
and make the proposed software improvement unlikely?


> (Well, having a label between the if and the function like
>>   	if (err)
>> + free_vblk_vqs:
>>  		kfree(vblk->vqs);
> 
> is certainly ugly in itself)

Would you find such a source code approach better if curly brackets will be
added there?


> Do you realize that your discussion style is not very helpful?

I find that this view is debatable.


> I just grepped the last LKML mails and you already pissed of several maintainers
> in totally different areas.

Yes. - It can happen that some contributors get occasionally grumpy.

Would you like to discuss their reasons a bit more?


> When that happens, why don't you stop for a moment
> and think about "what is going wrong right now". 

This happened also a few times already.


> Your attitude seems to be "I spend my spare time doing this, please thank me for that".

I guess that I am not so different in this aspect as I am trying to be another
respectable free software developer for years.


> The thing is, with each patch you actually request time from the maintainer.

This is a consequence of software development as usual.


> Now here begins the interesting part: Is the patch just a cosmetic change that does
> not give you any benefit or is the patch improving the code.

How do you categorise my concrete update suggestion which builds on previous contributions
by others?


> And remember: there are always tradeoffs: performance, code size, code maintainability etc.

I find that I pick some of such design goals up for my software contributions already
for a while.


> See, some of your patches are accepted, e.g. the memdup_user changes have usually
> been applied by most maintainers including myself.

I thank you once more that you could also accept one of the proposed special software updates.


> If maintainers won't take other change, please accept that.

It can happen that change acceptance needs to evolve over time.
Can a healthy pressure help to achieve special software improvements?


> If you continue to waste peoples time by discussing "maybe" patches

Can it be that this category of software updates has got a high potential for further
clarifications because some approaches are occasionally interpreted as controversial?


> you actually risk that people will stop taking any patches from you
> including the "yes" ones.

I assume that this risk is hard to avoid. - It is a matter for each contributor
on how the desired communication should evolve further.


My knowledge evolved in the way that I am using some tools for
static source code analysis. Such advanced tools can point various
change opportunities out. I picked a few special search patterns up.
It happened then that hundreds of source files were found which contain
update candidates.


Further challenges are relevant then as usual.

* Handling of the search process and their results

* Communication between contributors


Search patterns can occasionally be categorised as "too special".
The software technology contains also the risk for showing "false positives".

The reactions of code reviewers are varying between agreement and rejection.


The discussed concrete (and small) patch series is just another example
for usual difficulties or more interesting software development challenges.
I hope that they can be resolved in a systematic way.

So there are further constraints to consider, aren't there?

Regards,
Markus

Powered by blists - more mailing lists