Thanks Darrick for the review! Sorry again for this indentation mess, I'm going to basically align all function arguments and all the top-function declarations I was a bit confused because elsewhere in the code is not like that so it's a bit difficult to infere Offtopic: maybe we could also introduce an editorconfig setup? so that all various editors will be correctly set to see the tabs/spaces as needed (https://editorconfig.org/) Back on topic: On Tue, Jul 29, 2025 at 02:43:22PM -0700, Darrick J. Wong wrote: > > +will populate the root file system with the contents of the given directory. > > "...of the given directory tree." > > (It's a subtle hint that it recursively imports the whole tree, not > one single directory) Ack. > > +Content, timestamps (atime, mtime), attributes and extended attributes > > +are preserved for all file types. > > +.TP > > +.BI [file=] protofile > > +If the optional > > +.PD > > +.I prototype > > +argument is given, and points to a regular file, > > +.B mkfs.xfs > > +uses it as a prototype file and takes its directions from that file. > > The blocks and inodes specifiers in the > > .I protofile > > are provided for backwards compatibility, but are otherwise unused. > > @@ -1136,8 +1145,16 @@ always terminated with the dollar ( > > .B $ > > ) token. > > .TP > > +.BI atime= value > > +If set to 1, when we're populating the root filesystem from a directory ( > > Who is "we"? > > "If set to 1, mkfs will copy in access timestamps from the source > files. > Otherwise, access timestamps will be set to the current time." > Ack. > > + */ > > + if (S_ISDIR(statbuf.st_mode)) { > > + result.type = PROTO_SRC_DIR; > > + result.data = fname; > > + return result; > > Er... this leaks the fd that you opened above. > Ack. > > +static void > > +create_inode( > > Might want to call this create_nondir_inode to distinguish it from > create_directory_inode. > Ack. > > + char *fname) > > Hrmm, is @xname the filename we're creating in pip, and @fname is the > path to the original file so that we can copy the contents and various > other attributes? If so, then maybe s/fname/src_fname/ to make this > clearer? > Ack. > > +static void > > +handle_direntry( > > + struct xfs_mount *mp, > > + struct xfs_inode *pip, > > + struct fsxattr *fsxp, > > + char *path_buf, > > + struct dirent *entry) > > +{ > > + char cur_path_buf[PATH_MAX]; > > Hrmm. For each level of the source directory tree we allocate another > PATH_MAX buffer on the stack just to snprintf from @path_buf (which is > itself a stack buffer). > > Even assuming the usual 8M thread stack, that means a directory > structure more than ~2000 levels deep will overflow the stack and crash > mkfs. > > I really think you ought to consider allocating /one/ buffer at the > start, passing (path_buf, path_len) down the stack, and then snprinting > at the end of the buffer: > > size_t avail = PATH_MAX - path_len; > size_t wrote = snprintf(path_buf + path_len, avail, "/%s", entry->d_name); > > if (wrote > avail) > fail(path_buf, ENAMETOOLONG); > > ... > > if (S_ISDIR(...)) { > create_directory_inode(..., path_buf, path_len + strlen(entry->d_name), ...); > > ... > > path_buf[path_len] = 0; > > One buffer, much less memory usage. Right, It was like this before but I've Inadvertently unoptimized this in V11 when switched to openat(), will fix in next path, sorry. > > + if (!S_ISSOCK(file_stat.st_mode) && > > + !S_ISLNK(file_stat.st_mode) && > > + !S_ISFIFO(file_stat.st_mode)) { > > + close(fd); > > + fd = openat(pathfd, entry->d_name, > > + O_NOFOLLOW | O_RDONLY | O_NOATIME); > > Just out of curiosity, does O_NOATIME not work in the previous openat? Actually on my test setup (mainly using docker/podman to test), opening with and without O_NOATIME when using O_PATH, does not change accesstime checking with `stat`, but also it works if I add it. As a precautionary measure (not sure if podman/docker is messing with noatime) I'll add it, as it seems to work correctly. > > + * this will make flistxattr() and fgetxattr() fail wil EBADF, > > "fail with EBADF"... > Ack. > > + /* > > + * Copy over attributes. > > + */ > > + writeattrs(ip, path_buf, fd); > > Nothing closes fd here; does it leak? > > --D > Ack. L.
