threads / patch / 29930

patchUse macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain

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

## tl;dr

5 messages between Mar 13, 2012 and Mar 15, 2012. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

徐迪· Mar 13, 2012, 07:52 UTC · lore
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(-)
Show changes to 7 files +26 −36

Documentation/technical/api-sigchain.txt, credential-cache--daemon.c, diff.c, http-push.c, lockfile.c, pager.c, sigchain.h

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
Junio C Hamano· Mar 13, 2012, 18:11 UTC · re: 徐迪 · lore

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

徐迪 <xudifsd@gmail.com> writes:
Show 8 quoted lines
> 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.

Jeff King· Mar 13, 2012, 20:55 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 13, 2012 at 11:11:23AM -0700, Junio C Hamano wrote:
Show 19 quoted lines
> 徐迪 <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.

Show 7 quoted lines
> *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
Junio C Hamano· Mar 13, 2012, 21:17 UTC · re: Jeff King · lore

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

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.
Show 10 quoted lines
>> *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.
徐迪· Mar 15, 2012, 10:35 UTC · re: Junio C Hamano · lore

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?

← back to recent threads