From: Stefan Sperling <stsp@stsp.name>
Subject: Re: struct instead of buffer for got_imsg_fetch_{,have_}ref
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Wed, 1 Feb 2023 13:31:37 +0100

On Wed, Feb 01, 2023 at 09:41:44AM +0100, Omar Polo wrote:
> The usual treatment.  Run multiple times the fetch regress suite,
> still fine.
> 
> (note to ease review: got_imsg_fetch_ref is already serialized as if
> it were a struct object in send_fetch_ref.)
> 
> ok?

Thanks, ok

> 
> diff refs/heads/main refs/heads/w
> commit - 2aea6fc2c2c95ba4968b598247e00edb6f91048b
> commit + 05db677d577e42756e296c0262a02c65bfd614fb
> blob - cce4cea87c0a4dcf12328d3491728a50bcce8f84
> blob + 6a64f1edca2e533d797c2fc7ebd6e8d738023fa4
> --- lib/got_lib_privsep.h
> +++ lib/got_lib_privsep.h
> @@ -384,7 +384,7 @@ struct got_imsg_fetch_have_ref {
>  
>  /* Structure for GOT_IMSG_FETCH_HAVE_REF data. */
>  struct got_imsg_fetch_have_ref {
> -	uint8_t id[SHA1_DIGEST_LENGTH];
> +	struct got_object_id id;
>  	size_t name_len;
>  	/* Followed by name_len data bytes. */
>  } __attribute__((__packed__));
> @@ -433,7 +433,7 @@ struct got_imsg_fetch_ref {
>  /* Structure for GOT_IMSG_FETCH_REF data. */
>  struct got_imsg_fetch_ref {
>  	/* Describes a reference which will be fetched. */
> -	uint8_t refid[SHA1_DIGEST_LENGTH];
> +	struct got_object_id refid;
>  	/* Followed by reference name in remaining data of imsg buffer. */
>  };
>  
> blob - 48bdc5abc37116465cd5276b7720b4809ef3079c
> blob + 16a24da99f408f5487194b9f4ff88b84b5add165
> --- lib/privsep.c
> +++ lib/privsep.c
> @@ -578,7 +578,7 @@ got_privsep_send_fetch_req(struct imsgbuf *ibuf, int f
>  			return got_error_from_errno("imsg_create FETCH_HAVE_REF");
>  
>  		/* Keep in sync with struct got_imsg_fetch_have_ref! */
> -		if (imsg_add(wbuf, id->sha1, sizeof(id->sha1)) == -1)
> +		if (imsg_add(wbuf, id, sizeof(*id)) == -1)
>  			return got_error_from_errno("imsg_add FETCH_HAVE_REF");
>  		if (imsg_add(wbuf, &name_len, sizeof(name_len)) == -1)
>  			return got_error_from_errno("imsg_add FETCH_HAVE_REF");
> @@ -736,7 +736,7 @@ got_privsep_recv_fetch_progress(int *done, struct got_
>  		}
>  		break;
>  	case GOT_IMSG_FETCH_REF:
> -		if (datalen <= SHA1_DIGEST_LENGTH) {
> +		if (datalen <= sizeof(**id)) {
>  			err = got_error(GOT_ERR_PRIVSEP_MSG);
>  			break;
>  		}
> @@ -745,9 +745,9 @@ got_privsep_recv_fetch_progress(int *done, struct got_
>  			err = got_error_from_errno("malloc");
>  			break;
>  		}
> -		memcpy((*id)->sha1, imsg.data, SHA1_DIGEST_LENGTH);
> -		*refname = strndup(imsg.data + SHA1_DIGEST_LENGTH,
> -		    datalen - SHA1_DIGEST_LENGTH);
> +		memcpy(*id, imsg.data, sizeof(**id));
> +		*refname = strndup(imsg.data + sizeof(**id),
> +		    datalen - sizeof(**id));
>  		if (*refname == NULL) {
>  			err = got_error_from_errno("strndup");
>  			break;
> blob - 6e1ae2146ae101481c8223adead2d0fbc046ed00
> blob + 77ba914fb971f5e412b57bd1633bda6ee0e1f26d
> --- libexec/got-fetch-pack/got-fetch-pack.c
> +++ libexec/got-fetch-pack/got-fetch-pack.c
> @@ -281,7 +281,7 @@ send_fetch_ref(struct imsgbuf *ibuf, struct got_object
>  		return got_error_from_errno("imsg_create FETCH_REF");
>  
>  	/* Keep in sync with struct got_imsg_fetch_ref definition! */
> -	if (imsg_add(wbuf, refid->sha1, SHA1_DIGEST_LENGTH) == -1)
> +	if (imsg_add(wbuf, refid, sizeof(*refid)) == -1)
>  		return got_error_from_errno("imsg_add FETCH_REF");
>  	if (imsg_add(wbuf, refname, reflen) == -1)
>  		return got_error_from_errno("imsg_add FETCH_REF");
> @@ -866,7 +866,7 @@ main(int argc, char **argv)
>  			err = got_error_from_errno("malloc");
>  			goto done;
>  		}
> -		memcpy(id->sha1, href.id, SHA1_DIGEST_LENGTH);
> +		memcpy(id, &href.id, sizeof(*id));
>  		err = got_pathlist_append(&have_refs, refname, id);
>  		if (err) {
>  			free(refname);
> 
>