Re: [PATCH 04/10] kexec: add infrastructure for handling kexec images

["Jan Beulich" <JBeulich@suse.com>]

>>> On 24.06.13 at 19:42, David Vrabel <david.vrabel@citrix.com> wrote:
> +static struct page_info *kimage_alloc_zeroed_page(unsigned memflags)
> +{
> +    struct page_info *page;
> +
> +    page = alloc_domheap_page(NULL, memflags);
> +    if ( page == NULL )

Please be consistent - either always use "== NULL", ...

> +        return NULL;
> +
> +    clear_domain_page(page_to_mfn(page));
> +
> +    return page;
> +}
> +
> +static int do_kimage_alloc(struct kexec_image **rimage, paddr_t entry,
> +                           unsigned long nr_segments,
> +                           xen_kexec_segment_t *segments, uint8_t type)
> +{
> +    struct kexec_image *image;
> +    unsigned long i;
> +    int result;
> +
> +    /* Allocate a controlling structure */
> +    result = -ENOMEM;
> +    image = xzalloc(typeof(*image));
> +    if ( !image )

... or (preferably afaic) always use "!".

> +        goto out;
> +
> +    image->control_page = ~0; /* By default this does not apply */

Use a recognizable #define instead?

> +    image->entry_maddr = entry;
> +    image->type = type;
> +    image->nr_segments = nr_segments;
> +    image->segments = segments;
> +
> +    INIT_PAGE_LIST_HEAD(&image->control_pages);
> +    INIT_PAGE_LIST_HEAD(&image->dest_pages);
> +    INIT_PAGE_LIST_HEAD(&image->unusable_pages);
> +
> +    /*
> +     * Verify we have good destination addresses.  The caller is
> +     * responsible for making certain we don't attempt to load
> +     * the new image into invalid or reserved areas of RAM.  This
> +     * just verifies it is an address we can use.
> +     *
> +     * Since the kernel does everything in page size chunks ensure
> +     * the destination addresses are page aligned.  Too many
> +     * special cases crop of when we don't do this.  The most
> +     * insidious is getting overlapping destination addresses
> +     * simply because addresses are changed to page size
> +     * granularity.
> +     */
> +    result = -EADDRNOTAVAIL;
> +    for ( i = 0; i < nr_segments; i++ )
> +    {
> +        paddr_t mstart, mend;
> +
> +        mstart = image->segments[i].dest_maddr;
> +        mend   = mstart + image->segments[i].dest_size;
> +        if ( (mstart & ~PAGE_MASK) || (mend & ~PAGE_MASK) )

Expressions like this can be abbreviated to

        if ( (mstart | mend) & ~PAGE_MASK )

> +            goto out;
> +    }
> +
> +    /* Verify our destination addresses do not overlap.

Coding style (the comment immediately above is done correctly).
There are more of these further down...

There are also a few cases of bogus blank lines - while the coding
style document doesn't say anything in that regard, excessive
amounts of blank lines increase the risk of future patches getting
applied incorrectly due to the patch context becoming less
meaningful.

Jan