Hi Heinrich,
Thanks for the updated patch!
On 05/19/2014 10:20 PM, Heinrich Schuchardt wrote:
> This updated patch reflects the comments provided by Michael Kerrisk.
> * Macros are not used any more. (Thanks for catching the bug in one of them.)
> * Additional comments are supplied.
> * Typos have been corrected.
>
> I have tested the reworked example also with a 4 second delay before read.
(The way I did it was to just suspend the program with ^Z while I generated
some events.)
> This allowed me to verify that the example handles reading multiple events
> at once correctly.
Good. I've now looked at the code some more, and have more comments.
> ***
>
> An example for the usage of the inotify API is provided.
>
> It shows the usage of inotify_init1(2),inotify_add_watch(2) as well as
> polling and reading from the inotify file descriptor.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@xxxxxx>
> ---
> man7/inotify.7 | 223 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 223 insertions(+)
I suggest adding yourself to the copyright notice at the top of the page source
with the next version of the patch.
> diff --git a/man7/inotify.7 b/man7/inotify.7
> index 8e848b2..12def61 100644
> --- a/man7/inotify.7
> +++ b/man7/inotify.7
> @@ -752,6 +752,229 @@ if the older had not yet been read)
> instead checked if the most recent event could be coalesced with the
> .I oldest
> unread event.
> +.SH EXAMPLE
> +The following program demonstrates the usage of the inotify API.
> +It marks the directories passed as a command-line arguments
> +and waits for events of type
> +.BR IN_OPEN ,
> +.BR IN_CLOSE_NOWRITE
> +and
> +.BR IN_CLOSE_WRITE .
> +.PP
> +The following output was recorded while editing the file
> +.I /home/user/temp/foo
> +and listing directory
> +.IR /tmp .
> +Before the file and the directory were opened,
> +.B IN_OPEN
> +events occurred.
> +After the file was closed, an
> +.B IN_CLOSE_WRITE
> +event occurred.
> +After the directory was closed, an
> +.B IN_CLOSE_NOWRITE
> +event occurred.
> +Execution of the program ended when the user pressed the ENTER key.
Here, you write "ENTER", but in the program you use "enter".
It's probably better to be consistent.
> +.SS Example output
> +.in +4n
> +.nf
> +$ ./inotify.7.example /tmp /home/user/temp
> +Press enter key to terminate.
> +Listening for events.
> +IN_OPEN: /home/user/temp/foo [file]
> +IN_CLOSE_WRITE: /home/user/temp/foo [file]
> +IN_OPEN: /tmp/ [directory]
> +IN_CLOSE_NOWRITE: /tmp/ [directory]
> +
> +Listening for events stopped.
> +.fi
> +.in
> +.SS Program source
> +.nf
> +#include <errno.h>
> +#include <poll.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/inotify.h>
> +#include <unistd.h>
> +
> +/* Read all available inotify events from the file descriptor 'fd'
> + wd is the table of watch descriptors for the directories in argv
> + argc is the length of wd and argv
> + argv is the list of watched directories
> + Entry 0 of wd and argv is unused. */
> +
> +static void
> +handle_events(int fd, int *wd, int argc, char* argv[])
> +{
> + const struct inotify_event *event;
> + char buf[4096];
Make this char buf[BUF_LEN] __attribute__ ((aligned(4)));
See http://man7.org/tlpi/errata/index.html#p_383
and
http://stackoverflow.com/questions/7055495/what-is-meaning-of-the-attribute-aligned4-in-the-first-line/
I'd also add a suitable comment to the code.
> + int i;
> + ssize_t len;
> + ssize_t offset;
> +
> + /* Loop while events can be read from inotify file descriptor. */
> +
> + for(;;) {
> +
> + /* Read some events. */
> +
> + len = read(fd, (void *) &buf, sizeof(buf));
Drop the cast here. It's not needed.
> + if (len == \-1 && errno != EAGAIN) {
> + perror("read");
> + exit(EXIT_FAILURE);
> + }
> +
> + /* Check if end of available data reached.
> + This results in len == \-1 and errno == EAGAIN. */
I'd change this to something like
/* If the nonblocking read() found no events to read, then
it returns \-1 with errno set to EAGAIN. In that case,
we exit the loop. */
> +
> + if (len <= 0)
> + break;
> +
> + /* Point to the first event in the buffer. */
> +
> + event = (struct inotify_event *) buf;
> +
> + /* Loop over all events in the buffer. */
> +
> + while(len >= sizeof(struct inotify_event)
> + && len >= sizeof(struct inotify_event)
> + + event\->len) {
> +
This piece, plus the cast at the foot of the loop, seem excessive.
Compare with http://www.linuxjournal.com/article/8478?page=0,1
(article by Robert Love).
I'd go for the even simpler form:
char *p;
for (p = buf; p < buf + len; ) {
event = (struct inotify_event *) p;
...
p += sizeof(struct inotify_event) + event->len;
}
and then you can also dispense with recalculating 'len', and
eliminate 'offset'.
> + /* Print event type. */
> +
> + if (event\->mask & IN_OPEN)
> + printf("IN_OPEN: ");
> + if (event\->mask & IN_CLOSE_NOWRITE)
> + printf("IN_CLOSE_NOWRITE: ");
> + if (event\->mask & IN_CLOSE_WRITE)
> + printf("IN_CLOSE_WRITE: ");
> +
> + /* Print the name of the watched directory. */
> +
> + for(i = 1; i < argc; ++i) {
s/for/for /
But more to the point, why not code the loop to also stop when
the watch descriptor is found, rather than continuing to the end?
> + if(wd[i] == event\->wd)
> + printf("%s/", argv[i]);
> + }
> +
> + /* Print the name of the file. */
> +
> + if (event\->len)
> + printf("%s", event\->name);
> +
> + /* Print type of filesystem object. */
> +
> + if (event\->mask & IN_ISDIR)
> + printf(" [directory]\\n");
> + else
> + printf(" [file]\\n");
> +
> + /* Point to the next event and reduce the remaining
> + buffer length by the length of the current event. */
> +
> + offset = sizeof(struct inotify_event) + event\->len;
> + len \-= offset;
> + *((char **)&event) += offset;
See comments above. (The cast magic here works, but it make the
reader work harder than is needed.)
> + }
> + }
> +}
> +
> +int
> +main(int argc, char* argv[])
> +{
> + char buf;
> + int fd, i, poll_num;
> + int *wd;
> + nfds_t nfds;
> + struct pollfd fds[2];
> +
> + if (argc < 2) {
> + printf("Usage: %s DIRECTORY [DIRECTORY ...]\\n", argv[0]);
> + exit(EXIT_FAILURE);
> + }
> +
> + printf("Press enter key to terminate.\\n");
> +
> + /* Create the file descriptor for accessing the inotify API. */
> +
> + fd = inotify_init1(IN_NONBLOCK);
> + if (fd == \-1) {
> + perror("inotify_init1");
> + exit(EXIT_FAILURE);
> + }
> +
> + /* Allocate memory for watch descriptors. */
> +
> + wd = (int *) calloc(argc, sizeof(int));
Remove the cast here. It's needed for C++, but is bad practice for C. See
http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc
> + if (wd == NULL) {
> + perror("calloc");
> + exit(EXIT_FAILURE);
> + }
> +
> + /* Mark directories for events
> + \- file was opened
> + \- file was closed */
> +
> + for (i = 1; i < argc; i++) {
> + wd[i] = inotify_add_watch(fd, argv[i],
> + IN_OPEN | IN_CLOSE);
Above, your "Usage:" message suggests that the arguments are
directories. Should you be specifying the IN_ONLYDIR flag here?
> + if (wd[i] == \-1) {
> + perror("inotify_add_watch");
> + exit(EXIT_FAILURE);
> + }
> + }
> +
> + /* Prepare for polling. */
> +
> + nfds = 2;
> +
> + /* Console input. */
> +
> + fds[0].fd = STDIN_FILENO;
> + fds[0].events = POLLIN;
> +
> + /* Inotify input. */
> +
> + fds[1].fd = fd;
> + fds[1].events = POLLIN;
> +
> + /* This is the loop to wait for incoming events. */
Maybe: /* Wait for events and/or terminal input */
> +
> + printf("Listening for events.\\n");
> + while (1) {
> + poll_num = poll(fds, nfds, \-1);
> + if (poll_num == \-1) {
> + if (errno == EINTR)
> + continue;
FWIW, since you don't handle any signals, the EINTR case can't occur.
Maybe drop this piece? (Or add a comment to say why it would be useful
if you were handling signals.)
> + perror("poll");
> + exit(EXIT_FAILURE);
> + }
Add a blank line here.
> + if (poll_num > 0) {
> +
Why a blank line here?
> + if (fds[0].revents & POLLIN) {
> +
> + /* Console input is available. Empty stdin and quit. */
> +
> + while (read(STDIN_FILENO, &buf, 1) > 0 && buf != '\\n')
> + continue;
> + break;
> + }
> + if (fds[1].revents & POLLIN) {
> +
> + /* Inotify events are available. */
> + handle_events(fd, wd, argc, argv);
> + }
> + }
> + }
> +
> + /* Close inotify file descriptor. */
> +
> + close(fd);
> + free(wd);
> + printf("Listening for events stopped.\\n");
> + exit(EXIT_SUCCESS);
> +}
> +.fi
> .SH SEE ALSO
> .BR inotifywait (1),
> .BR inotifywatch (1),
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
