[PATCH v2] kexec/s390x: use mmap instead of read for slurp_file()

[holzheu@xxxxxxxxxxxxxxxxxx (Michael Holzheu)]

On Fri, 30 Oct 2015 10:03:22 +0800
Dave Young <dyoung at redhat.com> wrote:

> Hi,
> 

[snip]

> > --- a/kexec/arch/s390/kexec-image.c
> > +++ b/kexec/arch/s390/kexec-image.c
> > @@ -101,7 +101,7 @@ image_s390_load(int argc, char **argv, c
> >  	 * we load the ramdisk directly behind the image with 1 MiB alignment.
> >  	 */
> >  	if (ramdisk) {
> > -		rd_buffer = slurp_file(ramdisk, &ramdisk_len);
> > +		rd_buffer = slurp_file_mmap(ramdisk, &ramdisk_len);
> >  		if (rd_buffer == NULL) {
> >  			fprintf(stderr, "Could not read ramdisk.\n");
> >  			return -1;
> > --- a/kexec/kexec.c
> > +++ b/kexec/kexec.c
> > @@ -29,6 +29,7 @@
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> >  #include <sys/reboot.h>
> > +#include <sys/mman.h>
> >  #include <unistd.h>
> >  #include <fcntl.h>
> >  #ifndef _O_BINARY
> > @@ -514,7 +515,8 @@ static char *slurp_fd(int fd, const char
> >  	return buf;
> >  }
> >  
> > -char *slurp_file(const char *filename, off_t *r_size)
> > +static char *slurp_file_generic(const char *filename, off_t *r_size,
> > +				int use_mmap)
> 
> Add a function comment about the argument use_mmap so that one knows
> that it will be not used for character devices?#

Good idea, I put comments before slurp_file() and slurp_file_mmap().

> 
> >  {
> >  	int fd;
> >  	char *buf;
> > @@ -552,11 +554,17 @@ char *slurp_file(const char *filename, o
> >  		if (err < 0)
> >  			die("Can not seek to the begin of file %s: %s\n",
> >  					filename, strerror(errno));
> > +		buf = slurp_fd(fd, filename, size, &nread);
> >  	} else {
> >  		size = stats.st_size;
> > +		if (use_mmap) {
> > +			buf = mmap(NULL, size, PROT_READ|PROT_WRITE,
> > +				   MAP_PRIVATE, fd, 0);
> 
> Probably map it as readonly is enough.

Although I agree with you for the ramdisk case, I nevertheless would
prefer to keep it writable to have the same semantics as for the
malloc case.

So do you agree with the patch below?

Michael
---
 kexec/arch/s390/kexec-image.c |    2 +-
 kexec/kexec.c                 |   31 ++++++++++++++++++++++++++++---
 kexec/kexec.h                 |    1 +
 3 files changed, 30 insertions(+), 4 deletions(-)

--- a/kexec/arch/s390/kexec-image.c
+++ b/kexec/arch/s390/kexec-image.c
@@ -101,7 +101,7 @@ image_s390_load(int argc, char **argv, c
 	 * we load the ramdisk directly behind the image with 1 MiB alignment.
 	 */
 	if (ramdisk) {
-		rd_buffer = slurp_file(ramdisk, &ramdisk_len);
+		rd_buffer = slurp_file_mmap(ramdisk, &ramdisk_len);
 		if (rd_buffer == NULL) {
 			fprintf(stderr, "Could not read ramdisk.\n");
 			return -1;
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -29,6 +29,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/reboot.h>
+#include <sys/mman.h>
 #include <unistd.h>
 #include <fcntl.h>
 #ifndef _O_BINARY
@@ -514,7 +515,8 @@ static char *slurp_fd(int fd, const char
 	return buf;
 }
 
-char *slurp_file(const char *filename, off_t *r_size)
+static char *slurp_file_generic(const char *filename, off_t *r_size,
+				int use_mmap)
 {
 	int fd;
 	char *buf;
@@ -552,11 +554,17 @@ char *slurp_file(const char *filename, o
 		if (err < 0)
 			die("Can not seek to the begin of file %s: %s\n",
 					filename, strerror(errno));
+		buf = slurp_fd(fd, filename, size, &nread);
 	} else {
 		size = stats.st_size;
+		if (use_mmap) {
+			buf = mmap(NULL, size, PROT_READ|PROT_WRITE,
+				   MAP_PRIVATE, fd, 0);
+			nread = size;
+		} else {
+			buf = slurp_fd(fd, filename, size, &nread);
+		}
 	}
-
-	buf = slurp_fd(fd, filename, size, &nread);
 	if (!buf)
 		die("Cannot read %s", filename);
 
@@ -567,6 +575,23 @@ char *slurp_file(const char *filename, o
 	return buf;
 }
 
+/*
+ * Read file into malloced buffer
+ */
+char *slurp_file(const char *filename, off_t *r_size)
+{
+	return slurp_file_generic(filename, r_size, 0);
+}
+
+/*
+ * Map "normal" file or read "character device" into malloced buffer.
+ * You must not use free, realloc, etc. with the returned buffer.
+ */
+char *slurp_file_mmap(const char *filename, off_t *r_size)
+{
+	return slurp_file_generic(filename, r_size, 1);
+}
+
 /* This functions reads either specified number of bytes from the file or
    lesser if EOF is met. */
 
--- a/kexec/kexec.h
+++ b/kexec/kexec.h
@@ -253,6 +253,7 @@ extern void die(const char *fmt, ...)
 extern void *xmalloc(size_t size);
 extern void *xrealloc(void *ptr, size_t size);
 extern char *slurp_file(const char *filename, off_t *r_size);
+extern char *slurp_file_mmap(const char *filename, off_t *r_size);
 extern char *slurp_file_len(const char *filename, off_t size, off_t *nread);
 extern char *slurp_decompress_file(const char *filename, off_t *r_size);
 extern unsigned long virt_to_phys(unsigned long addr);