[Patch v1 0/3] Replace strbuf_write_fd with write_in_full

[Patch v1 0/3] Replace strbuf_write_fd with write_in_full

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

The strbuf_write_fd method does not check whether the buffer exceeds
MAX_IO_SIZE on the target platform. This fix replaces the use of that
method with write_in_full, which does. Since this is the only use of
strbuf_write_fd, and since the method was unsafe, it has been removed
from strbuf.c and strbuf.h.

Randall S. Becker (3):
  bugreport.c: replace strbuf_write_fd with write_in_full
  strbuf.c: remove unreferenced strbuf_write_fd method.
  strbuf.h: remove declaration of deprecated strbuf_write_fd method.

 bugreport.c | 5 ++++-
 strbuf.c    | 5 -----
 strbuf.h    | 1 -
 3 files changed, 4 insertions(+), 7 deletions(-)

-- 
2.21.0

[Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

The strbuf_write_fd method did not provide checks for buffers larger
than MAX_IO_SIZE. Replacing with write_in_full ensures the entire
buffer will always be written to disk or report an error and die.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 bugreport.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/bugreport.c b/bugreport.c
index aa8a489c35..bc359b7fa8 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)
 		die(_("couldn't create a new file at '%s'"), report_path.buf);
 	}
 
-	strbuf_write_fd(&buffer, report);
+	if (write_in_full(report, buffer.buf, buffer.len) < 0) {
+		die(_("couldn't write report contents '%s' to file '%s'"),
+			buffer.buf, report_path.buf);
+	}
 	close(report);
 
 	/*
-- 
2.21.0

Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full

[Đoàn Trần Công Danh]

On 2020-06-19 11:04:43-0400, randall.s.becker@rogers.com wrote:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> The strbuf_write_fd method did not provide checks for buffers larger
> than MAX_IO_SIZE. Replacing with write_in_full ensures the entire
> buffer will always be written to disk or report an error and die.
> 
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  bugreport.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/bugreport.c b/bugreport.c
> index aa8a489c35..bc359b7fa8 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)
>  		die(_("couldn't create a new file at '%s'"), report_path.buf);
>  	}
>  
> -	strbuf_write_fd(&buffer, report);
> +	if (write_in_full(report, buffer.buf, buffer.len) < 0) {
> +		die(_("couldn't write report contents '%s' to file '%s'"),
> +			buffer.buf, report_path.buf);

Doesn't this dump the whole report to the stderr?
If it's the case, the error would be very hard to grasp.

Nit: We wouldn't want the pair of {}.

> +	}
>  	close(report);
>  
>  	/*
> -- 
> 2.21.0
> 

-- 
Danh

RE: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full

[Randall S. Becker]

On June 19, 2020 12:36 PM, Đoàn Trần Công Danh wrote:
> On 2020-06-19 11:04:43-0400, randall.s.becker@rogers.com wrote:
> > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> >
> > The strbuf_write_fd method did not provide checks for buffers larger
> > than MAX_IO_SIZE. Replacing with write_in_full ensures the entire
> > buffer will always be written to disk or report an error and die.
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> >  bugreport.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/bugreport.c b/bugreport.c index aa8a489c35..bc359b7fa8
> > 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)
> >  		die(_("couldn't create a new file at '%s'"), report_path.buf);
> >  	}
> >
> > -	strbuf_write_fd(&buffer, report);
> > +	if (write_in_full(report, buffer.buf, buffer.len) < 0) {
> > +		die(_("couldn't write report contents '%s' to file '%s'"),
> > +			buffer.buf, report_path.buf);
> 
> Doesn't this dump the whole report to the stderr?
> If it's the case, the error would be very hard to grasp.

Where else can we put the error? By this point, we're likely out of disk or virtual memory.

> Nit: We wouldn't want the pair of {}.
> 
> > +	}
> >  	close(report);

I'm not sure what you mean in this nit? {} are balanced. You mean in the error message?

Randall

Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full

[Junio C Hamano]

"Randall S. Becker" <rsbecker@nexbridge.com> writes:

>> > +	if (write_in_full(report, buffer.buf, buffer.len) < 0) {
>> > +		die(_("couldn't write report contents '%s' to file '%s'"),
>> > +			buffer.buf, report_path.buf);
>> 
>> Doesn't this dump the whole report to the stderr?
>> If it's the case, the error would be very hard to grasp.
>
> Where else can we put the error? By this point, we're likely out of disk or virtual memory.
>
>> Nit: We wouldn't want the pair of {}.
>> 
>> > +	}
>> >  	close(report);
>
> I'm not sure what you mean in this nit? {} are balanced. You mean in the error message?

I think he means that a block that consists of a single statement
(i.e. call to die()) does not have to be enclosed in {}.

(partial quote from Documentation/CodingGuidelines):

 - We avoid using braces unnecessarily.  I.e.

	if (bla) {
		x = 1;
	}

   is frowned upon. But there are a few exceptions:

RE: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full

[Randall S. Becker]

On June 19, 2020 3:31 PM, Junio C Hamano wrote:
> To: Randall S. Becker <rsbecker@nexbridge.com>
> Cc: 'Đoàn Trần Công Danh' <congdanhqx@gmail.com>;
> randall.s.becker@rogers.com; git@vger.kernel.org
> Subject: Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with
> write_in_full
> 
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
> >> > +	if (write_in_full(report, buffer.buf, buffer.len) < 0) {
> >> > +		die(_("couldn't write report contents '%s' to file '%s'"),
> >> > +			buffer.buf, report_path.buf);
> >>
> >> Doesn't this dump the whole report to the stderr?
> >> If it's the case, the error would be very hard to grasp.
> >
> > Where else can we put the error? By this point, we're likely out of disk or
> virtual memory.
> >
> >> Nit: We wouldn't want the pair of {}.
> >>
> >> > +	}
> >> >  	close(report);
> >
> > I'm not sure what you mean in this nit? {} are balanced. You mean in the
> error message?
> 
> I think he means that a block that consists of a single statement (i.e. call to
> die()) does not have to be enclosed in {}.
> 
> (partial quote from Documentation/CodingGuidelines):
> 
>  - We avoid using braces unnecessarily.  I.e.
> 
> 	if (bla) {
> 		x = 1;
> 	}
> 
>    is frowned upon. But there are a few exceptions:

I get that. I was trying to maintain visual consistency with the rest of bugreport.c. Will redo it.

Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full

[Đoàn Trần Công Danh]

On 2020-06-19 13:17:19-0400, "Randall S. Becker" <rsbecker@nexbridge.com> wrote:
> On June 19, 2020 12:36 PM, Đoàn Trần Công Danh wrote:
> > On 2020-06-19 11:04:43-0400, randall.s.becker@rogers.com wrote:
> > > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> > >
> > > The strbuf_write_fd method did not provide checks for buffers larger
> > > than MAX_IO_SIZE. Replacing with write_in_full ensures the entire
> > > buffer will always be written to disk or report an error and die.
> > >
> > > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > > ---
> > >  bugreport.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/bugreport.c b/bugreport.c index aa8a489c35..bc359b7fa8
> > > 100644
> > > --- a/bugreport.c
> > > +++ b/bugreport.c
> > > @@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)
> > >  		die(_("couldn't create a new file at '%s'"), report_path.buf);
> > >  	}
> > >
> > > -	strbuf_write_fd(&buffer, report);
> > > +	if (write_in_full(report, buffer.buf, buffer.len) < 0) {
> > > +		die(_("couldn't write report contents '%s' to file '%s'"),
> > > +			buffer.buf, report_path.buf);
> > 
> > Doesn't this dump the whole report to the stderr?
> > If it's the case, the error would be very hard to grasp.
> 
> Where else can we put the error? By this point, we're likely out of
> disk or virtual memory.

Sorry, I forgot to suggest an alternatives.

I was thinking about ignore the report when writing the last email.

Since, the report is likely consists of multiple lines of text,
and they likely contains some single quote themselves.

Now, I think a bit more, I think it's way better to write as:

	if (write_in_full(report, buffer.buf, buffer.len) < 0)
		die(_("couldn't write the report contents to file '%s'.\n\n"
		"The original report was:\n\n"
		"%s\n"), report_path.buf, buffer.buf);

> > Nit: We wouldn't want the pair of {}.
> > 
> > > +	}
> > >  	close(report);
> 
> I'm not sure what you mean in this nit? {} are balanced. You mean in the error message?

Our style guides says we wouldn't want this pair of {} if it's single
statement.


-- 
Danh

RE: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full

[Randall S. Becker]

On June 19, 2020 7:02 PM, Ðoàn Tr?n Công Danh wrote:
> To: Randall S. Becker <rsbecker@nexbridge.com>
> Cc: randall.s.becker@rogers.com; git@vger.kernel.org
> Subject: Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with
> write_in_full
> 
> On 2020-06-19 13:17:19-0400, "Randall S. Becker"
> <rsbecker@nexbridge.com> wrote:
> > On June 19, 2020 12:36 PM, Đoàn Trần Công Danh wrote:
> > > On 2020-06-19 11:04:43-0400, randall.s.becker@rogers.com wrote:
> > > > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> > > >
> > > > The strbuf_write_fd method did not provide checks for buffers
> > > > larger than MAX_IO_SIZE. Replacing with write_in_full ensures the
> > > > entire buffer will always be written to disk or report an error and die.
> > > >
> > > > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > > > ---
> > > >  bugreport.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/bugreport.c b/bugreport.c index
> > > > aa8a489c35..bc359b7fa8
> > > > 100644
> > > > --- a/bugreport.c
> > > > +++ b/bugreport.c
> > > > @@ -174,7 +174,10 @@ int cmd_main(int argc, const char **argv)
> > > >  		die(_("couldn't create a new file at '%s'"), report_path.buf);
> > > >  	}
> > > >
> > > > -	strbuf_write_fd(&buffer, report);
> > > > +	if (write_in_full(report, buffer.buf, buffer.len) < 0) {
> > > > +		die(_("couldn't write report contents '%s' to file '%s'"),
> > > > +			buffer.buf, report_path.buf);
> > >
> > > Doesn't this dump the whole report to the stderr?
> > > If it's the case, the error would be very hard to grasp.
> >
> > Where else can we put the error? By this point, we're likely out of
> > disk or virtual memory.
> 
> Sorry, I forgot to suggest an alternatives.
> 
> I was thinking about ignore the report when writing the last email.
> 
> Since, the report is likely consists of multiple lines of text, and they likely
> contains some single quote themselves.
> 
> Now, I think a bit more, I think it's way better to write as:
> 
> 	if (write_in_full(report, buffer.buf, buffer.len) < 0)
> 		die(_("couldn't write the report contents to file '%s'.\n\n"
> 		"The original report was:\n\n"
> 		"%s\n"), report_path.buf, buffer.buf);

I went with Peff's suggestion of using die_error in v2. Thanks though.

> > > Nit: We wouldn't want the pair of {}.
> > >
> > > > +	}
> > > >  	close(report);
> >
> > I'm not sure what you mean in this nit? {} are balanced. You mean in the
> error message?
> 
> Our style guides says we wouldn't want this pair of {} if it's single statement.

Fixed in v2

Regards,
Randall

Re: [Patch v1 1/3] bugreport.c: replace strbuf_write_fd with write_in_full

[Jeff King]

On Fri, Jun 19, 2020 at 11:04:43AM -0400, randall.s.becker@rogers.com wrote:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> The strbuf_write_fd method did not provide checks for buffers larger
> than MAX_IO_SIZE. Replacing with write_in_full ensures the entire
> buffer will always be written to disk or report an error and die.

This also fixes problems with EINTR, etc.

> -	strbuf_write_fd(&buffer, report);
> +	if (write_in_full(report, buffer.buf, buffer.len) < 0) {
> +		die(_("couldn't write report contents '%s' to file '%s'"),
> +			buffer.buf, report_path.buf);
> +	}

I agree with the other comment not to bother reporting the contents. But
it is worth using die_errno() so we can see what happened. I.e.:

  die_errno(_("unable to write to %s"), report_path.buf);

would match our usual messages, and you'd get:

  unable to write to foo.out: No space left on device

or similar.

-Peff

[Patch v1 2/3] strbuf.c: remove unreferenced strbuf_write_fd method.

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

strbuf_write_fd was only used in bugreport.c. Since that file now uses
write_in_full, this method is no longer needed.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 strbuf.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 2f1a7d3209..e3397cc4c7 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -556,11 +556,6 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
 	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
 }
 
-ssize_t strbuf_write_fd(struct strbuf *sb, int fd)
-{
-	return sb->len ? write(fd, sb->buf, sb->len) : 0;
-}
-
 #define STRBUF_MAXLINK (2*PATH_MAX)
 
 int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
-- 
2.21.0

Re: [Patch v1 2/3] strbuf.c: remove unreferenced strbuf_write_fd method.

[Junio C Hamano]

randall.s.becker@rogers.com writes:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> strbuf_write_fd was only used in bugreport.c. Since that file now uses
> write_in_full, this method is no longer needed.

Yay!


> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  strbuf.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index 2f1a7d3209..e3397cc4c7 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -556,11 +556,6 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
>  	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
>  }
>  
> -ssize_t strbuf_write_fd(struct strbuf *sb, int fd)
> -{
> -	return sb->len ? write(fd, sb->buf, sb->len) : 0;
> -}
> -
>  #define STRBUF_MAXLINK (2*PATH_MAX)
>  
>  int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)

Re: [Patch v1 2/3] strbuf.c: remove unreferenced strbuf_write_fd method.

[Jeff King]

On Fri, Jun 19, 2020 at 11:04:44AM -0400, randall.s.becker@rogers.com wrote:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> strbuf_write_fd was only used in bugreport.c. Since that file now uses
> write_in_full, this method is no longer needed.

Just because there are no callers _now_ does not necessarily mean that
it's a good idea to get rid of a function. I think the real argument is
that we don't expect new ones, because it's a badly designed function
(for the reasons in our earlier discussion). Maybe worth summarizing
here.

-Peff

[Patch v1 3/3] strbuf.h: remove declaration of deprecated strbuf_write_fd method.

[randall.s.becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 strbuf.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 7062eb6410..223ee2094a 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -473,7 +473,6 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
  * NUL bytes.
  */
 ssize_t strbuf_write(struct strbuf *sb, FILE *stream);
-ssize_t strbuf_write_fd(struct strbuf *sb, int fd);
 
 /**
  * Read a line from a FILE *, overwriting the existing contents of
-- 
2.21.0

Re: [Patch v1 3/3] strbuf.h: remove declaration of deprecated strbuf_write_fd method.

[Junio C Hamano]

randall.s.becker@rogers.com writes:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  strbuf.h | 1 -
>  1 file changed, 1 deletion(-)

I think this should be part of 2/3 (otherwise we'd have a decl that
nobody references that declares a function that nobody implements).

> diff --git a/strbuf.h b/strbuf.h
> index 7062eb6410..223ee2094a 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -473,7 +473,6 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
>   * NUL bytes.
>   */
>  ssize_t strbuf_write(struct strbuf *sb, FILE *stream);
> -ssize_t strbuf_write_fd(struct strbuf *sb, int fd);
>  
>  /**
>   * Read a line from a FILE *, overwriting the existing contents of

RE: [Patch v1 3/3] strbuf.h: remove declaration of deprecated strbuf_write_fd method.

[Randall S. Becker]

On June 19, 2020 3:32 PM, Junio C Hamano wrote:
> randall.s.becker@rogers.com writes:
> 
> > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> >  strbuf.h | 1 -
> >  1 file changed, 1 deletion(-)
> 
> I think this should be part of 2/3 (otherwise we'd have a decl that nobody
> references that declares a function that nobody implements).

If I understand, combined the strbuf.c and strbuf.h modification into a
single commit, correct? I normally would do that but missed this part of the
contribution standard. If so, I will create v2 accordingly.

> 
> > diff --git a/strbuf.h b/strbuf.h
> > index 7062eb6410..223ee2094a 100644
> > --- a/strbuf.h
> > +++ b/strbuf.h
> > @@ -473,7 +473,6 @@ int strbuf_readlink(struct strbuf *sb, const char
> *path, size_t hint);
> >   * NUL bytes.
> >   */
> >  ssize_t strbuf_write(struct strbuf *sb, FILE *stream); -ssize_t
> > strbuf_write_fd(struct strbuf *sb, int fd);
> >
> >  /**
> >   * Read a line from a FILE *, overwriting the existing contents of