Patchwork [04,of,14] chg: check lockfd at freecmdserveropts

login
register
mail settings
Submitter Jun Wu
Date April 10, 2016, 11:57 p.m.
Message ID <f2400efd3bd61aef35de.1460332641@x1c>
Download mbox | patch
Permalink /patch/14501/
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 1460325491 -3600
#      Sun Apr 10 22:58:11 2016 +0100
# Node ID f2400efd3bd61aef35de18b462d50611be4d77d1
# Parent  400fbf5ed12b8585bea9fee27abb250fa4c98bd1
chg: check lockfd at freecmdserveropts

We check for sockdirfd at freecmdserveropts but not lockfd, which is a bit
strange to people new to the code. Add a comment and an assert to make it
clear that lockfd should be closed earlier.
timeless - April 11, 2016, 3:33 a.m.
Jun Wu wrote:
> +       /* lockfd should be closed by unlockcmdserver() */

I'd rather "unlockcmdserver should close lockfd"
also, "close" doesn't automatically imply "and set to -1" to me...
Yuya Nishihara - April 11, 2016, 1:50 p.m.
On Sun, 10 Apr 2016 23:33:35 -0400, timeless wrote:
> Jun Wu wrote:
> > +       /* lockfd should be closed by unlockcmdserver() */  
> 
> I'd rather "unlockcmdserver should close lockfd"
> also, "close" doesn't automatically imply "and set to -1" to me...

I prefer the original message because here we want to assert that
unlockcmdserver() should be called before.

I moved the message into assert() and queued the patch 3 and 4, thanks.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -49,6 +49,8 @@ 
 	free(opts->args);
 	opts->args = NULL;
 	opts->argsize = 0;
+	/* lockfd should be closed by unlockcmdserver() */
+	assert(opts->lockfd == -1);
 	if (opts->sockdirfd != AT_FDCWD) {
 		close(opts->sockdirfd);
 		opts->sockdirfd = AT_FDCWD;