On Wed, Jul 30, 2025 at 04:40:39PM +0200, Luca Di Maio wrote: > 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/) Hrmm, that /would/ be useful. > Back on topic: > > On Tue, Jul 29, 2025 at 02:43:22PM -0700, Darrick J. Wong wrote: <snipping> > > > + 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? [narrator: it doesn't] > 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. On second thought I think you might leave the double opens because O_NOATIME is only allowed if the current user owns source file, or has CAP_FOWNER. If you're running mkfs as an unprivileged user trying to capture a rootfs (with uid 0 files) then O_NOATIME won't be allowed. Maybe something along the lines of: /* * Try to open the source file noatime to avoid a flood of * writes to the source fs, but we can fall back to plain * readonly mode if we don't have enough permission. */ fd = openat(pathfd, entry->d_name, O_NOFOLLOW | O_RDONLY | O_NOATIME); if (fd < 0) fd = openat(pathfd, entry->d_name, O_NOFOLLOW | O_RDONLY); if (fd < 0) /* whine and exit */ Just to see if you can open the source file without touching atime? --D > > > + * 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.
