Messages in this thread | | | Date | Tue, 24 Nov 2015 22:54:17 +0100 | Subject | Re: block-rbd: One function call less in rbd_dev_probe_parent() after error detection | From | Ilya Dryomov <> |
| |
On Tue, Nov 24, 2015 at 9:34 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >> Well, there isn't any _literal_ linking (e.g. adding to a link list, >> etc) in this case. We just bump some refs and do probe to fill in the >> newly allocated parent. > > Thanks for your clarification. > > >> The actual linking (rbd_dev->parent = parent) is done right before >> returning so we never have to undo it in rbd_dev_probe_parent() and >> that's the only reason your patch probably doesn't break anything. > > Is this function implementation just also affected by an issue > which is mentioned in the Linux document "CodingStyle" as "one err bugs"?
No, why? "one err bug" as per CodingStyle is a NULL deref on line 2 if foo is NULL. If it was just "err: kfree(foo); return ret;", a NULL foo would be perfectly OK.
1 err: 2 kfree(foo->bar); 3 kfree(foo); 4 return ret;
If you can spot such a NULL deref in rbd_dev_probe_parent(), I'd gladly take a patch.
> > >> Think about what happens if, after your patch is applied, someone moves >> that assignment up or adds an extra step that can fail after it... > > Is such a software maintenance concern really enough to delay (or reject) > my second update suggestion in this small patch series?
Yes - it's rejected because it messes up the order of cleanup for no good reason. I realize why you think the patch is correct and it's not without merit, but it just doesn't fit the weird rbd_dev_probe_parent() contract.
Thanks,
Ilya
| |