From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jeff Garzik <jeff@garzik.org>
Subject: Re: [PATCH v2 2/2] cld: read the cld.port file using g_file_get_contents
Date: Sat, 28 Nov 2009 16:23:29 -0500
Message-ID: <4B1194D1.50103@garzik.org>
References: <1259375029-22050-1-git-send-email-cmccabe@alumni.cmu.edu> <20091128105815.7b5cd149@redhat.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Return-path: <hail-devel-owner@vger.kernel.org>
In-Reply-To: <20091128105815.7b5cd149@redhat.com>
Sender: hail-devel-owner@vger.kernel.org
List-ID: <hail-devel.vger.kernel.org>
Content-Type: text/plain; charset="us-ascii"; format="flowed"
To: Pete Zaitcev <zaitcev@redhat.com>
Cc: Colin McCabe <cmccabe@alumni.cmu.edu>, Project Hail List <hail-devel@vger.kernel.org>

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