From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757652AbcING5S (ORCPT ); Wed, 14 Sep 2016 02:57:18 -0400 Received: from mout.web.de ([212.227.15.3]:55914 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751584AbcING5Q (ORCPT ); Wed, 14 Sep 2016 02:57:16 -0400 Subject: Re: virtio_blk: Less function calls in init_vq() after error detection To: =?UTF-8?Q?Christian_Borntr=c3=a4ger?= , virtualization@lists.linux-foundation.org, "Michael S. Tsirkin" References: <566ABCD9.1060404@users.sourceforge.net> <02054675-8395-ac81-6863-e3a5cbfc9032@users.sourceforge.net> <7da823eb-939c-9ee6-32bf-db296e6a96f6@users.sourceforge.net> <44fd46f6-441a-d497-9157-7e2a0f3f45da@de.ibm.com> Cc: Minfei Huang , Cornelia Huck , Stefan Hajnoczi , Julia Lawall , kernel-janitors@vger.kernel.org, LKML , Chao Fan From: SF Markus Elfring Message-ID: <8b4106e0-8523-56b6-94ef-b7add020497a@users.sourceforge.net> Date: Wed, 14 Sep 2016 08:56:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <44fd46f6-441a-d497-9157-7e2a0f3f45da@de.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Provags-ID: V03:K0:d4GzYMYOWn+m3DV7YsKR486OXgrNkOw0+YK5XYEpoEbixukllR+ UJQtx7WWkaYQXcViS4lv6UnNiuLaGRjabH1gMSd/6jiW1UwVpl+v8Rt9f58BWPRDowcTRnp beDAGmwBb5EzU3zsj4Lf8BfsXgnyk+iuqz5pgvis8e0a5+nsj/uH9gMARdjcpVTSRkebrvY 7A9HEN89Xph3H4zHj5jrA== X-UI-Out-Filterresults: notjunk:1;V01:K0:viIQ4lSgvUs=:hReeuqwaxE0LO/m6ZnMCfn PR1mbOefKFrfpu87+S3x7tSV43E3lA1TuhvUBKV0dseh1Ynoh8KFwAiCTFtxhUpOGHZgzh0f4 0J9C5pdo0OTHfOIu45p5r9xScroJ921DQ3OJ0KotI3S+7/AibY5i4/HAVL1tGmb1KzWBk0bJ4 +pUs9hJ653HhJy61r4bH5ds7G/uNHNl/ScbgnaB2b2hLYaGuoYrIlVsyuAdn9vlrS5aA03Zzq RTJsaA20H2xirdeCTYhbRdgB+5QqVTlxQO1qu8LF10Gib9j/fKhrdVoDhSMu+H/8LD83OhJk4 ZPOY4MkFgNKXogVWPDIJlNsGWOSaOKuHuEhD82ewOga5l2ZsTs6/1bQvYXnQAf7KIRVhsHD+R GCHNoWioNXzirkC6ihwJzDEa27c8hyZaeNKlIJ7su11NML5HcBjcb7ObFUZsJdTSyBHHQ/9nC /dM25ivfoP4O0OqSKQggBiPmjCsEuIne9czo2uL5HG7LnUWZaB+BDDpS36aYpqMNISuXYIW6f HLLlXu4Lb0FKVgomfXmnGxuI6S/ifSLGVV6OSFztJlIofqnKGH84pXmLtquWOEnJ1EneiO68l XkEmahBgmoqfhFyZrAZZHFkCy99wy9MAfKRgmyT9fskfUcZR7wcJEiaof6gBWAm2Q/VdmrMWZ 9UfJrT454EZUEibaf3lC2r3dY7Lh8s5QypSpvmld9D7GiyThUUhc01i2TeRAx4nP2EbMAPK0N N6RS/eOV+00iI1BBhhpLLvbJe6WaDH1gIaReW8s+p9DXNVY5H0fPLclmEPrXV4TUMYLQYNa5n k4l68I+ Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >> 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