{"thread":{"id":"32989","subject":"[PATCH] pkt-line: Fix sparse errors and warnings","startedAt":"2013-02-23T18:44:04Z","lastAt":"2013-02-26T19:02:59Z","messageCount":6,"participants":["Ramsay Jones","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"210101","messageId":"51290DF4.4040309@ramsay1.demon.co.uk","threadId":"32989","inReplyTo":null,"subject":"[PATCH] pkt-line: Fix sparse errors and warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-02-23T18:44:04Z","receivedAt":"2013-02-23T18:44:04Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSparse issues the following error and warnings:\n\n    pkt-line.c:209:51: warning: Using plain integer as NULL pointer\n    sideband.c:41:52: warning: Using plain integer as NULL pointer\n    daemon.c:615:39: warning: Using plain integer as NULL pointer\n    remote-curl.c:220:75: error: incompatible types for operation (>)\n    remote-curl.c:220:75:    left side has type char *\n    remote-curl.c:220:75:    right side has type int\n    remote-curl.c:291:53: warning: Using plain integer as NULL pointer\n    remote-curl.c:408:43: warning: Using plain integer as NULL pointer\n    remote-curl.c:562:47: warning: Using plain integer as NULL pointer\n\nAll of these complaints \"blame\" to commit 17243606 (\"pkt-line: share\nbuffer/descriptor reading implementation\", 20-02-2013).\n\nIn order to suppress the warnings, we simply replace the integer\nconstant 0 with NULL.\n\nIn order to suppress the error message, we simply remove the \"> 0\" from\nthe while loop controlling expression.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nHi Jeff,\n\nWhen you next re-roll your 'jk/pkt-line-cleanup' patches, could you\nplease squash this (or something like it) into commit 17243606\n(\"pkt-line: share buffer/descriptor reading implementation\", 20-02-2013).\n\nPlease check the resolution of the sparse error, remote-curl.c:220, since\nI didn't think too deeply about it, making some assumptions ... ;-)\n\nNote: the commit message mentions an strbuf_get_line() function, but\nthat is supposed to be packet_get_line(), right?\n\nIn addition, that commit adds the following code as part of function\nget_packet_data():\n\n+       /* Read up to \"size\" bytes from our source, whatever it is. */\n+       if (src_buf && *src_buf) {\n+               ret = size < *src_size ? size : *src_size;\n+               memcpy(dst, *src_buf, ret);\n+               *src_buf += size;\n............................^^^^^\n+               *src_size -= size;\n+       } else {\n+\n\nThis could lead to the source buffer pointer being incremented past the\n\"one past the end\" of the buffer; ie to undefined behaviour. That use\nof 'size', along with the one on the following line, should be 'ret' no?\n\nATB,\nRamsay Jones\n\n daemon.c      | 2 +-\n pkt-line.c    | 2 +-\n remote-curl.c | 8 ++++----\n sideband.c    | 2 +-\n 4 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 9a241d9..82d5bf5 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -612,7 +612,7 @@ static int execute(void)\n \t\tloginfo(\"Connection from %s:%s\", addr, port);\n \n \talarm(init_timeout ? init_timeout : timeout);\n-\tpktlen = packet_read(0, NULL, 0, packet_buffer, sizeof(packet_buffer), 0);\n+\tpktlen = packet_read(0, NULL, NULL, packet_buffer, sizeof(packet_buffer), 0);\n \talarm(0);\n \n \tlen = strlen(line);\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 116d5f1..2793ecb 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -206,7 +206,7 @@ static char *packet_read_line_generic(int fd,\n \n char *packet_read_line(int fd, int *len_p)\n {\n-\treturn packet_read_line_generic(fd, NULL, 0, len_p);\n+\treturn packet_read_line_generic(fd, NULL, NULL, len_p);\n }\n \n char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 3d2b194..93a09a6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -217,7 +217,7 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \t\t * until a packet flush marker.  Ignore these now, but\n \t\t * in the future we might start to scan them.\n \t\t */\n-\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL) > 0)\n+\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n \t\t\t;\n \n \t\tlast->proto_git = 1;\n@@ -288,7 +288,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \n \tif (!avail) {\n \t\trpc->initial_buffer = 0;\n-\t\tavail = packet_read(rpc->out, NULL, 0, rpc->buf, rpc->alloc, 0);\n+\t\tavail = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 0);\n \t\tif (!avail)\n \t\t\treturn 0;\n \t\trpc->pos = 0;\n@@ -405,7 +405,7 @@ static int post_rpc(struct rpc_state *rpc)\n \t\t\tbreak;\n \t\t}\n \n-\t\tn = packet_read(rpc->out, 0, NULL, buf, left, 0);\n+\t\tn = packet_read(rpc->out, NULL, NULL, buf, left, 0);\n \t\tif (!n)\n \t\t\tbreak;\n \t\trpc->len += n;\n@@ -559,7 +559,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \trpc->hdr_accept = strbuf_detach(&buf, NULL);\n \n \twhile (!err) {\n-\t\tint n = packet_read(rpc->out, 0, NULL, rpc->buf, rpc->alloc, 0);\n+\t\tint n = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 0);\n \t\tif (!n)\n \t\t\tbreak;\n \t\trpc->pos = 0;\ndiff --git a/sideband.c b/sideband.c\nindex 857954c..d1125f5 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -38,7 +38,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \n \twhile (1) {\n \t\tint band, len;\n-\t\tlen = packet_read(in_stream, NULL, 0, buf + pf, LARGE_PACKET_MAX, 0);\n+\t\tlen = packet_read(in_stream, NULL, NULL, buf + pf, LARGE_PACKET_MAX, 0);\n \t\tif (len == 0)\n \t\t\tbreak;\n \t\tif (len < 1) {\n-- \n1.8.1\n"},{"id":"210113","messageId":"20130223223134.GA2504@sigill.intra.peff.net","threadId":"32989","inReplyTo":"51290DF4.4040309@ramsay1.demon.co.uk","subject":"Re: [PATCH] pkt-line: Fix sparse errors and warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-23T22:31:34Z","receivedAt":"2013-02-23T22:31:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 23, 2013 at 06:44:04PM +0000, Ramsay Jones wrote:\n\n> Sparse issues the following error and warnings:\n> \n>     pkt-line.c:209:51: warning: Using plain integer as NULL pointer\n>     sideband.c:41:52: warning: Using plain integer as NULL pointer\n>     daemon.c:615:39: warning: Using plain integer as NULL pointer\n>     remote-curl.c:220:75: error: incompatible types for operation (>)\n>     remote-curl.c:220:75:    left side has type char *\n>     remote-curl.c:220:75:    right side has type int\n>     remote-curl.c:291:53: warning: Using plain integer as NULL pointer\n>     remote-curl.c:408:43: warning: Using plain integer as NULL pointer\n>     remote-curl.c:562:47: warning: Using plain integer as NULL pointer\n> \n> All of these complaints \"blame\" to commit 17243606 (\"pkt-line: share\n> buffer/descriptor reading implementation\", 20-02-2013).\n> \n> In order to suppress the warnings, we simply replace the integer\n> constant 0 with NULL.\n\nThanks for catching. This was just a think-o on my part; we want to pass\n(NULL, 0) as the (buf, len) pair, but of course we are passing pointers\nto them, so it is actually (NULL, NULL). And yes, I made sure the\nfunction correctly handles both a NULL pointer-to-buf and a NULL\npointer-to-pointer-to-buf (and we do not even care about the\npointer-to-len unless the buf pointer is valid).\n\nSo no bug, but definitely a reasonable cleanup.\n\nOddly, you seemed to miss the one in connect.c (which my sparse does\ndetect).\n\n> In order to suppress the error message, we simply remove the \"> 0\" from\n> the while loop controlling expression.\n\nYeah, that is the right thing to do. The code that's there isn't wrong,\nbut I agree that checking \"ptr != NULL\" (i.e., just \"ptr\") is much more\nsane than \"ptr > 0\" (and is just a leftover that I missed during\nrefactoring the patch).\n\n> When you next re-roll your 'jk/pkt-line-cleanup' patches, could you\n> please squash this (or something like it) into commit 17243606\n> (\"pkt-line: share buffer/descriptor reading implementation\", 20-02-2013).\n\nI don't think we otherwise need a re-roll. Junio hasn't merged the\nseries to next yet, so I've included an updated patch 15 below with your\nchanges (your patch, the missing connect.c fix, the commit message fix,\nand the size/ret thing below).\n\n> In addition, that commit adds the following code as part of function\n> get_packet_data():\n> \n> +       /* Read up to \"size\" bytes from our source, whatever it is. */\n> +       if (src_buf && *src_buf) {\n> +               ret = size < *src_size ? size : *src_size;\n> +               memcpy(dst, *src_buf, ret);\n> +               *src_buf += size;\n> ............................^^^^^\n> +               *src_size -= size;\n> +       } else {\n> +\n> \n> This could lead to the source buffer pointer being incremented past the\n> \"one past the end\" of the buffer; ie to undefined behaviour. That use\n> of 'size', along with the one on the following line, should be 'ret' no?\n\nYeah, thanks for catching. We just die immediately afterwards anyway,\nand sane systems do not care what kind of junk you put into a pointer\nunless you dereference. But clearly it is supposed to be \"ret\".\n\nThanks for the report. Clearly I should start running \"make sparse\" more\noften (the reason I don't is that it produces dozens of complaints about\nconstants in /usr/include/bits/xopen_lim.h, which I could obviously care\nless about; I should look into suppressing that).\n\nUpdated patch 15/19 is below.\n\n-- >8 --\nSubject: [PATCH] pkt-line: share buffer/descriptor reading implementation\n\nThe packet_read function reads from a descriptor. The\npacket_get_line function is similar, but reads from an\nin-memory buffer, and uses a completely separate\nimplementation. This patch teaches the generic packet_read\nfunction to accept either source, and we can do away with\npacket_get_line's implementation.\n\nThere are two other differences to account for between the\nold and new functions. The first is that we used to read\ninto a strbuf, but now read into a fixed size buffer. The\nonly two callers are fine with that, and in fact it\nsimplifies their code, since they can use the same\nstatic-buffer interface as the rest of the packet_read_line\ncallers (and we provide a similar convenience wrapper for\nreading from a buffer rather than a descriptor).\n\nThis is technically an externally-visible behavior change in\nthat we used to accept arbitrary sized packets up to 65532\nbytes, and now cap out at LARGE_PACKET_MAX, 65520. In\npractice this doesn't matter, as we use it only for parsing\nsmart-http headers (of which there is exactly one defined,\nand it is small and fixed-size). And any extension headers\nwould be breaking the protocol to go over LARGE_PACKET_MAX\nanyway.\n\nThe other difference is that packet_get_line would return\non error rather than dying. However, both callers of\npacket_get_line are actually improved by dying.\n\nThe first caller does its own error checking, but we can\ndrop that; as a result, we'll actually get more specific\nreporting about protocol breakage when packet_read dies\ninternally. The only downside is that packet_read will not\nprint the smart-http URL that failed, but that's not a big\ndeal; anybody not debugging can already see the remote's URL\nalready, and anybody debugging would want to run with\nGIT_CURL_VERBOSE anyway to see way more information.\n\nThe second caller, which is just trying to skip past any\nextra smart-http headers (of which there are none defined,\nbut which we allow to keep room for future expansion), did\nnot error check at all. As a result, it would treat an error\njust like a flush packet. The resulting mess would generally\ncause an error later in get_remote_heads, but now we get\nerror reporting much closer to the source of the problem.\n\nBrown-paper-bag-fixes-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n connect.c     |  3 ++-\n daemon.c      |  2 +-\n pkt-line.c    | 76 +++++++++++++++++++++++++++++------------------------------\n pkt-line.h    | 23 +++++++++++++-----\n remote-curl.c | 22 ++++++++---------\n sideband.c    |  2 +-\n 6 files changed, 69 insertions(+), 59 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 611ffb4..3d99999 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -76,7 +76,8 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \t\tint len, name_len;\n \t\tchar *buffer = packet_buffer;\n \n-\t\tlen = packet_read(in, packet_buffer, sizeof(packet_buffer),\n+\t\tlen = packet_read(in, NULL, NULL,\n+\t\t\t\t  packet_buffer, sizeof(packet_buffer),\n \t\t\t\t  PACKET_READ_GENTLE_ON_EOF |\n \t\t\t\t  PACKET_READ_CHOMP_NEWLINE);\n \t\tif (len < 0)\ndiff --git a/daemon.c b/daemon.c\nindex 3f70e79..82d5bf5 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -612,7 +612,7 @@ static int execute(void)\n \t\tloginfo(\"Connection from %s:%s\", addr, port);\n \n \talarm(init_timeout ? init_timeout : timeout);\n-\tpktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);\n+\tpktlen = packet_read(0, NULL, NULL, packet_buffer, sizeof(packet_buffer), 0);\n \talarm(0);\n \n \tlen = strlen(line);\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 55fb688..70f1950 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -104,12 +104,28 @@ static int safe_read(int fd, void *buffer, unsigned size, int options)\n \tstrbuf_add(buf, buffer, n);\n }\n \n-static int safe_read(int fd, void *buffer, unsigned size, int options)\n+static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n+\t\t\t   void *dst, unsigned size, int options)\n {\n-\tssize_t ret = read_in_full(fd, buffer, size);\n-\tif (ret < 0)\n-\t\tdie_errno(\"read error\");\n-\telse if (ret < size) {\n+\tssize_t ret;\n+\n+\tif (fd >= 0 && src_buf && *src_buf)\n+\t\tdie(\"BUG: multiple sources given to packet_read\");\n+\n+\t/* Read up to \"size\" bytes from our source, whatever it is. */\n+\tif (src_buf && *src_buf) {\n+\t\tret = size < *src_size ? size : *src_size;\n+\t\tmemcpy(dst, *src_buf, ret);\n+\t\t*src_buf += ret;\n+\t\t*src_size -= ret;\n+\t} else {\n+\t\tret = read_in_full(fd, dst, size);\n+\t\tif (ret < 0)\n+\t\t\tdie_errno(\"read error\");\n+\t}\n+\n+\t/* And complain if we didn't get enough bytes to satisfy the read. */\n+\tif (ret < size) {\n \t\tif (options & PACKET_READ_GENTLE_ON_EOF)\n \t\t\treturn -1;\n \n@@ -144,12 +160,13 @@ int packet_read(int fd, char *buffer, unsigned size, int options)\n \treturn len;\n }\n \n-int packet_read(int fd, char *buffer, unsigned size, int options)\n+int packet_read(int fd, char **src_buf, size_t *src_len,\n+\t\tchar *buffer, unsigned size, int options)\n {\n \tint len, ret;\n \tchar linelen[4];\n \n-\tret = safe_read(fd, linelen, 4, options);\n+\tret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);\n \tif (ret < 0)\n \t\treturn ret;\n \tlen = packet_length(linelen);\n@@ -162,7 +179,7 @@ int packet_read(int fd, char *buffer, unsigned size, int options)\n \tlen -= 4;\n \tif (len >= size)\n \t\tdie(\"protocol error: bad line length %d\", len);\n-\tret = safe_read(fd, buffer, len, options);\n+\tret = get_packet_data(fd, src_buf, src_len, buffer, len, options);\n \tif (ret < 0)\n \t\treturn ret;\n \n@@ -175,41 +192,24 @@ int packet_get_line(struct strbuf *out,\n \treturn len;\n }\n \n-char *packet_read_line(int fd, int *len_p)\n+static char *packet_read_line_generic(int fd,\n+\t\t\t\t      char **src, size_t *src_len,\n+\t\t\t\t      int *dst_len)\n {\n-\tint len = packet_read(fd, packet_buffer, sizeof(packet_buffer),\n+\tint len = packet_read(fd, src, src_len,\n+\t\t\t      packet_buffer, sizeof(packet_buffer),\n \t\t\t      PACKET_READ_CHOMP_NEWLINE);\n-\tif (len_p)\n-\t\t*len_p = len;\n+\tif (dst_len)\n+\t\t*dst_len = len;\n \treturn len ? packet_buffer : NULL;\n }\n \n-int packet_get_line(struct strbuf *out,\n-\tchar **src_buf, size_t *src_len)\n+char *packet_read_line(int fd, int *len_p)\n {\n-\tint len;\n-\n-\tif (*src_len < 4)\n-\t\treturn -1;\n-\tlen = packet_length(*src_buf);\n-\tif (len < 0)\n-\t\treturn -1;\n-\tif (!len) {\n-\t\t*src_buf += 4;\n-\t\t*src_len -= 4;\n-\t\tpacket_trace(\"0000\", 4, 0);\n-\t\treturn 0;\n-\t}\n-\tif (*src_len < len)\n-\t\treturn -2;\n-\n-\t*src_buf += 4;\n-\t*src_len -= 4;\n-\tlen -= 4;\n+\treturn packet_read_line_generic(fd, NULL, NULL, len_p);\n+}\n \n-\tstrbuf_add(out, *src_buf, len);\n-\t*src_buf += len;\n-\t*src_len -= len;\n-\tpacket_trace(out->buf, out->len, 0);\n-\treturn len;\n+char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)\n+{\n+\treturn packet_read_line_generic(-1, src, src_len, dst_len);\n }\ndiff --git a/pkt-line.h b/pkt-line.h\nindex fa93e32..0a838d1 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,9 +25,16 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n \n /*\n- * Read a packetized line from the descriptor into the buffer, which must be at\n- * least size bytes long. The return value specifies the number of bytes read\n- * into the buffer.\n+ * Read a packetized line into the buffer, which must be at least size bytes\n+ * long. The return value specifies the number of bytes read into the buffer.\n+ *\n+ * If src_buffer is not NULL (and nor is *src_buffer), it should point to a\n+ * buffer containing the packet data to parse, of at least *src_len bytes.\n+ * After the function returns, src_buf will be incremented and src_len\n+ * decremented by the number of bytes consumed.\n+ *\n+ * If src_buffer (or *src_buffer) is NULL, then data is read from the\n+ * descriptor \"fd\".\n  *\n  * If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any\n  * of the following conditions:\n@@ -50,7 +57,8 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f\n  */\n #define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n-int packet_read(int fd, char *buffer, unsigned size, int options);\n+int packet_read(int fd, char **src_buffer, size_t *src_len, char\n+\t\t*buffer, unsigned size, int options);\n \n /*\n  * Convenience wrapper for packet_read that is not gentle, and sets the\n@@ -61,11 +69,14 @@ extern char packet_buffer[LARGE_PACKET_MAX];\n  */\n char *packet_read_line(int fd, int *size);\n \n+/*\n+ * Same as packet_read_line, but read from a buf rather than a descriptor;\n+ * see packet_read for details on how src_* is used.\n+ */\n+char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\n \n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n extern char packet_buffer[LARGE_PACKET_MAX];\n \n-int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);\n-\n #endif\ndiff --git a/remote-curl.c b/remote-curl.c\nindex b28f965..c8379a5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -138,28 +138,26 @@ static struct discovery* discover_refs(const char *service)\n \tif (maybe_smart &&\n \t    (5 <= last->len && last->buf[4] == '#') &&\n \t    !strbuf_cmp(&exp, &type)) {\n+\t\tchar *line;\n+\n \t\t/*\n \t\t * smart HTTP response; validate that the service\n \t\t * pkt-line matches our request.\n \t\t */\n-\t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n-\t\t\tdie(\"%s has invalid packet header\", refs_url);\n-\t\tif (buffer.len && buffer.buf[buffer.len - 1] == '\\n')\n-\t\t\tstrbuf_setlen(&buffer, buffer.len - 1);\n+\t\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\n \n \t\tstrbuf_reset(&exp);\n \t\tstrbuf_addf(&exp, \"# service=%s\", service);\n-\t\tif (strbuf_cmp(&exp, &buffer))\n-\t\t\tdie(\"invalid server response; got '%s'\", buffer.buf);\n+\t\tif (strcmp(line, exp.buf))\n+\t\t\tdie(\"invalid server response; got '%s'\", line);\n \t\tstrbuf_release(&exp);\n \n \t\t/* The header can include additional metadata lines, up\n \t\t * until a packet flush marker.  Ignore these now, but\n \t\t * in the future we might start to scan them.\n \t\t */\n-\t\tstrbuf_reset(&buffer);\n-\t\twhile (packet_get_line(&buffer, &last->buf, &last->len) > 0)\n-\t\t\tstrbuf_reset(&buffer);\n+\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n+\t\t\t;\n \n \t\tlast->proto_git = 1;\n \t}\n@@ -308,7 +306,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n \n \tif (!avail) {\n \t\trpc->initial_buffer = 0;\n-\t\tavail = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);\n+\t\tavail = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 0);\n \t\tif (!avail)\n \t\t\treturn 0;\n \t\trpc->pos = 0;\n@@ -425,7 +423,7 @@ static int post_rpc(struct rpc_state *rpc)\n \t\t\tbreak;\n \t\t}\n \n-\t\tn = packet_read(rpc->out, buf, left, 0);\n+\t\tn = packet_read(rpc->out, NULL, NULL, buf, left, 0);\n \t\tif (!n)\n \t\t\tbreak;\n \t\trpc->len += n;\n@@ -579,7 +577,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \trpc->hdr_accept = strbuf_detach(&buf, NULL);\n \n \twhile (!err) {\n-\t\tint n = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);\n+\t\tint n = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 0);\n \t\tif (!n)\n \t\t\tbreak;\n \t\trpc->pos = 0;\ndiff --git a/sideband.c b/sideband.c\nindex 15cc1ae..d1125f5 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -38,7 +38,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \n \twhile (1) {\n \t\tint band, len;\n-\t\tlen = packet_read(in_stream, buf + pf, LARGE_PACKET_MAX, 0);\n+\t\tlen = packet_read(in_stream, NULL, NULL, buf + pf, LARGE_PACKET_MAX, 0);\n \t\tif (len == 0)\n \t\t\tbreak;\n \t\tif (len < 1) {\n-- \n1.8.1.4.4.g265d2fa\n"},{"id":"210114","messageId":"20130223223700.GB2504@sigill.intra.peff.net","threadId":"32989","inReplyTo":"20130223223134.GA2504@sigill.intra.peff.net","subject":"Re: [PATCH] pkt-line: Fix sparse errors and warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-23T22:37:00Z","receivedAt":"2013-02-23T22:37:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 23, 2013 at 05:31:34PM -0500, Jeff King wrote:\n\n> On Sat, Feb 23, 2013 at 06:44:04PM +0000, Ramsay Jones wrote:\n> \n> > Sparse issues the following error and warnings:\n> > \n> >     pkt-line.c:209:51: warning: Using plain integer as NULL pointer\n> >     sideband.c:41:52: warning: Using plain integer as NULL pointer\n> >     daemon.c:615:39: warning: Using plain integer as NULL pointer\n> >     remote-curl.c:220:75: error: incompatible types for operation (>)\n> >     remote-curl.c:220:75:    left side has type char *\n> >     remote-curl.c:220:75:    right side has type int\n> >     remote-curl.c:291:53: warning: Using plain integer as NULL pointer\n> >     remote-curl.c:408:43: warning: Using plain integer as NULL pointer\n> >     remote-curl.c:562:47: warning: Using plain integer as NULL pointer\n> > \n> > All of these complaints \"blame\" to commit 17243606 (\"pkt-line: share\n> > buffer/descriptor reading implementation\", 20-02-2013).\n> > \n> > In order to suppress the warnings, we simply replace the integer\n> > constant 0 with NULL.\n> [...]\n> Oddly, you seemed to miss the one in connect.c (which my sparse does\n> detect).\n\nAh, I saw why as soon as I finished off the rebase: that (NULL, 0) goes\naway in the very next patch, and you probably ran sparse just on the tip\nof the topic (via pu). I still think it's worth fixing since we are\nsquashing anyway. Junio, it will give you a trivial conflict on patch\n16, but you can just resolve in favor of what patch 16 does. If it's\neasier, here's the revised patch 16:\n\n-- >8 --\nSubject: [PATCH] teach get_remote_heads to read from a memory buffer\n\nNow that we can read packet data from memory as easily as a\ndescriptor, get_remote_heads can take either one as a\nsource. This will allow further refactoring in remote-curl.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fetch-pack.c | 2 +-\n builtin/send-pack.c  | 2 +-\n cache.h              | 4 +++-\n connect.c            | 6 +++---\n remote-curl.c        | 2 +-\n transport.c          | 6 +++---\n 6 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c21cc2c..03ed2ca 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -125,7 +125,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\t   args.verbose ? CONNECT_VERBOSE : 0);\n \t}\n \n-\tget_remote_heads(fd[0], &ref, 0, NULL);\n+\tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL);\n \n \tref = fetch_pack(&args, fd, conn, ref, dest,\n \t\t\t &sought, pack_lockfile_ptr);\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 8778519..152c4ea 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -207,7 +207,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \n \tmemset(&extra_have, 0, sizeof(extra_have));\n \n-\tget_remote_heads(fd[0], &remote_refs, REF_NORMAL, &extra_have);\n+\tget_remote_heads(fd[0], NULL, 0, &remote_refs, REF_NORMAL, &extra_have);\n \n \ttransport_verify_remote_names(nr_refspecs, refspecs);\n \ndiff --git a/cache.h b/cache.h\nindex e493563..db646a2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1049,7 +1049,9 @@ struct extra_have_objects {\n \tint nr, alloc;\n \tunsigned char (*array)[20];\n };\n-extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);\n+extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n+\t\t\t\t     struct ref **list, unsigned int flags,\n+\t\t\t\t     struct extra_have_objects *);\n extern int server_supports(const char *feature);\n extern int parse_feature_request(const char *features, const char *feature);\n extern const char *server_feature_value(const char *feature, int *len_ret);\ndiff --git a/connect.c b/connect.c\nindex 3d99999..f57efd0 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -62,8 +62,8 @@ static void die_initial_contact(int got_at_least_one_head)\n /*\n  * Read all the refs from the other end\n  */\n-struct ref **get_remote_heads(int in, struct ref **list,\n-\t\t\t      unsigned int flags,\n+struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n+\t\t\t      struct ref **list, unsigned int flags,\n \t\t\t      struct extra_have_objects *extra_have)\n {\n \tint got_at_least_one_head = 0;\n@@ -76,7 +76,7 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \t\tint len, name_len;\n \t\tchar *buffer = packet_buffer;\n \n-\t\tlen = packet_read(in, NULL, NULL,\n+\t\tlen = packet_read(in, &src_buf, &src_len,\n \t\t\t\t  packet_buffer, sizeof(packet_buffer),\n \t\t\t\t  PACKET_READ_GENTLE_ON_EOF |\n \t\t\t\t  PACKET_READ_CHOMP_NEWLINE);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex c8379a5..24c8626 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -192,7 +192,7 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n \n \tif (start_async(&async))\n \t\tdie(\"cannot start thread to parse advertised refs\");\n-\tget_remote_heads(async.out, &list,\n+\tget_remote_heads(async.out, NULL, 0, &list,\n \t\t\tfor_push ? REF_NORMAL : 0, NULL);\n \tclose(async.out);\n \tif (finish_async(&async))\ndiff --git a/transport.c b/transport.c\nindex 886ffd8..62df466 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -507,7 +507,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n \tstruct ref *refs;\n \n \tconnect_setup(transport, for_push, 0);\n-\tget_remote_heads(data->fd[0], &refs,\n+\tget_remote_heads(data->fd[0], NULL, 0, &refs,\n \t\t\t for_push ? REF_NORMAL : 0, &data->extra_have);\n \tdata->got_remote_heads = 1;\n \n@@ -541,7 +541,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \tif (!data->got_remote_heads) {\n \t\tconnect_setup(transport, 0, 0);\n-\t\tget_remote_heads(data->fd[0], &refs_tmp, 0, NULL);\n+\t\tget_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0, NULL);\n \t\tdata->got_remote_heads = 1;\n \t}\n \n@@ -799,7 +799,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \t\tstruct ref *tmp_refs;\n \t\tconnect_setup(transport, 1, 0);\n \n-\t\tget_remote_heads(data->fd[0], &tmp_refs, REF_NORMAL, NULL);\n+\t\tget_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL, NULL);\n \t\tdata->got_remote_heads = 1;\n \t}\n \n-- \n1.8.1.4.4.g265d2fa\n"},{"id":"210171","messageId":"7v1uc66rc5.fsf@alter.siamese.dyndns.org","threadId":"32989","inReplyTo":"20130223223700.GB2504@sigill.intra.peff.net","subject":"Re: [PATCH] pkt-line: Fix sparse errors and warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-24T08:36:42Z","receivedAt":"2013-02-24T08:36:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Oddly, you seemed to miss the one in connect.c (which my sparse does\n>> detect).\n>\n> Ah, I saw why as soon as I finished off the rebase: that (NULL, 0) goes\n> away in the very next patch,...\n\nYeah, I noticed that myself while replacing 15.  The patch in the\nmessage I am responding to seems to match byte-for-byte with the\nresult of my rebase, too.\n\nThanks.\n"},{"id":"210349","messageId":"512D0474.7040508@ramsay1.demon.co.uk","threadId":"32989","inReplyTo":"20130223223134.GA2504@sigill.intra.peff.net","subject":"Re: [PATCH] pkt-line: Fix sparse errors and warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-02-26T18:52:36Z","receivedAt":"2013-02-26T18:52:36Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jeff King wrote:\n> Thanks for the report. Clearly I should start running \"make sparse\" more\n> often (the reason I don't is that it produces dozens of complaints about\n> constants in /usr/include/bits/xopen_lim.h, which I could obviously care\n> less about; I should look into suppressing that).\n\nHmm, I'm guessing you are on an 64bit system; if so, and you are running\nthe release version of sparse (v0.4.4 Nov 2011), I would clone the sparse\nrepo [1] and build from source. There have been many improvements to the\n64bit support since the last release (I hear it was unusable). [Note: I\ncan't confirm that - I'm currently stuck on 32bits!]\n\nATB,\nRamsay Jones\n\n[1] git://git.kernel.org/pub/scm/devel/sparse/sparse.git (which had been\ndormant for quite some time, but sprang to life again last week).\n"},{"id":"210348","messageId":"512D06E3.5020109@ramsay1.demon.co.uk","threadId":"32989","inReplyTo":"20130223223700.GB2504@sigill.intra.peff.net","subject":"Re: [PATCH] pkt-line: Fix sparse errors and warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-02-26T19:02:59Z","receivedAt":"2013-02-26T19:02:59Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jeff King wrote:\n> On Sat, Feb 23, 2013 at 05:31:34PM -0500, Jeff King wrote:\n> \n>> On Sat, Feb 23, 2013 at 06:44:04PM +0000, Ramsay Jones wrote:\n>>\n>>> Sparse issues the following error and warnings:\n>>>\n>>>     pkt-line.c:209:51: warning: Using plain integer as NULL pointer\n>>>     sideband.c:41:52: warning: Using plain integer as NULL pointer\n>>>     daemon.c:615:39: warning: Using plain integer as NULL pointer\n>>>     remote-curl.c:220:75: error: incompatible types for operation (>)\n>>>     remote-curl.c:220:75:    left side has type char *\n>>>     remote-curl.c:220:75:    right side has type int\n>>>     remote-curl.c:291:53: warning: Using plain integer as NULL pointer\n>>>     remote-curl.c:408:43: warning: Using plain integer as NULL pointer\n>>>     remote-curl.c:562:47: warning: Using plain integer as NULL pointer\n>>>\n>>> All of these complaints \"blame\" to commit 17243606 (\"pkt-line: share\n>>> buffer/descriptor reading implementation\", 20-02-2013).\n>>>\n>>> In order to suppress the warnings, we simply replace the integer\n>>> constant 0 with NULL.\n>> [...]\n>> Oddly, you seemed to miss the one in connect.c (which my sparse does\n>> detect).\n> \n> Ah, I saw why as soon as I finished off the rebase: that (NULL, 0) goes\n> away in the very next patch, and you probably ran sparse just on the tip\n> of the topic (via pu). \n\nYes, sorry I should have mentioned that! Ahem, *blush* [Having created and\ntested the patch (including running \"make test\") on the tip of pu, I applied\nthe patch directly to commit 17243606 (using git-am). Since it applied\ncleanly and git-show looked OK, I said to myself \"yep, that's OK, send it\" ;-) ]\n\n>                         I still think it's worth fixing since we are\n> squashing anyway. Junio, it will give you a trivial conflict on patch\n> 16, but you can just resolve in favor of what patch 16 does. If it's\n> easier, here's the revised patch 16:\n\nSo, sorry for being a bit sloppy guys!\n\nATB,\nRamsay Jones\n"}]}