On Wed, Dec 04, 2024 at 02:44:51PM +0100, Amir Goldstein wrote: > On Wed, Dec 4, 2024 at 12:35 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > Add selftests for pidfs file handles. > > > > Link: https://lore.kernel.org/r/20241202-imstande-einsicht-d78753e1c632@brauner > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx> > > --- > > I've added a bunch more selftests. > > Frankly, I'm going to probably be adding more as corner cases come to me. > > And I'm just going to be amending the patch and stuffing them into the > > tree unless I hear objections. > > --- > > Generally, tests look good and you may add > > Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx> > > But I wonder... > > [...] > > > +/* > > + * Test valid flags to open a pidfd file handle. Note, that > > + * PIDFD_NONBLOCK is defined as O_NONBLOCK and O_NONBLOCK is an alias to > > + * O_NDELAY. Also note that PIDFD_THREAD is an alias for O_EXCL. > > + */ > > +TEST_F(file_handle, open_by_handle_at_valid_flags) > > +{ > > + int mnt_id; > > + struct file_handle *fh; > > + int pidfd = -EBADF; > > + struct stat st1, st2; > > + > > + fh = malloc(sizeof(struct file_handle) + MAX_HANDLE_SZ); > > + ASSERT_NE(fh, NULL); > > + memset(fh, 0, sizeof(struct file_handle) + MAX_HANDLE_SZ); > > + fh->handle_bytes = MAX_HANDLE_SZ; > > + > > + ASSERT_EQ(name_to_handle_at(self->child_pidfd2, "", fh, &mnt_id, AT_EMPTY_PATH), 0); > > + > > + ASSERT_EQ(fstat(self->child_pidfd2, &st1), 0); > > + > > + pidfd = open_by_handle_at(self->pidfd, fh, > > + O_RDONLY | > > + O_WRONLY | > > + O_RDWR | > > + O_NONBLOCK | > > + O_NDELAY | > > + O_CLOEXEC | > > + O_EXCL); > > + ASSERT_GE(pidfd, 0); > > + > > IIRC, your patch always opens an fd with O_RDWR. Right? > > Isn't it confusing to request O_RDONLY and get O_RDWR? > Should we only allow requests to request open_by_handle_at() > with O_RDWR mode? I think it's nicer to allow people to use open_by_handle_at() with a zero flag value as O_RDONLY is 0 and have things just work instead of confusing them by forcing them to specify O_RDWR. Similar to how pidfd_open() always gives you a O_RDWR file.
