{"thread":{"id":"37658","subject":"[PATCH 0/3] Support stdio access to lockfiles","startedAt":"2014-10-01T11:14:46Z","lastAt":"2014-10-12T06:17:19Z","messageCount":9,"participants":["Michael Haggerty","Jeff King","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"250105","messageId":"1412162089-3233-1-git-send-email-mhagger@alum.mit.edu","threadId":"37658","inReplyTo":null,"subject":"[PATCH 0/3] Support stdio access to lockfiles","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-01T11:14:46Z","receivedAt":"2014-10-01T11:14:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This series applies on top of the series \"Lockfile correctness and\nrefactoring\" (Junio's branch mh/lockfile).\n\nThere are already two callers that write to lockfiles using stdio. But\nthey currently need intimate knowledge of the lockfile implementation\nto work correctly; for example, they have to call fclose() themselves\nand set lk->fd to -1 to prevent the file from being closed again. This\nis awkward and error-prone.\n\nSo provide official API support for stdio-based access to lockfiles.\nAdd a new function fdopen_lock_file(), which returns a (FILE *)\nassociated with an open lockfile, and teach close_lock_file() (and\ntherefore also commit_lock_file(), rollback_lock_file(), etc.) to use\nfclose() instead of close() on lockfiles for which fdopen_lock_file()\nhas been called.\n\n...except in the signal handler, where calling fclose() is not\npermitted. In the signal handler call close() on any still-open\nlockfiles regardless of whether they have been fdopen()ed. Since the\nvery next step is to delete the file, this should be OK.\n\nThe second and third patches rewrite the two callers who currently\nfdopen() lockfiles to use the new function. I didn't look around for\nother lockfile users that might be simplified and/or sped up by\nconverting them to use stdio; probably there are some.\n\nThis improvement was initially discussed when the second fdopen()\ncallsite was added [1] and also when discussing inconsistencies\nbetween the documentation and real life in the context of the\nmh/lockfile patch series [2].\n\nMichael\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/256729/focus=256734\n[2] http://thread.gmane.org/gmane.comp.version-control.git/257504/focus=257553\n\nMichael Haggerty (3):\n  fdopen_lock_file(): access a lockfile using stdio\n  dump_marks(): reimplement using fdopen_lock_file()\n  commit_packed_refs(): reimplement using fdopen_lock_file()\n\n Documentation/technical/api-lockfile.txt | 34 +++++++++++++++--------\n fast-import.c                            | 21 ++-------------\n lockfile.c                               | 46 ++++++++++++++++++++++++++++----\n lockfile.h                               |  4 +++\n refs.c                                   |  5 +---\n 5 files changed, 71 insertions(+), 39 deletions(-)\n\n-- \n2.1.0\n"},{"id":"250108","messageId":"1412162089-3233-2-git-send-email-mhagger@alum.mit.edu","threadId":"37658","inReplyTo":"1412162089-3233-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/3] fdopen_lock_file(): access a lockfile using stdio","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-01T11:14:47Z","receivedAt":"2014-10-01T11:14:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a new function, fdopen_lock_file(), which returns a FILE pointer\nopen to the lockfile. If a stream is open on a lock_file object, it is\nclosed using fclose() on commit, rollback, or close_lock_file().\n\nThis change will allow callers to use stdio to write to a lockfile\nwithout having to muck around in the internal representation of the\nlock_file object (callers will be rewritten in upcoming commits).\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nI thought about adding second stdio-oriented entrance points analogous\nto hold_lock_file_for_update(), hold_lock_file_for_append(), and\nreopen_lock_file(), but it seemed simpler to add just the one new\nfunction instead of three or four. If using stdio on lockfiles becomes\nmore popular, we might want to add some helper functions to make it a\nbit more convenient.\n\nIn close_lock_file(), if ferror() returns an error, then errno is not\nnecessarily still set in a way that reflects the original error. I\ndon't see a way to ensure that errno is set correctly in this\nsituation. But hopefully, callers are monitoring their calls to\nfwrite()/fprintf() etc and will have noticed write errors on their own\nalready. If anybody can suggest an improvement here, please let me\nknow.\n\n Documentation/technical/api-lockfile.txt | 34 +++++++++++++++--------\n lockfile.c                               | 46 ++++++++++++++++++++++++++++----\n lockfile.h                               |  4 +++\n 3 files changed, 68 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex d4484d1..93b5f23 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -42,9 +42,13 @@ The caller:\n   of the final destination (e.g. `$GIT_DIR/index`) to\n   `hold_lock_file_for_update` or `hold_lock_file_for_append`.\n \n-* Writes new content for the destination file by writing to the file\n-  descriptor returned by those functions (also available via\n-  `lock->fd`).\n+* Writes new content for the destination file by either:\n+\n+  * writing to the file descriptor returned by the `hold_lock_file_*`\n+    functions (also available via `lock->fd`).\n+\n+  * calling `fdopen_lock_file` to get a `FILE` pointer for the open\n+    file and writing to the file using stdio.\n \n When finished writing, the caller can:\n \n@@ -70,10 +74,10 @@ any uncommitted changes.\n \n If you need to close the file descriptor you obtained from a\n `hold_lock_file_*` function yourself, do so by calling\n-`close_lock_file`. You should never call `close(2)` yourself!\n-Otherwise the `struct lock_file` structure would still think that the\n-file descriptor needs to be closed, and a commit or rollback would\n-result in duplicate calls to `close(2)`. Worse yet, if you `close(2)`\n+`close_lock_file`. You should never call `close(2)` or `fclose(3)`\n+yourself! Otherwise the `struct lock_file` structure would still think\n+that the file descriptor needs to be closed, and a commit or rollback\n+would result in duplicate calls to `close(2)`. Worse yet, if you close\n and then later open another file descriptor for a completely different\n purpose, then a commit or rollback might close that unrelated file\n descriptor.\n@@ -143,6 +147,13 @@ hold_lock_file_for_append::\n \tthe existing contents of the file (if any) to the lockfile and\n \tposition its write pointer at the end of the file.\n \n+fdopen_lock_file::\n+\n+\tAssociate a stdio stream with the lockfile. Return NULL\n+\t(*without* rolling back the lockfile) on error. The stream is\n+\tclosed automatically when `close_lock_file` is called or when\n+\tthe file is committed or rolled back.\n+\n get_locked_file_path::\n \n \tReturn the path of the file that is locked by the specified\n@@ -179,10 +190,11 @@ close_lock_file::\n \n \tTake a pointer to the `struct lock_file` initialized with an\n \tearlier call to `hold_lock_file_for_update` or\n-\t`hold_lock_file_for_append`, and close the file descriptor.\n-\tReturn 0 upon success. On failure to `close(2)`, return a\n-\tnegative value and roll back the lock file. Usually\n-\t`commit_lock_file`, `commit_lock_file_to`, or\n+\t`hold_lock_file_for_append`. Close the file descriptor (and\n+\tthe file pointer if it has been opened using\n+\t`fdopen_lock_file`). Return 0 upon success. On failure to\n+\t`close(2)`, return a negative value and roll back the lock\n+\tfile. Usually `commit_lock_file`, `commit_lock_file_to`, or\n \t`rollback_lock_file` should eventually be called if\n \t`close_lock_file` succeeds.\n \ndiff --git a/lockfile.c b/lockfile.c\nindex d27e61c..e046027 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -7,20 +7,29 @@\n \n static struct lock_file *volatile lock_file_list;\n \n-static void remove_lock_files(void)\n+static void remove_lock_files(int skip_fclose)\n {\n \tpid_t me = getpid();\n \n \twhile (lock_file_list) {\n-\t\tif (lock_file_list->owner == me)\n+\t\tif (lock_file_list->owner == me) {\n+\t\t\t/* fclose() is not safe to call in a signal handler */\n+\t\t\tif (skip_fclose)\n+\t\t\t\tlock_file_list->fp = NULL;\n \t\t\trollback_lock_file(lock_file_list);\n+\t\t}\n \t\tlock_file_list = lock_file_list->next;\n \t}\n }\n \n+static void remove_lock_files_on_exit(void)\n+{\n+\tremove_lock_files(0);\n+}\n+\n static void remove_lock_files_on_signal(int signo)\n {\n-\tremove_lock_files();\n+\tremove_lock_files(1);\n \tsigchain_pop(signo);\n \traise(signo);\n }\n@@ -97,7 +106,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tif (!lock_file_list) {\n \t\t/* One-time initialization */\n \t\tsigchain_push_common(remove_lock_files_on_signal);\n-\t\tatexit(remove_lock_files);\n+\t\tatexit(remove_lock_files_on_exit);\n \t}\n \n \tif (lk->active)\n@@ -106,6 +115,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tif (!lk->on_list) {\n \t\t/* Initialize *lk and add it to lock_file_list: */\n \t\tlk->fd = -1;\n+\t\tlk->fp = NULL;\n \t\tlk->active = 0;\n \t\tlk->owner = 0;\n \t\tstrbuf_init(&lk->filename, pathlen + LOCK_SUFFIX_LEN);\n@@ -214,6 +224,17 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \treturn fd;\n }\n \n+FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)\n+{\n+\tif (!lk->active)\n+\t\tdie(\"BUG: fdopen_lock_file() called for unlocked object\");\n+\tif (lk->fp)\n+\t\tdie(\"BUG: fdopen_lock_file() called twice for file '%s'\", lk->filename.buf);\n+\n+\tlk->fp = fdopen(lk->fd, mode);\n+\treturn lk->fp;\n+}\n+\n char *get_locked_file_path(struct lock_file *lk)\n {\n \tif (!lk->active)\n@@ -226,17 +247,32 @@ char *get_locked_file_path(struct lock_file *lk)\n int close_lock_file(struct lock_file *lk)\n {\n \tint fd = lk->fd;\n+\tFILE *fp = lk->fp;\n+\tint err;\n \n \tif (fd < 0)\n \t\treturn 0;\n \n \tlk->fd = -1;\n-\tif (close(fd)) {\n+\tif (fp) {\n+\t\tlk->fp = NULL;\n+\n+\t\t/*\n+\t\t * Note: no short-circuiting here; we want to fclose()\n+\t\t * in any case!\n+\t\t */\n+\t\terr = ferror(fp) | fclose(fp);\n+\t} else {\n+\t\terr = close(fd);\n+\t}\n+\n+\tif (err) {\n \t\tint save_errno = errno;\n \t\trollback_lock_file(lk);\n \t\terrno = save_errno;\n \t\treturn -1;\n \t}\n+\n \treturn 0;\n }\n \ndiff --git a/lockfile.h b/lockfile.h\nindex 9059e89..dc066d1 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -34,6 +34,8 @@\n  *   - active is set\n  *   - filename holds the filename of the lockfile\n  *   - fd holds a file descriptor open for writing to the lockfile\n+ *   - fp holds a pointer to an open FILE object if and only if\n+ *     fdopen_lock_file() has been called on the object\n  *   - owner holds the PID of the process that locked the file\n  *\n  * - Locked, lockfile closed (after successful close_lock_file()).\n@@ -56,6 +58,7 @@ struct lock_file {\n \tstruct lock_file *volatile next;\n \tvolatile sig_atomic_t active;\n \tvolatile int fd;\n+\tFILE *volatile fp;\n \tvolatile pid_t owner;\n \tchar on_list;\n \tstruct strbuf filename;\n@@ -74,6 +77,7 @@ extern void unable_to_lock_message(const char *path, int err,\n extern NORETURN void unable_to_lock_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n+extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);\n extern char *get_locked_file_path(struct lock_file *);\n extern int commit_lock_file_to(struct lock_file *, const char *path);\n extern int commit_lock_file(struct lock_file *);\n-- \n2.1.0\n"},{"id":"250106","messageId":"1412162089-3233-3-git-send-email-mhagger@alum.mit.edu","threadId":"37658","inReplyTo":"1412162089-3233-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/3] dump_marks(): reimplement using fdopen_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-01T11:14:48Z","receivedAt":"2014-10-01T11:14:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n fast-import.c | 21 ++-------------------\n 1 file changed, 2 insertions(+), 19 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex deadc33..fee7906 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1794,20 +1794,18 @@ static void dump_marks_helper(FILE *f,\n static void dump_marks(void)\n {\n \tstatic struct lock_file mark_lock;\n-\tint mark_fd;\n \tFILE *f;\n \n \tif (!export_marks_file)\n \t\treturn;\n \n-\tmark_fd = hold_lock_file_for_update(&mark_lock, export_marks_file, 0);\n-\tif (mark_fd < 0) {\n+\tif (hold_lock_file_for_update(&mark_lock, export_marks_file, 0) < 0) {\n \t\tfailure |= error(\"Unable to write marks file %s: %s\",\n \t\t\texport_marks_file, strerror(errno));\n \t\treturn;\n \t}\n \n-\tf = fdopen(mark_fd, \"w\");\n+\tf = fdopen_lock_file(&mark_lock, \"w\");\n \tif (!f) {\n \t\tint saved_errno = errno;\n \t\trollback_lock_file(&mark_lock);\n@@ -1816,22 +1814,7 @@ static void dump_marks(void)\n \t\treturn;\n \t}\n \n-\t/*\n-\t * Since the lock file was fdopen()'ed, it should not be close()'ed.\n-\t * Assign -1 to the lock file descriptor so that commit_lock_file()\n-\t * won't try to close() it.\n-\t */\n-\tmark_lock.fd = -1;\n-\n \tdump_marks_helper(f, 0, marks);\n-\tif (ferror(f) || fclose(f)) {\n-\t\tint saved_errno = errno;\n-\t\trollback_lock_file(&mark_lock);\n-\t\tfailure |= error(\"Unable to write marks file %s: %s\",\n-\t\t\texport_marks_file, strerror(saved_errno));\n-\t\treturn;\n-\t}\n-\n \tif (commit_lock_file(&mark_lock)) {\n \t\tfailure |= error(\"Unable to commit marks file %s: %s\",\n \t\t\texport_marks_file, strerror(errno));\n-- \n2.1.0\n"},{"id":"250107","messageId":"1412162089-3233-4-git-send-email-mhagger@alum.mit.edu","threadId":"37658","inReplyTo":"1412162089-3233-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/3] commit_packed_refs(): reimplement using fdopen_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-01T11:14:49Z","receivedAt":"2014-10-01T11:14:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1d73f1d..a77458f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2309,16 +2309,13 @@ int commit_packed_refs(void)\n \tif (!packed_ref_cache->lock)\n \t\tdie(\"internal error: packed-refs not locked\");\n \n-\tout = fdopen(packed_ref_cache->lock->fd, \"w\");\n+\tout = fdopen_lock_file(packed_ref_cache->lock, \"w\");\n \tif (!out)\n \t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n \n \tfprintf_or_die(out, \"%s\", PACKED_REFS_HEADER);\n \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n \t\t\t\t 0, write_packed_entry_fn, out);\n-\tif (fclose(out))\n-\t\tdie_errno(\"write error\");\n-\tpacked_ref_cache->lock->fd = -1;\n \n \tif (commit_lock_file(packed_ref_cache->lock)) {\n \t\tsave_errno = errno;\n-- \n2.1.0\n"},{"id":"250112","messageId":"20141001124847.GB10772@peff.net","threadId":"37658","inReplyTo":"1412162089-3233-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/3] fdopen_lock_file(): access a lockfile using stdio","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-01T12:48:47Z","receivedAt":"2014-10-01T12:48:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 01, 2014 at 01:14:47PM +0200, Michael Haggerty wrote:\n\n> I thought about adding second stdio-oriented entrance points analogous\n> to hold_lock_file_for_update(), hold_lock_file_for_append(), and\n> reopen_lock_file(), but it seemed simpler to add just the one new\n> function instead of three or four. If using stdio on lockfiles becomes\n> more popular, we might want to add some helper functions to make it a\n> bit more convenient.\n\nI think it makes sense to wait until we see more potential callers crop\nup.\n\n> In close_lock_file(), if ferror() returns an error, then errno is not\n> necessarily still set in a way that reflects the original error. I\n> don't see a way to ensure that errno is set correctly in this\n> situation. But hopefully, callers are monitoring their calls to\n> fwrite()/fprintf() etc and will have noticed write errors on their own\n> already. If anybody can suggest an improvement here, please let me\n> know.\n\nI was careful in the packed-refs stdio caller to check all of my fprintf\ncalls (because I was using fclose myself). I wonder if it would be nicer\nto back off from that and just depend on the ferror() call at\ncommit-time. The exact value of errno is not usually that important\nafter the open() has succeeded.\n\n> -static void remove_lock_files(void)\n> +static void remove_lock_files(int skip_fclose)\n>  {\n>  \tpid_t me = getpid();\n>  \n>  \twhile (lock_file_list) {\n> -\t\tif (lock_file_list->owner == me)\n> +\t\tif (lock_file_list->owner == me) {\n> +\t\t\t/* fclose() is not safe to call in a signal handler */\n> +\t\t\tif (skip_fclose)\n> +\t\t\t\tlock_file_list->fp = NULL;\n\nI wondered when reading the commit message if it should mention this\nsignal-handling case (which you brought up in the cover letter). This\ncomment is probably enough, though.\n\n> +FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)\n> +{\n> +\tif (!lk->active)\n> +\t\tdie(\"BUG: fdopen_lock_file() called for unlocked object\");\n> +\tif (lk->fp)\n> +\t\tdie(\"BUG: fdopen_lock_file() called twice for file '%s'\", lk->filename.buf);\n\nI would have expected calling this twice to be a noop (i.e., make the\nfunction more \"give me the associated filehandle, and create one if\nnecessary\"). But I don't think any current callers should need that, so\nit probably makes sense to play it safe and die(\"BUG\"), at least for\nnow.\n\n> +\tif (fp) {\n> +\t\tlk->fp = NULL;\n> +\n> +\t\t/*\n> +\t\t * Note: no short-circuiting here; we want to fclose()\n> +\t\t * in any case!\n> +\t\t */\n> +\t\terr = ferror(fp) | fclose(fp);\n\nWould this be more clear as:\n\n\terr = ferror(fp);\n\terr |= fclose(fp);\n\n-Peff\n"},{"id":"250113","messageId":"20141001125214.GC10772@peff.net","threadId":"37658","inReplyTo":"1412162089-3233-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/3] Support stdio access to lockfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-01T12:52:15Z","receivedAt":"2014-10-01T12:52:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 01, 2014 at 01:14:46PM +0200, Michael Haggerty wrote:\n\n> This series applies on top of the series \"Lockfile correctness and\n> refactoring\" (Junio's branch mh/lockfile).\n> \n> There are already two callers that write to lockfiles using stdio. But\n> they currently need intimate knowledge of the lockfile implementation\n> to work correctly; for example, they have to call fclose() themselves\n> and set lk->fd to -1 to prevent the file from being closed again. This\n> is awkward and error-prone.\n\nI think it's also wrong on systems where you cannot delete an open file.\nA signal or atexit handler will not be able to close the file (since it\ndoes not know the fd), and the unlink() will fail. IIRC, either cygwin\nor msysgit (or both?) is such a platform.\n\n> Michael Haggerty (3):\n>   fdopen_lock_file(): access a lockfile using stdio\n>   dump_marks(): reimplement using fdopen_lock_file()\n>   commit_packed_refs(): reimplement using fdopen_lock_file()\n\nI had a few minor comments on the first patch, but I would also be OK\nwith it going in as-is. The other two looked fine to me. Thanks for\nworking on this.\n\n-Peff\n"},{"id":"250130","messageId":"xmqq8ukzpjw1.fsf@gitster.dls.corp.google.com","threadId":"37658","inReplyTo":"20141001124847.GB10772@peff.net","subject":"Re: [PATCH 1/3] fdopen_lock_file(): access a lockfile using stdio","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-01T21:20:14Z","receivedAt":"2014-10-01T21:20:14Z","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> On Wed, Oct 01, 2014 at 01:14:47PM +0200, Michael Haggerty wrote:\n>\n>> ... If using stdio on lockfiles becomes\n>> more popular, we might want to add some helper functions to make it a\n>> bit more convenient.\n>\n> I think it makes sense to wait until we see more potential callers crop\n> up.\n\nYup.\n\n>> In close_lock_file(), if ferror() returns an error, then errno is not\n>> necessarily still set in a way that reflects the original error. I\n>> don't see a way to ensure that errno is set correctly in this\n>> situation. But hopefully, callers are monitoring their calls to\n>> fwrite()/fprintf() etc and will have noticed write errors on their own\n>> already. If anybody can suggest an improvement here, please let me\n>> know.\n>\n> I was careful in the packed-refs stdio caller to check all of my fprintf\n> calls (because I was using fclose myself). I wonder if it would be nicer\n> to back off from that and just depend on the ferror() call at\n> commit-time.\n\nThat's a thought (I think the same can be said for \"close-time\").\n\n>> -static void remove_lock_files(void)\n>> +static void remove_lock_files(int skip_fclose)\n>>  {\n>>  \tpid_t me = getpid();\n>>  \n>>  \twhile (lock_file_list) {\n>> -\t\tif (lock_file_list->owner == me)\n>> +\t\tif (lock_file_list->owner == me) {\n>> +\t\t\t/* fclose() is not safe to call in a signal handler */\n>> +\t\t\tif (skip_fclose)\n>> +\t\t\t\tlock_file_list->fp = NULL;\n>\n> I wondered when reading the commit message if it should mention this\n> signal-handling case (which you brought up in the cover letter). This\n> comment is probably enough, though.\n\nNo strong opinion either way.\n\n>> +FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)\n>> +{\n>> +\tif (!lk->active)\n>> +\t\tdie(\"BUG: fdopen_lock_file() called for unlocked object\");\n>> +\tif (lk->fp)\n>> +\t\tdie(\"BUG: fdopen_lock_file() called twice for file '%s'\", lk->filename.buf);\n>\n> I would have expected calling this twice to be a noop (i.e., make the\n> function more \"give me the associated filehandle, and create one if\n> necessary\"). But I don't think any current callers should need that, so\n> it probably makes sense to play it safe and die(\"BUG\"), at least for\n> now.\n\nInteresting.  One could imagine a sane call-chain whose top-level\ncreates a lockfile, associates a FILE * with it to write into it\nitself, then calls set of helpers.  You could pass only FILE * to\nsuch helpers that does nothing other than writing to lk->fp to the\nlockfile, but it would be cumbersome if a helper wants to have\naccess to the lockfile itself and FILE * (i.e. it writes and then\neither commits or rolls back; name it foo_finish() or something).\n\nSuch a call-chain certainly would want a way to ask \"I know this lk\nis already associated with a FILE *; give me that\".  But that still\ndoes not require \"I do not know if this lk already has FILE * or\nnot, but I want a FILE * associated with it now.  Peek or create.\"\n\nSo I think this is OK.\n\n>> +\tif (fp) {\n>> +\t\tlk->fp = NULL;\n>> +\n>> +\t\t/*\n>> +\t\t * Note: no short-circuiting here; we want to fclose()\n>> +\t\t * in any case!\n>> +\t\t */\n>> +\t\terr = ferror(fp) | fclose(fp);\n>\n> Would this be more clear as:\n>\n> \terr = ferror(fp);\n> \terr |= fclose(fp);\n\nNo strong opinion either way.\n\nThanks, both.\n"},{"id":"250159","messageId":"542D1AF2.8050508@web.de","threadId":"37658","inReplyTo":"1412162089-3233-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/3] fdopen_lock_file(): access a lockfile using stdio","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-10-02T09:29:22Z","receivedAt":"2014-10-02T09:29:22Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\nOn 01.10.14 13:14, Michael Haggerty wrote:\n[]\nNice done, small comments inline\n> diff --git a/lockfile.c b/lockfile.c\n> index d27e61c..e046027 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -7,20 +7,29 @@\n>  \n>  static struct lock_file *volatile lock_file_list;\n>  \n> -static void remove_lock_files(void)\n> +static void remove_lock_files(int skip_fclose)\nEven if the motivation to skip is clear now and here,\nI would consider to do it the other way around,\nand actively order the fclose():\n\nstatic void remove_lock_files(int call_fclose)\n\n\n>  {\n>  \tpid_t me = getpid();\n>  \n>  \twhile (lock_file_list) {\n> -\t\tif (lock_file_list->owner == me)\n> +\t\tif (lock_file_list->owner == me) {\n> +\t\t\t/* fclose() is not safe to call in a signal handler */\n> +\t\t\tif (skip_fclose)\n> +\t\t\t\tlock_file_list->fp = NULL;\n>  \t\t\trollback_lock_file(lock_file_list);\n> +\t\t}\n>  \t\tlock_file_list = lock_file_list->next;\n>  \t}\n>  }\n>  \n> +static void remove_lock_files_on_exit(void)\n> +{\n> +\tremove_lock_files(0);\nWhat does \"0\" mean ?\n\nremove_lock_files(LK_DO_FCLOSE) ?\n\n> +}\n> +\n>  static void remove_lock_files_on_signal(int signo)\n>  {\n> -\tremove_lock_files();\n> +\tremove_lock_files(1);\nAnd what does this \"1\" mean ?\n\nremove_lock_files(LK_SKIP_FCLOSE) ?\n\nWe can even have an emum, or use #define\n>\n"},{"id":"250527","messageId":"543A1CEF.7020700@alum.mit.edu","threadId":"37658","inReplyTo":"542D1AF2.8050508@web.de","subject":"Re: [PATCH 1/3] fdopen_lock_file(): access a lockfile using stdio","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-12T06:17:19Z","receivedAt":"2014-10-12T06:17:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Sorry, I see I never responded to this email.\n\nOn 10/02/2014 11:29 AM, Torsten Bögershausen wrote:\n> On 01.10.14 13:14, Michael Haggerty wrote:\n> []\n> Nice done, small comments inline\n>> diff --git a/lockfile.c b/lockfile.c\n>> index d27e61c..e046027 100644\n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -7,20 +7,29 @@\n>>  \n>>  static struct lock_file *volatile lock_file_list;\n>>  \n>> -static void remove_lock_files(void)\n>> +static void remove_lock_files(int skip_fclose)\n> Even if the motivation to skip is clear now and here,\n> I would consider to do it the other way around,\n> and actively order the fclose():\n> \n> static void remove_lock_files(int call_fclose)\n\nI don't think inverting the logic will help the reader remember the\nmotivation for skipping the call to fclose(). I think this way was\nclearer because skipping the call to fclose() is the \"unusual\" case; it\nhas to actively sabotage the fclose() that would otherwise take place in\nrollback_lock_file(). Also, \"call_fclose\" slightly implies that fclose()\nwill be called for the lockfiles, whereas in fact it will only be called\nfor the lockfiles for which fdopen_lock_file() has been called.\n\n>>  {\n>>  \tpid_t me = getpid();\n>>  \n>>  \twhile (lock_file_list) {\n>> -\t\tif (lock_file_list->owner == me)\n>> +\t\tif (lock_file_list->owner == me) {\n>> +\t\t\t/* fclose() is not safe to call in a signal handler */\n>> +\t\t\tif (skip_fclose)\n>> +\t\t\t\tlock_file_list->fp = NULL;\n>>  \t\t\trollback_lock_file(lock_file_list);\n>> +\t\t}\n>>  \t\tlock_file_list = lock_file_list->next;\n>>  \t}\n>>  }\n>>  \n>> +static void remove_lock_files_on_exit(void)\n>> +{\n>> +\tremove_lock_files(0);\n> What does \"0\" mean ?\n> \n> remove_lock_files(LK_DO_FCLOSE) ?\n> \n>> +}\n>> +\n>>  static void remove_lock_files_on_signal(int signo)\n>>  {\n>> -\tremove_lock_files();\n>> +\tremove_lock_files(1);\n> And what does this \"1\" mean ?\n> \n> remove_lock_files(LK_SKIP_FCLOSE) ?\n> \n> We can even have an emum, or use #define\n\nMeh. These are private functions, all defined within a few lines of each\nother. I think that an enum would be overkill here when a \"boolean\"\nsuffices.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}