[{"id":17408,"web_url":"https://patchwork.libcamera.org/comment/17408/","msgid":"","date":"2021-06-06T18:02:24","subject":"Re: [libcamera-devel] [RFC PATCH 02/10] libcamera:\n\tEventDispatcherPoll: Manage fd by ScopedFD","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nI'm reviewing users of the new class first, to better assert its API.\nI'll reply to patch 01/10 last.\n\nOn Thu, Apr 15, 2021 at 05:38:35PM +0900, Hirokazu Honda wrote:\n> EventDispatcherPoll owns the event file descriptor. This manages\n> the fd with ScopedFD to avoid leakage.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/event_dispatcher_poll.h | 4 +++-\n> src/libcamera/event_dispatcher_poll.cpp | 12 ++++++------\n> 2 files changed, 9 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/event_dispatcher_poll.h b/include/libcamera/internal/event_dispatcher_poll.h\n> index 33de051d..b686183d 100644\n> --- a/include/libcamera/internal/event_dispatcher_poll.h\n> +++ b/include/libcamera/internal/event_dispatcher_poll.h\n> @@ -11,6 +11,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \"libcamera/internal/event_dispatcher.h\"\n> \n> struct pollfd;\n> @@ -48,7 +50,7 @@ private:\n> \n> \tstd::map notifiers_;\n> \tstd::list timers_;\n> -\tint eventfd_;\n> +\tScopedFD eventfd_;\n> \n> \tbool processingEvents_;\n> };\n> diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp\n> index 456c6def..6b22a8a3 100644\n> --- a/src/libcamera/event_dispatcher_poll.cpp\n> +++ b/src/libcamera/event_dispatcher_poll.cpp\n> @@ -54,14 +54,14 @@ EventDispatcherPoll::EventDispatcherPoll()\n> \t * Create the event fd. Failures are fatal as we can't implement an\n> \t * interruptible dispatcher without the fd.\n> \t */\n> -\teventfd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n> -\tif (eventfd_ < 0)\n> +\tint fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n> +\tif (fd < 0)\n> \t\tLOG(Event, Fatal) << \"Unable to create eventfd\";\n> +\teventfd_ = ScopedFD(fd);\n\nYou could write this\n\n\teventfd_ = ScopedFD(eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK));\n\tif (!eventfd_.isValid())\n \t\tLOG(Event, Fatal) << \"Unable to create eventfd\";\n\nIt doesn't matter much.\n\n> }\n> \n> EventDispatcherPoll::~EventDispatcherPoll()\n> {\n> -\tclose(eventfd_);\n> }\n> \n> void EventDispatcherPoll::registerEventNotifier(EventNotifier *notifier)\n> @@ -154,7 +154,7 @@ void EventDispatcherPoll::processEvents()\n> \tfor (auto notifier : notifiers_)\n> \t\tpollfds.push_back({ notifier.first, notifier.second.events(), 0 });\n> \n> -\tpollfds.push_back({ eventfd_, POLLIN, 0 });\n> +\tpollfds.push_back({ eventfd_.get(), POLLIN, 0 });\n\nI wonder if a conversation operator would be useful:\n\n\toperator int() const { return fd_; }\n\nwould allow us to write\n\n\tpollfds.push_back({ eventfd_, POLLIN, 0 });\n\nIt's probably a dangerous idea though.\n\nReviewed-by: Laurent Pinchart \n\n> \n> \t/* Wait for events and process notifiers and timers. */\n> \tdo {\n> @@ -176,7 +176,7 @@ void EventDispatcherPoll::processEvents()\n> void EventDispatcherPoll::interrupt()\n> {\n> \tuint64_t value = 1;\n> -\tssize_t ret = write(eventfd_, &value, sizeof(value));\n> +\tssize_t ret = write(eventfd_.get(), &value, sizeof(value));\n> \tif (ret != sizeof(value)) {\n> \t\tif (ret < 0)\n> \t\t\tret = -errno;\n> @@ -230,7 +230,7 @@ void EventDispatcherPoll::processInterrupt(const struct pollfd &pfd)\n> \t\treturn;\n> \n> \tuint64_t value;\n> -\tssize_t ret = read(eventfd_, &value, sizeof(value));\n> +\tssize_t ret = read(eventfd_.get(), &value, sizeof(value));\n> \tif (ret != sizeof(value)) {\n> \t\tif (ret < 0)\n> \t\t\tret = -errno;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E5C3DC3206\n\tfor ;\n\tSun, 6 Jun 2021 18:02:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CF906892B;\n\tSun, 6 Jun 2021 20:02:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C8F2602A7\n\tfor ;\n\tSun, 6 Jun 2021 20:02:39 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 05DEBEF;\n\tSun, 6 Jun 2021 20:02:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ssysndP5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623002558;\n\tbh=n/6oCtHn7xjRUU4GVOpR61ZE2VA3JIqeukhyPSUx9Ik=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ssysndP5g15kXGPvLek2rWj7HoV+vl9aIdAX6qgDjki5wTXtAYbRRT/pnE1bPPuNW\n\tkyWpSxnChZ8Xse7doWBIF3MRg0lx7wYxF1xdXFqW3sCuLMIrktK5L9ebUdbRVuFx7W\n\ttX5wHcl3/3TDKhT7YuA4Om2ILsBXm9V6yS4SBlBU=","Date":"Sun, 6 Jun 2021 21:02:24 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210415083843.3399502-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 02/10] libcamera:\n\tEventDispatcherPoll: Manage fd by ScopedFD","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17430,"web_url":"https://patchwork.libcamera.org/comment/17430/","msgid":"","date":"2021-06-07T03:21:04","subject":"Re: [libcamera-devel] [RFC PATCH 02/10] libcamera:\n\tEventDispatcherPoll: Manage fd by ScopedFD","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for reviewing.\n\nOn Mon, Jun 7, 2021 at 3:02 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> I'm reviewing users of the new class first, to better assert its API.\n> I'll reply to patch 01/10 last.\n>\n> On Thu, Apr 15, 2021 at 05:38:35PM +0900, Hirokazu Honda wrote:\n> > EventDispatcherPoll owns the event file descriptor. This manages\n> > the fd with ScopedFD to avoid leakage.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/internal/event_dispatcher_poll.h | 4 +++-\n> > src/libcamera/event_dispatcher_poll.cpp | 12 ++++++------\n> > 2 files changed, 9 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/event_dispatcher_poll.h\n> b/include/libcamera/internal/event_dispatcher_poll.h\n> > index 33de051d..b686183d 100644\n> > --- a/include/libcamera/internal/event_dispatcher_poll.h\n> > +++ b/include/libcamera/internal/event_dispatcher_poll.h\n> > @@ -11,6 +11,8 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > #include \"libcamera/internal/event_dispatcher.h\"\n> >\n> > struct pollfd;\n> > @@ -48,7 +50,7 @@ private:\n> >\n> > std::map notifiers_;\n> > std::list timers_;\n> > - int eventfd_;\n> > + ScopedFD eventfd_;\n> >\n> > bool processingEvents_;\n> > };\n> > diff --git a/src/libcamera/event_dispatcher_poll.cpp\n> b/src/libcamera/event_dispatcher_poll.cpp\n> > index 456c6def..6b22a8a3 100644\n> > --- a/src/libcamera/event_dispatcher_poll.cpp\n> > +++ b/src/libcamera/event_dispatcher_poll.cpp\n> > @@ -54,14 +54,14 @@ EventDispatcherPoll::EventDispatcherPoll()\n> > * Create the event fd. Failures are fatal as we can't implement an\n> > * interruptible dispatcher without the fd.\n> > */\n> > - eventfd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n> > - if (eventfd_ < 0)\n> > + int fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n> > + if (fd < 0)\n> > LOG(Event, Fatal) << \"Unable to create eventfd\";\n> > + eventfd_ = ScopedFD(fd);\n>\n> You could write this\n>\n> eventfd_ = ScopedFD(eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK));\n> if (!eventfd_.isValid())\n> LOG(Event, Fatal) << \"Unable to create eventfd\";\n>\n> It doesn't matter much.\n>\n> > }\n> >\n> > EventDispatcherPoll::~EventDispatcherPoll()\n> > {\n> > - close(eventfd_);\n> > }\n> >\n> > void EventDispatcherPoll::registerEventNotifier(EventNotifier *notifier)\n> > @@ -154,7 +154,7 @@ void EventDispatcherPoll::processEvents()\n> > for (auto notifier : notifiers_)\n> > pollfds.push_back({ notifier.first,\n> notifier.second.events(), 0 });\n> >\n> > - pollfds.push_back({ eventfd_, POLLIN, 0 });\n> > + pollfds.push_back({ eventfd_.get(), POLLIN, 0 });\n>\n> I wonder if a conversation operator would be useful:\n>\n> operator int() const { return fd_; }\n>\n> would allow us to write\n>\n> pollfds.push_back({ eventfd_, POLLIN, 0 });\n>\n> It's probably a dangerous idea though.\n>\n>\nI would not do so. It is unclear that an integer is passed by that.\nget() should be the unique way of getting the owned file descriptor like\nunique_ptr.\n\nRegards,\n-Hiro\n\nReviewed-by: Laurent Pinchart \n\n>\n> > /* Wait for events and process notifiers and timers. */\n> > do {\n> > @@ -176,7 +176,7 @@ void EventDispatcherPoll::processEvents()\n> > void EventDispatcherPoll::interrupt()\n> > {\n> > uint64_t value = 1;\n> > - ssize_t ret = write(eventfd_, &value, sizeof(value));\n> > + ssize_t ret = write(eventfd_.get(), &value, sizeof(value));\n> > if (ret != sizeof(value)) {\n> > if (ret < 0)\n> > ret = -errno;\n> > @@ -230,7 +230,7 @@ void EventDispatcherPoll::processInterrupt(const\n> struct pollfd &pfd)\n> > return;\n> >\n> > uint64_t value;\n> > - ssize_t ret = read(eventfd_, &value, sizeof(value));\n> > + ssize_t ret = read(eventfd_.get(), &value, sizeof(value));\n> > if (ret != sizeof(value)) {\n> > if (ret < 0)\n> > ret = -errno;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 956D4C320B\n\tfor ;\n\tMon, 7 Jun 2021 03:21:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CAF536892E;\n\tMon, 7 Jun 2021 05:21:18 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 40D596029E\n\tfor ;\n\tMon, 7 Jun 2021 05:21:17 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id r7so4079523edv.12\n\tfor ;\n\tSun, 06 Jun 2021 20:21:17 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Qa9IonGq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=ra5MU3mzkm0mjaRlugEvtjsILE/tYUXic8l8b7G0tj0=;\n\tb=Qa9IonGqTO08VV16iuOxlgccaNqRPAObjUfarkCs7bS6/Dc7yhJrQcvxLoSlWf6vhm\n\to/pRGInG56S9yw+xzspOeB4NA9kx6+IVecLYVDmVqjmpuKOZcPSWEbCkYdZ+UafIv98s\n\tRIdVwWOo/PHTVGDtRu+hETb7HZ9G3NngynFd0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ra5MU3mzkm0mjaRlugEvtjsILE/tYUXic8l8b7G0tj0=;\n\tb=A79UpNPJxduc/hioakmOJ6d6cFRj8eMyVr1sJZGg9QPE+EMDbDy9Wvu9+i4uzOh+ai\n\t9rTlJZpkbwnNwG4VD46q1lbFvnkdMi0h7tO9+38pZReTaVRKK6TTJFhMT+Us9+CG5ZQL\n\t38Xyrk4zyCt7ztkCHcvN95skeE36mnhaT7bM+SCjYh1P++Tbu20BPFuT20kvqj5hHWui\n\tRyGreWQMzECVzLH+e+QWfbBcYceJXdgnBeliCgr8GGz5302helbZ7nVxx72RYrxZNZgO\n\t33SkmFQm2LhE0Jxf3aw5WxhTXpEI+YD18M/dt/JWubt5UTidxjGVuaOm3LuNW1YKIc/v\n\tyl5w==","X-Gm-Message-State":"AOAM531Pzv/lbVvZVYfKQvReLY34xgVBGCCEK2nwLIyy+6VHLcLZMGB4\n\tu2MjWtTDCcCpfPE5xetxhWXmp4aoD49ToD7epYDpMw==","X-Google-Smtp-Source":"ABdhPJzF18cb+0fJOO2IQgu/59xNGcZYK6rxeMx+n5CYE+Mz5lJQ6bGFMkg5vVXFzJVSvz7ZA8QitgjigV3owHbDN4I=","X-Received":"by 2002:a05:6402:2547:: with SMTP id\n\tl7mr18052551edb.73.1623036074675; \n\tSun, 06 Jun 2021 20:21:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-2-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 7 Jun 2021 12:21:04 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000b061f305c424886f\"","Subject":"Re: [libcamera-devel] [RFC PATCH 02/10] libcamera:\n\tEventDispatcherPoll: Manage fd by ScopedFD","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]