Re: [Unbound-core] Re: proposed diff to cli_loop()

[Juli M. <jma...@Fr...>]

> Index: cli.c
> ===================================================================
> RCS file: /cvsroot/unbound/unbound/lib/cli/cli.c,v
> retrieving revision 1.3
> diff -u -r1.3 cli.c
> --- cli.c	19 Nov 2002 01:19:36 -0000	1.3
> +++ cli.c	19 Nov 2002 02:52:42 -0000
> @@ -43,25 +43,34 @@
>  #include "cli.h"
>  
>  int
> -cli_loop(int fd, const char *prompt, cli_handler callback) {
> +cli_loop(int fdin, int fdout, const char *prompt, cli_handler callback) {
>  	char           *cli_prompt[MAX_PROMPT_SIZE];
>  	char           *p;
> -	FILE	       *stream;
> +	FILE	       *streamin, *streamout;
>  	size_t		len;
>  
>  	strlcpy((char *)cli_prompt, prompt, MAX_PROMPT_SIZE);	/* XXX */
>  
> -	if ((stream = fdopen (fd, "r")) == NULL) {
> +	if ((streamin = fdopen (fdin, "r")) == NULL) {
>  		warn("Failed to open specified file descriptor");
>  		return(errno);
>  	}
>  
> +	if (fdout > -1) {
> +		if ((streamout = fdopen (fdout, "w+")) == NULL) {
> +			warn("Failed to open specified file descriptor");
> +			return(errno);
> +		}
> +	} else {
> +		streamout = fopen("/dev/null", "w+");
> +	}

I'd take a different approach here, by setting 'streamout = NULL' at the
beginning of the function, and avoiding this 'else' altogether, and then...

> +
>  	for (;;) {
>  		int             i = 0;
>  		char            *input, *forward[MAX_TOKENS], **temp;
>  
> -		fprintf(stdout, "%s", prompt);		/* XXX */
> -		if ((input = fparseln(stream, NULL, NULL, NULL, 0)) == NULL) {
> +		fprintf(streamout, "%s", prompt);		/* XXX */

Here I would do:

	if (streamout != NULL)
		fprintf(streamout, "%s", prompt);

In accordance with the above.

> +		if ((input = fparseln(streamin, NULL, NULL, NULL, 0)) == NULL) {
>  			break;
>  		}
>  		
> @@ -78,6 +87,7 @@
>  		free(input);
>  	}
>  
> -	fclose(stream);
> +	fclose(streamout);

I don't recall if fclose(NULL) has any special meaning, so I'd add a
check for NULL as above, too.

> +	fclose(streamin);
>  	return (0);
>  }
> Index: include/cli.h
> ===================================================================
> RCS file: /cvsroot/unbound/unbound/lib/cli/include/cli.h,v
> retrieving revision 1.2
> diff -u -r1.2 cli.h
> --- include/cli.h	19 Nov 2002 01:16:19 -0000	1.2
> +++ include/cli.h	19 Nov 2002 02:52:42 -0000
> @@ -29,7 +29,7 @@
>  #define CLI_H
>  
>  typedef int     (*cli_handler)(int, char *[]);
> -int             cli_loop(int, const char *, cli_handler);
> +int             cli_loop(int, int, const char *, cli_handler);
>  
>  #define MAX_PROMPT_SIZE		255
>  #define MAX_TOKENS 		512
> Index: man/cli.3
> ===================================================================
> RCS file: /cvsroot/unbound/unbound/lib/cli/man/cli.3,v
> retrieving revision 1.3
> diff -u -r1.3 cli.3
> --- man/cli.3	19 Nov 2002 02:14:28 -0000	1.3
> +++ man/cli.3	19 Nov 2002 02:52:42 -0000
> @@ -12,17 +12,19 @@
>  .Sh SYNOPSIS
>  .Fd #include <cli.h>
>  .Ft int
> -.Fn cli_loop "int fd" "const char *prompt" "cli_handler callback"
> +.Fn cli_loop "int fdin" "int fdout" "const char *prompt" "cli_handler callback"
>  .Sh DESCRIPTION
>  .Nm
>  provides a simple API to develop command line based utilities.
>  .Pp
>  .Fn cli_loop
>  enters an infinite loop listening on 
> -.Fa fd
> +.Fa fdin
>  for input, presenting
>  .Fa prompt
> -each time, and using
> +each time on
> +.Fa fdout
> +and using
>  .Fa callback
>  to process the input.
>  .Fa callback
> @@ -31,7 +33,10 @@
>  number of arguments and a char 
>  .Fa args
>  which is read from the command line. Any non-zero return value is treated
> -as an error.
> +as an error. Setting 
> +.Fa fdout
> +to -1 causes the output from cli_loop to be silenced. This is designed for
> +non interactive shells.
>  .Sh BUGS
>  To link completely against 
>  .Nm
> Index: test/test1.c
> ===================================================================
> RCS file: /cvsroot/unbound/unbound/lib/cli/test/test1.c,v
> retrieving revision 1.2
> diff -u -r1.2 test1.c
> --- test/test1.c	19 Nov 2002 01:16:19 -0000	1.2
> +++ test/test1.c	19 Nov 2002 02:52:42 -0000
> @@ -9,7 +9,7 @@
>  {
>  	printf("c %d, v %s\n", argc, *argv);
>  
> -	cli_loop(1, "test1> ", print_opts);
> +	cli_loop(0, 1, "test1> ", print_opts);

STDIN_FILENO and STDOUT_FILENO may be a good idea.

>  
>  	exit(0);
>  }

Other than that it's OK with me :)

Thanks,
juli.
-- 
Juli Mallett <jma...@Fr...>
OpenDarwin, Mono, FreeBSD Developer.
ircd-hybrid Developer, EFnet addict.
FreeBSD on MIPS-Anything on FreeBSD.