lkml.org 
[lkml]   [2014]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: For review: open_by_name_at(2) man page [v2]
Date
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[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2014-03-19 08:21    [W:0.114 / U:25.708 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site