{"thread":{"id":"9061","subject":"Changes in function read_pipe","startedAt":"2007-07-16T16:13:45Z","lastAt":"2007-07-16T23:50:58Z","messageCount":2,"participants":["Carlos Rica","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"47553","messageId":"469B9939.5050800@gmail.com","threadId":"9061","inReplyTo":null,"subject":"Changes in function read_pipe","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-07-16T16:13:45Z","receivedAt":"2007-07-16T16:13:45Z","isPatch":false,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"Some people talked recently about renaming the function read_pipe\n(sha1_file.c) to the better name read_fd.\nHere I discuss other possible changes to the current version:\n\n1. It now requires to allocate memory for the buffer before calling it,\nyou cannot pass it a pointer set to NULL or not initializated at all.\n\n2. The function doesn't terminate the data with NUL, and if you\nneed that, you must to realloc before adding the '\\0', because\nthe function only returns the size of the data, not the buffer size.\n\n3. When function fails in reading (xread returns < 0), buffer is not\nfreed.\n\nI'm not sure which of those issues are really important,\nso I thought it was better to ask this in the list.\nThis is the current implementation of the function:\n\n/*\n * reads from fd as long as possible into a supplied buffer of size bytes.\n * If necessary the buffer's size is increased using realloc()\n *\n * returns 0 if anything went fine and -1 otherwise\n *\n * NOTE: both buf and size may change, but even when -1 is returned\n * you still have to free() it yourself.\n */\nint read_pipe(int fd, char** return_buf, unsigned long* return_size)\n{\n\tchar* buf = *return_buf;\n\tunsigned long size = *return_size;\n\tssize_t iret;\n\tunsigned long off = 0;\n\n\tdo {\n\t\tiret = xread(fd, buf + off, size - off);\n\t\tif (iret > 0) {\n\t\t\toff += iret;\n\t\t\tif (off == size) {\n\t\t\t\tsize *= 2;\n\t\t\t\tbuf = xrealloc(buf, size);\n\t\t\t}\n\t\t}\n\t} while (iret > 0);\n\n\t*return_buf = buf;\n\t*return_size = off;\n\n\tif (iret < 0)\n\t\treturn -1;\n\treturn 0;\n}\n\nKristian Høgsberg recently sent some changes to the function\nto replace the function with another one easier to call, but\nwith a different behavior:\n\n /*\n- * reads from fd as long as possible into a supplied buffer of size bytes.\n- * If necessary the buffer's size is increased using realloc()\n+ * reads from fd as long as possible and allocates a buffer to hold\n+ * the contents.  The buffer and size of the contents is returned in\n+ * *return_buf and *return_size.  In case of failure, the allocated\n+ * buffers are freed, otherwise, the buffer must be freed using xfree.\n  *\n  * returns 0 if anything went fine and -1 otherwise\n- *\n- * NOTE: both buf and size may change, but even when -1 is returned\n- * you still have to free() it yourself.\n  */\n-int read_pipe(int fd, char** return_buf, unsigned long* return_size)\n+int read_fd(int fd, char** return_buf, unsigned long* return_size)\n {\n-\tchar* buf = *return_buf;\n-\tunsigned long size = *return_size;\n+\tunsigned long size = 4096;\n+\tchar* buf = xmalloc(size);\n \tssize_t iret;\n \tunsigned long off = 0;\n\n@@ -2328,21 +2327,22 @@ int read_pipe(int fd, char** return_buf, unsigned long* return_size)\n \t*return_buf = buf;\n \t*return_size = off;\n\n-\tif (iret < 0)\n+\tif (iret < 0) {\n+\t\tfree(buf);\n \t\treturn -1;\n+\t}\n+\n \treturn 0;\n }\n\nAny other ideas? read_pipe is really handy to reuse on builtins,\nso we need to decide how we should call to it now before starting\nto reuse it in many places.\n\nComments will be appreciated.\n"},{"id":"47605","messageId":"7vfy3nc3h9.fsf@assigned-by-dhcp.cox.net","threadId":"9061","inReplyTo":"469B9939.5050800@gmail.com","subject":"Re: Changes in function read_pipe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-16T23:50:58Z","receivedAt":"2007-07-16T23:50:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Rica <jasampler@gmail.com> writes:\n\n> Some people talked recently about renaming the function read_pipe\n> (sha1_file.c) to the better name read_fd.\n> Here I discuss other possible changes to the current version:\n>\n> 1. It now requires to allocate memory for the buffer before calling it,\n> you cannot pass it a pointer set to NULL or not initializated at all.\n\nIt should be a trivial and easy fix to add \"initial allocation\"\nat the beginning of the function.  While I suspect that the\noptimal initial allocation would be different for different\ncallers, probably we would not care that much.\n\n> 2. The function doesn't terminate the data with NUL, and if you\n> need that, you must to realloc before adding the '\\0', because\n> the function only returns the size of the data, not the buffer size.\n\nAmong the existing callers, index_fd() does not need NUL\ntermination, but we are almost always allocating more than\nneeded when the final size is unknown, so giving an extra NUL\n(of course not including that in the final result size) would\nnot hurt.  Other callers are 'stripspace' and 'mktag', and I\nwould imagine 'tag' and 'commit' when they are reading the\nmessage from the standard input would benefit from the implicit\nNUL termination.  I'd say go for it.\n\n> 3. When function fails in reading (xread returns < 0), buffer is not\n> freed.\n\nI do not think this is a problem.  The current callers seem to\ndo something like this:\n\n\tif (read_pipe(fd, ...)) {\n        \tfree(buf);\n                return error(...);\n\t}\n\nbut not freeing, and reporting how many you read so far, would\nallow readers to work on results partially read (not to be\nconfused with the EAGAIN which is already handled with xread()).\n"}]}