Re: [RFC] Add first whack at interpolated daemon paths.

From: Junio C Hamano <junkio@cox.net>
Date: Jun 5, 2006, 06:20 UTC
Message-ID: <7vac8s5dk2.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <E1Fn6B9-00017u-BV@jdl.com>

Jon Loeliger <jdl@jdl.com> writes:

> This is really RFC-ish.  No canonicalization of hostname is done.

That's fine.  RFC-ish is what gets the ball rolling.

Show 12 quoted lines

> diff --git a/Makefile b/Makefile
> index 004c216..6a02236 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -211,6 +211,7 @@ DIFF_OBJS = \
>  LIB_OBJS = \
>  	blob.o commit.o connect.o csum-file.o cache-tree.o base85.o \
>  	date.o diff-delta.o entry.o exec_cmd.o ident.o index.o \
> +	interpolate.o \
>  	object.o pack-check.o patch-delta.o path.o pkt-line.o \
>  	quote.o read-cache.o refs.o run-command.o dir.o \
>  	server-info.o setup.o sha1_file.o sha1_name.o strbuf.o \

A separate helper library is always a good idea. Is it "interpolate", I wonder, however?

Show 16 quoted lines

> diff --git a/connect.c b/connect.c
> index 54f7bf7..9e7b276 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -374,7 +374,13 @@ static int git_tcp_connect(int fd[2], co
>  
>  	fd[0] = sockfd;
>  	fd[1] = sockfd;
> -	packet_write(sockfd, "%s %s\n", prog, path);
> +
> +	/*
> +	 * Separate original protocol components prog and path
> +	 * from extended components with a NUL byte.
> +	 */
> +	packet_write(sockfd, "%s %s%cHOST=%s\n", prog, path, 0, host);
> +

Since packet interface reader knows the length of the total string, it might make sense to use NUL as the terminator betweeen extended attributes (i.e. end the format with "...HOST=%s%c" and append 0 as the last argument). Some of our later extended attributes might want to have LF in them for whatever reason we do not foresee here.

Show 17 quoted lines

>  	return 0;
>  }
>  
> @@ -443,7 +449,13 @@ static int git_tcp_connect(int fd[2], co
>  
>  	fd[0] = sockfd;
>  	fd[1] = sockfd;
> -	packet_write(sockfd, "%s %s\n", prog, path);
> +
> +	/*
> +	 * Separate original protocol components prog and path
> +	 * from extended components with a NUL byte.
> +	 */
> +	packet_write(sockfd, "%s %s%cHOST=%s\n", prog, path, 0, host);
> +
>  	return 0;
>  }

We probably would want to share this part between two git_tcp_connect() implementations. Rename two #ifdef'ed implemenations of git_tcp_connect() to git_tcp_connect_sock(), make its sole purpose to just open the connection and return the sockfd. Write a new git_tcp_connect() which is not #ifdef'ed, call git_tcp_connect_sock() from there, and do the part after "if (sockfd < 0)" in the new git_tcp_connect().

Show 22 quoted lines

> +/* Flag indicating client sent extra args. */
> +int saw_extended_args = 0;
>  
>  /* If defined, ~user notation is allowed and the string is inserted
>   * after ~user/.  E.g. a request to git://host/~alice/frotz would
> @@ -41,6 +50,23 @@ static char *user_path = NULL;
>  static unsigned int timeout = 0;
>  static unsigned int init_timeout = 0;
>  
> +/*
> + * Static table for now.  Ugh.
> + * Feel free to make dynamic as needed.
> + */
> +#define INTERP_SLOT_HOST	(0)
> +#define INTERP_SLOT_DIR		(1)
> +#define INTERP_SLOT_PERCENT	(2)
> +
> +struct interp interp_table[] = {
> +	{ "%H", 0},
> +	{ "%D", 0},
> +	{ "%%", "%"},
> +};

As Linus mentioned, %H for full host and %h for the most specific part may make sense. I wonder if %D has a practical value. It lets you splice the client supplied directory path in the middle of the final string, but would it be useful in practice? Otherwise, maybe dropping %D and always appending at the tail of "interporated" path that is tailored for the virtual host might be easier to code and explain.

As to naming, "interp" sounds as if we are dealing with some "interpreter" here...

> +#define N_INTERPS	(sizeof(interp_table) / sizeof(struct interp))

I would use ARRAY_SIZE() from git-compat-util.h where you would use N_INTERPS and drop the #define altogether.

Show 17 quoted lines

> +	else if (interpolated_path && saw_extended_args) {
> +		if (*dir != '/') {
> +			/* Allow only absolute */
> +			logerror("'%s': Non-absolute path denied (interpolated-path active)", dir);
> +			return NULL;
> +		}
> +
> +		loginfo("Before interpolation '%s'", dir);
> +		loginfo("Interp slot 0 (%s,%s)",
> +			interp_table[0].name, interp_table[0].value);
> +		loginfo("Interp slot 1 (%s,%s)",
> +			interp_table[1].name, interp_table[1].value);
> +		interpolate(interp_path, PATH_MAX, interpolated_path,
> +			    interp_table, N_INTERPS);
> +		loginfo("After interpolation '%s'", interp_path);
> +		dir = interp_path;
> +	}

I suspect it would be easier to maintain the site if you let the site administrator the default virtual host name, and for requests from older clients use that name to interpolate %H, and always use this codepath and nothing else (in other words, lose "saw_extended_args" check) for both old and new clients.

With your patch, the administrator needs to configure the daemon with --base_path=/mnt1/git.or.cz/git --interpolate=/mnt1/%H/git and maintain them in sync.

Show 15 quoted lines

> diff --git a/interpolate.c b/interpolate.c
>...
> +	if (p) {
> +	    /*
> +	     * Found a potential interpolation point.
> +	     */
> +	    for (i = 0; i < ninterps; i++) {
> +		name = interps[i].name;
> +		if (strncmp(p, name, strlen(name)) == 0)
> +		    break;
> +	    }
> +
> +	    value = interps[i].value;
> +	    valuelen = strlen(value);
> +	    printf("Interp: %s to %s\n", name, value);

Wouldn't a daemon misconfigured with --interpolate="/mnt/%X" barf here by overstepping interps[] array?