On Fri, Feb 28, 2014 at 11:56:28AM -0500, Vivek Goyal wrote:
> This is more of future proofing it. I have been putting this check to
> catch any accidental errors if somebody ends up calling this function
> from old mode.
>
> But I am not very particular about it. If you don't like it, I can get
> rid of it.
Yeah, it doesn't hurt to be overly cautious - I guess it can be removed
later when this code settles.
> I think address does not matter here. You can't add a segemnt after you
> have allocated a control page. So I am not sure how printing address will
> help.
Ok, so what's the urgency of that warning? The "can't add a segment"
thing sounds kinda final to me and that everything breaks if we do add a
segment after all, so maybe it should error out with -EINVAL and caller
should stop adding segments if we have allocated the control page..?
IOW, how is that error message supposed to help me when I see it as a
user?
> Ok, there is not much difference between two, but I can use PAGE_ALIGN().
Yeah, they're the same thing but the name PAGE_ALIGN is more descriptive
:-)
> > That's the retval of validate_ram_range_callback, right? So
> >
> > if (!ret)
> >
> > And shouldn't the convention be the opposite? 0 on success, !0 on error?
>
> Ok, this one is little twisted.
>
> walk_system_ram_res() stops calling callback function if callback
> function returned non zero code.
>
> So in this case, once we have found the range to be valid, we don't want
> to continue to loop and look at any more ranges. So we return "1". If
> we return "0" for success, outer loop of walk_system_ram_res() will
> continue with next ranges.
Huh, I was only talking about flipping that logic, in walk_system_ram_res():
ret = (*func)(res.start, res.end, arg);
if (!res)
break;
This way you still can return negative values as errors.
> Given the fact that "0" is interpreted as success by walk_system_ram_res()
> and it continues with next set of ranges, I could not use 0 as final
> measure of success. Negative returns are errors. So I thought of using
And?
When the loop finishes, you will have the last negative error in ret...
Besides, in load_crashdump_segments() you have:
ret = walk_system_ram_res(KEXEC_BACKUP_SRC_START, KEXEC_BACKUP_SRC_END,
image, determine_backup_region);
/* Zero or postive return values are ok */
if (ret < 0)
return ret;
So 0 is ok, as you say.
Also:
/* Validate memory range */
ret = walk_system_ram_res(base, base + memsz - 1, &ksegment,
validate_ram_range_callback);
/* If a valid range is found, 1 is returned */
if (ret != 1)
return -EINVAL;
Now this looks a bit fragile - only 1 is ok? Normally we do it like this:
if (ret)
return ret;
return __kexec_add_segment(...)
and this way you can propagate the error value up without rewriting it
here.
Am I missing something here?
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
