Patchwork [13,of,14] chg: extra the logic opening current directory to opencwdx()

login
register
mail settings
Submitter Jun Wu
Date April 10, 2016, 11:57 p.m.
Message ID <58ea17bb6caad0e680f2.1460332650@x1c>
Download mbox | patch
Permalink /patch/14510/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - April 10, 2016, 11:57 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1460332228 -3600
#      Mon Apr 11 00:50:28 2016 +0100
# Node ID 58ea17bb6caad0e680f23473b37f68c03c546146
# Parent  a4651e43ceb70c27db15ebe43950e9a788192096
chg: extra the logic opening current directory to opencwdx()

We have 2 places opening current directory and will have another one. Move
the logic to a standalone utility function.
timeless - April 11, 2016, 3:42 a.m.
Jun Wu wrote:
> We have 2 places opening current directory and will have another one.

fwiw, I'd write out "two"
Yuya Nishihara - April 13, 2016, 3:28 p.m.
On Mon, 11 Apr 2016 00:57:30 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1460332228 -3600
> #      Mon Apr 11 00:50:28 2016 +0100
> # Node ID 58ea17bb6caad0e680f23473b37f68c03c546146
> # Parent  a4651e43ceb70c27db15ebe43950e9a788192096
> chg: extra the logic opening current directory to opencwdx()

Looks good, please resend with V2.

>  void fchdirx(int dirfd);
>  void fsetcloexec(int fd);
> +int opencwdx();
>  void *mallocx(size_t size);
>  void *reallocx(void *ptr, size_t size);

OT: Maybe we can sort out functions that causes hard failure. I mean
s/fsetcloexec/fsetcloexecx/.
Jun Wu - April 13, 2016, 4:55 p.m.
On 04/13/2016 04:28 PM, Yuya Nishihara wrote:
> OT: Maybe we can sort out functions that causes hard failure. I mean
> s/fsetcloexec/fsetcloexecx/.

Thanks for reviewing!

I'm a bit busy with internal packing (currently handing mac os x) and I'm
spending some time improving internal tools related to release.

Do you think it's okay to have a marco for all "*x", or "*at" stuff? It will
make the code shorter. Although I'm not a big fan of macros, I think current
code duplication is a bit too much and macro can help.

And about OS X feature detection. What do you think is the best way to do
feature detection here? I personally dislike autoconf. But is compiling
a dummy C program okay? Or should I use "#ifdef SYS_openat"? I think the
C library and system header may not in sync though.
Yuya Nishihara - April 14, 2016, 2:48 p.m.
On Wed, 13 Apr 2016 17:55:55 +0100, Jun Wu wrote:
> Do you think it's okay to have a marco for all "*x", or "*at" stuff? It will
> make the code shorter. Although I'm not a big fan of macros, I think current
> code duplication is a bit too much and macro can help.

Using macros should be fine if that makes sense.

> And about OS X feature detection. What do you think is the best way to do
> feature detection here? I personally dislike autoconf. But is compiling
> a dummy C program okay? Or should I use "#ifdef SYS_openat"? I think the
> C library and system header may not in sync though.

No idea, and I don't like autoconf, too. It would be too complicated for small
tools like chg.

I think manually-set HAVE_OPENAT macro is good start. Then, we can add a dummy
C program later when we're sure it is the best way.
Sean Farley - April 14, 2016, 4:59 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> On Wed, 13 Apr 2016 17:55:55 +0100, Jun Wu wrote:
>> Do you think it's okay to have a marco for all "*x", or "*at" stuff? It will
>> make the code shorter. Although I'm not a big fan of macros, I think current
>> code duplication is a bit too much and macro can help.
>
> Using macros should be fine if that makes sense.

Agreed.

>> And about OS X feature detection. What do you think is the best way to do
>> feature detection here? I personally dislike autoconf. But is compiling
>> a dummy C program okay? Or should I use "#ifdef SYS_openat"? I think the
>> C library and system header may not in sync though.
>
> No idea, and I don't like autoconf, too. It would be too complicated for small
> tools like chg.

I, too, hate autotools.

> I think manually-set HAVE_OPENAT macro is good start. Then, we can add a dummy
> C program later when we're sure it is the best way.

In our other C files, we have #ifdef for Mac OS X vs Windows vs Linux,
so I'm ok with macros and preprocessors.
Katsunori FUJIWARA - April 18, 2016, 12:16 p.m.
At Thu, 14 Apr 2016 09:59:06 -0700,
Sean Farley wrote:

> Yuya Nishihara <yuya@tcha.org> writes:
> 
> > On Wed, 13 Apr 2016 17:55:55 +0100, Jun Wu wrote:
> >> Do you think it's okay to have a marco for all "*x", or "*at" stuff? It will
> >> make the code shorter. Although I'm not a big fan of macros, I think current
> >> code duplication is a bit too much and macro can help.
> >
> > Using macros should be fine if that makes sense.
> 
> Agreed.
> 
> >> And about OS X feature detection. What do you think is the best way to do
> >> feature detection here? I personally dislike autoconf. But is compiling
> >> a dummy C program okay? Or should I use "#ifdef SYS_openat"? I think the
> >> C library and system header may not in sync though.
> >
> > No idea, and I don't like autoconf, too. It would be too complicated for small
> > tools like chg.
> 
> I, too, hate autotools.
> 
> > I think manually-set HAVE_OPENAT macro is good start. Then, we can add a dummy
> > C program later when we're sure it is the best way.
> 
> In our other C files, we have #ifdef for Mac OS X vs Windows vs Linux,
> so I'm ok with macros and preprocessors.

As far as I confirm actual header files, "#if defined(AT_FDCWD)" after
"#include <fcntl.h>" can detect availability of "openat()" on Linux,
(Open)Solaris, FreeBSD and MacOS X (10.10).

Defining HAVE_OPENAT macro according to existence of AT_FDCWD macro
can avoid manually-setting it on almost all platforms.

BTW, #3 of this series uses AT_FDCWD macro without any feature
examination. But for portability, it should examine availability of
AT_FDCWD, and define dummy AT_FDCWD if it isn't defined, shouldn't it ?

    https://www.mercurial-scm.org/repo/hg-committed/rev/7b5f5a1b4b41


> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Jun Wu - April 18, 2016, 12:47 p.m.
On 04/18/2016 01:16 PM, FUJIWARA Katsunori wrote:
> As far as I confirm actual header files, "#if defined(AT_FDCWD)" after
> "#include <fcntl.h>" can detect availability of "openat()" on Linux,
> (Open)Solaris, FreeBSD and MacOS X (10.10).
>
> Defining HAVE_OPENAT macro according to existence of AT_FDCWD macro can
> avoid manually-setting it on almost all platforms.
>
> BTW, #3 of this series uses AT_FDCWD macro without any feature examination.
> But for portability, it should examine availability of AT_FDCWD, and define
> dummy AT_FDCWD if it isn't defined, shouldn't it ?

Thanks for the careful examination of these platforms!

I was unaware of the AT_FDCWD portability issue. I will set dirfd to -1 by
default (I was doing that but later found set it to AT_FDCWD can make the
code a bit shorter).
Yuya Nishihara - April 18, 2016, 12:56 p.m.
On Mon, 18 Apr 2016 21:16:25 +0900, FUJIWARA Katsunori wrote:
> As far as I confirm actual header files, "#if defined(AT_FDCWD)" after
> "#include <fcntl.h>" can detect availability of "openat()" on Linux,
> (Open)Solaris, FreeBSD and MacOS X (10.10).

So old Mac OS X, FreeBSD < 8.0, etc. didn't have xxxat() functions at all,
which sounds very convenient for us.

> Defining HAVE_OPENAT macro according to existence of AT_FDCWD macro
> can avoid manually-setting it on almost all platforms.
> 
> BTW, #3 of this series uses AT_FDCWD macro without any feature
> examination. But for portability, it should examine availability of
> AT_FDCWD, and define dummy AT_FDCWD if it isn't defined, shouldn't it ?
> 
>     https://www.mercurial-scm.org/repo/hg-committed/rev/7b5f5a1b4b41

or just initialize sockdirfd to -1.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -251,9 +251,7 @@ 
 		return name;
 
 	static char path[PATH_MAX];
-	int cwdfd = open(".", O_DIRECTORY);
-	if (cwdfd == -1)
-		abortmsgerrno("failed to open current directory");
+	int cwdfd = opencwdx();
 	fchdirx(dirfd);
 	if (getcwd(path, sizeof(path)) == NULL)
 		abortmsgerrno("failed to getcwd");
diff --git a/contrib/chg/util.c b/contrib/chg/util.c
--- a/contrib/chg/util.c
+++ b/contrib/chg/util.c
@@ -101,6 +101,14 @@ 
 		abortmsgerrno("cannot set flags of fd %d", fd);
 }
 
+int opencwdx()
+{
+	int cwdfd = open(".", O_DIRECTORY);
+	if (cwdfd == -1)
+		abortmsgerrno("cannot open cwd");
+	return cwdfd;
+}
+
 void *mallocx(size_t size)
 {
 	void *result = malloc(size);
@@ -193,9 +201,7 @@ 
 {
 	if (fd == AT_FDCWD)
 		return connect(s, addr, addrlen);
-	int cwdfd = open(".", O_DIRECTORY);
-	if (cwdfd == -1)
-		abortmsgerrno("cannot open cwd");
+	int cwdfd = opencwdx();
 	fchdirx(fd);
 	int r = connect(s, addr, addrlen);
 	fchdirx(cwdfd);
diff --git a/contrib/chg/util.h b/contrib/chg/util.h
--- a/contrib/chg/util.h
+++ b/contrib/chg/util.h
@@ -27,6 +27,7 @@ 
 
 void fchdirx(int dirfd);
 void fsetcloexec(int fd);
+int opencwdx();
 void *mallocx(size_t size);
 void *reallocx(void *ptr, size_t size);