| Message ID | e20a6835-a404-e894-d0d0-a408bfcd7fb6@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/12/16 13:48, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sat, 10 Dec 2016 09:29:24 +0100 > > The kfree() function was called in one case by the > bttv_input_init() function during error handling > even if the passed variable contained a null pointer. kfree() is safe to call on a NULL pointer. Despite that, you have found several instances of similar constructs: { a = kmalloc(...) b = kmalloc(...) if (!a || !b) goto out; ... out: kfree(a); kfree(b); } and similar patches you submitted to change those construct to something different have been rejected because they are seen as unnecessary changes that make the code harder to read. Didn't it occur to you that maybe those constructs are fine the way they are and this is the idiomatic way to write that kind of code? Why are you submitting patches implementing changes that have already been rejected? Submitting mechanical patches that fix trivial style issues (existing and widely acknowledged ones) is a fine way to learn how to work on kernel development. They constitute additional work load on the maintainers that need to review and merge them. Thus, hopefully, they are only a way for new developers to familiarize themselves with the process and then move to some more constructive contributions. Judging from your recent submissions, it seems that this process is not working well for you. I'm probably not the only one that is wonderign what are you trying to obtain with your patch submissions, other than having your name in the git log. Cheers, Daniele -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> kfree() is safe to call on a NULL pointer. This is true. > Despite that, you have found several instances of similar constructs: Yes. - Special source code search pattern can point such places out for further considerations. > Didn't it occur to you that maybe those constructs are fine the way > they are and this is the idiomatic way to write that kind of code? Such a programming approach might look convenient. - I would prefer a safer coding style for the corresponding exception handling. > Why are you submitting patches implementing changes that have already > been rejected? The feedback to my update mixture is varying between acceptance and disagreements as usual. > Judging from your recent submissions, it seems that this process is not > working well for you. I'm probably not the only one that is wonderign > what are you trying to obtain with your patch submissions, other than > having your name in the git log. I am picking some change possibilities up in the hope of related software improvements. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/12/16 15:10, SF Markus Elfring wrote: >> Despite that, you have found several instances of similar constructs: > > Yes. - Special source code search pattern can point such places out > for further considerations. This is one of the things that makes reviewing the patches you submit quire annoying: if a class of changes has already been turned town, why do you insist in proposing it? >> Didn't it occur to you that maybe those constructs are fine the way >> they are and this is the idiomatic way to write that kind of code? > > Such a programming approach might look convenient. - I would prefer > a safer coding style for the corresponding exception handling. Can you please point out what is wrong in the current code and how the changes you propose fix the problem? Clearly the people reading your patches do not see the problem you are seeing. >> Why are you submitting patches implementing changes that have already >> been rejected? > > The feedback to my update mixture is varying between acceptance and > disagreements as usual. No one has expressed acceptance for the kind of change you propose with this patch, or to previous patches you proposed changing similar constructs. >> Judging from your recent submissions, it seems that this process is not >> working well for you. I'm probably not the only one that is wonderign >> what are you trying to obtain with your patch submissions, other than >> having your name in the git log. > > I am picking some change possibilities up in the hope of related > software improvements. The fact that you propose over and over again a class of changes that has been already vocally rejected would suggest otherwise. The major achievement you obtained so far is that one of the maintainers of a large fraction of the kernel refuses to look at your patch submissions. Maybe you should revise how you contribute to Linux kernel development. Apparently ignoring comments to your previous patch submission and not showing that you have been learning from previous interactions, is not going to help. Cheers, Daniele -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> I would prefer a safer coding style for the corresponding >> exception handling. > > Can you please point out what is wrong in the current code Is it useful to reconsider the software situation that another memory allocation is attempted when it could be determined that a previous one failed already? Are two successful allocations finally needed to achieve the desired task? > and how the changes you propose fix the problem? I suggest to check return values immediately after each function call. An error situation can be detected earlier then and only the required clean-up functionality will be executed at the end. > No one has expressed acceptance for the kind of change you propose with > this patch, or to previous patches you proposed changing similar constructs. I got a mixed impression from the acceptance statistics about my published patches. > The fact that you propose over and over again a class of changes that > has been already vocally rejected would suggest otherwise. I dare to propose another look at results from source code search patterns. > The major achievement you obtained so far is that one of the maintainers > of a large fraction of the kernel refuses to look at your patch submissions. It can happen that some patterns are occasionally "too special" to grow the popularity for such change possibilities and desired software improvements quickly. There are also different views about affected implementation details by the software development community, aren't there? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/12/16 00:33, SF Markus Elfring wrote: >>> I would prefer a safer coding style for the corresponding >>> exception handling. >> >> Can you please point out what is wrong in the current code > > Is it useful to reconsider the software situation that another memory > allocation is attempted when it could be determined that a previous one > failed already? No. > Are two successful allocations finally needed to achieve the desired task? Yes. >> and how the changes you propose fix the problem? > > I suggest to check return values immediately after each function call. > An error situation can be detected earlier then and only the required > clean-up functionality will be executed at the end. Which improvement does this bring? >> No one has expressed acceptance for the kind of change you propose with >> this patch, or to previous patches you proposed changing similar constructs. > > I got a mixed impression from the acceptance statistics about my > published patches. Have you proposed a similar patch that was accepted? I don't find record of it, but I may be wrong. >> The fact that you propose over and over again a class of changes that >> has been already vocally rejected would suggest otherwise. > > I dare to propose another look at results from source code search patterns. Why? Cheers, Daniele -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> I suggest to check return values immediately after each function call. >> An error situation can be detected earlier then and only the required >> clean-up functionality will be executed at the end. > > Which improvement does this bring? * How do you think about to avoid requesting additional system resources when it was determined that a previously required memory allocation failed? * Can the corresponding exception handling become a bit more efficient? > Why? Do you care for any results from static source code analysis? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/12/16 10:15 AM, SF Markus Elfring wrote: >>> I suggest to check return values immediately after each function call. >>> An error situation can be detected earlier then and only the required >>> clean-up functionality will be executed at the end. >> >> Which improvement does this bring? > > * How do you think about to avoid requesting additional system resources > when it was determined that a previously required memory allocation failed? I think it is an irrelevant problem in the case at hand. > * Can the corresponding exception handling become a bit more efficient? Where more efficient merely means sparing one function call? I think it is completely irrelevant in the case at hand and code clarity must be preferred to pointless optimization. >> Why? > > Do you care for any results from static source code analysis? Static source code analysis, in the form you are doing with Coccinelle, may help in identifying problems in a code base when a specific pattern has been identified to be problematic. In the static code analysis results you present, it is not clear what the problematic pattern is. Independently of how you identified the code section you propose to change, there is no problem to fix. As a general advise, Markus, replying to questions with other questions is a a bad debate form. Questions, as opposed to statements, cannot be confuted. Also, every time you receive an answer to one of your questions, you reply with another question broadening the span of the discussion. However, you do not present evidence supporting your initial statement that some changes in the code are beneficial. Cheers, Daniele -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Have you proposed a similar patch that was accepted? Yes. - It happened a few times. > I don't find record of it, but I may be wrong. It is really needed to clarify the corresponding software development history any further? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
You will never win an ELIZA bot challenge against Markus. :P I admire your optimism though. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/12/16 11:03 AM, SF Markus Elfring wrote: >> Have you proposed a similar patch that was accepted? > > Yes. - It happened a few times. The question was: have you ever had a patch changing code in the form { a = kmalloc(...); b = kmalloc(...); if (!a || !b) goto out; ... out: kfree(a); kfree(b); } to something else, accepted? I went checking and I haven't found such a patch. Did you understand my question? > It is really needed to clarify the corresponding software development > history any further? It is relevant because you are submitting a patch and your changelog implies that it makes the code follow some code structure rule that needs to be applied to the kernel. As the above is a recurring pattern in kernel code, it is legitimate to ask if such a rule exist, and has been enforced before, or you are making it up. My conclusion is that you are making it up. As a proposer of a new pattern, what is the evidence you can bring to the discussion that supports that your solution is better? What is the metric you are using to define "better"? Cheers, Daniele -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> The question was: have you ever had a patch changing code in the form > > { > a = kmalloc(...); > b = kmalloc(...); > > if (!a || !b) > goto out; > > ... > > out: > kfree(a); > kfree(b); > } > > to something else, accepted? It seems that this case did not happen for me so far if you are looking for this exact source code search pattern. Variants of the current pattern were occasionally discussed a bit. > I went checking and I haven't found such a patch. A few similar update suggestions are still in development waiting queues. > Did you understand my question? Partly. - My interpretation of similar changes was eventually too broad in my previous answer. >> It is really needed to clarify the corresponding software development >> history any further? > > It is relevant because you are submitting a patch and your changelog > implies that it makes the code follow some code structure rule that > needs to be applied to the kernel. I am proposing a change which was described also around various other functions in some software already. > As the above is a recurring pattern in kernel code, it is legitimate > to ask if such a rule exist, and has been enforced before, or you are > making it up. I got the impression that special software development habits can also evolve over time. > As a proposer of a new pattern, what is the evidence you can bring to > the discussion that supports that your solution is better? I am trying to increase the software development attention on error detection and corresponding exception handling at various places. > What is the metric you are using to define "better"? Do response times for system failures matter here? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/12/16 3:11 PM, SF Markus Elfring wrote: >>> It is really needed to clarify the corresponding software development >>> history any further? >> >> It is relevant because you are submitting a patch and your changelog >> implies that it makes the code follow some code structure rule that >> needs to be applied to the kernel. > > I am proposing a change which was described also around various other > functions in some software already. What is this supposed to mean? >> As the above is a recurring pattern in kernel code, it is legitimate >> to ask if such a rule exist, and has been enforced before, or you are >> making it up. > > I got the impression that special software development habits can also > evolve over time. > >> As a proposer of a new pattern, what is the evidence you can bring to >> the discussion that supports that your solution is better? > > I am trying to increase the software development attention on error > detection and corresponding exception handling at various places. Are you doing this submitting random patches to the kernel sources? >> What is the metric you are using to define "better"? > > Do response times for system failures matter here? No. And you are again answering a question with a question. Cheers, Daniele -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/pci/bt8xx/bttv-input.c b/drivers/media/pci/bt8xx/bttv-input.c index 4da720e4867e..9187993d23ea 100644 --- a/drivers/media/pci/bt8xx/bttv-input.c +++ b/drivers/media/pci/bt8xx/bttv-input.c @@ -418,15 +418,20 @@ int bttv_input_init(struct bttv *btv) struct bttv_ir *ir; char *ir_codes = NULL; struct rc_dev *rc; - int err = -ENOMEM; + int err; if (!btv->has_remote) return -ENODEV; - ir = kzalloc(sizeof(*ir),GFP_KERNEL); + ir = kzalloc(sizeof(*ir), GFP_KERNEL); + if (!ir) + return -ENOMEM; + rc = rc_allocate_device(); - if (!ir || !rc) - goto err_out_free; + if (!rc) { + err = -ENOMEM; + goto free_ir; + } /* detect & configure */ switch (btv->c.type) { @@ -569,6 +574,7 @@ int bttv_input_init(struct bttv *btv) btv->remote = NULL; err_out_free: rc_free_device(rc); +free_ir: kfree(ir); return err; }