[PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain

[PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain

[徐迪]

The sigchain APIs require user to code like:

void clean_foo_on_signal(int sig)
{
 clean_foo();
 sigchain_pop(sig);
 raise(sig);
}

this force user to know how sigchain works internally, and
sigchain_pop is something like hard-coded constant value, if we change
it someday, we shall change it everywhere where we use sigchain APIs.
We could use macro to override this. The HANDLER_WRAPPER take two
arguments: one for the function name it will define, and another for
clean function which handler should call before sigchain_pop(sig) and
raise(sig).

The only problem of this patch is HANDLER_WRAPPER is not generic
enough, some clean function requires one argument which is signo to
finish its work, but the only example of this is the function named
'cleanup_children' from run-command.c. Maybe we should force every
clean function take signo argument?

Signed-off-by: Xu Di <xudifsd@gmail.com>
---
 Documentation/technical/api-sigchain.txt |   19 +++++++++++++------
 credential-cache--daemon.c               |    7 +------
 diff.c                                   |    7 +------
 http-push.c                              |    7 +------
 lockfile.c                               |    7 +------
 pager.c                                  |    7 +------
 sigchain.h                               |    8 ++++++++
 7 files changed, 26 insertions(+), 36 deletions(-)

diff --git a/Documentation/technical/api-sigchain.txt
b/Documentation/technical/api-sigchain.txt
index 9e1189e..b6a0b88 100644
--- a/Documentation/technical/api-sigchain.txt
+++ b/Documentation/technical/api-sigchain.txt
@@ -17,12 +17,7 @@ Sigchain is a tiny library for keeping a stack of
handlers. Your handler
 and installation code should look something like:

 ------------------------------------------
-  void clean_foo_on_signal(int sig)
-  {
-	  clean_foo();
-	  sigchain_pop(sig);
-	  raise(sig);
-  }
+  HANDLER_WRAPPER(clean_foo_on_signal, clean_foo)

   void other_func()
   {
@@ -32,6 +27,18 @@ and installation code should look something like:
   }
 ------------------------------------------

+We use the macro which is named HANDLER_WRAPPER to help us wrap the
+clean function, the macro above will finally expanded into:
+
+------------------------------------------
+  void clean_foo_on_signal(int sig)
+  {
+	  clean_foo();
+	  sigchain_pop(sig);
+	  raise(sig);
+  }
+------------------------------------------
+
 Handlers are given the typedef of sigchain_fun. This is the same type
 that is given to signal() or sigaction(). It is perfectly reasonable to
 push SIG_DFL or SIG_IGN onto the stack.
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 390f194..8bb983d 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -11,12 +11,7 @@ static void cleanup_socket(void)
 		unlink(socket_path);
 }

-static void cleanup_socket_on_signal(int sig)
-{
-	cleanup_socket();
-	sigchain_pop(sig);
-	raise(sig);
-}
+static HANDLER_WRAPPER(cleanup_socket_on_signal, cleanup_socket)

 struct credential_cache_entry {
 	struct credential item;
diff --git a/diff.c b/diff.c
index 377ec1e..9535171 100644
--- a/diff.c
+++ b/diff.c
@@ -525,12 +525,7 @@ static void remove_tempfile(void)
 	}
 }

-static void remove_tempfile_on_signal(int signo)
-{
-	remove_tempfile();
-	sigchain_pop(signo);
-	raise(signo);
-}
+static HANDLER_WRAPPER(remove_tempfile_on_signal, remove_tempfile)

 static void print_line_count(FILE *file, int count)
 {
diff --git a/http-push.c b/http-push.c
index f22f7e4..b2e33a7 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1027,12 +1027,7 @@ static void remove_locks(void)
 	}
 }

-static void remove_locks_on_signal(int signo)
-{
-	remove_locks();
-	sigchain_pop(signo);
-	raise(signo);
-}
+static HANDLER_WRAPPER(remove_locks_on_signal, remove_locks)

 static void remote_ls(const char *path, int flags,
 		      void (*userFunc)(struct remote_ls_ctx *ls),
diff --git a/lockfile.c b/lockfile.c
index c6fb77b..fd972f3 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -22,12 +22,7 @@ static void remove_lock_file(void)
 	}
 }

-static void remove_lock_file_on_signal(int signo)
-{
-	remove_lock_file();
-	sigchain_pop(signo);
-	raise(signo);
-}
+static HANDLER_WRAPPER(remove_lock_file_on_signal, remove_lock_file)

 /*
  * p = absolute or relative path name
diff --git a/pager.c b/pager.c
index 05584de..4ba2b41 100644
--- a/pager.c
+++ b/pager.c
@@ -39,12 +39,7 @@ static void wait_for_pager(void)
 	finish_command(&pager_process);
 }

-static void wait_for_pager_signal(int signo)
-{
-	wait_for_pager();
-	sigchain_pop(signo);
-	raise(signo);
-}
+static HANDLER_WRAPPER(wait_for_pager_signal, wait_for_pager)

 const char *git_pager(int stdout_is_tty)
 {
diff --git a/sigchain.h b/sigchain.h
index 618083b..949f0d4 100644
--- a/sigchain.h
+++ b/sigchain.h
@@ -1,6 +1,14 @@
 #ifndef SIGCHAIN_H
 #define SIGCHAIN_H

+#define HANDLER_WRAPPER(name, handler) \
+void name(int sig)				\
+{								\
+		handler();			\
+		sigchain_pop(sig);		\
+		raise(sig);				\
+}
+
 typedef void (*sigchain_fun)(int);

 int sigchain_push(int sig, sigchain_fun f);
-- 
1.7.9.1.265.gb3a76

Re: [PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain

[Junio C Hamano]

徐迪 <xudifsd@gmail.com> writes:

> The sigchain APIs require user to code like:
>
> void clean_foo_on_signal(int sig)
> {
>  clean_foo();
>  sigchain_pop(sig);
>  raise(sig);
> }

I can see the repetition, but I do not think your macro is a very good way
to reduce it.  Can't we fix the sigchain API a bit, perhaps first by
making a bit more state available to the sigchain stack?

	typedef void (*sigchain_fn)(int, void *);
        int sigchain_push(int sig, sigchain_fn fn, void *cb_data);
        int sigchain_pop(int sig);
        void sigchain_push_common(sigchain_fn fn, void *cb_data);

And then make the repetitive one into a single copy of a helper function,
while giving a new external interface to register a on_signal handler:

        static void relay_fn(int sig, void *cb_data)
        {
                void (*fn)(void) = cb_data; /* [*1*] */
        	fn();
                sigchain_pop(sig);
                raise(sig);
        }

        void sigchain_add_relayed(void (*fn)(void))
        {
		sigchain_push_common(relay_fn, fn);
        }

The code that wants to register a clean-up action would do:

	sigchain_add_relayed(clean_foo);

instead of

	sigchain_push_common(clean_foo_on_signal);

with repeated definition of clean_xxx_on_signal.

Hmm?

[Footnote]

*1* Yes, I know, this casts between a data pointer and a function pointer
that is not portable, but the purpose of this pseudo-code is primarily to
illustrate the high level view of the idea.  We would probably want to be
able to pass callback value to the clean_foo() function and at that point,
we would likely to be passing a pointer to a struct, and we could declare
the first element of such a struct is a pointer to a sigchain_fn, or
something.

Re: [PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain

[Jeff King]

On Tue, Mar 13, 2012 at 11:11:23AM -0700, Junio C Hamano wrote:

> 徐迪 <xudifsd@gmail.com> writes:
> 
> > The sigchain APIs require user to code like:
> >
> > void clean_foo_on_signal(int sig)
> > {
> >  clean_foo();
> >  sigchain_pop(sig);
> >  raise(sig);
> > }
> 
> I can see the repetition, but I do not think your macro is a very good way
> to reduce it.  Can't we fix the sigchain API a bit, perhaps first by
> making a bit more state available to the sigchain stack?
>
> 	typedef void (*sigchain_fn)(int, void *);
>         int sigchain_push(int sig, sigchain_fn fn, void *cb_data);
>         int sigchain_pop(int sig);
>         void sigchain_push_common(sigchain_fn fn, void *cb_data);

FWIW, when I wrote the original sigchain implementation I considered
doing something more complex like this. The calling code ends up
shorter, but I wonder if the end result is really any easier to
understand. Ditto for the macro. It's shorter, but much harder for a
reader to understand. The boilerplate, while ugly, reads very simply,
and is not error-prone.

So I don't like boilerplate or repetition, but this may be one of those
times where it is simply more clear to just be verbose.

> *1* Yes, I know, this casts between a data pointer and a function pointer
> that is not portable, but the purpose of this pseudo-code is primarily to
> illustrate the high level view of the idea.  We would probably want to be
> able to pass callback value to the clean_foo() function and at that point,
> we would likely to be passing a pointer to a struct, and we could declare
> the first element of such a struct is a pointer to a sigchain_fn, or
> something.

That is the correct way to do it, but where does the struct memory come
from? You'd have to allocate it on the heap, unless you want to burden
the caller.

-Peff

Re: [PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I don't like boilerplate or repetition, but this may be one of those
> times where it is simply more clear to just be verbose.

Yes, I agree with you 100% re: these particular repetitions.

>> *1* Yes, I know, this casts between a data pointer and a function pointer
>> that is not portable, but the purpose of this pseudo-code is primarily to
>> illustrate the high level view of the idea.  We would probably want to be
>> able to pass callback value to the clean_foo() function and at that point,
>> we would likely to be passing a pointer to a struct, and we could declare
>> the first element of such a struct is a pointer to a sigchain_fn, or
>> something.
>
> That is the correct way to do it, but where does the struct memory come
> from?

Probably somewhere near "struct sigchain_signal" are kept.

Re: [PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain

[徐迪]

Eh. I really don't see why it is difficult to read. Although it _is_
error-prone, and also difficult to do further functionality extension.

BTW, if we want to generalize the clean_xxx_on_signal, how should we
deal with the exception I mentioned before(function named
cleanup_children in run-command.c which require one argument)? By
forcing all clean_xxx take one argument?

Also, many books mentioned that we should never use signal() in
portable program, use sigaction() instead, but there are plenty of
signal() in git. Why should we do so?