lkml.org 
[lkml]   [2014]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.
    --


    \
     
     \ /
      Last update: 2014-03-10 13:01    [W:4.382 / U:0.152 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site