| Message ID | 20230312212205.44168-1-mail@eliasnaur.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Elias, Thank you for the patch. On Sun, Mar 12, 2023 at 03:22:05PM -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 real (corner-)case: I have an excutable that (1) uses > v4l2-compat to drive a RPi camera, (2) self-updates through exec(3). > Without O_CLOEXEC of the kernel devices, an update while the > camera is opened will result in -EBUSY errors when the update tries to > open the camera. I agree about the change, but I wonder what returns -EBUSY, as V4L2 allows opening device nodes multiple times. Do you get a -EBUSY error from V4L2Device::open() after exec(), or from a different location ? If the error comes from V4L2Device::open(), what device does it come from ? > Signed-off-by: Elias Naur <mail@eliasnaur.com> > --- > > This update fixes a style issue raised through review and clarifies the > motivatition for the fix. > > 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..9eb26839 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 '"
On Mon, 20 Mar 2023 at 17:56, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Sun, Mar 12, 2023 at 03:22:05PM -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 real (corner-)case: I have an excutable that (1) uses > > v4l2-compat to drive a RPi camera, (2) self-updates through exec(3). > > Without O_CLOEXEC of the kernel devices, an update while the > > camera is opened will result in -EBUSY errors when the update tries to > > open the camera. > > I agree about the change, but I wonder what returns -EBUSY, as V4L2 > allows opening device nodes multiple times. Do you get a -EBUSY error > from V4L2Device::open() after exec(), or from a different location ? If > the error comes from V4L2Device::open(), what device does it come from ? > Efter exec'ing an update while the camera device is open, the next open results in [91] INFO RPI raspberrypi.cpp:1487 Registered camera /base/soc/i2c0mux/i2c@1/ov5647@36 to Unicam device /dev/media3 and ISP device /dev/media1 [89] INFO Camera camera.cpp:1028 configuring streams: (0) 960x960-YUV420 [91] ERROR V4L2 v4l2_videodevice.cpp:1047 /dev/video0[149:cap]: Unable to set format: Resource busy Let me know if you need more. Elias
Hi Elias, On Thu, Mar 23, 2023 at 08:05:00AM -0600, Elias Naur wrote: > On Mon, 20 Mar 2023 at 17:56, Laurent Pinchart wrote: > > On Sun, Mar 12, 2023 at 03:22:05PM -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 real (corner-)case: I have an excutable that (1) uses > > > v4l2-compat to drive a RPi camera, (2) self-updates through exec(3). > > > Without O_CLOEXEC of the kernel devices, an update while the > > > camera is opened will result in -EBUSY errors when the update tries to > > > open the camera. > > > > I agree about the change, but I wonder what returns -EBUSY, as V4L2 > > allows opening device nodes multiple times. Do you get a -EBUSY error > > from V4L2Device::open() after exec(), or from a different location ? If > > the error comes from V4L2Device::open(), what device does it come from ? > > Efter exec'ing an update while the camera device is open, the next > open results in > > [91] INFO RPI raspberrypi.cpp:1487 Registered camera > /base/soc/i2c0mux/i2c@1/ov5647@36 to Unicam device /dev/media3 and ISP > device /dev/media1 > [89] INFO Camera camera.cpp:1028 configuring streams: (0) 960x960-YUV420 > [91] ERROR V4L2 v4l2_videodevice.cpp:1047 /dev/video0[149:cap]: Unable > to set format: Resource busy Ah ok, the error comes from setFormat(), not open(). That makes sense. Thanks, that's the information I was looking for. I'll update the commit message to record this information, as follows: When an executable using libcamera calls exec(3) while a camera is in use, file descriptors corresponding to the V4L2 video devices are kept open has they have been created without O_CLOEXEC. This results in the video devices staying busy, preventing the new executable from using them: [91] ERROR V4L2 v4l2_videodevice.cpp:1047 /dev/video0[149:cap]: Unableto set format: Resource busy Fix this by opening video devices with O_CLOEXEC, which is generally a good idea in libraries. > > > It's generally a good idea to openat(2) with O_CLOEXEC, but this patch > > > also fixes a real (corner-)case: I have an excutable that (1) uses > > > v4l2-compat to drive a RPi camera, (2) self-updates through exec(3). > > > Without O_CLOEXEC of the kernel devices, an update while the > > > camera is opened will result in -EBUSY errors when the update tries to > > > open the camera. > Let me know if you need more.
Hi Elias, One more comment, I'd highly recommend stopping the cameras and camera manager before calling exec(), as there may be more side effects due to objects not being destroyed. On Sun, Mar 26, 2023 at 12:03:25PM +0300, Laurent Pinchart via libcamera-devel wrote: > On Thu, Mar 23, 2023 at 08:05:00AM -0600, Elias Naur wrote: > > On Mon, 20 Mar 2023 at 17:56, Laurent Pinchart wrote: > > > On Sun, Mar 12, 2023 at 03:22:05PM -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 real (corner-)case: I have an excutable that (1) uses > > > > v4l2-compat to drive a RPi camera, (2) self-updates through exec(3). > > > > Without O_CLOEXEC of the kernel devices, an update while the > > > > camera is opened will result in -EBUSY errors when the update tries to > > > > open the camera. > > > > > > I agree about the change, but I wonder what returns -EBUSY, as V4L2 > > > allows opening device nodes multiple times. Do you get a -EBUSY error > > > from V4L2Device::open() after exec(), or from a different location ? If > > > the error comes from V4L2Device::open(), what device does it come from ? > > > > Efter exec'ing an update while the camera device is open, the next > > open results in > > > > [91] INFO RPI raspberrypi.cpp:1487 Registered camera > > /base/soc/i2c0mux/i2c@1/ov5647@36 to Unicam device /dev/media3 and ISP > > device /dev/media1 > > [89] INFO Camera camera.cpp:1028 configuring streams: (0) 960x960-YUV420 > > [91] ERROR V4L2 v4l2_videodevice.cpp:1047 /dev/video0[149:cap]: Unable > > to set format: Resource busy > > Ah ok, the error comes from setFormat(), not open(). That makes sense. > Thanks, that's the information I was looking for. I'll update the commit > message to record this information, as follows: > > When an executable using libcamera calls exec(3) while a camera is in > use, file descriptors corresponding to the V4L2 video devices are kept > open has they have been created without O_CLOEXEC. This results in the > video devices staying busy, preventing the new executable from using > them: > > [91] ERROR V4L2 v4l2_videodevice.cpp:1047 /dev/video0[149:cap]: Unableto set format: Resource busy > > Fix this by opening video devices with O_CLOEXEC, which is generally a > good idea in libraries. > > > > > It's generally a good idea to openat(2) with O_CLOEXEC, but this patch > > > > also fixes a real (corner-)case: I have an excutable that (1) uses > > > > v4l2-compat to drive a RPi camera, (2) self-updates through exec(3). > > > > Without O_CLOEXEC of the kernel devices, an update while the > > > > camera is opened will result in -EBUSY errors when the update tries to > > > > open the camera. > > > Let me know if you need more.
diff --git src/libcamera/v4l2_device.cpp src/libcamera/v4l2_device.cpp index 57a88d96..9eb26839 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 real (corner-)case: I have an excutable that (1) uses v4l2-compat to drive a RPi camera, (2) self-updates through exec(3). Without O_CLOEXEC of the kernel devices, an update while the camera is opened will result in -EBUSY errors when the update tries to open the camera. Signed-off-by: Elias Naur <mail@eliasnaur.com> --- This update fixes a style issue raised through review and clarifies the motivatition for the fix. src/libcamera/v4l2_device.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)