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

On Sun, Jun 28, 2015 at 9:48 PM, Torsten Bögershausen [off-list ref] wrote:

On 06/28/2015 04:05 PM, Paul Tan wrote:

quoted

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 [off-list ref]
Helped-by: Jeff King [off-list ref]
Signed-off-by: Paul Tan <redacted>
---
  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())

Yeah I think we usually do

    if (!assertion)
        die("BUG: assertion not met");

- 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);
+