{"thread":{"id":"5437","subject":"[PATCH] Trace into open fd and refactor tracing code.","startedAt":"2006-08-31T06:42:11Z","lastAt":"2006-08-31T21:49:44Z","messageCount":4,"participants":["Christian Couder","Linus Torvalds","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"26127","messageId":"20060831084211.28d38764.chriscool@tuxfamily.org","threadId":"5437","inReplyTo":null,"subject":"[PATCH] Trace into open fd and refactor tracing code.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2006-08-31T06:42:11Z","receivedAt":"2006-08-31T06:42:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Now if GIT_TRACE is set to an integer value greater than 1\nand lower than 10, we interpret this as an open fd value\nand we trace into it. Note that this behavior is not\ncompatible with the previous one.\n\nWe also trace whole messages using one write(2) call to\nmake sure messages from processes do net get mixed up in\nthe middle.\n\nIt's now possible to run the tests like this:\n\n\tGIT_TRACE=9 make test 9>/var/tmp/trace.log\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n\n---\n Makefile       |    2 -\n cache.h        |    6 +++\n exec_cmd.c     |   18 +-------\n git.c          |   25 ++---------\n imap-send.c    |   25 -----------\n quote.c        |   32 ++++++++++++++\n quote.h        |    1 \n t/test-lib.sh  |    1 \n trace.c        |  125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n write_or_die.c |   25 +++++++++++\n 10 files changed, 196 insertions(+), 64 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 48eb5f5..8c9de6b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -260,7 +260,7 @@ LIB_OBJS = \\\n \tserver-info.o setup.o sha1_file.o sha1_name.o strbuf.o \\\n \ttag.o tree.o usage.o config.o environment.o ctype.o copy.o \\\n \tfetch-clone.o revision.o pager.o tree-walk.o xdiff-interface.o \\\n-\twrite_or_die.o \\\n+\twrite_or_die.o trace.o \\\n \talloc.o merge-file.o path-list.o unpack-trees.o help.o $(DIFF_OBJS)\n \n BUILTIN_OBJS = \\\ndiff --git a/cache.h b/cache.h\nindex e26ab7d..9a700ee 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -401,6 +401,7 @@ extern char git_commit_encoding[MAX_ENCO\n \n extern int copy_fd(int ifd, int ofd);\n extern void write_or_die(int fd, const void *buf, size_t count);\n+extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);\n \n /* Finish off pack transfer receiving end */\n extern int receive_unpack_pack(int fd[2], const char *me, int quiet, int);\n@@ -426,4 +427,9 @@ extern struct commit *alloc_commit_node(\n extern struct tag *alloc_tag_node(void);\n extern void alloc_report(void);\n \n+/* trace.c */\n+extern int nfvasprintf(char **str, const char *fmt, va_list va);\n+extern void trace_printf(const char *format, ...);\n+extern void trace_argv_printf(const char **argv, int count, const char *format, ...);\n+\n #endif /* CACHE_H */\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex e30936d..5d6a124 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -97,26 +97,12 @@ int execv_git_cmd(const char **argv)\n \t\ttmp = argv[0];\n \t\targv[0] = git_command;\n \n-\t\tif (getenv(\"GIT_TRACE\")) {\n-\t\t\tconst char **p = argv;\n-\t\t\tfputs(\"trace: exec:\", stderr);\n-\t\t\twhile (*p) {\n-\t\t\t\tfputc(' ', stderr);\n-\t\t\t\tsq_quote_print(stderr, *p);\n-\t\t\t\t++p;\n-\t\t\t}\n-\t\t\tputc('\\n', stderr);\n-\t\t\tfflush(stderr);\n-\t\t}\n+\t\ttrace_argv_printf(argv, -1, \"trace: exec:\");\n \n \t\t/* execve() can only ever return if it fails */\n \t\texecve(git_command, (char **)argv, environ);\n \n-\t\tif (getenv(\"GIT_TRACE\")) {\n-\t\t\tfprintf(stderr, \"trace: exec failed: %s\\n\",\n-\t\t\t\tstrerror(errno));\n-\t\t\tfflush(stderr);\n-\t\t}\n+\t\ttrace_printf(\"trace: exec failed: %s\\n\", strerror(errno));\n \n \t\targv[0] = tmp;\n \t}\ndiff --git a/git.c b/git.c\nindex a01d195..0b78697 100644\n--- a/git.c\n+++ b/git.c\n@@ -179,17 +179,9 @@ static int handle_alias(int *argcp, cons\n \t\tif (!strcmp(alias_command, new_argv[0]))\n \t\t\tdie(\"recursive alias: %s\", alias_command);\n \n-\t\tif (getenv(\"GIT_TRACE\")) {\n-\t\t\tint i;\n-\t\t\tfprintf(stderr, \"trace: alias expansion: %s =>\",\n-\t\t\t\talias_command);\n-\t\t\tfor (i = 0; i < count; ++i) {\n-\t\t\t\tfputc(' ', stderr);\n-\t\t\t\tsq_quote_print(stderr, new_argv[i]);\n-\t\t\t}\n-\t\t\tfputc('\\n', stderr);\n-\t\t\tfflush(stderr);\n-\t\t}\n+\t\ttrace_argv_printf(new_argv, count,\n+\t\t\t\t  \"trace: alias expansion: %s =>\",\n+\t\t\t\t  alias_command);\n \n \t\tnew_argv = realloc(new_argv, sizeof(char*) *\n \t\t\t\t   (count + *argcp + 1));\n@@ -291,16 +283,7 @@ static void handle_internal_command(int \n \t\t\tprefix = setup_git_directory();\n \t\tif (p->option & USE_PAGER)\n \t\t\tsetup_pager();\n-\t\tif (getenv(\"GIT_TRACE\")) {\n-\t\t\tint j;\n-\t\t\tfprintf(stderr, \"trace: built-in: git\");\n-\t\t\tfor (j = 0; j < argc; ++j) {\n-\t\t\t\tfputc(' ', stderr);\n-\t\t\t\tsq_quote_print(stderr, argv[j]);\n-\t\t\t}\n-\t\t\tputc('\\n', stderr);\n-\t\t\tfflush(stderr);\n-\t\t}\n+\t\ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n \n \t\texit(p->fn(argc, argv, prefix));\n \t}\ndiff --git a/imap-send.c b/imap-send.c\nindex 65c71c6..6a52dbd 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -110,7 +110,6 @@ static char *next_arg( char ** );\n \n static void free_generic_messages( message_t * );\n \n-static int nfvasprintf( char **str, const char *fmt, va_list va );\n static int nfsnprintf( char *buf, int blen, const char *fmt, ... );\n \n \n@@ -372,21 +371,6 @@ free_generic_messages( message_t *msgs )\n }\n \n static int\n-git_vasprintf( char **strp, const char *fmt, va_list ap )\n-{\n-\tint len;\n-\tchar tmp[1024];\n-\n-\tif ((len = vsnprintf( tmp, sizeof(tmp), fmt, ap )) < 0 || !(*strp = xmalloc( len + 1 )))\n-\t\treturn -1;\n-\tif (len >= (int)sizeof(tmp))\n-\t\tvsprintf( *strp, fmt, ap );\n-\telse\n-\t\tmemcpy( *strp, tmp, len + 1 );\n-\treturn len;\n-}\n-\n-static int\n nfsnprintf( char *buf, int blen, const char *fmt, ... )\n {\n \tint ret;\n@@ -399,15 +383,6 @@ nfsnprintf( char *buf, int blen, const c\n \treturn ret;\n }\n \n-static int\n-nfvasprintf( char **str, const char *fmt, va_list va )\n-{\n-\tint ret = git_vasprintf( str, fmt, va );\n-\tif (ret < 0)\n-\t\tdie( \"Fatal: Out of memory\\n\");\n-\treturn ret;\n-}\n-\n static struct {\n \tunsigned char i, j, s[256];\n } rs;\ndiff --git a/quote.c b/quote.c\nindex e220dcc..f90091e 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -74,6 +74,38 @@ char *sq_quote(const char *src)\n \treturn buf;\n }\n \n+char *sq_quote_argv(const char** argv, int count)\n+{\n+\tchar *buf, *to;\n+\tint i;\n+\tsize_t len = 0;\n+\n+\t/* Count argv if needed. */\n+\tif (count < 0) {\n+\t\tfor (count = 0; argv[count]; count++)\n+\t\t\t; /* just counting */\n+\t}\n+\n+\t/* Special case: no argv. */\n+\tif (!count)\n+\t\treturn xcalloc(1,1);\n+\t\n+\t/* Get destination buffer length. */\n+\tfor (i = 0; i < count; i++)\n+\t\tlen += sq_quote_buf(NULL, 0, argv[i]) + 1;\n+\n+\t/* Alloc destination buffer. */\n+\tto = buf = xmalloc(len + 1);\n+\n+\t/* Copy into destination buffer. */\n+\tfor (i = 0; i < count; ++i) {\n+\t\t*to++ = ' ';\n+\t\tto += sq_quote_buf(to, len, argv[i]);\n+\t}\n+\n+\treturn buf;\n+}\n+\n char *sq_dequote(char *arg)\n {\n \tchar *dst = arg;\ndiff --git a/quote.h b/quote.h\nindex fc5481e..a6c4611 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -31,6 +31,7 @@ #include <stdio.h>\n extern char *sq_quote(const char *src);\n extern void sq_quote_print(FILE *stream, const char *src);\n extern size_t sq_quote_buf(char *dst, size_t n, const char *src);\n+extern char *sq_quote_argv(const char** argv, int count);\n \n /* This unwraps what sq_quote() produces in place, but returns\n  * NULL if the input does not look like what sq_quote would have\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex b6d119a..695243e 100755\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -28,7 +28,6 @@ unset GIT_DIR\n unset GIT_EXTERNAL_DIFF\n unset GIT_INDEX_FILE\n unset GIT_OBJECT_DIRECTORY\n-unset GIT_TRACE\n unset SHA1_FILE_DIRECTORIES\n unset SHA1_FILE_DIRECTORY\n export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME\ndiff --git a/trace.c b/trace.c\nnew file mode 100644\nindex 0000000..939fa76\n--- /dev/null\n+++ b/trace.c\n@@ -0,0 +1,125 @@\n+/*\n+ * GIT - The information manager from hell\n+ *\n+ * Copyright (C) 2000-2002 Michael R. Elkins <me@mutt.org>\n+ * Copyright (C) 2002-2004 Oswald Buddenhagen <ossi@users.sf.net>\n+ * Copyright (C) 2004 Theodore Y. Ts'o <tytso@mit.edu>\n+ * Copyright (C) 2006 Mike McCormack\n+ * Copyright (C) 2006 Christian Couder \n+ *\n+ *  This program is free software; you can redistribute it and/or modify\n+ *  it under the terms of the GNU General Public License as published by\n+ *  the Free Software Foundation; either version 2 of the License, or\n+ *  (at your option) any later version.\n+ *\n+ *  This program is distributed in the hope that it will be useful,\n+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of\n+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the\n+ *  GNU General Public License for more details.\n+ *\n+ *  You should have received a copy of the GNU General Public License\n+ *  along with this program; if not, write to the Free Software\n+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA\n+ */\n+\n+#include \"cache.h\"\n+#include \"quote.h\"\n+\n+/* Stolen from \"imap-send.c\". */\n+static int git_vasprintf(char **strp, const char *fmt, va_list ap)\n+{\n+\tint len;\n+\tchar tmp[1024];\n+\n+\tif ((len = vsnprintf(tmp, sizeof(tmp), fmt, ap)) < 0 ||\n+\t    !(*strp = xmalloc(len + 1)))\n+\t\treturn -1;\n+\tif (len >= (int)sizeof(tmp))\n+\t\tvsprintf(*strp, fmt, ap);\n+\telse\n+\t\tmemcpy(*strp, tmp, len + 1);\n+\treturn len;\n+}\n+\n+/* Stolen from \"imap-send.c\". */\n+int nfvasprintf(char **str, const char *fmt, va_list va)\n+{\n+\tint ret = git_vasprintf(str, fmt, va);\n+\tif (ret < 0)\n+\t\tdie(\"Fatal: Out of memory\\n\");\n+\treturn ret;\n+}\n+\n+/* Get a trace file descriptor from GIT_TRACE env variable. */\n+static int get_trace_fd()\n+{\n+\tchar *trace = getenv(\"GIT_TRACE\");\n+\n+\tif (!trace || !strcmp(trace, \"0\") || !strcasecmp(trace,\" false\"))\n+\t\treturn 0;\n+\tif (!strcmp(trace, \"1\") || !strcasecmp(trace, \"true\"))\n+\t\treturn STDERR_FILENO;\n+\tif (strlen(trace) == 1 && isdigit(*trace))\n+\t\treturn atoi(trace);\n+\n+\tfprintf(stderr, \"What does '%s' for GIT_TRACE means ?\\n\", trace);\n+\tfprintf(stderr, \"Defaulting to tracing on stderr...\\n\");\n+\treturn STDERR_FILENO;\n+}\n+\n+static const char err_msg[] = \"Could not trace into fd given by \"\n+\t\"GIT_TRACE environment variable\";\n+\n+void trace_printf(const char *format, ...)\n+{\n+\tchar *trace_str;\n+\tva_list rest;\n+\tint fd = get_trace_fd();\n+\n+\tif (!fd)\n+\t\treturn;\n+\n+\tva_start(rest, format);\n+\tnfvasprintf(&trace_str, format, rest);\n+\tva_end(rest);\n+\n+\twrite_or_whine(fd, trace_str, strlen(trace_str), err_msg);\n+\n+\tfree(trace_str);\n+}\n+\n+void trace_argv_printf(const char **argv, int count, const char *format, ...)\n+{\n+\tchar *argv_str, *format_str, *trace_str;\n+\tsize_t argv_len, format_len, trace_len;\n+\tva_list rest;\n+\tint fd = get_trace_fd();\n+\n+\tif (!fd)\n+\t\treturn;\n+\n+\t/* Get the argv string. */\n+\targv_str = sq_quote_argv(argv, count);\n+\targv_len = strlen(argv_str);\n+\n+\t/* Get the formated string. */\n+\tva_start(rest, format);\n+\tnfvasprintf(&format_str, format, rest);\n+\tva_end(rest);\n+\n+\t/* Allocate buffer for trace string. */\n+\tformat_len = strlen(format_str);\n+\ttrace_len = argv_len + format_len + 1; /* + 1 for \\n */\n+\ttrace_str = xmalloc(trace_len + 1);\n+\n+\t/* Copy everything into the trace string. */\n+\tstrncpy(trace_str, format_str, format_len);\n+\tstrncpy(trace_str + format_len, argv_str, argv_len);\n+\tstrcpy(trace_str + trace_len - 1, \"\\n\");\n+\n+\twrite_or_whine(fd, trace_str, trace_len, err_msg);\n+\t\n+\tfree(argv_str);\n+\tfree(format_str);\n+\tfree(trace_str);\n+}\ndiff --git a/write_or_die.c b/write_or_die.c\nindex ab4cb8a..bfe4eeb 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -18,3 +18,28 @@ void write_or_die(int fd, const void *bu\n \t\tp += written;\n \t}\n }\n+\n+int write_or_whine(int fd, const void *buf, size_t count, const char *msg)\n+{\n+\tconst char *p = buf;\n+\tssize_t written;\n+\n+\twhile (count > 0) {\n+\t\twritten = xwrite(fd, p, count);\n+\t\tif (written == 0) {\n+\t\t\tfprintf(stderr, \"%s: disk full?\\n\", msg);\n+\t\t\treturn 0;\n+\t\t}\n+\t\telse if (written < 0) {\n+\t\t\tif (errno == EPIPE)\n+\t\t\t\texit(0);\n+\t\t\tfprintf(stderr, \"%s: write error (%s)\\n\",\n+\t\t\t\tmsg, strerror(errno));\n+\t\t\treturn 0;\n+\t\t}\n+\t\tcount -= written;\n+\t\tp += written;\n+\t}\n+\n+\treturn 1;\n+}\n-- \n1.4.2.g584f7-dirty\n"},{"id":"26158","messageId":"Pine.LNX.4.64.0608311409470.27779@g5.osdl.org","threadId":"5437","inReplyTo":"20060831084211.28d38764.chriscool@tuxfamily.org","subject":"Re: [PATCH] Trace into open fd and refactor tracing code.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-08-31T21:14:26Z","receivedAt":"2006-08-31T21:14:26Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 31 Aug 2006, Christian Couder wrote:\n>\n> It's now possible to run the tests like this:\n> \n> \tGIT_TRACE=9 make test 9>/var/tmp/trace.log\n\nWouldn't it me _much_ more natural to instead do\n\n\tGIT_TRACE=/var/tmp/trace.log make test\n\nand just have git open that file with \"O_APPEND | O_CREAT\"?\n\nMaybe make the rule be that this only happens if the GIT_TRACE variable \nstarts with a '/' character (ie you have to make it an absolute path).\n\nThat way, if you want to do the fd thing, you can always just do\n\n\tGIT_TRACE=/proc/self/fd/9 make test 9>/var/tmp/trace.log\n\nalthough I really don't know how well that works across a fork/exec that \nmay or may not be closing the file descriptor (I think it's much simpler \nand more obvious to just give the filename directly).\n\nHmm?\n\n\t\t\tLinus\n"},{"id":"26159","messageId":"7vsljc8udx.fsf@assigned-by-dhcp.cox.net","threadId":"5437","inReplyTo":"Pine.LNX.4.64.0608311409470.27779@g5.osdl.org","subject":"Re: [PATCH] Trace into open fd and refactor tracing code.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-31T21:31:22Z","receivedAt":"2006-08-31T21:31:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> That way, if you want to do the fd thing, you can always just do\n>\n> \tGIT_TRACE=/proc/self/fd/9 make test 9>/var/tmp/trace.log\n>\n> although I really don't know how well that works across a fork/exec that \n> may or may not be closing the file descriptor (I think it's much simpler \n> and more obvious to just give the filename directly).\n\nActually it was me who suggested the use of fd directly, along\nwith trying to do one message in a single write (i.e. not doing\nstdio as much as possible).\n\nProcess A has fd=9 opened to /var/tmp/trace.log and forks\nprocess B.\n\n(1) process B execs something else, and just writes into fd=9\n\nor\n\n(2) process B execs something else, perhaps after closing fd=9,\n    and the exec'ed image opens /var/tmp/trace.log with\n    \"O_APPEND|O_CREAT\" and gets a fd; it writes into this fd.\n\nI somehow felt there would be less interference between writes\nissued by process A and process B, but probably I am mistaken.\n"},{"id":"26162","messageId":"Pine.LNX.4.64.0608311444200.27779@g5.osdl.org","threadId":"5437","inReplyTo":"7vsljc8udx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Trace into open fd and refactor tracing code.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-08-31T21:49:44Z","receivedAt":"2006-08-31T21:49:44Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 31 Aug 2006, Junio C Hamano wrote:\n> \n> Actually it was me who suggested the use of fd directly, along\n> with trying to do one message in a single write (i.e. not doing\n> stdio as much as possible).\n\nI definitely agree with the \"one message in a single write\" thing.\n\nThat's in fact required to make sure that O_APPEND works properly (ie that \nyou don't have potentially different processes mixing output)\n\n> I somehow felt there would be less interference between writes\n> issued by process A and process B, but probably I am mistaken.\n\nThe atomicity guarantees are the same regardless of whether you do a \nseparate open(), the only thing that matters is that you do want to try to \ndo messages with a single \"write()\" call, and that you use O_APPEND. That \nshould guarantee that you get good behaviour.\n\nWithout O_APPEND, you would obviously have to share one single file \ndescriptor, because otherwise people will just be writing all over each \nother.\n\n\t\t\tLinus\n"}]}