From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jeff Garzik <jeff@garzik.org>
Subject: Re: [Patch] cldcli: better error messages
Date: Tue, 04 Aug 2009 21:24:40 -0400
Message-ID: <4A78DF58.6050200@garzik.org>
References: <20090804190602.4d7918df@redhat.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Return-path: <hail-devel-owner@vger.kernel.org>
In-Reply-To: <20090804190602.4d7918df@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: Project Hail List <hail-devel@vger.kernel.org>

Pete Zaitcev wrote:
> Produce more specific error messages for those who think that host
> does not necesserily imply port. Also, clean up a bit.
> 
> Signed-Off-By: Pete Zaitcev <zaitcev@redhat.com>

Generally OK, but needs some minor revisions...


> diff --git a/tools/cldcli.c b/tools/cldcli.c
> index 758385d..1367b43 100644
> --- a/tools/cldcli.c
> +++ b/tools/cldcli.c
> @@ -9,13 +9,10 @@
>  #include <argp.h>
>  #include <poll.h>
>  #include <locale.h>
> -#include <syslog.h>
>  #include <stdarg.h>
>  #include <ctype.h>
>  #include <cldc.h>
>  
> -#define PROGRAM_NAME "cld"
> -
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>  
>  const char *argp_program_version = PACKAGE_VERSION;
> @@ -32,8 +29,8 @@ enum thread_codes {
>  static struct argp_option options[] = {
>  	{ "debug", 'D', "LEVEL", 0,
>  	  "Set debug output to LEVEL (0 = off, 2 = max verbose)" },
> -	{ "host", 'h', "HOST", 0,
> -	  "Connect to remote host.  Used once in normal case (DNS SRV records), or may be specified multiple times." },
> +	{ "host", 'h', "HOST:PORT", 0,
> +	  "Connect to remote CLD at specified HOST:PORT" },
>  	{ "user", 'u', "USER", 0,
>  	  "Set username to USER" },
>  	{ }
> @@ -749,22 +746,32 @@ static bool push_host(const char *arg)
>  	struct cldc_host *dr;
>  
>  	dr = malloc(sizeof(*dr));
> -	if (!dr)
> -		return false;
> +	if (!dr) {
> +		fprintf(stderr, "no core for %u", (int) sizeof(*dr));
> +		goto err;
> +	}
>  	memset(dr, 0, sizeof(*dr));
>  
>  	dr->host = strdup(arg);
> -	if (!dr->host)
> +	if (!dr->host) {
> +		fprintf(stderr, "no core for host"); // don't print, too long
>  		goto err_out;

Both of these need newlines.  And additional consistency would be nice: 
  the rest of cld/chunkd/tabled tends to use the simple "OOM" string, 
though perhaps "OOM (%u bytes)" would be better?


>  	colon = strrchr(dr->host, ':');
> -	if (!colon)
> +	if (!colon) {
> +		fprintf(stderr, "no port in host specifier `%s'\n", dr->host);
>  		goto err_out_host;
> +	}
>  
> -	if (sscanf(colon, ":%u", &port) != 1)
> +	if (sscanf(colon, ":%u", &port) != 1) {
> +		fprintf(stderr, "port `%s' is invalid\n", colon+1);
>  		goto err_out_host;
> -	if (port < 1 || port > 65535)
> +	}
> +	if (port < 1 || port > 65535) {
> +		fprintf(stderr, "port `%s' is out of range\n", colon+1);
>  		goto err_out_host;
> +	}
>  
>  	dr->port = port;
>  
> @@ -778,6 +785,7 @@ err_out_host:
>  	free(dr->host);
>  err_out:
>  	free(dr);
> +err:
>  	return false;
>  }
>  
> @@ -793,10 +801,8 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state)
>  		}
>  		break;
>  	case 'h':
> -		if (!push_host(arg)) {
> -			fprintf(stderr, "invalid host: '%s'\n", arg);
> +		if (!push_host(arg))
>  			argp_usage(state);
> -		}
>  		break;
>  	case 'u':
>  		if (strlen(arg) >= CLD_MAX_USERNAME) {
>