[PATCH v2 2/2] cld: read the cld.port file using g_file_get_contents

[PATCH v2 2/2] cld: read the cld.port file using g_file_get_contents

[Colin McCabe]

Signed-off-by: Colin McCabe <cmccabe@alumni.cmu.edu>

---
 lib/common.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/lib/common.c b/lib/common.c
index 68f60f8..db20e2a 100644
--- a/lib/common.c
+++ b/lib/common.c
@@ -1,4 +1,5 @@
 
+#include <stdio.h>
 #include <stdlib.h>
 #include <fcntl.h>
 #include <unistd.h>
@@ -61,26 +62,26 @@ const char *cld_errstr(enum cle_err_codes ecode)
  */
 int cld_readport(const char *fname)
 {
-	enum { LEN = 11 };
-	char buf[LEN+1];
 	long port;
-	int fd;
-	int rc;
+	gchar *buf;
+	GError *err = NULL;
+	gsize len;
 
-	if ((fd = open(fname, O_RDONLY)) == -1)
-		return -errno;
-	rc = read(fd, buf, LEN);
-	close(fd);
-	if (rc < 0)
-		return -errno;
-	if (rc == 0)
-		return -EPIPE;
-	buf[rc] = 0;
+	if (!g_file_get_contents(fname, &buf, &len, &err)) {
+		fprintf(stderr, "Unable to read port file: %s\n",
+			 err->message);
+		g_error_free(err);
+		return -EIO;
+	}
 
+	if (len == 0) {
+		g_free(buf);
+		return -EPIPE;
+	}
 	port = strtol(buf, NULL, 10);
+	g_free(buf);
 	if (port <= 0 || port >= 65636)
 		return -EDOM;
 
 	return (int)port;
 }
-
-- 
1.6.2.5

Re: [PATCH v2 2/2] cld: read the cld.port file using g_file_get_contents

[Jeff Garzik]

On 11/27/2009 09:23 PM, Colin McCabe wrote:
> Signed-off-by: Colin McCabe<cmccabe@alumni.cmu.edu>
>
> ---
>   lib/common.c |   29 +++++++++++++++--------------
>   1 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/lib/common.c b/lib/common.c
> index 68f60f8..db20e2a 100644
> --- a/lib/common.c
> +++ b/lib/common.c
> @@ -1,4 +1,5 @@
>
> +#include<stdio.h>
>   #include<stdlib.h>
>   #include<fcntl.h>
>   #include<unistd.h>
> @@ -61,26 +62,26 @@ const char *cld_errstr(enum cle_err_codes ecode)
>    */
>   int cld_readport(const char *fname)
>   {
> -	enum { LEN = 11 };
> -	char buf[LEN+1];
>   	long port;
> -	int fd;
> -	int rc;
> +	gchar *buf;
> +	GError *err = NULL;
> +	gsize len;
>
> -	if ((fd = open(fname, O_RDONLY)) == -1)
> -		return -errno;
> -	rc = read(fd, buf, LEN);
> -	close(fd);
> -	if (rc<  0)
> -		return -errno;
> -	if (rc == 0)
> -		return -EPIPE;
> -	buf[rc] = 0;
> +	if (!g_file_get_contents(fname,&buf,&len,&err)) {
> +		fprintf(stderr, "Unable to read port file: %s\n",
> +			 err->message);
> +		g_error_free(err);
> +		return -EIO;
> +	}
>
> +	if (len == 0) {
> +		g_free(buf);
> +		return -EPIPE;
> +	}
>   	port = strtol(buf, NULL, 10);
> +	g_free(buf);

Two added wrinkles, unfortunately:

1) 'buf' is no longer nul-terminated, which means strtol() has become a 
buffer overrun.

2) this is used in daemons, so we cannot make assumptions about error 
output.  The best you can do is return a useful numeric error code, 
because fprintf(stderr,...) does nothing in a daemon that redirected 
stderr to null.

Re: [PATCH v2 2/2] cld: read the cld.port file using g_file_get_contents

[Jeff Garzik]

On 11/28/2009 05:37 AM, Jeff Garzik wrote:
> 1) 'buf' is no longer nul-terminated, which means strtol() has become a
> buffer overrun.

Whoops, it seems I am incorrect on this, according to
http://library.gnome.org/devel/glib/stable/glib-File-Utilities.html#g-file-get-contents

Ignore that point.

Re: [PATCH v2 2/2] cld: read the cld.port file using g_file_get_contents

[Pete Zaitcev]

On Fri, 27 Nov 2009 18:23:49 -0800
Colin McCabe <cmccabe@alumni.cmu.edu> wrote:

> Signed-off-by: Colin McCabe <cmccabe@alumni.cmu.edu>
> +	if (!g_file_get_contents(fname, &buf, &len, &err)) {
> +		fprintf(stderr, "Unable to read port file: %s\n",
> +			 err->message);
> +		g_error_free(err);
> +		return -EIO;
> +	}

We should not print errors from library routines, because it
leads to double-printing (so even if you swap the fprintf to
applog, it's still not a good thing to do).

Otherwise seems fine at first blush.

BTW, speaking of double-something, here's a funny buglet:

diff --git cld/server/server.c cld-tip/server/server.c
--- cld/server/server.c
+++ cld-tip/server/server.c
@@ -722,7 +741,7 @@ static int net_open_any(void)
 
 	if (cld_srv.port_file) {
 		char portstr[7];
-		snprintf(portstr, sizeof(portstr), "%u\n", port);
+		snprintf(portstr, sizeof(portstr), "%u", port);
 		return net_write_port(cld_srv.port_file, portstr);
 	}
 	return 0;

Our PID files contain a linefeed, as a courtesy to those who
examine them from a shell. But CLD's contains two.

-- Pete

Re: [PATCH v2 2/2] cld: read the cld.port file using g_file_get_contents

[Jeff Garzik]

On 11/28/2009 12:58 PM, Pete Zaitcev wrote:
> On Fri, 27 Nov 2009 18:23:49 -0800
> Colin McCabe<cmccabe@alumni.cmu.edu>  wrote:
>
>> Signed-off-by: Colin McCabe<cmccabe@alumni.cmu.edu>
>> +	if (!g_file_get_contents(fname,&buf,&len,&err)) {
>> +		fprintf(stderr, "Unable to read port file: %s\n",
>> +			 err->message);
>> +		g_error_free(err);
>> +		return -EIO;
>> +	}
>
> We should not print errors from library routines, because it
> leads to double-printing (so even if you swap the fprintf to
> applog, it's still not a good thing to do).
>
> Otherwise seems fine at first blush.
>
> BTW, speaking of double-something, here's a funny buglet:
>
> diff --git cld/server/server.c cld-tip/server/server.c
> --- cld/server/server.c
> +++ cld-tip/server/server.c
> @@ -722,7 +741,7 @@ static int net_open_any(void)
>
>   	if (cld_srv.port_file) {
>   		char portstr[7];
> -		snprintf(portstr, sizeof(portstr), "%u\n", port);
> +		snprintf(portstr, sizeof(portstr), "%u", port);
>   		return net_write_port(cld_srv.port_file, portstr);
>   	}
>   	return 0;
>
> Our PID files contain a linefeed, as a courtesy to those who
> examine them from a shell. But CLD's contains two.
>

Agreed... wanna wrap that in a signed-off-by?  :)

	Jeff

Re: [PATCH v2 2/2] cld: read the cld.port file using g_file_get_contents

[Pete Zaitcev]

On Sat, 28 Nov 2009 16:23:29 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> > +++ cld-tip/server/server.c
> > @@ -722,7 +741,7 @@ static int net_open_any(void)
> >
> >   	if (cld_srv.port_file) {
> >   		char portstr[7];
> > -		snprintf(portstr, sizeof(portstr), "%u\n", port);
> > +		snprintf(portstr, sizeof(portstr), "%u", port);
> >   		return net_write_port(cld_srv.port_file, portstr);
> >   	}
> >   	return 0;
> >
> > Our PID files contain a linefeed, as a courtesy to those who
> > examine them from a shell. But CLD's contains two.

> Agreed... wanna wrap that in a signed-off-by?  :)

Sure, I can do it. Although, it was intended as a hint for Colin
about the kind of bugs we have scattered all over (probably).
This one was harmless.

-- pete