Date:	Mon, 10 Mar 2014 11:01:43 +0100
From:	Borislav Petkov <bp@...en8.de>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	linux-kernel@...r.kernel.org, kexec@...ts.infradead.org,
	ebiederm@...ssion.com, hpa@...or.com, mjg59@...f.ucam.org,
	greg@...ah.com, jkosina@...e.cz
Subject: Re: [PATCH 09/11] kexec: Provide a function to add a segment at
 fixed address

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.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists