Date:	Wed, 19 Mar 2014 02:42:25 -0400
From:	Mike Frysinger <vapier@...too.org>
To:	"Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>
Cc:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	"linux-man@...r.kernel.org" <linux-man@...r.kernel.org>,
	Linux-Fsdevel <linux-fsdevel@...r.kernel.org>,
	lkml <linux-kernel@...r.kernel.org>,
	Andreas Dilger <adilger@...ger.ca>, NeilBrown <neilb@...e.de>,
	Christoph Hellwig <hch@...radead.org>
Subject: Re: For review: open_by_name_at(2) man page [v2]

On Tue 18 Mar 2014 13:55:15 Michael Kerrisk wrote:
> The
> .I flags
> argument is a bit mask constructed by ORing together
> zero or more of the following value:
> .TP
> .B AT_EMPTY_PATH
> Allow
> .I pathname
> to be an empty string.
> See above.
> (which may have been obtained using the
> .BR open (2)
> .B O_PATH
> flag).
> .TP
> .B AT_SYMLINK_FOLLOW
> By default,
> .BR name_to_handle_at ()
> does not dereference
> .I pathname
> if it is a symbolic link.
> The flag
> .B AT_SYMLINK_FOLLOW
> can be specified in
> .I flags
> to cause
> .I pathname
> to be dereferenced if it is a symbolic link.

this section is only talking about |flags|, and further this part is only 
talking about AT_SYMLINK_FOLLOW.  so this last sentence sounds super 
redundant.

how about reversing the sentence order so that both are implicit like is done 
in the openat() page and the description of O_NOFOLLOW ?

> .B ENOTDIR
> The file descriptor supplied in
> .I dirfd
> does not refer to a directory,
> and it it is not the case that both

"it" is duplicated

> .SS Obtaining a persistent filesystem ID
> The mount IDs in
> .IR /proc/self/mountinfo
> can be reused as filesystems are unmounted and mounted.
> Therefore, the mount ID returned by
> .BR name_to_handle_at (3)

should be () and not (3)

side note: this seems like an easy error to script for ...

> $ \fBecho 'Kannst du bitte überlegen?' > cecilia.txt\fP

aber, ich spreche kein Deutsch :(

do we have a standard about sticking to english ?  i wonder if people are more 
likely to be confused or to appreciate it ...

> #define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \\
>                         } while (0)

i wonder if err.h makes sense now that this is a man page for completely 
linux-specific syscalls :).  and you use _GNU_SOURCE.

> int
> main(int argc, char *argv[])
> {
>     struct file_handle *fhp;
>     int mount_id, fhsize, s;
> 
>     if (argc < 2 || strcmp(argv[1], "\-\-help") == 0) {

argc != 2 ?

>     /* Allocate file_handle structure */
> 
>     fhsize = sizeof(struct file_handle *);

pretty sure this is wrong as sizeof() here returns the size of a pointer, not 
the size of the struct.  it's why i prefer the form:

	fhsize = sizeof(*fhp);

less typing and harder to screw up by accident.

granted, the case below won't crash since the kernel only reads/writes 
sizeof(unsigned int) and i'm not aware of any system where that is larger than 
sizeof(void *), but it's still wrong :).

>     s = name_to_handle_at(AT_FDCWD, argv[1], fhp, &mount_id, 0);

another personal style: create dedicated variables for each arg you unpack out 
of argv[1].  it's generally OK when you only take one arg, but when you get 
more than one, you end up flipping back and forth between the usage trying to 
figure out what index 1 represents instead of focusing on what the code is 
doing.
	const char *pathname = argv[1];

>     fhsize = sizeof(struct file_handle) + fhp\->handle_bytes;

fhsize += fhp->handle_bytes ?

it's the same, but i think nicer ;)

>     /* Write mount ID, file handle size, and file handle to stdout,
>        for later reuse by t_open_by_handle_at.c */
> 
>     if (write(STDOUT_FILENO, &mount_id, sizeof(int)) != sizeof(int) ||
>             write(STDOUT_FILENO, &fhsize, sizeof(int)) != sizeof(int) ||
>             write(STDOUT_FILENO, fhp, fhsize) != fhsize) {

seems like a whole lot of code spew for a simple printf() ?  you'd have to 
adjust the other program to use scanf(), but seems like the end result would 
be nicer for users to experiment with ?

> static int
> open_mount_path_by_id(int mount_id)
> {
>     char *linep;
>     size_t lsize;
>     char mount_path[PATH_MAX];
>     int fmnt_id, fnd, nread;

could we buy a few more letters for these vars ?  i guess fmnt_id is the 
filesystem mount id, and fnd is "find".

also, getline() returns a ssize_t, not an int.

>     FILE *fp;
> 
>     fp  = fopen("/proc/self/mountinfo", "r");

only one space before the =

i would encourage using the "e" flag whenever possible in the hopes that 
someone might start using it in their own code base.

	fp = fopen("/proc/self/mountinfo", "re");

>     for (fnd = 0; !fnd ; ) {

in my experience, seems like a while() loop makes more sense when you're 
implementing a while() loop ...
	fnd = 0;
	while (!fnd) {

>         linep = NULL;
>         nread = getline(&linep, &lsize, fp);

this works, but it's unusual when using getline() as it kind of defeats the 
purpose of using the dyn allocation feature.

	fnd = 0;
	linep = NULL;
	while (!fnd) {
		nread = getline(&linep, &lsize, fp);
		...
	}
	free(linep);

i don't think it complicates the code much more ?

>         if (nread == \-1)
>             break;
> 
> 	nread = sscanf(linep, "%d %*d %*s %*s %s", &fmnt_id, mount_path);

indent is off here

>     return open(mount_path, O_RDONLY | O_DIRECTORY);

O_CLOEXEC for funsies ?

> int
> main(int argc, char *argv[])
> {
>     struct file_handle *fhp;
>     int mount_id, fd, mount_fd, fhsize;
>     ssize_t nread;
> #define BSIZE 1000
>     char buf[BSIZE];

why not sizeof(buf) and avoid the define ?

>     if (argc > 1 && strcmp(argv[1], "\-\-help") == 0) {
>         fprintf(stderr, "Usage: %s [mount\-dir]]\\n",
>                 argv[0]);

how about also aborting when argc > 2 ?

>     if (argc > 1)
>         mount_fd = open(argv[1], O_RDONLY | O_DIRECTORY);

O_CLOEXEC ?

>     nread = read(fd, buf, BSIZE);
>     if (nread == \-1)
>         errExit("read");
>     printf("Read %ld bytes\\n", (long) nread);

yikes, that's a bad habit to encourage.  read() returns a ssize_t, so print it 
out using %zd.

> .SH SEE ALSO
> .BR blkid (1),
> .BR findfs (1),

i don't have a findfs(1).  i do have a findfs(8) ...
-mike
Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists