Re: [PATCH v4 01/44] wrapper: implement xopen()

["Torsten Bögershausen" <tboegi@web.de>]

On 06/28/2015 04:05 PM, Paul Tan wrote:
> A common usage pattern of open() is to check if it was successful, and
> die() if it was not:
>
> 	int fd = open(path, O_WRONLY | O_CREAT, 0777);
> 	if (fd < 0)
> 		die_errno(_("Could not open '%s' for writing."), path);
>
> Implement a wrapper function xopen() that does the above so that we can
> save a few lines of code, and make the die() messages consistent.
>
> Helped-by: Torsten Bögershausen <tboegi@web.de>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>   git-compat-util.h |  1 +
>   wrapper.c         | 25 +++++++++++++++++++++++++
>   2 files changed, 26 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index c6d391f..e168dfd 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -717,6 +717,7 @@ extern void *xrealloc(void *ptr, size_t size);
>   extern void *xcalloc(size_t nmemb, size_t size);
>   extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
>   extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);
> +extern int xopen(const char *path, int flags, ...);
>   extern ssize_t xread(int fd, void *buf, size_t len);
>   extern ssize_t xwrite(int fd, const void *buf, size_t len);
>   extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
> diff --git a/wrapper.c b/wrapper.c
> index ff49807..7e13ae0 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -189,6 +189,31 @@ void *xcalloc(size_t nmemb, size_t size)
>   # endif
>   #endif
>   
> +/**
> + * xopen() is the same as open(), but it die()s if the open() fails.
> + */
> +int xopen(const char *path, int oflag, ...)
> +{
> +	mode_t mode = 0;
> +	va_list ap;
> +
> +	va_start(ap, oflag);
> +	if (oflag & O_CREAT)
> +		mode = va_arg(ap, mode_t);
> +	va_end(ap);
> +
> +	assert(path);
> +
2 remarks:
- I don't know if and why we need the assert() here (but don't know if 
we have a strategie in Git for assert())
- Having xopen() with 2 or 3 parameter is good, but the current may need 
some tweaks for better portability:

int xopen(const char *path, int oflag, ...)
{
	mode_t mode = 0;
	if (oflag & O_CREAT) {
		va_list ap;
		va_start(ap, oflag);
		mode = va_arg(ap, int);
		va_end(ap);

             }

See e.g.
<http://blitiri.com.ar/git/r/libfiu/c/37f6a98110e3bb59bbb4971241baa3a385c3f724/>
why va_arg(ap, int) should be used:


+
+		/* va_arg() can only take fully promoted types, and mode_t
+		 * sometimes is smaller than an int, so we should always pass
+		 * int to it, and not mode_t. Not doing so would may result in
+		 * a compile-time warning and run-time error. We asume that it
+		 * is never bigger than an int, which holds in practise. */
+		mode = va_arg(l, int);
+