"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
Re: use size_t for loop indices to avoid signedness warnings
To:
Ed Maste <emaste@freebsd.org>
Cc:
gameoftrees@openbsd.org
Date:
Fri, 18 Dec 2020 16:43:38 +0100

Download raw body.

Thread
On Fri, Dec 18, 2020 at 10:24:24AM -0500, Ed Maste wrote:
> On Thu, 17 Dec 2020 at 15:48, Stefan Sperling <stsp@stsp.name> wrote:
> >
> > On Thu, Dec 17, 2020 at 09:52:58AM -0500, Ed Maste wrote:
> > > Same change as 16aeacf7088d, for subdirectories other than lib/
> >
> > Unfortunately, this patch introduced a segfault in one of the tests:
> 
> Oh no, I'm sorry I missed that; clearly my next step here is to be
> sure to get the test suite running conveniently and consistently on
> FreeBSD. Thank you and Todd for addressing this.

No worries about that. I have now learned to be more careful about
changing signed to unsigned, and I appreciate that :)

I am trying to get a clean build with -Wsign-compare on OpenBSD. There are
quite a few warnings that appear with this compiler option.
The patch below attempts to address some of these, but is not yet complete.
Most of the changes are trivial, though in some cases I am simply adding
casts to make the compiler shut up. I am not sure if this the best way to
go about it?

Are the lines I'm changing in this patch also causing warnings on FreeBSD?

See also my other patch from yesterday which fixes off_t vs size_t issues
related to struct got_pack. I also noticed that issue due to -Wsign-compare
but decided to separate those changes out from my WIP patch below.

diff refs/heads/main refs/heads/signcompare
blob - 328c4a1e1795d88a1e53519b52ad67b9e682f85a
blob + f3af4a1feed3609386071ece1967946ad7604256
--- Makefile.inc
+++ Makefile.inc
@@ -10,7 +10,7 @@ BINDIR ?= ${PREFIX}/bin
 LIBEXECDIR ?= ${PREFIX}/libexec
 MANDIR ?= ${PREFIX}/man/man
 .else
-CFLAGS += -Werror -Wall -Wstrict-prototypes -Wunused-variable
+CFLAGS += -Werror -Wall -Wstrict-prototypes -Wunused-variable -Wsign-compare
 PREFIX ?= ${HOME}
 BINDIR ?= ${PREFIX}/bin
 LIBEXECDIR ?= ${BINDIR}
blob - 772c50ed2ea44fc86f4cb411b60f6bbf55ef0a88
blob + 8a27a7a4b65de4777987b0a7b709a554dacb3ae1
--- lib/error.c
+++ lib/error.c
@@ -157,7 +157,7 @@ got_error_no_obj(struct got_object_id *id)
 		return got_error(GOT_ERR_NO_OBJ);
 
 	ret = snprintf(msg, sizeof(msg), "object %s not found", id_str);
-	if (ret == -1 || ret >= sizeof(msg))
+	if (ret == -1 || ret >= (int)sizeof(msg))
 		return got_error(GOT_ERR_NO_OBJ);
 
 	return got_error_msg(GOT_ERR_NO_OBJ, msg);
@@ -170,7 +170,7 @@ got_error_not_ref(const char *refname)
 	int ret;
 
 	ret = snprintf(msg, sizeof(msg), "reference %s not found", refname);
-	if (ret == -1 || ret >= sizeof(msg))
+	if (ret == -1 || ret >= (int)sizeof(msg))
 		return got_error(GOT_ERR_NOT_REF);
 
 	return got_error_msg(GOT_ERR_NOT_REF, msg);
blob - 1197bbb4a3b7b764589d056de56d5e8f5542a159
blob + 7fe95bf9ef1675447ceaaba59977027c9c24e465
--- lib/inflate.c
+++ lib/inflate.c
@@ -456,10 +456,14 @@ got_inflate_to_fd(size_t *outlen, FILE *infile, int ou
 		if (avail > 0) {
 			ssize_t n;
 			n = write(outfd, zb.outbuf, avail);
-			if (n != avail) {
+			if (n == -1) {
 				err = got_error_from_errno("write");
 				goto done;
 			}
+			if (n != (ssize_t)avail) {
+				err = got_error(GOT_ERR_IO);
+				goto done;
+			}
 			*outlen += avail;
 		}
 	} while (zb.flags & GOT_INFLATE_F_HAVE_MORE);
blob - adc0e664003a4f5feb61676517c02a3063685c16
blob + e3c65a399af4b173c494876c934ceed425ba0738
--- lib/object_cache.c
+++ lib/object_cache.c
@@ -133,7 +133,7 @@ got_object_cache_add(struct got_object_cache *cache, s
 {
 	const struct got_error *err = NULL;
 	struct got_object_cache_entry *ce;
-	int nelem;
+	size_t nelem;
 	size_t size;
 
 	switch (cache->type) {
blob - b46583a6e3ce2fe952e1c30726026d492c7f5734
blob + da98c8a26a12ba75506d2904a4bca4695e999a6a
--- lib/pack.c
+++ lib/pack.c
@@ -75,9 +75,8 @@ got_packidx_init_hdr(struct got_packidx *p, int verify
 	struct got_packidx_v2_hdr *h;
 	SHA1_CTX ctx;
 	uint8_t sha1[SHA1_DIGEST_LENGTH];
-	size_t nobj, len_fanout, len_ids, offset, remain;
+	size_t i, nobj, len_fanout, len_ids, offset, remain;
 	ssize_t n;
-	int i;
 
 	SHA1Init(&ctx);
 
@@ -165,7 +164,7 @@ got_packidx_init_hdr(struct got_packidx *p, int verify
 		if (n < 0) {
 			err = got_error_from_errno("read");
 			goto done;
-		} else if (n != len_fanout) {
+		} else if (n != (ssize_t)len_fanout) {
 			err = got_error(GOT_ERR_BAD_PACKIDX);
 			goto done;
 		}
@@ -196,7 +195,7 @@ got_packidx_init_hdr(struct got_packidx *p, int verify
 		n = read(p->fd, h->sorted_ids, len_ids);
 		if (n < 0)
 			err = got_error_from_errno("read");
-		else if (n != len_ids) {
+		else if (n != (ssize_t)len_ids) {
 			err = got_error(GOT_ERR_BAD_PACKIDX);
 			goto done;
 		}
@@ -221,7 +220,7 @@ got_packidx_init_hdr(struct got_packidx *p, int verify
 		n = read(p->fd, h->crc32, nobj * sizeof(*h->crc32));
 		if (n < 0)
 			err = got_error_from_errno("read");
-		else if (n != nobj * sizeof(*h->crc32)) {
+		else if (n != (ssize_t)(nobj * sizeof(*h->crc32))) {
 			err = got_error(GOT_ERR_BAD_PACKIDX);
 			goto done;
 		}
@@ -246,7 +245,7 @@ got_packidx_init_hdr(struct got_packidx *p, int verify
 		n = read(p->fd, h->offsets, nobj * sizeof(*h->offsets));
 		if (n < 0)
 			err = got_error_from_errno("read");
-		else if (n != nobj * sizeof(*h->offsets)) {
+		else if (n != (ssize_t)(nobj * sizeof(*h->offsets))) {
 			err = got_error(GOT_ERR_BAD_PACKIDX);
 			goto done;
 		}
@@ -283,7 +282,8 @@ got_packidx_init_hdr(struct got_packidx *p, int verify
 		    p->nlargeobj * sizeof(*h->large_offsets));
 		if (n < 0)
 			err = got_error_from_errno("read");
-		else if (n != p->nlargeobj * sizeof(*h->large_offsets)) {
+		else if (n != (ssize_t)(p->nlargeobj *
+		    sizeof(*h->large_offsets))) {
 			err = got_error(GOT_ERR_BAD_PACKIDX);
 			goto done;
 		}
@@ -422,7 +422,7 @@ get_object_offset(struct got_packidx *packidx, int idx
 	if (offset & GOT_PACKIDX_OFFSET_VAL_IS_LARGE_IDX) {
 		uint64_t loffset;
 		idx = offset & GOT_PACKIDX_OFFSET_VAL_MASK;
-		if (idx < 0 || idx >= packidx->nlargeobj ||
+		if (idx < 0 || (size_t)idx >= packidx->nlargeobj ||
 		    packidx->hdr.large_offsets == NULL)
 			return -1;
 		loffset = be64toh(packidx->hdr.large_offsets[idx]);
@@ -827,7 +827,7 @@ resolve_ref_delta(struct got_delta_chain *deltas, stru
 		return got_error(GOT_ERR_NO_OBJ);
 
 	base_offset = get_object_offset(packidx, idx);
-	if (base_offset == (uint64_t)-1)
+	if (base_offset == -1)
 		return got_error(GOT_ERR_BAD_PACKIDX);
 
 	if (base_offset >= pack->filesize)
@@ -930,7 +930,7 @@ got_packfile_open_object(struct got_object **obj, stru
 	*obj = NULL;
 
 	offset = get_object_offset(packidx, idx);
-	if (offset == (uint64_t)-1)
+	if (offset == -1)
 		return got_error(GOT_ERR_BAD_PACKIDX);
 
 	err = got_pack_parse_object_type_and_size(&type, &size, &tslen,
blob - d94d085c6a81b1232aacfe54421d5ffa740b8393
blob + f50b17ae1f1bd12b6cf371ba2425828fa074980f
--- lib/path.c
+++ lib/path.c
@@ -59,7 +59,7 @@ got_canonpath(const char *input, char *buf, size_t buf
 
 	p = input;
 	q = buf;
-	while (*p && (q - buf < bufsize)) {
+	while (*p && ((size_t)(q - buf) < bufsize)) {
 		if (p[0] == '/' && (p[1] == '/' || p[1] == '\0')) {
 			p += 1;
 
@@ -78,7 +78,7 @@ got_canonpath(const char *input, char *buf, size_t buf
 			*q++ = *p++;
 		}
 	}
-	if ((*p == '\0') && (q - buf < bufsize)) {
+	if ((*p == '\0') && ((size_t)(q - buf) < bufsize)) {
 		*q = 0;
 		return NULL;
 	} else
@@ -508,7 +508,7 @@ got_path_create_file(const char *path, const char *con
 	}
 
 	if (content) {
-		int len = dprintf(fd, "%s\n", content);
+		size_t len = dprintf(fd, "%s\n", content);
 		if (len != strlen(content) + 1) {
 			err = got_error_from_errno("dprintf");
 			goto done;
blob - a17a8788d58b000507ac5d3d079dfaba0fe6ac2d
blob + a5d5d38e46aeac4b4518dc103bda16a8c91918cf
--- lib/privsep.c
+++ lib/privsep.c
@@ -81,7 +81,7 @@ static const struct got_error *
 read_imsg(struct imsgbuf *ibuf)
 {
 	const struct got_error *err;
-	size_t n;
+	ssize_t n;
 
 	err = poll_fd(ibuf->fd, POLLIN, INFTIM);
 	if (err)
@@ -577,7 +577,7 @@ got_privsep_recv_fetch_progress(int *done, struct got_
 	struct got_imsg_fetch_symrefs *isymrefs = NULL;
 	size_t n, remain;
 	off_t off;
-	int i;
+	size_t i;
 
 	*done = 0;
 	*id = NULL;
@@ -2183,7 +2183,7 @@ got_privsep_recv_traversed_commits(struct got_commit_o
 	struct imsg imsg;
 	struct got_imsg_traversed_commits *icommits;
 	size_t datalen;
-	int i, done = 0;
+	size_t i, done = 0;
 
 	*changed_commit = NULL;
 	*changed_commit_id = NULL;
blob - 9d3276731396175a544ea337396a6dad1abbe92e
blob + 96b602ef060823f505561f98dd635deb73137d31
--- libexec/got-fetch-pack/got-fetch-pack.c
+++ libexec/got-fetch-pack/got-fetch-pack.c
@@ -58,7 +58,7 @@ static int chattygot;
 static struct got_object_id zhash = {.sha1={0}};
 
 static const struct got_error *
-readn(ssize_t *off, int fd, void *buf, size_t n)
+readn(ssize_t *off, int fd, void *buf, ssize_t n)
 {
 	ssize_t r;
 
@@ -195,7 +195,7 @@ writepkt(int fd, char *buf, int nbuf)
 	int i;
 	ssize_t w;
 
-	if (snprintf(len, sizeof(len), "%04x", nbuf + 4) >= sizeof(len))
+	if (snprintf(len, sizeof(len), "%04x", nbuf + 4) >= (int)sizeof(len))
 		return got_error(GOT_ERR_NO_SPACE);
 	w = write(fd, len, 4);
 	if (w == -1)
@@ -279,11 +279,12 @@ match_wanted_ref(const char *refname, const char *want
 }
 
 static const struct got_error *
-tokenize_refline(char **tokens, char *line, int len, int maxtokens)
+tokenize_refline(char **tokens, char *line, int len, size_t maxtokens)
 {
 	const struct got_error *err = NULL;
 	char *p;
-	size_t i, n = 0;
+	size_t i;
+	ssize_t n = 0;
 
 	for (i = 0; i < maxtokens; i++)
 		tokens[i] = NULL;
@@ -314,7 +315,7 @@ tokenize_refline(char **tokens, char *line, int len, i
 		err = got_error(GOT_ERR_NOT_REF);
 done:
 	if (err) {
-		int j;
+		size_t j;
 		for (j = 0; j < i; j++) {
 			free(tokens[j]);
 			tokens[j] = NULL;
@@ -845,7 +846,7 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 		got_sha1_digest_to_str(want[i].sha1, hashstr, sizeof(hashstr));
 		n = snprintf(buf, sizeof(buf), "want %s%s\n", hashstr,
 		    sent_my_capabilites ? "" : my_capabilities);
-		if (n >= sizeof(buf)) {
+		if (n >= (int)sizeof(buf)) {
 			err = got_error(GOT_ERR_NO_SPACE);
 			goto done;
 		}
@@ -867,7 +868,7 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 			continue;
 		got_sha1_digest_to_str(have[i].sha1, hashstr, sizeof(hashstr));
 		n = snprintf(buf, sizeof(buf), "have %s\n", hashstr);
-		if (n >= sizeof(buf)) {
+		if (n >= (int)sizeof(buf)) {
 			err = got_error(GOT_ERR_NO_SPACE);
 			goto done;
 		}
@@ -951,7 +952,7 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 				    "short packet");
 				goto done;
 			}
-			if (datalen > sizeof(buf) - 5) {
+			if (datalen > (int)sizeof(buf) - 5) {
 				err = got_error_msg(GOT_ERR_BAD_PACKET,
 				    "bad packet length");
 				goto done;
@@ -1106,7 +1107,8 @@ int
 main(int argc, char **argv)
 {
 	const struct got_error *err = NULL;
-	int fetchfd, packfd = -1, i;
+	int fetchfd, packfd = -1;
+	size_t i;
 	uint8_t pack_sha1[SHA1_DIGEST_LENGTH];
 	struct imsgbuf ibuf;
 	struct imsg imsg;
blob - bcef0951dbb771b4b70ab05125138f52e08c7b7a
blob + 2008221a416eaf914ecefdd70e339b2503ffb9e3
--- libexec/got-index-pack/got-index-pack.c
+++ libexec/got-index-pack/got-index-pack.c
@@ -288,7 +288,7 @@ read_packed_object(struct got_pack *pack, struct got_i
 				err = got_error_from_errno("read");
 				break;
 			}
-			if (n < sizeof(obj->id)) {
+			if (n < (ssize_t)sizeof(obj->id)) {
 				err = got_error(GOT_ERR_BAD_PACKFILE);
 				break;
 			}
@@ -647,7 +647,7 @@ index_pack(struct got_pack *pack, int idxfd, FILE *tmp
 		r = read(pack->fd, &hdr, sizeof(hdr));
 		if (r == -1)
 			return got_error_from_errno("read");
-		if (r < sizeof(hdr))
+		if (r < (ssize_t)sizeof(hdr))
 			return got_error_msg(GOT_ERR_BAD_PACKFILE,
 			    "short pack file");
 	}
blob - 27d10ab755d53e96136938c0a9d1d5a574974686
blob + 18d020443d6d000b13359edb806c01d8dcc78892
--- libexec/got-read-pack/got-read-pack.c
+++ libexec/got-read-pack/got-read-pack.c
@@ -252,7 +252,7 @@ tree_request(struct imsg *imsg, struct imsgbuf *ibuf, 
 }
 
 static const struct got_error *
-receive_file(FILE **f, struct imsgbuf *ibuf, int imsg_code)
+receive_file(FILE **f, struct imsgbuf *ibuf, unsigned int imsg_code)
 {
 	const struct got_error *err;
 	struct imsg imsg;
@@ -535,7 +535,7 @@ tree_path_changed(int *changed, uint8_t **buf1, uint8_
 }
 
 static const struct got_error *
-send_traversed_commits(struct got_object_id *commit_ids, size_t ncommits,
+send_traversed_commits(struct got_object_id *commit_ids, int ncommits,
     struct imsgbuf *ibuf)
 {
 	const struct got_error *err;