Re: [PATCH v3] add kexec_load() syscall

[Roland McGrath <roland at hack dot frob dot com>]

> Motivation is to axe the syscall maze in kexec-tools itself and
> have this syscall supported in glibc for installers or other
> interested projects (kexecboot, ..).

I'm not really convinced this is worthwhile.  Calling 'syscall'
seems quite sufficient for such arcane and rarely-used calls.

> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/sys/kexec.h
> @@ -0,0 +1,50 @@
> +/* Copyright (C) 2012 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.

Please make the top line of every new file a descriptive comment.
Put the copyright on the second line.

> +/* This structure is used to hold the arguments that are used when
> +   loading  kernel binaries.  */

Extra space there.

> +struct kexec_segment {
> +	const void *buf;
> +	size_t bufsz;
> +	const void *mem;
> +	size_t memsz;
> +};

Please follow GNU indentation style:

	struct kexec_segment
	{
	  const void *buf;
	  size_t bufsz;
	  const void *mem;
	  size_t memsz;
	};

> +__BEGIN_DECLS
> +
> +/* kexec interface function 
> +   __entry - memory location of the new kimage
> +   __nr_segments - number of kexec_segments
> +   __segments - segments to be read
> +   __flags - kexec flags  */

When mentioning arguments in a comment, write "NAME" rather than "__name".
It's our style to use complete English sentences with proper punctuation
and capitalization in comments.  e.g.

/* Load a new kernel image according consisting of NR_SEGMENTS segments,
   as described by the SEGMENTS array and entry-point address ENTRY.
   FLAGS says...  */

The last sentence should be replaced with an actual description of what
the flags are for.  It must also say where you find the flag bits.  The
macros for the flag bits should be made available by this header file.
It would be best to let a kernel header provide the actual bits, so we
don't have to modify the libc file every time the kernel adds a flag
bit.  But the linux/kexec.h header is not set up for that.  Frankly, I
think it would be best not to try to add this to libc at all until you
have done the kernel-side changes to make a header that we can use to
provide the data structure and flag bits.

> +extern long int kexec_load(void *__entry, unsigned long __nr_segments,
> +		struct kexec_segment *__segments,
> +		unsigned long __flags) __THROW;

The return value is always just success or failure, so 'int' is the
usual type to use.  

Never write 'unsigned long', always 'unsigned long int'.  

For an argument like NR_SEGMENTS, size_t is the usual type to use.

'void *' is a very questionable type for the ENTRY argument.  It's not a
pointer in user-space at all, it's an address in the kernel address
space.  Either uintptr_t or 'unsigned long int' is probably right.

> +kexec_load	EXTRA	kexec_load	i:pipi	kexec_load

It's really i:iipi


Thanks,
Roland