Message ID | 20230311190526.17104-1-mail@eliasnaur.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Elias, Thank you for the patch. On Sat, Mar 11, 2023 at 01:05:25PM -0600, Elias Naur via libcamera-devel wrote: > It's generally a good idea to openat(2) with O_CLOEXEC, but this patch > also fixes a problem where the v4l2 devices would report -EBUSY after > an exec(3). Could you elaborate a bit on this -EBUSY problem ? > Signed-off-by: Elias Naur <mail@eliasnaur.com> > --- > src/libcamera/v4l2_device.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git src/libcamera/v4l2_device.cpp src/libcamera/v4l2_device.cpp > index 57a88d96..ad9d1e37 100644 > --- src/libcamera/v4l2_device.cpp > +++ src/libcamera/v4l2_device.cpp > @@ -86,7 +86,7 @@ int V4L2Device::open(unsigned int flags) > return -EBUSY; > } > > - UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags)); > + UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags|O_CLOEXEC)); There should be spaces around the '|'. The utils/checkstyle.py script should have reported that. You can automate running checkstyle.py as a git post-commit (or pre-commit, if desired) hook by copying utils/hooks/post-commit to .git/hooks/. There's no need to submit a v2 of this patch just for this, I can fix it locally. > if (!fd.isValid()) { > int ret = -errno; > LOG(V4L2, Error) << "Failed to open V4L2 device '"
Hi Elias, Laurent On Sun, Mar 12, 2023 at 11:39:30AM +0200, Laurent Pinchart via libcamera-devel wrote: > Hi Elias, > > Thank you for the patch. > > On Sat, Mar 11, 2023 at 01:05:25PM -0600, Elias Naur via libcamera-devel wrote: > > It's generally a good idea to openat(2) with O_CLOEXEC, but this patch > > also fixes a problem where the v4l2 devices would report -EBUSY after > > an exec(3). > > Could you elaborate a bit on this -EBUSY problem ? > I also wonder if you got into a real bug or this is just potentially dangerous. In example I know "libcamerify" execs, did you get issues with the file descriptor being kept open across it ? > > Signed-off-by: Elias Naur <mail@eliasnaur.com> > > --- > > src/libcamera/v4l2_device.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git src/libcamera/v4l2_device.cpp src/libcamera/v4l2_device.cpp > > index 57a88d96..ad9d1e37 100644 > > --- src/libcamera/v4l2_device.cpp > > +++ src/libcamera/v4l2_device.cpp > > @@ -86,7 +86,7 @@ int V4L2Device::open(unsigned int flags) > > return -EBUSY; > > } > > > > - UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags)); > > + UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags|O_CLOEXEC)); > > There should be spaces around the '|'. The utils/checkstyle.py script > should have reported that. You can automate running checkstyle.py as a > git post-commit (or pre-commit, if desired) hook by copying > utils/hooks/post-commit to .git/hooks/. > > There's no need to submit a v2 of this patch just for this, I can fix it > locally. > Anyway, I think this is generally a good idea and with Laurent's comment on style addressed at apply time Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > if (!fd.isValid()) { > > int ret = -errno; > > LOG(V4L2, Error) << "Failed to open V4L2 device '" > > -- > Regards, > > Laurent Pinchart
Thank you for your reviews. See updated patch[0] that correct the style issue and elaborates the problem O_CLOEXEC solves for me. [0] https://lists.libcamera.org/pipermail/libcamera-devel/2023-March/037100.html On Mon, 13 Mar 2023 at 02:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Elias, Laurent > > On Sun, Mar 12, 2023 at 11:39:30AM +0200, Laurent Pinchart via libcamera-devel wrote: > > Hi Elias, > > > > Thank you for the patch. > > > > On Sat, Mar 11, 2023 at 01:05:25PM -0600, Elias Naur via libcamera-devel wrote: > > > It's generally a good idea to openat(2) with O_CLOEXEC, but this patch > > > also fixes a problem where the v4l2 devices would report -EBUSY after > > > an exec(3). > > > > Could you elaborate a bit on this -EBUSY problem ? > > > > I also wonder if you got into a real bug or this is just potentially > dangerous. In example I know "libcamerify" execs, did you get issues > with the file descriptor being kept open across it ? > It's a weird corner-case: a self-updating executable for an appliance-style deployment of a buildroot-like environment. I hope I explained the problem better in the v2 patch. > > > > Signed-off-by: Elias Naur <mail@eliasnaur.com> > > > --- > > > src/libcamera/v4l2_device.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git src/libcamera/v4l2_device.cpp src/libcamera/v4l2_device.cpp > > > index 57a88d96..ad9d1e37 100644 > > > --- src/libcamera/v4l2_device.cpp > > > +++ src/libcamera/v4l2_device.cpp > > > @@ -86,7 +86,7 @@ int V4L2Device::open(unsigned int flags) > > > return -EBUSY; > > > } > > > > > > - UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags)); > > > + UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags|O_CLOEXEC)); > > > > There should be spaces around the '|'. The utils/checkstyle.py script > > should have reported that. You can automate running checkstyle.py as a > > git post-commit (or pre-commit, if desired) hook by copying > > utils/hooks/post-commit to .git/hooks/. > > > > There's no need to submit a v2 of this patch just for this, I can fix it > > locally. > > > Anyway, I think this is generally a good idea and with Laurent's > comment on style addressed at apply time > Addressed in v2.
diff --git src/libcamera/v4l2_device.cpp src/libcamera/v4l2_device.cpp index 57a88d96..ad9d1e37 100644 --- src/libcamera/v4l2_device.cpp +++ src/libcamera/v4l2_device.cpp @@ -86,7 +86,7 @@ int V4L2Device::open(unsigned int flags) return -EBUSY; } - UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags)); + UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags|O_CLOEXEC)); if (!fd.isValid()) { int ret = -errno; LOG(V4L2, Error) << "Failed to open V4L2 device '"
It's generally a good idea to openat(2) with O_CLOEXEC, but this patch also fixes a problem where the v4l2 devices would report -EBUSY after an exec(3). Signed-off-by: Elias Naur <mail@eliasnaur.com> --- src/libcamera/v4l2_device.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)