Hi Luca,
thanks again for this work.
Some mostly cosmetic nitpicking below, I'll leave the real review to
Darrick as he knows the code much better than me.
> +will populate the root file system with the contents of the given directory.
> +Content, timestamps (atime, mtime), attributes and extended attributes are preserved
Please keep lines under 80 characters also for the man page.
> +static void populate_from_dir(struct xfs_mount *mp,
> + struct fsxattr *fsxp, char *source_dir);
> +static void walk_dir(struct xfs_mount *mp, struct xfs_inode *pip,
> + struct fsxattr *fsxp, char *path_buf);
Is there any easy way to place these new helpers so that no
forward declaration is needed? If we really have to keep them, the
standard formatting would be:
static void populate_from_dir(struct xfs_mount *mp, struct fsxattr *fsxp,
char *source_dir);
static void walk_dir(struct xfs_mount *mp, struct xfs_inode *pip,
struct fsxattr *fsxp, char *path_buf);
> + struct proto_source result = {0};
No need for the 0, {} is a valid all-zeroing array initializer.
> + struct stat statbuf;
> +
> + /*
> + * If no prototype path is
> + * supplied, use the default protofile
> + * which creates only a root
> + * directory.
> + */
Use up all 80 characters for comments:
/*
* If no prototype path is supplied, use the default protofile which
* creates only a root directory.
*/
Same in a few others places.
> + if (stat(fname, &statbuf) < 0) {
> + fail(_("invalid or unreadable source path"), errno);
> + }
No need for the braces around single line statements. This also happens
a few more times further down.
> +
> + /*
> + * handle directory inputs
> + */
> + if (S_ISDIR(statbuf.st_mode)) {
> + result.type = PROTO_SRC_DIR;
> + result.data = fname;
> + return result;
> + }
> +
> + /*
> + * else this is a protofile, let's handle traditionally
> + */
> if ((fd = open(fname, O_RDONLY)) < 0 || (size = filesize(fd)) < 0) {
Shouldb't we open first, then fstat to avoid races?
> ret = flistxattr(fd, namebuf, XATTR_LIST_MAX);
> + /*
> + * in case of filedescriptors with O_PATH, flistxattr() will
> + * fail with EBADF. let's try to fallback to llistxattr() using input
> + * path.
Btw, comments usually are full sentences, and then start capitalized and
end with a dot. or are short single-line ramblings that start lower
case end end without a dot. But a mix of the styles is hard to read.
> +/* Growth strategy for hardlink tracking array */
> +#define HARDLINK_DEFAULT_GROWTH_FACTOR 2 /* Double size for small arrays */
> +#define HARDLINK_LARGE_GROWTH_FACTOR 0.25 /* Grow by 25% for large arrays */
> +#define HARDLINK_THRESHOLD 1024 /* Threshold to switch growth strategies */
> +#define HARDLINK_TRACKER_INITIAL_SIZE 4096 /* Initial allocation size */
If you move the comments above the lines it avoids the long lines
and makes this more readable.
> + /*
> + * save original path length so we can
> + * restore the original value at the end
> + * of the function
> + */
> + size_t path_save_len = strlen(path_buf);
> + size_t path_len = path_save_len;
> + size_t entry_len = strlen(entry->d_name);
Instead of messsing with the path, can we use openat to do relative
opens using openat, which avoids the string handling and also
various races.
> + /*
> + * symlinks and sockets will need to be opened with O_PATH to work,
> + * so we handle this special case.
> + */
> + if (S_ISSOCK(file_stat.st_mode) ||
> + S_ISLNK(file_stat.st_mode) ||
> + S_ISFIFO(file_stat.st_mode))
> + open_flags = O_NOFOLLOW | O_PATH;
> + if ((fd = open(path_buf, open_flags)) < 0) {
> + fprintf(stderr, _("%s: cannot open %s: %s\n"), progname, path_buf,
> + strerror(errno));
> + exit(1);
> + }
And I guess if we want this non-reacy we'd need to open first
(using O_PATH) and then re-open for regular files where we need
to do I/O.
> +
> + switch (file_stat.st_mode & S_IFMT) {
> + case S_IFDIR:
Can you split the code for each file type into a separate helper
to split up the currently huge function and make it a bit more readable?
