Messages in this thread | | | From | Baolin Wang <> | Date | Tue, 6 Sep 2016 16:25:10 +0800 | Subject | Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size |
| |
On 6 September 2016 at 15:19, Andreas Mohr <andi@lisas.de> wrote: > On Tue, Sep 06, 2016 at 02:26:06PM +0800, Baolin Wang wrote: >> On 6 September 2016 at 12:34, Andreas Mohr <andi@lisas.de> wrote: >> >> - to = from + nr; >> >> - >> >> - if (to <= from) >> >> - return -EINVAL; >> > >> > Hmm, this is swallowing -EINVAL behaviour >> > i.e., now possibly violating protocol? >> >> I didn't see what situation will make variable 'to' is less than >> 'from' since I think variable 'nr' is always larger than 0, right? If >> so, we should remove this useless checking. Thanks. > > Hmm, indeed, since all participating variables are unsigned, > the existing calculation should never hit this. > However, one could argue that this is an additional safeguard > against implementation source getting modified in a way > that will suddenly result in this pathologic case becoming true > (where a -EINVAL bailout surely will then pinpoint things > much more visibly for some users, > as opposed to potential data corruption or some such).
OK. I'll add this checking.
> > > > I have seen another change > >> - if (nr == 0) >> - return 0; > > where it gets moved out of common path > and into MMC_ERASE_ARG-specific branch > (likely because the subsequent common-path conditional of > nr > rem > is deemed sufficient). > > This seems to be again a change > where a simple yet crucial > device geometry calculation post-condition > (either to > from, or nr > 0) > is then not verified specifically/separately. > > Ultimately, I am not sure whether or not > these (post-)conditions should be verified > in their most basic, simple form, > as an extra/separate verification step.
After some investigation, I think we add this checking is more safer. Thanks for your comments.
-- Baolin.wang Best Regards
| |