Hi Mike,

On 03/19/2014 07:42 AM, Mike Frysinger wrote:

On Tue 18 Mar 2014 13:55:15 Michael Kerrisk wrote:

quoted

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 ?

I'm not sure that I completely understand you here, but I agree that this could 
be better. I've rewritten somewhat.

quoted

.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

Fixed.

quoted

.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)

Fixed.

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

Yep, I've got some scripts that I run manually now and then to 
check for this sort of stuff.

quoted

$ \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 ...

Fair enough. I'm too influenced by recent work on the locale pages (and 
family conversations ;-)). I'll switch it to English

quoted

#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.

I'm not really convinced about using these functions, but I'll reflect 
on it more.

quoted

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 ?

Yes, some cruft crept in.

quoted

    /* Allocate file_handle structure */

    fhsize = sizeof(struct file_handle *);

pretty sure this is wrong 

<blush>

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.

Yep, changed.

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 :).

quoted

    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];

Yup.

quoted

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

fhsize += fhp->handle_bytes ?

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

Depends on your perspective. It relies on no one changing the code
so that fhsize is modified after the earlier initialization. And
also, with this line, I see exactly what is going on, in one place.
I'll leave as is.

quoted

    /* 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 ?

Yes. I'd already reflected on exactly that and made a change to using text 
formats.

quoted

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".

When I was a kid, you had to pay a dollar for each letter...
(I've made a few changes.)

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

Fixed.

quoted

    FILE *fp;

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

only one space before the =

Yup.

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");

I'm of two minds about this. I foresee the day when I get a bug report that
says: "Why did you use 'e' here (or O_CLOEXEC)? It's not needed". So, I'm 
inclined to leave this.

quoted

    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) {

Yup. ;-}.

quoted

        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 ?

Yes. Fixed.

quoted

        if (nread == \-1)
            break;

	nread = sscanf(linep, "%d %*d %*s %*s %s", &fmnt_id, mount_path);

indent is off here

Fixed.

quoted

    return open(mount_path, O_RDONLY | O_DIRECTORY);

O_CLOEXEC for funsies ?

See above comment.

quoted

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 ?

Done.

quoted

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

how about also aborting when argc > 2 ?

Yes.

quoted

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

O_CLOEXEC ?

See comment above.

quoted

    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.

Calling it a bad habit seems a bit too strong. It's a habit conditioned by writing 
code that runs on systems that don't have C99. Less important these days, of course.
I've changed it.

quoted

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

i don't have a findfs(1).  i do have a findfs(8) ...

Thanks. blkid(8) also, actually.

Thanks for the comments, Mike.

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/