[PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
Signed-off-by: Yuri Bushmelev <jay4mail@gmail.com>
---
 kexec/kexec.c |   38 ++++++++++++++++++++++++++++++--------
 1 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/kexec/kexec.c b/kexec/kexec.c
index ce105cd..a3ca79c 100644
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -933,15 +933,32 @@ void usage(void)
 
 static int kexec_loaded(void)
 {
-	int ret;
+	long ret = -1;
 	FILE *fp;
+	char *p;
+	char line[3];
 
 	fp = fopen("/sys/kernel/kexec_loaded", "r");
 	if (fp == NULL)
 		return -1;
-	fscanf(fp, "%d", &ret);
+/*	fscanf(fp, "%d", &ret); */
+	p = fgets(line, sizeof(line), fp);
 	fclose(fp);
-	return ret;
+
+	if ( NULL == p)
+		return -1;
+
+	ret = strtol(line, &p, 10);
+
+	if (ret > INT_MAX)
+	/* Too long */
+		return -1;
+
+	if (p == line)
+	/* No digits were found */
+		return -1;
+
+	return (int)ret;
 }
 
 /*
@@ -989,18 +1006,23 @@ static void remove_parameter(char *line, const char *param_name)
 char *get_command_line(void)
 {
 	FILE *fp;
-	size_t len;
-	char *line = NULL;
+	const int sizeof_line = 1024;
+	char *line = malloc(sizeof_line); /* according to strdup() later */
 
 	fp = fopen("/proc/cmdline", "r");
 	if (!fp)
-		die("Could not read /proc/cmdline.");
-	getline(&line, &len, fp);
+		die("Could not open /proc/cmdline.");
+
+	if ( NULL == fgets(line, sizeof(line), fp) ) {
+		die("Can't read /proc/cmdline.");
+
+/* 	getline(&line, &len, fp); */
 	fclose(fp);
+	}
 
 	if (line) {
 		/* strip newline */
-		*(line + strlen(line) - 1) = 0;
+		line[strlen(line) - 1] = '\0';
 
 		remove_parameter(line, "BOOT_IMAGE");
 		if (kexec_flags & KEXEC_ON_CRASH)
-- 
1.6.4.4


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Simon Horman]

On Sun, Nov 22, 2009 at 12:12:49AM +0100, Andrea Adami wrote:
> 
> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
> Signed-off-by: Yuri Bushmelev <jay4mail@gmail.com>
> ---
>  kexec/kexec.c |   38 ++++++++++++++++++++++++++++++--------
>  1 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/kexec/kexec.c b/kexec/kexec.c
> index ce105cd..a3ca79c 100644
> --- a/kexec/kexec.c
> +++ b/kexec/kexec.c
> @@ -933,15 +933,32 @@ void usage(void)
>  
>  static int kexec_loaded(void)
>  {
> -	int ret;
> +	long ret = -1;
>  	FILE *fp;
> +	char *p;
> +	char line[3];
>  
>  	fp = fopen("/sys/kernel/kexec_loaded", "r");
>  	if (fp == NULL)
>  		return -1;
> -	fscanf(fp, "%d", &ret);
> +/*	fscanf(fp, "%d", &ret); */

	I'm not sure this comment is needed.
	If you'd like to keep it, please indent it by one tab
	and replace the between '/*' and 'fscanf' with a single space.

	/* fscanf(fp, "%d", &ret); */

> +	p = fgets(line, sizeof(line), fp);
>  	fclose(fp);
> -	return ret;
> +
> +	if ( NULL == p)
> +		return -1;
> +
> +	ret = strtol(line, &p, 10);
> +
> +	if (ret > INT_MAX)
> +	/* Too long */

	Please remove this comment or move it to immediately
	above the if statement.

	/* Too long */
	if (ret > INT_MAX)
		...

> +		return -1;
> +
> +	if (p == line)
> +	/* No digits were found */

	Again, please remove this comment or move it to immediately
	above the if statement.

> +		return -1;
> +
> +	return (int)ret;
>  }
>  
>  /*
> @@ -989,18 +1006,23 @@ static void remove_parameter(char *line, const char *param_name)
>  char *get_command_line(void)
>  {
>  	FILE *fp;
> -	size_t len;
> -	char *line = NULL;
> +	const int sizeof_line = 1024;

	Should they type of sizeof_line be const size_t ?

> +	char *line = malloc(sizeof_line); /* according to strdup() later */

	Could line be char line[1024] ?
>  
>  	fp = fopen("/proc/cmdline", "r");
>  	if (!fp)
> -		die("Could not read /proc/cmdline.");
> -	getline(&line, &len, fp);
> +		die("Could not open /proc/cmdline.");
> +
> +	if ( NULL == fgets(line, sizeof(line), fp) ) {

	* The reverse of this logic makes more sense to me:

	  if (fgets(line, sizeof(line), fp) == NULL) {

	* Should sizeof(line) be sizeof_line?
	  Or alternatively line changed from a pointer to an array.

	  As it stands sizeof(line) == 4 (on x86_32, glibc)

> +		die("Can't read /proc/cmdline.");
> +
> +/* 	getline(&line, &len, fp); */

	Again, I don't think this comment is needed,
	but if you want to keep it please indent it by one tab
	and replace the between '/*' and 'getline' with a single space.

>  	fclose(fp);

	fclose looks like it needs to be indented by two tabs.

> +	}
>  
>  	if (line) {
>  		/* strip newline */
> -		*(line + strlen(line) - 1) = 0;
> +		line[strlen(line) - 1] = '\0';
>  
>  		remove_parameter(line, "BOOT_IMAGE");
>  		if (kexec_flags & KEXEC_ON_CRASH)
> -- 
> 1.6.4.4
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Bernhard Walle]

Simon Horman schrieb:
>
>> +	char *line = malloc(sizeof_line); /* according to strdup() later */
>
> 	Could line be char line[1024] ?

Better would be a constant. However, 1024 is too less since the maximum
command line size can be 2048 currently on x86
(arch/x86/include/asm/setup.h, #define COMMAND_LINE_SIZE 2048).



Regards,
Bernhard

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Simon Horman]

On Sun, Nov 22, 2009 at 10:24:30AM +0100, Bernhard Walle wrote:
> Simon Horman schrieb:
> >
> >> +	char *line = malloc(sizeof_line); /* according to strdup() later */
> >
> > 	Could line be char line[1024] ?
> 
> Better would be a constant. However, 1024 is too less since the maximum
> command line size can be 2048 currently on x86
> (arch/x86/include/asm/setup.h, #define COMMAND_LINE_SIZE 2048).

Hi,

I have taken a stab at revising this patch:

----------------------------------------------------------------------

kexec.c: workaround getline and fscanf to make it *libc agnostic.
	Tested against klibc and dietlibc.

Based on a patch by Andrea Adami and Yuri Bushmelev
I have:

* Cleaned up indentation
* Rearranged the logic in get_command_line()
* Increased the buffer for reading the command line
  from 1024 to 2048 bytes as this is now possible on x86

Only compile tested against glibc.

Cc: Andrea Adami <andrea.adami@gmail.com>
Cc: Yuri Bushmelev <jay4mail@gmail.com>
Cc: Bernhard Walle <bernhard@bwalle.de>
Signed-off-by: Simon Horman <horms@verge.net.au>

Index: kexec-tools/kexec/kexec.c
===================================================================
--- kexec-tools.orig/kexec/kexec.c	2009-11-26 10:10:01.000000000 +1100
+++ kexec-tools/kexec/kexec.c	2009-11-26 10:10:22.000000000 +1100
@@ -933,15 +933,32 @@ void usage(void)
 
 static int kexec_loaded(void)
 {
-	int ret;
+	long ret = -1;
 	FILE *fp;
+	char *p;
+	char line[3];
 
 	fp = fopen("/sys/kernel/kexec_loaded", "r");
 	if (fp == NULL)
 		return -1;
-	fscanf(fp, "%d", &ret);
+
+	p = fgets(line, sizeof(line), fp);
 	fclose(fp);
-	return ret;
+
+	if ( NULL == p)
+		return -1;
+
+	ret = strtol(line, &p, 10);
+
+	/* Too long */
+	if (ret > INT_MAX)
+		return -1;
+
+	/* No digits were found */
+	if (p == line)
+		return -1;
+
+	return (int)ret;
 }
 
 /*
@@ -989,24 +1006,28 @@ static void remove_parameter(char *line,
 char *get_command_line(void)
 {
 	FILE *fp;
-	size_t len;
-	char *line = NULL;
+	char *line;
+	const int sizeof_line = 2048;
+
+	line = malloc(sizeof_line);
+	if (line == NULL)
+		die("Could not allocate memory to read /proc/cmdline.");
 
 	fp = fopen("/proc/cmdline", "r");
 	if (!fp)
-		die("Could not read /proc/cmdline.");
-	getline(&line, &len, fp);
+		die("Could not open /proc/cmdline.");
+
+	if (fgets(line, sizeof_line, fp) == NULL)
+		die("Can't read /proc/cmdline.");
+
 	fclose(fp);
 
-	if (line) {
-		/* strip newline */
-		*(line + strlen(line) - 1) = 0;
-
-		remove_parameter(line, "BOOT_IMAGE");
-		if (kexec_flags & KEXEC_ON_CRASH)
-			remove_parameter(line, "crashkernel");
-	} else
-		line = strdup("");
+	/* strip newline */
+	line[strlen(line) - 1] = '\0';
+
+	remove_parameter(line, "BOOT_IMAGE");
+	if (kexec_flags & KEXEC_ON_CRASH)
+		remove_parameter(line, "crashkernel");
 
 	return line;
 }

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Bernhard Walle]

Simon Horman schrieb:
>  
> +
> +	if ( NULL == p)
> +		return -1;

Wouldn't that be better 'p == NULL'?

> +	ret = strtol(line, &p, 10);
> +
> +	/* Too long */
> +	if (ret > INT_MAX)
> +		return -1;

An integer that can be represented in 2 (strlen("__\0")==3) cannot be
larger than INT_MAX on any platform I can imagine. ;-)




Regards,
Bernhard

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Simon Horman]

On Thu, Nov 26, 2009 at 12:37:08AM +0100, Bernhard Walle wrote:
> Simon Horman schrieb:
> >  
> > +
> > +	if ( NULL == p)
> > +		return -1;
> 
> Wouldn't that be better 'p == NULL'?

Agreed

> > +	ret = strtol(line, &p, 10);
> > +
> > +	/* Too long */
> > +	if (ret > INT_MAX)
> > +		return -1;
> 
> An integer that can be represented in 2 (strlen("__\0")==3) cannot be
> larger than INT_MAX on any platform I can imagine. ;-)

True, though it doesn't seem outrageous to check anyway.
Perhaps the string format will change in future.

----------------------------------------------------------------------

From: Andrea Adami <andrea.adami@gmail.com>
To: kexec@lists.infradead.org
Subject: [PATCH] kexec.c: workaround getline and fscanf to make it *libc
	agnostic. Tested against klibc and dietlibc.

Based on a patch by Andrea Adami and Yuri Bushmelev
I have:

* Cleaned up indentation
* Rearranged the logic in get_command_line()
* Increased the buffer for reading the command line
  from 1024 to 2048 bytes as this is now possible on x86

Only compile tested against glibc.

Cc: Andrea Adami <andrea.adami@gmail.com>
Cc: Yuri Bushmelev <jay4mail@gmail.com>
Cc: Bernhard Walle <bernhard@bwalle.de>
Signed-off-by: Simon Horman <horms@verge.net.au>

Index: kexec-tools/kexec/kexec.c
===================================================================
--- kexec-tools.orig/kexec/kexec.c	2009-11-26 10:10:01.000000000 +1100
+++ kexec-tools/kexec/kexec.c	2009-11-26 11:13:46.000000000 +1100
@@ -933,15 +933,32 @@ void usage(void)
 
 static int kexec_loaded(void)
 {
-	int ret;
+	long ret = -1;
 	FILE *fp;
+	char *p;
+	char line[3];
 
 	fp = fopen("/sys/kernel/kexec_loaded", "r");
 	if (fp == NULL)
 		return -1;
-	fscanf(fp, "%d", &ret);
+
+	p = fgets(line, sizeof(line), fp);
 	fclose(fp);
-	return ret;
+
+	if (p == NULL)
+		return -1;
+
+	ret = strtol(line, &p, 10);
+
+	/* Too long */
+	if (ret > INT_MAX)
+		return -1;
+
+	/* No digits were found */
+	if (p == line)
+		return -1;
+
+	return (int)ret;
 }
 
 /*
@@ -989,24 +1006,28 @@ static void remove_parameter(char *line,
 char *get_command_line(void)
 {
 	FILE *fp;
-	size_t len;
-	char *line = NULL;
+	char *line;
+	const int sizeof_line = 2048;
+
+	line = malloc(sizeof_line);
+	if (line == NULL)
+		die("Could not allocate memory to read /proc/cmdline.");
 
 	fp = fopen("/proc/cmdline", "r");
 	if (!fp)
-		die("Could not read /proc/cmdline.");
-	getline(&line, &len, fp);
+		die("Could not open /proc/cmdline.");
+
+	if (fgets(line, sizeof_line, fp) == NULL)
+		die("Can't read /proc/cmdline.");
+
 	fclose(fp);
 
-	if (line) {
-		/* strip newline */
-		*(line + strlen(line) - 1) = 0;
-
-		remove_parameter(line, "BOOT_IMAGE");
-		if (kexec_flags & KEXEC_ON_CRASH)
-			remove_parameter(line, "crashkernel");
-	} else
-		line = strdup("");
+	/* strip newline */
+	line[strlen(line) - 1] = '\0';
+
+	remove_parameter(line, "BOOT_IMAGE");
+	if (kexec_flags & KEXEC_ON_CRASH)
+		remove_parameter(line, "crashkernel");
 
 	return line;
 }

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Yuri Bushmelev]

Hello!
> On Thu, Nov 26, 2009 at 12:37:08AM +0100, Bernhard Walle wrote:
> > Simon Horman schrieb:
> > > +
> > > +	if ( NULL == p)
> > > +		return -1;
> >
> > Wouldn't that be better 'p == NULL'?
> 
> Agreed

That is form of 'early bug detection' of mistyped '=='.
E.g. you can write by mistype
	if (p = NULL) {}
but can't
	if (NULL = p) {}
because you will get compiler error.

-- 
Yuri Bushmelev

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Bernhard Walle]

Yuri Bushmelev schrieb:
> 
> That is form of 'early bug detection' of mistyped '=='.
> E.g. you can write by mistype
> 	if (p = NULL) {}
> but can't
> 	if (NULL = p) {}
> because you will get compiler error.
> 

I know that kind of argument. However, it makes code IMO quite
unreadable since it's uncommon at least in the kernel code. And that's
the coding style we follow in kexec.

BTW: gcc warns about 'if (p = NULL)' anyways ...


Regards,
Bernhard


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Simon Horman]

On Thu, Nov 26, 2009 at 08:28:22PM +0100, Bernhard Walle wrote:
> Yuri Bushmelev schrieb:
> > 
> > That is form of 'early bug detection' of mistyped '=='.
> > E.g. you can write by mistype
> > 	if (p = NULL) {}
> > but can't
> > 	if (NULL = p) {}
> > because you will get compiler error.
> > 
> 
> I know that kind of argument. However, it makes code IMO quite
> unreadable since it's uncommon at least in the kernel code. And that's
> the coding style we follow in kexec.
> 
> BTW: gcc warns about 'if (p = NULL)' anyways ...

I agree with Bernhard, though Yuri's point is a good one.

This aside, is everyone happy with the patch?
I'd like to merge it if possible.



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Bernhard Walle]

Simon Horman schrieb:
> This aside, is everyone happy with the patch?
> I'd like to merge it if possible.

No objections from my side.


Regards,
Bernhard


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Yuri Bushmelev]

Hello!
> On Thu, Nov 26, 2009 at 08:28:22PM +0100, Bernhard Walle wrote:
> > Yuri Bushmelev schrieb:
> > > That is form of 'early bug detection' of mistyped '=='.
> > > E.g. you can write by mistype
> > > 	if (p = NULL) {}
> > > but can't
> > > 	if (NULL = p) {}
> > > because you will get compiler error.
> >
> > I know that kind of argument. However, it makes code IMO quite
> > unreadable since it's uncommon at least in the kernel code. And that's
> > the coding style we follow in kexec.
> >
> > BTW: gcc warns about 'if (p = NULL)' anyways ...

Ok, np :)

> I agree with Bernhard, though Yuri's point is a good one.
> 
> This aside, is everyone happy with the patch?
> I'd like to merge it if possible.

Merge it please!

-- 
Yuri Bushmelev

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec.c: workaround getline and fscanf to make it *libc agnostic. Tested against klibc and dietlibc.

[Simon Horman]

On Fri, Nov 27, 2009 at 12:26:40PM +0300, Yuri Bushmelev wrote:
> Hello!
> > On Thu, Nov 26, 2009 at 08:28:22PM +0100, Bernhard Walle wrote:
> > > Yuri Bushmelev schrieb:
> > > > That is form of 'early bug detection' of mistyped '=='.
> > > > E.g. you can write by mistype
> > > > 	if (p = NULL) {}
> > > > but can't
> > > > 	if (NULL = p) {}
> > > > because you will get compiler error.
> > >
> > > I know that kind of argument. However, it makes code IMO quite
> > > unreadable since it's uncommon at least in the kernel code. And that's
> > > the coding style we follow in kexec.
> > >
> > > BTW: gcc warns about 'if (p = NULL)' anyways ...
> 
> Ok, np :)
> 
> > I agree with Bernhard, though Yuri's point is a good one.
> > 
> > This aside, is everyone happy with the patch?
> > I'd like to merge it if possible.
> 
> Merge it please!

Done :-)


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec