Re: [PATCH] implemented strbuf_write_or_die()

[Johannes Sixt <j6t@kdbg.org>]

Am 01.03.2014 12:21, schrieb Faiz Kothari:
> Signed-off-by: Faiz Kothari <faiz.off93@gmail.com>
> ---
> Implemented write_or_die.c:strbuf_write_or_die() and used in relevant places
> to substitute write_or_die(). I spotted other places where strbuf can be used
> in place of buf[MAX_PATH] but that would require a change in prototype of a 
> lot of functions and functions calling them and so on....
> I'll look for more places where strbuf can be used safely.

You haven't given a justifiction of the change (why is this change good?)

> diff --git a/write_or_die.c b/write_or_die.c
> index b50f99a..5fb309b 100644
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -1,4 +1,5 @@
>  #include "cache.h"
> +#include "strbuf.h"

I think you have the layering backwards here: strbuf_write_or_die should
be part of the (higher-level) strbuf API, and not an extension of the
low-level write_or_die function.

>  
>  static void check_pipe(int err)
>  {
> @@ -64,6 +65,14 @@ void write_or_die(int fd, const void *buf, size_t count)
>  	}
>  }
>  
> +void strbuf_write_or_die(int fd, const struct strbuf *sbuf)

And when you make the function a strbuf API, the prototype should be

void strbuf_write_or_die(const struct strbuf *sbuf, int fd)

as in "hey, strbuf object, write your content to this file descriptor!"

> +{
> +	if(write_in_full(fd, sbuf->buf, sbuf->len) < 0){
> +		check_pipe(errno);
> +		die_errno("write error");
> +	}
> +}
> +
>  int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
>  {
>  	if (write_in_full(fd, buf, count) < 0) {
> 

-- Hannes