{"thread":{"id":"29930","subject":"[PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain","startedAt":"2012-03-13T07:52:56Z","lastAt":"2012-03-15T10:35:28Z","messageCount":5,"participants":["徐迪","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"186822","messageId":"CAMocUqRsEvwnoV32Cr05dJeUj7iNDj1cLP5kAzgyMNEo1O0kCw@mail.gmail.com","threadId":"29930","inReplyTo":null,"subject":"[PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain","fromName":"徐迪","fromEmail":"xudifsd@gmail.com","sentAt":"2012-03-13T07:52:56Z","receivedAt":"2012-03-13T07:52:56Z","isPatch":true,"sender":{"key":"xudifsd@gmail.com","avatar":"https://gravatar.com/avatar/260814589d486b8dba6ff105f193d831bf9de466d688a20c53f7a516c7ffe146?d=mp&s=160"},"body":"The sigchain APIs require user to code like:\n\nvoid clean_foo_on_signal(int sig)\n{\n clean_foo();\n sigchain_pop(sig);\n raise(sig);\n}\n\nthis force user to know how sigchain works internally, and\nsigchain_pop is something like hard-coded constant value, if we change\nit someday, we shall change it everywhere where we use sigchain APIs.\nWe could use macro to override this. The HANDLER_WRAPPER take two\narguments: one for the function name it will define, and another for\nclean function which handler should call before sigchain_pop(sig) and\nraise(sig).\n\nThe only problem of this patch is HANDLER_WRAPPER is not generic\nenough, some clean function requires one argument which is signo to\nfinish its work, but the only example of this is the function named\n'cleanup_children' from run-command.c. Maybe we should force every\nclean function take signo argument?\n\nSigned-off-by: Xu Di <xudifsd@gmail.com>\n---\n Documentation/technical/api-sigchain.txt |   19 +++++++++++++------\n credential-cache--daemon.c               |    7 +------\n diff.c                                   |    7 +------\n http-push.c                              |    7 +------\n lockfile.c                               |    7 +------\n pager.c                                  |    7 +------\n sigchain.h                               |    8 ++++++++\n 7 files changed, 26 insertions(+), 36 deletions(-)\n\ndiff --git a/Documentation/technical/api-sigchain.txt\nb/Documentation/technical/api-sigchain.txt\nindex 9e1189e..b6a0b88 100644\n--- a/Documentation/technical/api-sigchain.txt\n+++ b/Documentation/technical/api-sigchain.txt\n@@ -17,12 +17,7 @@ Sigchain is a tiny library for keeping a stack of\nhandlers. Your handler\n and installation code should look something like:\n\n ------------------------------------------\n-  void clean_foo_on_signal(int sig)\n-  {\n-\t  clean_foo();\n-\t  sigchain_pop(sig);\n-\t  raise(sig);\n-  }\n+  HANDLER_WRAPPER(clean_foo_on_signal, clean_foo)\n\n   void other_func()\n   {\n@@ -32,6 +27,18 @@ and installation code should look something like:\n   }\n ------------------------------------------\n\n+We use the macro which is named HANDLER_WRAPPER to help us wrap the\n+clean function, the macro above will finally expanded into:\n+\n+------------------------------------------\n+  void clean_foo_on_signal(int sig)\n+  {\n+\t  clean_foo();\n+\t  sigchain_pop(sig);\n+\t  raise(sig);\n+  }\n+------------------------------------------\n+\n Handlers are given the typedef of sigchain_fun. This is the same type\n that is given to signal() or sigaction(). It is perfectly reasonable to\n push SIG_DFL or SIG_IGN onto the stack.\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex 390f194..8bb983d 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -11,12 +11,7 @@ static void cleanup_socket(void)\n \t\tunlink(socket_path);\n }\n\n-static void cleanup_socket_on_signal(int sig)\n-{\n-\tcleanup_socket();\n-\tsigchain_pop(sig);\n-\traise(sig);\n-}\n+static HANDLER_WRAPPER(cleanup_socket_on_signal, cleanup_socket)\n\n struct credential_cache_entry {\n \tstruct credential item;\ndiff --git a/diff.c b/diff.c\nindex 377ec1e..9535171 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -525,12 +525,7 @@ static void remove_tempfile(void)\n \t}\n }\n\n-static void remove_tempfile_on_signal(int signo)\n-{\n-\tremove_tempfile();\n-\tsigchain_pop(signo);\n-\traise(signo);\n-}\n+static HANDLER_WRAPPER(remove_tempfile_on_signal, remove_tempfile)\n\n static void print_line_count(FILE *file, int count)\n {\ndiff --git a/http-push.c b/http-push.c\nindex f22f7e4..b2e33a7 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1027,12 +1027,7 @@ static void remove_locks(void)\n \t}\n }\n\n-static void remove_locks_on_signal(int signo)\n-{\n-\tremove_locks();\n-\tsigchain_pop(signo);\n-\traise(signo);\n-}\n+static HANDLER_WRAPPER(remove_locks_on_signal, remove_locks)\n\n static void remote_ls(const char *path, int flags,\n \t\t      void (*userFunc)(struct remote_ls_ctx *ls),\ndiff --git a/lockfile.c b/lockfile.c\nindex c6fb77b..fd972f3 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -22,12 +22,7 @@ static void remove_lock_file(void)\n \t}\n }\n\n-static void remove_lock_file_on_signal(int signo)\n-{\n-\tremove_lock_file();\n-\tsigchain_pop(signo);\n-\traise(signo);\n-}\n+static HANDLER_WRAPPER(remove_lock_file_on_signal, remove_lock_file)\n\n /*\n  * p = absolute or relative path name\ndiff --git a/pager.c b/pager.c\nindex 05584de..4ba2b41 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -39,12 +39,7 @@ static void wait_for_pager(void)\n \tfinish_command(&pager_process);\n }\n\n-static void wait_for_pager_signal(int signo)\n-{\n-\twait_for_pager();\n-\tsigchain_pop(signo);\n-\traise(signo);\n-}\n+static HANDLER_WRAPPER(wait_for_pager_signal, wait_for_pager)\n\n const char *git_pager(int stdout_is_tty)\n {\ndiff --git a/sigchain.h b/sigchain.h\nindex 618083b..949f0d4 100644\n--- a/sigchain.h\n+++ b/sigchain.h\n@@ -1,6 +1,14 @@\n #ifndef SIGCHAIN_H\n #define SIGCHAIN_H\n\n+#define HANDLER_WRAPPER(name, handler) \\\n+void name(int sig)\t\t\t\t\\\n+{\t\t\t\t\t\t\t\t\\\n+\t\thandler();\t\t\t\\\n+\t\tsigchain_pop(sig);\t\t\\\n+\t\traise(sig);\t\t\t\t\\\n+}\n+\n typedef void (*sigchain_fun)(int);\n\n int sigchain_push(int sig, sigchain_fun f);\n-- \n1.7.9.1.265.gb3a76\n"},{"id":"186885","messageId":"7vmx7kweus.fsf@alter.siamese.dyndns.org","threadId":"29930","inReplyTo":"CAMocUqRsEvwnoV32Cr05dJeUj7iNDj1cLP5kAzgyMNEo1O0kCw@mail.gmail.com","subject":"Re: [PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-13T18:11:23Z","receivedAt":"2012-03-13T18:11:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"徐迪 <xudifsd@gmail.com> writes:\n\n> The sigchain APIs require user to code like:\n>\n> void clean_foo_on_signal(int sig)\n> {\n>  clean_foo();\n>  sigchain_pop(sig);\n>  raise(sig);\n> }\n\nI can see the repetition, but I do not think your macro is a very good way\nto reduce it.  Can't we fix the sigchain API a bit, perhaps first by\nmaking a bit more state available to the sigchain stack?\n\n\ttypedef void (*sigchain_fn)(int, void *);\n        int sigchain_push(int sig, sigchain_fn fn, void *cb_data);\n        int sigchain_pop(int sig);\n        void sigchain_push_common(sigchain_fn fn, void *cb_data);\n\nAnd then make the repetitive one into a single copy of a helper function,\nwhile giving a new external interface to register a on_signal handler:\n\n        static void relay_fn(int sig, void *cb_data)\n        {\n                void (*fn)(void) = cb_data; /* [*1*] */\n        \tfn();\n                sigchain_pop(sig);\n                raise(sig);\n        }\n\n        void sigchain_add_relayed(void (*fn)(void))\n        {\n\t\tsigchain_push_common(relay_fn, fn);\n        }\n\nThe code that wants to register a clean-up action would do:\n\n\tsigchain_add_relayed(clean_foo);\n\ninstead of\n\n\tsigchain_push_common(clean_foo_on_signal);\n\nwith repeated definition of clean_xxx_on_signal.\n\nHmm?\n\n[Footnote]\n\n*1* Yes, I know, this casts between a data pointer and a function pointer\nthat is not portable, but the purpose of this pseudo-code is primarily to\nillustrate the high level view of the idea.  We would probably want to be\nable to pass callback value to the clean_foo() function and at that point,\nwe would likely to be passing a pointer to a struct, and we could declare\nthe first element of such a struct is a pointer to a sigchain_fn, or\nsomething.\n"},{"id":"186902","messageId":"20120313205501.GB27436@sigill.intra.peff.net","threadId":"29930","inReplyTo":"7vmx7kweus.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-13T20:55:01Z","receivedAt":"2012-03-13T20:55:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 13, 2012 at 11:11:23AM -0700, Junio C Hamano wrote:\n\n> 徐迪 <xudifsd@gmail.com> writes:\n> \n> > The sigchain APIs require user to code like:\n> >\n> > void clean_foo_on_signal(int sig)\n> > {\n> >  clean_foo();\n> >  sigchain_pop(sig);\n> >  raise(sig);\n> > }\n> \n> I can see the repetition, but I do not think your macro is a very good way\n> to reduce it.  Can't we fix the sigchain API a bit, perhaps first by\n> making a bit more state available to the sigchain stack?\n>\n> \ttypedef void (*sigchain_fn)(int, void *);\n>         int sigchain_push(int sig, sigchain_fn fn, void *cb_data);\n>         int sigchain_pop(int sig);\n>         void sigchain_push_common(sigchain_fn fn, void *cb_data);\n\nFWIW, when I wrote the original sigchain implementation I considered\ndoing something more complex like this. The calling code ends up\nshorter, but I wonder if the end result is really any easier to\nunderstand. Ditto for the macro. It's shorter, but much harder for a\nreader to understand. The boilerplate, while ugly, reads very simply,\nand is not error-prone.\n\nSo I don't like boilerplate or repetition, but this may be one of those\ntimes where it is simply more clear to just be verbose.\n\n> *1* Yes, I know, this casts between a data pointer and a function pointer\n> that is not portable, but the purpose of this pseudo-code is primarily to\n> illustrate the high level view of the idea.  We would probably want to be\n> able to pass callback value to the clean_foo() function and at that point,\n> we would likely to be passing a pointer to a struct, and we could declare\n> the first element of such a struct is a pointer to a sigchain_fn, or\n> something.\n\nThat is the correct way to do it, but where does the struct memory come\nfrom? You'd have to allocate it on the heap, unless you want to burden\nthe caller.\n\n-Peff\n"},{"id":"186904","messageId":"7vlin4td4g.fsf@alter.siamese.dyndns.org","threadId":"29930","inReplyTo":"20120313205501.GB27436@sigill.intra.peff.net","subject":"Re: [PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-13T21:17:03Z","receivedAt":"2012-03-13T21:17:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I don't like boilerplate or repetition, but this may be one of those\n> times where it is simply more clear to just be verbose.\n\nYes, I agree with you 100% re: these particular repetitions.\n\n>> *1* Yes, I know, this casts between a data pointer and a function pointer\n>> that is not portable, but the purpose of this pseudo-code is primarily to\n>> illustrate the high level view of the idea.  We would probably want to be\n>> able to pass callback value to the clean_foo() function and at that point,\n>> we would likely to be passing a pointer to a struct, and we could declare\n>> the first element of such a struct is a pointer to a sigchain_fn, or\n>> something.\n>\n> That is the correct way to do it, but where does the struct memory come\n> from?\n\nProbably somewhere near \"struct sigchain_signal\" are kept.\n"},{"id":"187039","messageId":"CAMocUqRuKXoNAmqzcxvDmt4Oo=NqkM=5WyqgDqfDBMcTK0Cq7w@mail.gmail.com","threadId":"29930","inReplyTo":"7vlin4td4g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Use macro HANDLER_WRAPPER in sigchain to wrap clean function of sigchain","fromName":"徐迪","fromEmail":"xudifsd@gmail.com","sentAt":"2012-03-15T10:35:28Z","receivedAt":"2012-03-15T10:35:28Z","isPatch":true,"sender":{"key":"xudifsd@gmail.com","avatar":"https://gravatar.com/avatar/260814589d486b8dba6ff105f193d831bf9de466d688a20c53f7a516c7ffe146?d=mp&s=160"},"body":"Eh. I really don't see why it is difficult to read. Although it _is_\nerror-prone, and also difficult to do further functionality extension.\n\nBTW, if we want to generalize the clean_xxx_on_signal, how should we\ndeal with the exception I mentioned before(function named\ncleanup_children in run-command.c which require one argument)? By\nforcing all clean_xxx take one argument?\n\nAlso, many books mentioned that we should never use signal() in\nportable program, use sigaction() instead, but there are plenty of\nsignal() in git. Why should we do so?\n"}]}