git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 00/26] inotify support

From
Torsten Bögershausen <tboegi@web.de>
Date
Feb 8, 2014, 08:04 UTC
Message-ID
<52F5E521.4090707@web.de>
In-Reply-To
<1391401754-15347-1-git-send-email-pclouds@gmail.com>
On 03.02.14 05:28, Nguyễn Thái Ngọc Duy wrote:

I managed to review the code 0..12/26, so some parts are missing. The list below became longer than what I intended, my comments may be hard to read, and there is a mixture of minor and major remarks.

I would appreciate if we could have an outline of the protocol as a seperate "document" somewhere, to be able to have a look at the protocol first, before looking into the code.

(Am I using wireshark too much to dream about a dissector ?)
All in all I like the concept, thanks for the work.
1)
  write_in_full_timeout()
  packet_read_line_timeout()
  At other places we handle EINTR after calling poll().
  Looking at the code, it could be easier to introduce
  a new function xpoll() in wrapper.c, and use that instead
  of poll().
2)
  Similar for the usage of accept().
  I like the idea of xread() xwrite() and all the x functions
  so it coud make sense to establish xpoll() and xaccept()
  before inotify suppport.
3)
Show 12 quoted lines
> -int unix_stream_listen(const char *path)
> +int unix_stream_listen(const char *path, int replace)
>  {
>  	int fd, saved_errno;
>  	struct sockaddr_un sa;
> @@ -103,7 +103,8 @@ int unix_stream_listen(const char *path)
>  		return -1;
>  	fd = unix_stream_socket();
>  
> -	unlink(path);
> +	if (replace)
> +		unlink(path);

Minor remark: As we do not do the replace here: s/replace/un_link/ may be ?

4)
> +++ b/file-watcher.c
{}
> +static const char *const file_watcher_usage[] = {
> +	N_("git file-watcher [options] <socket directory>"),
> +	NULL
> +};
Do we already have options here?
I can think about having 
-d daemoniye
-u uses Unix doain socket
(And later -t to use a TCP/IP socket, when the repo
 is on a mounted NFS (or SAMBA) drive, and  the daemon is on a 
 different machine.
 I don't say this patch should include this logic in first round,
 But I can see a gain for this kind of setup)
5)
> +++ b/file-watcher.c
[]
Show 11 quoted lines
> +static int shutdown_connection(int id)
> +{
> +	struct connection *conn = conns[id];
> +	conns[id] = NULL;
> +	pfd[id].fd = -1;
> +	if (!conn)
> +		return 0;
> +	close(conn->sock);
> +	free(conn);
> +	return 0;
> +}

The function is called shutdown_connection(), but it does a close() Could it be named close_connection() ?

6) 
> +++ b/file-watcher.c

[] Do we have a sequence chart about the command flow between the watcher daemon and the client ?

------------ 7)

in 03/26:
>This version is also gentler than its friend packet_read_line()
gentler, what does this mean?
>because it's designed for side channel I/O that should not abort the
>program even if the channel is broken.
I'm not so familar with side-channel I/O. How does it fit in here?

Does this make sense: In opposite to packet_read_line() which can call die() to abort the program, read_in_full_timeout() will keep the program running. (or something like this)

Show 30 quoted lines
>
>Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>---
> cache.h    |  1 +
> pkt-line.c | 35 +++++++++++++++++++++++++++++++++++
> pkt-line.h |  1 +
> wrapper.c  | 21 +++++++++++++++++++++
> 4 files changed, 58 insertions(+)
>
>diff --git a/cache.h b/cache.h
>index 718e32b..939db46 100644
>--- a/cache.h
>+++ b/cache.h
>@@ -1230,6 +1230,7 @@ extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char
> extern void fsync_or_die(int fd, const char *);
> 
> extern ssize_t read_in_full(int fd, void *buf, size_t count);
>+extern ssize_t read_in_full_timeout(int fd, void *buf, size_t count, int timeout);
> extern ssize_t write_in_full(int fd, const void *buf, size_t count);
> extern ssize_t write_in_full_timeout(int fd, const void *buf, size_t count, int timeout);
> static inline ssize_t write_str_in_full(int fd, const char *str)
>diff --git a/pkt-line.c b/pkt-line.c
>index cf681e9..5a07e97 100644
>--- a/pkt-line.c
>+++ b/pkt-line.c
>@@ -229,3 +229,38 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
> {
> 	return packet_read_line_generic(-1, src, src_len, dst_len);
> }
>+

In what is the timeout measured ? seconds, milli years? As we use poll() I think it is milli seconds. (I like the idea of naming timeout timeout_ms)

[]
Show 7 quoted lines
>+	len -= 4;
>+	if (len >= buf_len) {
>+		error("protocol error: bad line length %d", len);
>+		return NULL;
>+	}
>+	if ((ret = read_in_full_timeout(fd, buf, len, timeout)) < 0)
>+		return NULL;
Do we want a packet_trace here?

When a timeout occurs, do we want to close the connection, marking it as dead? Or need to look at errno?

Show 6 quoted lines
>+	buf[len] = '\0';
>+	if (len_p)
>+		*len_p = len;
>+	packet_trace(buf, len, 0);
>+	return buf;
>+}
Show 39 quoted lines
>diff --git a/pkt-line.h b/pkt-line.h
>index 4b93a0c..d47dca5 100644
>--- a/pkt-line.h
>+++ b/pkt-line.h
>@@ -69,6 +69,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char
>  * packet is written to it.
>  */
> char *packet_read_line(int fd, int *size);
>+char *packet_read_line_timeout(int fd, int timeout, int *size);
> 
> /*
>  * Same as packet_read_line, but read from a buf rather than a descriptor;
>diff --git a/wrapper.c b/wrapper.c
>index 9a0e289..9cf10b2 100644
>--- a/wrapper.c
>+++ b/wrapper.c
>@@ -193,6 +193,27 @@ ssize_t read_in_full(int fd, void *buf, size_t count)
> 	return total;
> }
> 
>+ssize_t read_in_full_timeout(int fd, void *buf, size_t count, int timeout)
>+{
>+	char *p = buf;
>+	ssize_t total = 0;
>+	struct pollfd pfd;
>+
>+	pfd.fd = fd;
>+	pfd.events = POLLIN;
>+	while (count > 0 && poll(&pfd, 1, timeout) > 0 &&
>+	       (pfd.revents & POLLIN)) {
>+		ssize_t loaded = xread(fd, p, count);
>+		if (loaded <= 0)
>+			return -1;
>+		count -= loaded;
>+		p += loaded;
>+		total += loaded;
>+	}
>+
>+	return count ? -1 : total;

Isn't it that ret < 0 means "error of some kind" ret == 0 means EOF, ret > 0 means some data Why do we turn 0 into -1?

-------------- 8)

+++ b/unix-socket.c
@@ -93,7 +93,7 @@ fail:
 	return -1;
 }
 
-int unix_stream_listen(const char *path)
+int unix_stream_listen(const char *path, int replace)
 {
 	int fd, saved_errno;
 	struct sockaddr_un sa;
@@ -103,7 +103,8 @@ int unix_stream_listen(const char *path)
 		return -1;
 	fd = unix_stream_socket();
 
-	unlink(path);
+	if (replace)
+		unlink(path);
 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
Why do we call the parameter replace, when it does an
unlink() ?
s/replace/un_link/ ? 


9)
>Access to the unix socket $WATCHER/socket is covered by $WATCHER's
>permission. While the file watcher does not carry much information,
>repo file listing is sometimes not something you want other users to
>see. Make sure $WATCHER has 0700 permission to stop unwanted access.

>Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>---
> file-watcher.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)

I feel a little bit unsure about the 700.
Most often Git does not care about permissions,
and relies on umask being set appropriatly.
(Please correct me if I'm wrong)

My spontanous feeling is that adjust_shared_perm() could be used.

10)
An other thing:
>strbuf_addf(&sb, "%s/socket", socket_path);
Does it make sense to name the socket "%s/watcher" ?


11)
>In daemon mode, stdout and stderr are saved in $WATCHER/log.

[]
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-file-watcher.txt |  2 ++
 cache.h                            |  1 +
 daemon.c                           | 30 ++++--------------------------
 file-watcher.c                     | 17 +++++++++++++++++
 setup.c                            | 25 +++++++++++++++++++++++++
 5 files changed, 49 insertions(+), 26 deletions(-)

diff --git a/Documentation/git-file-watcher.txt b/Documentation/git-file-watcher.txt
index 625a389..ec81f18 100644
--- a/Documentation/git-file-watcher.txt
+++ b/Documentation/git-file-watcher.txt
@@ -18,6 +18,8 @@ lstat(2) to detect that itself.
 
 OPTIONS
 -------
+--detach::
+	Run in background.

Shouldn't that be named --daemon ?


12)
>read_cache() connects to the file watcher, specified by
>filewatcher.path config, and performs basic hand shaking. CE_WATCHED
>is cleared if git and file watcher have different views on the index
>state.
>
>All send/receive calls must be complete within a limited time to avoid
>a buggy file-watcher hang "git status" forever. And the whole point of
>doing this is speed. If file watcher can't respond fast enough, for
>whatever reason, then it should not be used.
>
>Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>---
> Documentation/config.txt           |  10 +++
> Documentation/git-file-watcher.txt |   4 +-
> Makefile                           |   1 +
> cache.h                            |   1 +
> file-watcher-lib.c (new)           |  91 ++++++++++++++++++++++
> file-watcher-lib.h (new)           |   6 ++
> file-watcher.c                     | 152 ++++++++++++++++++++++++++++++++++++-
> read-cache.c                       |   6 ++
> 8 files changed, 269 insertions(+), 2 deletions(-)
> create mode 100644 file-watcher-lib.c
> create mode 100644 file-watcher-lib.h
>
>diff --git a/Documentation/config.txt b/Documentation/config.txt
>index 5f4d793..6ad653a 100644
>--- a/Documentation/config.txt
>+++ b/Documentation/config.txt
>@@ -1042,6 +1042,16 @@ difftool.<tool>.cmd::
> difftool.prompt::
> 	Prompt before each invocation of the diff tool.
> 
>+filewatcher.path::
>+	The directory that contains the socket of `git	file-watcher`.
>+	If it's not an absolute path, it's relative to $GIT_DIR. An
>+	empty path means no connection to file watcher.
I would really like to be able to have different tranport mechanismen
using the same config.
Today this is only a matter of documentation and naming things:

How about this idea:
filewatcher.url::
The directory that contains the socket of `git	file-watcher`.
It can be an absolute path, meaning a unix domain socket in the
file system.
It can be a path relative to $GIT_DIR.

It must start with either 
"/" (absolut path) or 
"./" (relative path to $GIT_DIR) or 
"~/" (relaive path in your $HOME directoy.
An empty url means no connection to file watcher.
(Later we can add url schemes like "tcp://" or "pipe://")


>+
>+filewatcher.timeout::
>+	This is the maximum time in milliseconds that Git waits for
>+	the file watcher to respond before giving up. Default value is
>+	50. Setting to -1 makes Git wait forever.
50 feels low, especially on "older/slower machines"
200 is probably acceptable even for the very impatient user,
I could think of 500 to make the user aware of it.

[]
>+	/*
>+	 * ">" denotes an incoming packet, "<" outgoing. The lack of
>+	 * "<" means no reply expected.
>+	 *
>+	 * < "error" SP ERROR-STRING
>+	 *
>+	 * This can be sent whenever the client violates the protocol.
>+	 */
>+
>+	msg = packet_read_line(fd, &len);
>+	if (!msg) {
>+		packet_write(fd, "error invalid pkt-line");
>+		return shutdown_connection(conn_id);
>+	}
>+
>+	/*
>+	 * > "hello" [SP CAP [SP CAP..]]
>+	 * < "hello" [SP CAP [SP CAP..]]
>+	 *
>+	 * Advertise capabilities of both sides. File watcher may
>+	 * disconnect if the client does not advertise the required
>+	 * capabilities. Capabilities in uppercase MUST be
>+	 * supported. If any side does not understand any of the
>+	 * advertised uppercase capabilities, it must disconnect.
>+	 */
>+	if ((arg = skip_prefix(msg, "hello"))) {
>+		if (*arg) {	/* no capabilities supported yet */
>+			packet_write(fd, "error capabilities not supported");
>+			return shutdown_connection(conn_id);
>+		}
>+		packet_write(fd, "hello");
>+		conns[conn_id]->polite = 1;
>+	}
>+
>+	/*
>+	 * > "index" SP INDEX-SIGNATURE SP WORK-TREE-PATH
>+	 * < "ok" | "inconsistent"
>+	 *
>+	 * INDEX-SIGNATURE consists of 40 hexadecimal letters
>+	 * WORK-TREE-PATH must be absolute and normalized path
>+	 *
>+	 * Watch file changes in index. The client sends the index and
>+	 * work tree info. File watcher validates that it holds the
>+	 * same info. If so it sends "ok" back indicating both sides
>+	 * are on the same page and CE_WATCHED bits can be ketpt.
>+	 *
>+	 * Otherwise it sends "inconsistent" and both sides must reset
>+	 * back to initial state. File watcher keeps its index
>+	 * signature all-zero until the client has updated the index
>+	 * ondisk and request to update index signature.
>+	 *
>+	 * "hello" must be exchanged first. After this command the
>+	 * connection is associated with a worktree/index. Many
>+	 * commands may require this to proceed.
>+	 */
>+	else if (starts_with(msg, "index ")) {
>+		struct repository *repo;
>+		struct stat st;
>+		if (!conns[conn_id]->polite) {
>+			packet_write(fd, "error why did you not greet me? go away");
>+			return shutdown_connection(conn_id);
>+		}
>+		if (len < 47 || msg[46] != ' ' || !is_absolute_path(msg + 47)) {
>+			packet_write(fd, "error invalid index line %s", msg);
>+			return shutdown_connection(conn_id);
>+		}
>+
>+		if (lstat(msg + 47, &st) || !S_ISDIR(st.st_mode)) {
>+			packet_write(fd, "error work tree does not exist: %s",
>+				     strerror(errno));
>+			return shutdown_connection(conn_id);
>+		}
>+		repo = get_repo(msg + 47);
>+		conns[conn_id]->repo = repo;
>+		if (memcmp(msg + 6, repo->index_signature, 40) ||
>+		    !memcmp(msg + 6, invalid_signature, 40) ||
>+		    repo->inode != st.st_ino) {
>+			packet_write(fd, "inconsistent");
>+			reset_repo(repo, st.st_ino);
>+			return 0;
>+		}
>+		packet_write(fd, "ok");
>+	}
>+	else {
>+		packet_write(fd, "error unrecognized command %s", msg);
>+		return shutdown_connection(conn_id);
I feel a little bit unsure about this.
We establish a protocol which is not extendable.
Do we need to call shutdown_connection() ?
(And as I noticed earlier, close_connection() could be a better name)
Previous: Nguyễn Thái Ngọc DuyNext: Duy Nguyen
Message 64 of 72 in “inotify support”
  1. 0/6 inotify supportNguyễn Thái Ngọc Duy, Jan 12, 2014
  2. 1/6 read-cache: save trailing sha-1Nguyễn Thái Ngọc Duy, Jan 12, 2014
  3. 2/6 read-cache: new extension to mark what file is watchedNguyễn Thái Ngọc Duy, Jan 12, 2014
  4. Jonathan NiederJan 13, 2014
  5. Duy NguyenJan 14, 2014
  6. Duy NguyenJan 14, 2014
  7. 3/6 read-cache: connect to file watcherNguyễn Thái Ngọc Duy, Jan 12, 2014
  8. Jeff KingJan 15, 2014
  9. 4/6 read-cache: get "updated" path list from file watcherNguyễn Thái Ngọc Duy, Jan 12, 2014
  10. 5/6 read-cache: ask file watcher to watch filesNguyễn Thái Ngọc Duy, Jan 12, 2014
  11. 6/6 file-watcher: support inotifyNguyễn Thái Ngọc Duy, Jan 12, 2014
  12. 00/14 inotify supportNguyễn Thái Ngọc Duy, Jan 17, 2014
  13. 01/14 read-cache: save trailing sha-1Nguyễn Thái Ngọc Duy, Jan 17, 2014
  14. 02/14 read-cache: new extension to mark what file is watchedNguyễn Thái Ngọc Duy, Jan 17, 2014
  15. Thomas GummererJan 17, 2014
  16. Thomas RastJan 19, 2014
  17. Duy NguyenJan 20, 2014
  18. 03/14 read-cache: connect to file watcherNguyễn Thái Ngọc Duy, Jan 17, 2014
  19. Torsten BögershausenJan 17, 2014
  20. Duy NguyenJan 17, 2014
  21. 04/14 read-cache: ask file watcher to watch filesNguyễn Thái Ngọc Duy, Jan 17, 2014
  22. 05/14 read-cache: put some limits on file watchingNguyễn Thái Ngọc Duy, Jan 17, 2014
  23. Thomas RastJan 19, 2014
  24. Duy NguyenJan 20, 2014
  25. 06/14 read-cache: get modified file list from file watcherNguyễn Thái Ngọc Duy, Jan 17, 2014
  26. 07/14 read-cache: add config to start file watcher automaticallyNguyễn Thái Ngọc Duy, Jan 17, 2014
  27. 08/14 read-cache: add GIT_TEST_FORCE_WATCHER for testingNguyễn Thái Ngọc Duy, Jan 17, 2014
  28. Thomas RastJan 19, 2014
  29. Duy NguyenJan 20, 2014
  30. 09/14 file-watcher: add --shutdown and --log optionsNguyễn Thái Ngọc Duy, Jan 17, 2014
  31. 10/14 file-watcher: automatically quitNguyễn Thái Ngọc Duy, Jan 17, 2014
  32. 11/14 file-watcher: support inotifyNguyễn Thái Ngọc Duy, Jan 17, 2014
  33. Thomas RastJan 19, 2014
  34. Duy NguyenJan 20, 2014
  35. Thomas RastJan 20, 2014
  36. Duy NguyenJan 28, 2014
  37. 00/26 inotify supportNguyễn Thái Ngọc Duy, Feb 3, 2014
  38. 01/26 pkt-line.c: rename global variable buffer[] to something less genericNguyễn Thái Ngọc Duy, Feb 3, 2014
  39. 02/26 pkt-line.c: add packet_write_timeout()Nguyễn Thái Ngọc Duy, Feb 3, 2014
  40. 03/26 pkt-line.c: add packet_read_line_timeout()Nguyễn Thái Ngọc Duy, Feb 3, 2014
  41. 04/26 unix-socket: make unlink() optional in unix_stream_listen()Nguyễn Thái Ngọc Duy, Feb 3, 2014
  42. 05/26 Add git-file-watcher and basic connection handling logicNguyễn Thái Ngọc Duy, Feb 3, 2014
  43. 06/26 file-watcher: check socket directory permissionNguyễn Thái Ngọc Duy, Feb 3, 2014
  44. 07/26 file-watcher: remove socket on exitNguyễn Thái Ngọc Duy, Feb 3, 2014
  45. 08/26 file-watcher: add --detachNguyễn Thái Ngọc Duy, Feb 3, 2014
  46. 09/26 read-cache: save trailing sha-1Nguyễn Thái Ngọc Duy, Feb 3, 2014
  47. 10/26 read-cache: new flag CE_WATCHED to mark what file is watchedNguyễn Thái Ngọc Duy, Feb 3, 2014
  48. 11/26 Clear CE_WATCHED when set CE_VALID aloneNguyễn Thái Ngọc Duy, Feb 3, 2014
  49. 12/26 read-cache: basic hand shaking to the file watcherNguyễn Thái Ngọc Duy, Feb 3, 2014
  50. 13/26 read-cache: ask file watcher to watch filesNguyễn Thái Ngọc Duy, Feb 3, 2014
  51. 14/26 read-cache: put some limits on file watchingNguyễn Thái Ngọc Duy, Feb 3, 2014
  52. 15/26 read-cache: get changed file list from file watcherNguyễn Thái Ngọc Duy, Feb 3, 2014
  53. 16/26 git-compat-util.h: add inotify stubs on non-Linux platformsNguyễn Thái Ngọc Duy, Feb 3, 2014
  54. 17/26 file-watcher: inotify support, watching partNguyễn Thái Ngọc Duy, Feb 3, 2014
  55. 18/26 file-watcher: inotify support, notification partNguyễn Thái Ngọc Duy, Feb 3, 2014
  56. 19/26 Wrap CE_VALID test with ce_valid()Nguyễn Thái Ngọc Duy, Feb 3, 2014
  57. 20/26 read-cache: new variable to verify file-watcher resultsNguyễn Thái Ngọc Duy, Feb 3, 2014
  58. 21/26 Support running file watcher with the test suiteNguyễn Thái Ngọc Duy, Feb 3, 2014
  59. 22/26 file-watcher: quit if $WATCHER/socket is goneNguyễn Thái Ngọc Duy, Feb 3, 2014
  60. 23/26 file-watcher: tests for the daemonNguyễn Thái Ngọc Duy, Feb 3, 2014
  61. 24/26 ls-files: print CE_WATCHED as W (or "w" with CE_VALID)Nguyễn Thái Ngọc Duy, Feb 3, 2014
  62. 25/26 file-watcher: tests for the client sideNguyễn Thái Ngọc Duy, Feb 3, 2014
  63. 26/26 Disable file-watcher with system inotify on some testsNguyễn Thái Ngọc Duy, Feb 3, 2014
  64. Torsten BögershausenFeb 8, 2014
  65. Duy NguyenFeb 8, 2014
  66. Torsten BögershausenFeb 9, 2014
  67. Duy NguyenFeb 10, 2014
  68. Torsten BögershausenFeb 10, 2014
  69. Duy NguyenFeb 10, 2014
  70. Duy NguyenFeb 17, 2014
  71. Shawn PearceFeb 19, 2014
  72. Duy NguyenFeb 19, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.