From: Stefan Sperling <stsp@stsp.name>
Subject: Re: small fix: return err instead of NULL
To: Mark Jamsek <mark@jamsek.com>
Cc: Game of Trees <gameoftrees@openbsd.org>
Date: Fri, 3 Feb 2023 08:19:37 +0100

On Fri, Feb 03, 2023 at 05:22:15PM +1100, Mark Jamsek wrote:
> I stumbled upon another case in got.c where we mistakenly return NULL
> instead of err, so I grepped the codebase and caught a couple more in
> gotwebd/got_operations.c and lib/object_open_privsep.c
> 
> There's also a small typo and style(9) fix in gotd/repo_write.c
 
ok

> -----------------------------------------------
> commit adcf89fdd9ac45e9f4eb4631af7f170343896bb3 (main)
> from: Mark Jamsek <mark@jamsek.dev>
> date: Fri Feb  3 06:07:51 2023 UTC
>  
>  typo and style(9): do not use function calls in initialisers.
>  
>  M  gotd/repo_write.c  |  4+  2-
> 
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff 6b697adbe4a7eb0024dd06c695a5a922d15bb307 adcf89fdd9ac45e9f4eb4631af7f170343896bb3
> commit - 6b697adbe4a7eb0024dd06c695a5a922d15bb307
> commit + adcf89fdd9ac45e9f4eb4631af7f170343896bb3
> blob - 06a5bdff23de1154538510232e0cf2998523a312
> blob + b425ba49c41d4c20c076b2c6c6a77b84366abf0d
> --- gotd/repo_write.c
> +++ gotd/repo_write.c
> @@ -938,15 +938,17 @@ recv_packfile(int *have_packfile, struct imsg *imsg)
>  	}
>  
>  	for (i = 0; i < nitems(tempfiles); i++) {
> -		int fd = dup(repo_tempfiles[i].fd);
> +		int fd;
>  		FILE *f;
> +
> +		fd = dup(repo_tempfiles[i].fd);
>  		if (fd == -1) {
>  			err = got_error_from_errno("dup");
>  			goto done;
>  		}
>  		f = fdopen(fd, "w+");
>  		if (f == NULL) {
> -			err = got_error_from_errno("dup");
> +			err = got_error_from_errno("fdopen");
>  			close(fd);
>  			goto done;
>  		}
> 
> -----------------------------------------------
> commit 6b697adbe4a7eb0024dd06c695a5a922d15bb307
> from: Mark Jamsek <mark@jamsek.dev>
> date: Fri Feb  3 06:07:09 2023 UTC
>  
>  fix mistaken instances returning NULL instead of err
>  
>  While here, for consistency, check dup() return value for -1 rather than < 0.
>  
>  M  got/got.c                  |  1+  1-
>  M  gotwebd/got_operations.c   |  2+  2-
>  M  lib/object_open_privsep.c  |  1+  1-
> 
> 3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff 49672ed88ead342f74c0a8f753e8606a9f9b7818 6b697adbe4a7eb0024dd06c695a5a922d15bb307
> commit - 49672ed88ead342f74c0a8f753e8606a9f9b7818
> commit + 6b697adbe4a7eb0024dd06c695a5a922d15bb307
> blob - 8b62a11fd84c9ac104aee2c98521c54f0ea943ec
> blob + fe66f5fc26c7e236053a98d519915b574179a529
> --- got/got.c
> +++ got/got.c
> @@ -2088,7 +2088,7 @@ done:
>  done:
>  	free(id);
>  	free(id_str);
> -	return NULL;
> +	return err;
>  }
>  
>  static const struct got_error *
> blob - e22d214bd2d3e68c26a5394cc271c0a6e204a5aa
> blob + d5d0541d8afdcc978ab860c284e920413c003e97
> --- gotwebd/got_operations.c
> +++ gotwebd/got_operations.c
> @@ -93,8 +93,8 @@ got_gotweb_dupfd(int *priv_fd, int *fd)
>  {
>  	*fd = dup(*priv_fd);
>  
> -	if (*fd < 0)
> -		return NULL;
> +	if (*fd == -1)
> +		return got_error_from_errno("dup");
>  
>  	return NULL;
>  }
> blob - 86db216cc1bca6e3651b62b90bb2d68c238065dd
> blob + f80d86a8686711ff5888dd6847e0d84b98bc0672
> --- lib/object_open_privsep.c
> +++ lib/object_open_privsep.c
> @@ -104,7 +104,7 @@ done:
>  			close(accumfd);
>  	} else
>  		pack->child_has_tempfiles = 1;
> -	return NULL;
> +	return err;
>  }
>  
>  static const struct got_error *
> 
> -- 
> Mark Jamsek <fnc.bsdbox.org>
> GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68