[PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern

[PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As explained in commit 06f46f237 (avoid "write_in_full(fd, buf, len)
!= len" pattern, 2017–09–13) the return value of write_in_full() is
either -1 or the requested number of bytes. As such comparing the
return value to an unsigned value such as strbuf.len will fail to
catch errors. Change the code to use the preferred '< 0' check.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 903abf9533b188fd472c213c29a9f968eb90eb8b..d377161113009f394f118d81d27fa6117cde8e9f 100644
--- a/config.c
+++ b/config.c
@@ -2810,7 +2810,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
 			 * multiple [branch "$name"] sections.
 			 */
 			if (copystr.len > 0) {
-				if (write_in_full(out_fd, copystr.buf, copystr.len) != copystr.len) {
+				if (write_in_full(out_fd, copystr.buf, copystr.len) < 0) {
 					ret = write_error(get_lock_file_path(&lock));
 					goto out;
 				}
@@ -2872,7 +2872,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
 	 * logic in the loop above.
 	 */
 	if (copystr.len > 0) {
-		if (write_in_full(out_fd, copystr.buf, copystr.len) != copystr.len) {
+		if (write_in_full(out_fd, copystr.buf, copystr.len) < 0) {
 			ret = write_error(get_lock_file_path(&lock));
 			goto out;
 		}
-- 
2.15.0

Re: [PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> As explained in commit 06f46f237 (avoid "write_in_full(fd, buf, len)
> != len" pattern, 2017–09–13) the return value of write_in_full() is
> either -1 or the requested number of bytes. As such comparing the
> return value to an unsigned value such as strbuf.len will fail to
> catch errors. Change the code to use the preferred '< 0' check.

Thanks, queued.  This seems to have come from 9a5abfc7 ("After
renaming a section, print any trailing variable definitions",
2009-07-24), which is rather ancient, but was made worse by getting
duplicated by 52d59cc6 ("branch: add a --copy (-c) option to go with
--move (-m)", 2017-06-18) recently.

Re: [PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern

[Jeff King]

On Wed, Nov 15, 2017 at 12:40:43PM +0000, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> As explained in commit 06f46f237 (avoid "write_in_full(fd, buf, len)
> != len" pattern, 2017–09–13) the return value of write_in_full() is
> either -1 or the requested number of bytes. As such comparing the
> return value to an unsigned value such as strbuf.len will fail to
> catch errors. Change the code to use the preferred '< 0' check.

Thanks for catching this. I wondered at first how I missed these obvious
cases, but the answer is that they were added after my commit. :)

There's one more case in write_section() that uses "==". That's not
actually wrong, but I wonder if we'd want to make it "< 0" for
consistency.

-Peff

Re: [PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern

[René Scharfe]

Am 17.11.2017 um 23:06 schrieb Jeff King:
> There's one more case in write_section() that uses "==". That's not
> actually wrong, but I wonder if we'd want to make it "< 0" for
> consistency.

Actually it *is* wrong.

-- >8 --
Subject: [PATCH] config: flip return value of write_section()

d9bd4cbb9cc (config: flip return value of store_write_*()) made
write_section() follow the convention of write(2) to return -1 on error
and the number of written bytes on success.  3b48045c6c7 (Merge branch
'sd/branch-copy') changed it back to returning 0 on error and 1 on
success, but left its callers still checking for negative values.

Let write_section() follow the convention of write(2) again to meet the
expectations of its callers.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 903abf9533..3f079c77ad 100644
--- a/config.c
+++ b/config.c
@@ -2315,7 +2315,7 @@ static ssize_t write_section(int fd, const char *key)
 	struct strbuf sb = store_create_section(key);
 	ssize_t ret;
 
-	ret = write_in_full(fd, sb.buf, sb.len) == sb.len;
+	ret = write_in_full(fd, sb.buf, sb.len);
 	strbuf_release(&sb);
 
 	return ret;
-- 
2.15.0

Re: [PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern

[Phillip Wood]

On 17/11/17 22:06, Jeff King wrote:
> On Wed, Nov 15, 2017 at 12:40:43PM +0000, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> As explained in commit 06f46f237 (avoid "write_in_full(fd, buf, len)
>> != len" pattern, 2017–09–13) the return value of write_in_full() is
>> either -1 or the requested number of bytes. As such comparing the
>> return value to an unsigned value such as strbuf.len will fail to
>> catch errors. Change the code to use the preferred '< 0' check.
> 
> Thanks for catching this. I wondered at first how I missed these obvious
> cases, but the answer is that they were added after my commit. :)
> 
> There's one more case in write_section() that uses "==". That's not
> actually wrong, but I wonder if we'd want to make it "< 0" for
> consistency.

Yes, I noticed that but didn't get round to looking at it properly the
other day. Rene's fix looks good to me.

Best Wishes

Phillip

Re: [PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern

[Jeff King]

On Sat, Nov 18, 2017 at 11:20:04AM +0100, René Scharfe wrote:

> Am 17.11.2017 um 23:06 schrieb Jeff King:
> > There's one more case in write_section() that uses "==". That's not
> > actually wrong, but I wonder if we'd want to make it "< 0" for
> > consistency.
> 
> Actually it *is* wrong.

Thanks for digging, I didn't look beyond that single line.

> -- >8 --
> Subject: [PATCH] config: flip return value of write_section()
> 
> d9bd4cbb9cc (config: flip return value of store_write_*()) made
> write_section() follow the convention of write(2) to return -1 on error
> and the number of written bytes on success.  3b48045c6c7 (Merge branch
> 'sd/branch-copy') changed it back to returning 0 on error and 1 on
> success, but left its callers still checking for negative values.
> 
> Let write_section() follow the convention of write(2) again to meet the
> expectations of its callers.

Yikes. It looks like this slipped by on the tests because we always
check "< 0" in the callers, not non-zero. So success would not look like
failure, but failure would look like success. And write failure does not
happen regularly in the test suite.

So this looks correct, and well-explained.

> Reported-by: Jeff King <peff@peff.net>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>

I'm not sure I deserve a reported-by if I say "it looks fine" but am
totally wrong. ;)

-Peff

Re: [PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern

[René Scharfe]

Am 18.11.2017 um 18:52 schrieb Jeff King:
> On Sat, Nov 18, 2017 at 11:20:04AM +0100, René Scharfe wrote:
>> Reported-by: Jeff King <peff@peff.net>
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> 
> I'm not sure I deserve a reported-by if I say "it looks fine" but am
> totally wrong. ;)

Right, wrong -- mere details.  You pointed out that there was work
to do. :)

René