Re: dateutils: FTBFS on hurd-i386 (for review)

["Philipp A. Hartmann" <pah@qo.cx>]

Svante,

I have some additional comments.  Please see below.

On 15/05/14 08:43, Svante Signell wrote:
>  
>  static void
> -mkfifofn(char *restrict buf, size_t bsz, const char *key, unsigned int tid)
> +mkfifofn(char **buf, const char *key, unsigned int tid)
>  {
> -	snprintf(buf, bsz, "%s output  %x", key, tid);
> +	size_t len = strlen(key) + 9 + 8 + 1;
> +	*buf = malloc(len);
> +	if (*buf == NULL)
> +		fprintf(stderr, "malloc failed\n");
> +	snprintf(*buf, len, "%s output	%x", key, tid);

If the malloc fails before, you'll dereference a NULL pointer here.
Instead of passing a char**, you should probably allocate and return the
formatted fifo name instead and adjust the use accordingly (see below):

static char*
mkfifofn(const char *key, unsigned int tid)
{
	char  *buf = NULL;
	size_t len = strlen(key) + 9 + 8 + 1;
	buf = malloc(len);
	if (buf)
		snprintf(buf, len, "%s output	%x", key, tid);
	return buf;
}

>  
> @@ -670,23 +674,32 @@ differ(struct clit_chld_s ctx[static 1],
>  #if !defined L_tmpnam
>  # define L_tmpnam	(PATH_MAX)
>  #endif	/* !L_tmpnam */
> -	static char expfn[PATH_MAX];
> -	static char actfn[PATH_MAX];

Why not simply replace PATH_MAX with L_tmpnam and be done with
everything?  Is this defined on GNU/Hurd?  Is it used elsewhere in the file?

> +	static char *expfn = NULL;
> +	static char *actfn = NULL;

Why keep them static?

> +	if (clit_bit_fn_p(exp)) {
> +		expfn = malloc(strlen(exp.d) + 1);
> +		if (expfn == NULL) {
> +			error( "malloc failed\n");
> +			goto out;
> +		}
> +		if (strcpy(expfn, exp.d) == NULL) {
> +			error("cannot prepare in file `%s'", exp.d);
> +			goto out;
> +		}

It's probably not necessary to explicitly distinguish these two errors.
 I would just use:
		if (expfn == NULL || strcpy(expfn, exp.d) == NULL)
			error("cannot prepare in file `%s'", exp.d);

> +	if (!clit_bit_fn_p(exp)) {
> +		if (mkfifofn(&expfn, "expected", ctx->test_id),
> +		    mkfifo(expfn, 0666) < 0) {

In your patch, mkfifofn can fail.  By adopting my suggestion to return
the allocated and formatted name instead, you can use an || instead of
the comma operator here:
		if (!(expfn = mkfifofn("expected", ctx->test_id)) ||
		    mkfifo(expfn, 0666) < 0) {

Alternatively, and maybe a easier to read, you should use
		expfn = mkfifofn("expected", ctx->test_id);
		if (expfn == NULL || mkfifo(expfn, 0666) < 0) {

> +	if (mkfifofn(&actfn, "actual", ctx->test_id),
> +	    mkfifo(actfn, 0666) < 0) {

Same here:
	actfn = mkfifofn("actual", ctx->test_id);
        if (actfn == NULL || mkfifo(actfn, 0666) < 0) {

>  out:
> -	if (*expfn && !clit_bit_fn_p(exp)) {
> -		unlink(expfn);
> +	if (*expfn) {

Here, you need to check expfn itself, because it may not have been
allocated.  Based on above's mkfifofn() implementation, it will then
always start with a non-NULL element.

> +		if (!clit_bit_fn_p(exp))
> +			unlink(expfn);
> +	free(expfn);
>  	}

Indentation broken? Or did you mean to free(expfn) outside of the condition?

>  	if (*actfn) {
>  		unlink(actfn);
> +		free(actfn);
>  	}

Same here, you need to check actfn, as actfn might not have been allocated.

My 2¢,
Philipp