| Message ID | 20220701084521.31831-7-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi On Fri, Jul 01, 2022 at 11:45:10AM +0300, Tomi Valkeinen wrote: > Use UniqueFD to automate the eventfd lifetime management. > Here you go :) Can you squash with the previous patch ? > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > src/py/libcamera/py_camera_manager.cpp | 16 ++++------------ > src/py/libcamera/py_camera_manager.h | 4 ++-- > 2 files changed, 6 insertions(+), 14 deletions(-) > > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp > index ad47271d..51a890c8 100644 > --- a/src/py/libcamera/py_camera_manager.cpp > +++ b/src/py/libcamera/py_camera_manager.cpp > @@ -27,25 +27,17 @@ PyCameraManager::PyCameraManager() > throw std::system_error(errno, std::generic_category(), > "Failed to create eventfd"); > > - eventFd_ = fd; > + eventFd_ = UniqueFD(fd); > > int ret = cameraManager_->start(); > - if (ret) { > - close(fd); > - eventFd_ = -1; > + if (ret) > throw std::system_error(-ret, std::generic_category(), > "Failed to start CameraManager"); > - } > } > > PyCameraManager::~PyCameraManager() > { > LOG(Python, Debug) << "~PyCameraManager()"; > - > - if (eventFd_ != -1) { > - close(eventFd_); > - eventFd_ = -1; > - } > } > > py::list PyCameraManager::cameras() > @@ -93,7 +85,7 @@ void PyCameraManager::writeFd() > { > uint64_t v = 1; > > - size_t s = write(eventFd_, &v, 8); > + size_t s = write(eventFd_.get(), &v, 8); > /* > * We should never fail, and have no simple means to manage the error, > * so let's log a fatal error. > @@ -106,7 +98,7 @@ void PyCameraManager::readFd() > { > uint8_t buf[8]; > > - if (read(eventFd_, buf, 8) != 8) > + if (read(eventFd_.get(), buf, 8) != 8) > throw std::system_error(errno, std::generic_category()); > } > > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h > index 9c15f814..710163e8 100644 > --- a/src/py/libcamera/py_camera_manager.h > +++ b/src/py/libcamera/py_camera_manager.h > @@ -24,7 +24,7 @@ public: > > static const std::string &version() { return CameraManager::version(); } > > - int eventFd() const { return eventFd_; } > + int eventFd() const { return eventFd_.get(); } > > std::vector<pybind11::object> getReadyRequests(); > > @@ -33,7 +33,7 @@ public: > private: > std::unique_ptr<CameraManager> cameraManager_; > > - int eventFd_ = -1; > + UniqueFD eventFd_; > std::mutex completedRequestsMutex_; > std::vector<Request *> completedRequests_; > > -- > 2.34.1 >
On 18/08/2022 17:23, Jacopo Mondi wrote: > Hi Tomi > > On Fri, Jul 01, 2022 at 11:45:10AM +0300, Tomi Valkeinen wrote: >> Use UniqueFD to automate the eventfd lifetime management. >> > > Here you go :) > > Can you squash with the previous patch ? No, I don't think it's a good idea to squash changes like this. The point of the previous patch was to convert the existing code to PyCameraManager. I don't do any changes there that are not needed. Squashing this, EFD_CLOEXEC and the mutex change would hide these changes inside the large refactoring done in the previous patch. Tomi
On Thu, Aug 18, 2022 at 05:43:46PM +0300, Tomi Valkeinen wrote: > On 18/08/2022 17:23, Jacopo Mondi wrote: > > Hi Tomi > > > > On Fri, Jul 01, 2022 at 11:45:10AM +0300, Tomi Valkeinen wrote: > > > Use UniqueFD to automate the eventfd lifetime management. > > > > > > > Here you go :) > > > > Can you squash with the previous patch ? > > No, I don't think it's a good idea to squash changes like this. The point of > the previous patch was to convert the existing code to PyCameraManager. I > don't do any changes there that are not needed. > > Squashing this, EFD_CLOEXEC and the mutex change would hide these changes > inside the large refactoring done in the previous patch. > You'reintroducing a new class, and then patching it on top. I understand you have moved existing code inside that class, but it's a new component. Anyway, converting to use UniqueFD + EFD_CLOEXEC (or even better the one liner eventfd_ = UniqueFD(eventfd(...));) can be made one change. Anway, it's a minor thing, up to you > Tomi
On 18/08/2022 17:51, Jacopo Mondi wrote: > > On Thu, Aug 18, 2022 at 05:43:46PM +0300, Tomi Valkeinen wrote: >> On 18/08/2022 17:23, Jacopo Mondi wrote: >>> Hi Tomi >>> >>> On Fri, Jul 01, 2022 at 11:45:10AM +0300, Tomi Valkeinen wrote: >>>> Use UniqueFD to automate the eventfd lifetime management. >>>> >>> >>> Here you go :) >>> >>> Can you squash with the previous patch ? >> >> No, I don't think it's a good idea to squash changes like this. The point of >> the previous patch was to convert the existing code to PyCameraManager. I >> don't do any changes there that are not needed. >> >> Squashing this, EFD_CLOEXEC and the mutex change would hide these changes >> inside the large refactoring done in the previous patch. >> > > You'reintroducing a new class, and then patching it on top. Well, I see it as encapsulating the existing code to be inside a class, i.e. moving code. =) It is true that I could first do the small changes, and only after those changes do the refactoring. I have them in the current order due to the time the things were implemented. > I understand you have moved existing code inside that class, but it's > a new component. Yes, but isn't it much more difficult to review if, while moving the code inside a class, I also change the code (when not needed)? > Anyway, converting to use UniqueFD + EFD_CLOEXEC (or even better the > one liner eventfd_ = UniqueFD(eventfd(...));) can be made one change. True, but... Why? One shouldn't squash unrelated commits together. If I did what you suggest, then in the commit descs I would have to write something like "This patch does two things:..." or first describe the first change and then continue "also, we change...". When you write such things to a commit desc, it's almost a sure sign that you should split the patch =). Tomi
Hi Tomi, Thank you for the patch. On Fri, Jul 01, 2022 at 11:45:10AM +0300, Tomi Valkeinen wrote: > Use UniqueFD to automate the eventfd lifetime management. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/py/libcamera/py_camera_manager.cpp | 16 ++++------------ > src/py/libcamera/py_camera_manager.h | 4 ++-- > 2 files changed, 6 insertions(+), 14 deletions(-) > > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp > index ad47271d..51a890c8 100644 > --- a/src/py/libcamera/py_camera_manager.cpp > +++ b/src/py/libcamera/py_camera_manager.cpp > @@ -27,25 +27,17 @@ PyCameraManager::PyCameraManager() > throw std::system_error(errno, std::generic_category(), > "Failed to create eventfd"); > > - eventFd_ = fd; > + eventFd_ = UniqueFD(fd); > > int ret = cameraManager_->start(); > - if (ret) { > - close(fd); > - eventFd_ = -1; > + if (ret) > throw std::system_error(-ret, std::generic_category(), > "Failed to start CameraManager"); > - } > } > > PyCameraManager::~PyCameraManager() > { > LOG(Python, Debug) << "~PyCameraManager()"; > - > - if (eventFd_ != -1) { > - close(eventFd_); > - eventFd_ = -1; > - } > } > > py::list PyCameraManager::cameras() > @@ -93,7 +85,7 @@ void PyCameraManager::writeFd() > { > uint64_t v = 1; > > - size_t s = write(eventFd_, &v, 8); > + size_t s = write(eventFd_.get(), &v, 8); > /* > * We should never fail, and have no simple means to manage the error, > * so let's log a fatal error. > @@ -106,7 +98,7 @@ void PyCameraManager::readFd() > { > uint8_t buf[8]; > > - if (read(eventFd_, buf, 8) != 8) > + if (read(eventFd_.get(), buf, 8) != 8) > throw std::system_error(errno, std::generic_category()); > } > > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h > index 9c15f814..710163e8 100644 > --- a/src/py/libcamera/py_camera_manager.h > +++ b/src/py/libcamera/py_camera_manager.h > @@ -24,7 +24,7 @@ public: > > static const std::string &version() { return CameraManager::version(); } > > - int eventFd() const { return eventFd_; } > + int eventFd() const { return eventFd_.get(); } > > std::vector<pybind11::object> getReadyRequests(); > > @@ -33,7 +33,7 @@ public: > private: > std::unique_ptr<CameraManager> cameraManager_; > > - int eventFd_ = -1; > + UniqueFD eventFd_; > std::mutex completedRequestsMutex_; > std::vector<Request *> completedRequests_; >
diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp index ad47271d..51a890c8 100644 --- a/src/py/libcamera/py_camera_manager.cpp +++ b/src/py/libcamera/py_camera_manager.cpp @@ -27,25 +27,17 @@ PyCameraManager::PyCameraManager() throw std::system_error(errno, std::generic_category(), "Failed to create eventfd"); - eventFd_ = fd; + eventFd_ = UniqueFD(fd); int ret = cameraManager_->start(); - if (ret) { - close(fd); - eventFd_ = -1; + if (ret) throw std::system_error(-ret, std::generic_category(), "Failed to start CameraManager"); - } } PyCameraManager::~PyCameraManager() { LOG(Python, Debug) << "~PyCameraManager()"; - - if (eventFd_ != -1) { - close(eventFd_); - eventFd_ = -1; - } } py::list PyCameraManager::cameras() @@ -93,7 +85,7 @@ void PyCameraManager::writeFd() { uint64_t v = 1; - size_t s = write(eventFd_, &v, 8); + size_t s = write(eventFd_.get(), &v, 8); /* * We should never fail, and have no simple means to manage the error, * so let's log a fatal error. @@ -106,7 +98,7 @@ void PyCameraManager::readFd() { uint8_t buf[8]; - if (read(eventFd_, buf, 8) != 8) + if (read(eventFd_.get(), buf, 8) != 8) throw std::system_error(errno, std::generic_category()); } diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h index 9c15f814..710163e8 100644 --- a/src/py/libcamera/py_camera_manager.h +++ b/src/py/libcamera/py_camera_manager.h @@ -24,7 +24,7 @@ public: static const std::string &version() { return CameraManager::version(); } - int eventFd() const { return eventFd_; } + int eventFd() const { return eventFd_.get(); } std::vector<pybind11::object> getReadyRequests(); @@ -33,7 +33,7 @@ public: private: std::unique_ptr<CameraManager> cameraManager_; - int eventFd_ = -1; + UniqueFD eventFd_; std::mutex completedRequestsMutex_; std::vector<Request *> completedRequests_;
Use UniqueFD to automate the eventfd lifetime management. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- src/py/libcamera/py_camera_manager.cpp | 16 ++++------------ src/py/libcamera/py_camera_manager.h | 4 ++-- 2 files changed, 6 insertions(+), 14 deletions(-)