[{"id":81,"web_url":"https://patchwork.libcamera.org/comment/81/","msgid":"<20181224112232.GE2364@archlinux>","date":"2018-12-24T11:22:32","subject":"Re: [libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator:\n\tadd DeviceEnumerator class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n a few minor nits\n\nOn Sun, Dec 23, 2018 at 12:00:34AM +0100, Niklas S??derlund wrote:\n> Provide a DeviceEnumerator base class which enumerates all media devices\n> in the system and information about them, resolving V4L2 data structures\n> to paths and a method to search in all the enumerated information.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/deviceenumerator.cpp | 144 +++++++++++++++++++++++\n> src/libcamera/include/deviceenumerator.h | 22 ++++\n> 2 files changed, 166 insertions(+)\n>\n> diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp\n> index 17b6874d229c791c..b2f59017e94d14aa 100644\n> --- a/src/libcamera/deviceenumerator.cpp\n> +++ b/src/libcamera/deviceenumerator.cpp\n> @@ -5,6 +5,10 @@\n> * deviceenumerator.cpp - Enumeration and matching\n> */\n>\n> +#include \n> +#include \n> +#include \n> +\n> #include \"deviceenumerator.h\"\n> #include \"log.h\"\n>\n> @@ -129,4 +133,144 @@ bool DeviceMatch::matchEntities(const std::vector &entities) const\n> \treturn true;\n> }\n>\n> +/* -----------------------------------------------------------------------------\n> + * Enumerator Base\n> + */\n> +\n> +DeviceEnumerator::~DeviceEnumerator()\n> +{\n> +\tfor (DeviceInfo *dev : devices_) {\n> +\t\tif (dev->busy())\n> +\t\t\tLOG(Error) << \"Removing device info while still in use\";\n> +\n> +\t\tdelete dev;\n> +\t}\n> +}\n> +\n> +int DeviceEnumerator::addDevice(const std::string &devnode)\n> +{\n> +\tint fd, ret;\n> +\n> +\tstruct media_device_info info = {};\n> +\tstd::map entities;\n> +\n> +\tfd = open(devnode.c_str(), O_RDWR);\n> +\tif (fd < 0)\n> +\t\treturn fd;\n\nreturn -errno and printout error with strerror() ?\nWe should standardize on error handline as much as we could.\n\n> +\n> +\tret = readInfo(fd, info);\n> +\tif (ret)\n> +\t\tgoto out;\n> +\n> +\tret = readTopology(fd, entities);\n> +\tif (ret)\n> +\t\tgoto out;\n> +\n> +\tdevices_.push_back(new DeviceInfo(devnode, info, entities));\n> +out:\n> +\tclose(fd);\n> +\n> +\treturn ret;\n> +}\n> +\n> +int DeviceEnumerator::readInfo(int fd, struct media_device_info &info)\n> +{\n> +\tint ret;\n> +\n> +\tret = ioctl(fd, MEDIA_IOC_DEVICE_INFO, &info);\n> +\tif (ret < 0)\n> +\t\treturn -errno;\n\nI found having strerror printing the error out a good practice.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +int DeviceEnumerator::readTopology(int fd, std::map &entities)\n> +{\n> +\tstruct media_v2_topology topology;\n> +\tstruct media_v2_entity *ents = NULL;\n> +\tstruct media_v2_interface *ifaces = NULL;\n> +\tstruct media_v2_link *links = NULL;\n> +\tint ret;\n> +\n> +\twhile (true) {\n> +\t\ttopology = {};\n> +\n> +\t\tret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology);\n> +\t\tif (ret < 0)\n> +\t\t\treturn -errno;\n> +\n> +\t\t__u64 version = topology.topology_version;\n> +\n> +\t\tents = new media_v2_entity[topology.num_entities]();\n\nwhy the ending braces?\n\n> +\t\tifaces = new media_v2_interface[topology.num_interfaces]();\n> +\t\tlinks = new media_v2_link[topology.num_links]();\n> +\t\ttopology.ptr_entities = (__u64)ents;\n\nC style cast should be avoided even if that's trivial. I have:\n\n+\tstruct media_v2_entity *_ptr_e =\n+\t\t\t\tnew struct media_v2_entity[topology.num_entities];\n+\ttopology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);\n\nI think the nicer one would be:\n\n ents = new media_v2_entity[topology.num_entities];\n topology.ptr_entities = reinterpret_cast<__u64>(ents);\n\n> +\t\ttopology.ptr_interfaces = (__u64)ifaces;\n> +\t\ttopology.ptr_links = (__u64)links;\n> +\n> +\t\tret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology);\n> +\t\tif (ret < 0) {\n> +\t\t\tret = -errno;\n> +\t\t\tgoto done;\n> +\t\t}\n\nAgain, strerror is nice to have\n> +\n> +\t\tif (version == topology.topology_version)\n> +\t\t\tbreak;\n> +\n> +\t\tdelete[] links;\n> +\t\tdelete[] ifaces;\n> +\t\tdelete[] ents;\n> +\t}\n> +\n> +\tfor (unsigned int link_id = 0; link_id < topology.num_links; link_id++) {\n> +\t\tunsigned int iface_id, ent_id;\n> +\t\tstd::string devnode;\n> +\n> +\t\tif ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_INTERFACE_LINK)\n\nThis is debatable, but if not strictly necessary, let's aim for 80\ncolumns.\n\n\t\tif ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n MEDIA_LNK_FL_INTERFACE_LINK)\n\nIs equally readable\n\n> +\t\t\tcontinue;\n> +\n> +\t\tfor (iface_id = 0; iface_id < topology.num_interfaces; iface_id++)\n> +\t\t\tif (links[link_id].source_id == ifaces[iface_id].id)\n> +\t\t\t\tbreak;\n> +\n> +\t\tfor (ent_id = 0; ent_id < topology.num_entities; ent_id++)\n> +\t\t\tif (links[link_id].sink_id == ents[ent_id].id)\n> +\t\t\t\tbreak;\n> +\n> +\t\tif (ent_id >= topology.num_entities || iface_id >= topology.num_interfaces)\n\nSame here\n\n\t\tif (ent_id >= topology.num_entities ||\n iface_id >= topology.num_interfaces)\n\n> +\t\t\tcontinue;\n> +\n> +\t\tret = lookupDevnode(devnode,\n> +\t\t\t\t ifaces[iface_id].devnode.major,\n> +\t\t\t\t ifaces[iface_id].devnode.minor);\n> +\t\tif (ret)\n> +\t\t\tbreak;\n> +\n> +\t\tentities[ents[ent_id].name] = devnode;\n> +\t}\n> +done:\n> +\tdelete[] links;\n> +\tdelete[] ifaces;\n> +\tdelete[] ents;\n> +\n> +\treturn ret;\n> +}\n> +\n> +DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const\n> +{\n> +\tDeviceInfo *info = NULL;\n\nIn C++ code is more commonly used 'nullptr'\nI know... :)\n\n> +\n> +\tfor (DeviceInfo *dev : devices_) {\n> +\t\tif (dev->busy())\n> +\t\t\tcontinue;\n> +\n> +\t\tif (dm.match(dev)) {\n> +\t\t\tinfo = dev;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\treturn info;\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h\n> index fb412b8840cb2ffe..cbe17774edb7fcc5 100644\n> --- a/src/libcamera/include/deviceenumerator.h\n> +++ b/src/libcamera/include/deviceenumerator.h\n> @@ -56,6 +56,28 @@ private:\n> \tbool matchEntities(const std::vector &entities) const;\n> };\n>\n> +class DeviceEnumerator\n> +{\n> +public:\n> +\tvirtual ~DeviceEnumerator();\n> +\n> +\tvirtual int init() = 0;\n> +\tvirtual int enumerate() = 0;\n> +\n> +\tDeviceInfo *search(DeviceMatch &dm) const;\n> +\n> +protected:\n> +\tint addDevice(const std::string &devnode);\n> +\n> +private:\n> +\tstd::vector devices_;\n> +\n> +\tint readInfo(int fd, struct media_device_info &info);\n> +\tint readTopology(int fd, std::map &entities);\n> +\n> +\tvirtual int lookupDevnode(std::string &devnode, int major, int minor) = 0;\n> +};\n> +\n> } /* namespace libcamera */\n>\n> #endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> --\n> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4762C60B1F\n\tfor ;\n\tMon, 24 Dec 2018 12:22:34 +0100 (CET)","from archlinux (host54-51-dynamic.16-87-r.retail.telecomitalia.it\n\t[87.16.51.54]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 6FE8F60002;\n\tMon, 24 Dec 2018 11:22:33 +0000 (UTC)"],"X-Originating-IP":"87.16.51.54","Date":"Mon, 24 Dec 2018 12:22:32 +0100","From":"Jacopo Mondi ","To":"Niklas S??derlund ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181224112232.GE2364@archlinux>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"SWTRyWv/ijrBap1m\"","Content-Disposition":"inline","In-Reply-To":"<20181222230041.29999-6-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator:\n\tadd DeviceEnumerator class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Dec 2018 11:22:34 -0000"}},{"id":88,"web_url":"https://patchwork.libcamera.org/comment/88/","msgid":"<20181228032325.GR19796@bigcity.dyn.berto.se>","date":"2018-12-28T03:23:25","subject":"Re: [libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator:\n\tadd DeviceEnumerator class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"HI Jacopo,\n\nThanks for your comments.\n\nOn 2018-12-24 12:22:32 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> a few minor nits\n> \n> On Sun, Dec 23, 2018 at 12:00:34AM +0100, Niklas S??derlund wrote:\n> > Provide a DeviceEnumerator base class which enumerates all media devices\n> > in the system and information about them, resolving V4L2 data structures\n> > to paths and a method to search in all the enumerated information.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/deviceenumerator.cpp | 144 +++++++++++++++++++++++\n> > src/libcamera/include/deviceenumerator.h | 22 ++++\n> > 2 files changed, 166 insertions(+)\n> >\n> > diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp\n> > index 17b6874d229c791c..b2f59017e94d14aa 100644\n> > --- a/src/libcamera/deviceenumerator.cpp\n> > +++ b/src/libcamera/deviceenumerator.cpp\n> > @@ -5,6 +5,10 @@\n> > * deviceenumerator.cpp - Enumeration and matching\n> > */\n> >\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > #include \"deviceenumerator.h\"\n> > #include \"log.h\"\n> >\n> > @@ -129,4 +133,144 @@ bool DeviceMatch::matchEntities(const std::vector &entities) const\n> > \treturn true;\n> > }\n> >\n> > +/* -----------------------------------------------------------------------------\n> > + * Enumerator Base\n> > + */\n> > +\n> > +DeviceEnumerator::~DeviceEnumerator()\n> > +{\n> > +\tfor (DeviceInfo *dev : devices_) {\n> > +\t\tif (dev->busy())\n> > +\t\t\tLOG(Error) << \"Removing device info while still in use\";\n> > +\n> > +\t\tdelete dev;\n> > +\t}\n> > +}\n> > +\n> > +int DeviceEnumerator::addDevice(const std::string &devnode)\n> > +{\n> > +\tint fd, ret;\n> > +\n> > +\tstruct media_device_info info = {};\n> > +\tstd::map entities;\n> > +\n> > +\tfd = open(devnode.c_str(), O_RDWR);\n> > +\tif (fd < 0)\n> > +\t\treturn fd;\n> \n> return -errno and printout error with strerror() ?\n> We should standardize on error handline as much as we could.\n\n I agree. What path should we take?\n\n> \n> > +\n> > +\tret = readInfo(fd, info);\n> > +\tif (ret)\n> > +\t\tgoto out;\n> > +\n> > +\tret = readTopology(fd, entities);\n> > +\tif (ret)\n> > +\t\tgoto out;\n> > +\n> > +\tdevices_.push_back(new DeviceInfo(devnode, info, entities));\n> > +out:\n> > +\tclose(fd);\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +int DeviceEnumerator::readInfo(int fd, struct media_device_info &info)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tret = ioctl(fd, MEDIA_IOC_DEVICE_INFO, &info);\n> > +\tif (ret < 0)\n> > +\t\treturn -errno;\n> \n> I found having strerror printing the error out a good practice.\n\nNo objection, see comment above.\n\n\n> \n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int DeviceEnumerator::readTopology(int fd, std::map &entities)\n> > +{\n> > +\tstruct media_v2_topology topology;\n> > +\tstruct media_v2_entity *ents = NULL;\n> > +\tstruct media_v2_interface *ifaces = NULL;\n> > +\tstruct media_v2_link *links = NULL;\n> > +\tint ret;\n> > +\n> > +\twhile (true) {\n> > +\t\ttopology = {};\n> > +\n> > +\t\tret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn -errno;\n> > +\n> > +\t\t__u64 version = topology.topology_version;\n> > +\n> > +\t\tents = new media_v2_entity[topology.num_entities]();\n> \n> why the ending braces?\n\nIn C++ they zero the object, think memset().\n\n> \n> > +\t\tifaces = new media_v2_interface[topology.num_interfaces]();\n> > +\t\tlinks = new media_v2_link[topology.num_links]();\n> > +\t\ttopology.ptr_entities = (__u64)ents;\n> \n> C style cast should be avoided even if that's trivial. I have:\n> \n> +\tstruct media_v2_entity *_ptr_e =\n> +\t\t\t\tnew struct media_v2_entity[topology.num_entities];\n> +\ttopology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);\n> \n> I think the nicer one would be:\n> \n> ents = new media_v2_entity[topology.num_entities];\n> topology.ptr_entities = reinterpret_cast<__u64>(ents);\n> \n\nYou might be right.\n\n\n> > +\t\ttopology.ptr_interfaces = (__u64)ifaces;\n> > +\t\ttopology.ptr_links = (__u64)links;\n> > +\n> > +\t\tret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > +\t\tif (ret < 0) {\n> > +\t\t\tret = -errno;\n> > +\t\t\tgoto done;\n> > +\t\t}\n> \n> Again, strerror is nice to have\n> > +\n> > +\t\tif (version == topology.topology_version)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tdelete[] links;\n> > +\t\tdelete[] ifaces;\n> > +\t\tdelete[] ents;\n> > +\t}\n> > +\n> > +\tfor (unsigned int link_id = 0; link_id < topology.num_links; link_id++) {\n> > +\t\tunsigned int iface_id, ent_id;\n> > +\t\tstd::string devnode;\n> > +\n> > +\t\tif ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_INTERFACE_LINK)\n> \n> This is debatable, but if not strictly necessary, let's aim for 80\n> columns.\n> \n> \t\tif ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n> MEDIA_LNK_FL_INTERFACE_LINK)\n> \n> Is equally readable\n> \n> > +\t\t\tcontinue;\n> > +\n> > +\t\tfor (iface_id = 0; iface_id < topology.num_interfaces; iface_id++)\n> > +\t\t\tif (links[link_id].source_id == ifaces[iface_id].id)\n> > +\t\t\t\tbreak;\n> > +\n> > +\t\tfor (ent_id = 0; ent_id < topology.num_entities; ent_id++)\n> > +\t\t\tif (links[link_id].sink_id == ents[ent_id].id)\n> > +\t\t\t\tbreak;\n> > +\n> > +\t\tif (ent_id >= topology.num_entities || iface_id >= topology.num_interfaces)\n> \n> Same here\n> \n> \t\tif (ent_id >= topology.num_entities ||\n> iface_id >= topology.num_interfaces)\n> \n\nNo strong opinion, with a 120 limit I think this and the thing above is \nmore readable. I will fix as the group judge.\n\n\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tret = lookupDevnode(devnode,\n> > +\t\t\t\t ifaces[iface_id].devnode.major,\n> > +\t\t\t\t ifaces[iface_id].devnode.minor);\n> > +\t\tif (ret)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tentities[ents[ent_id].name] = devnode;\n> > +\t}\n> > +done:\n> > +\tdelete[] links;\n> > +\tdelete[] ifaces;\n> > +\tdelete[] ents;\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const\n> > +{\n> > +\tDeviceInfo *info = NULL;\n> \n> In C++ code is more commonly used 'nullptr'\n> I know... :)\n\nNo preference, I let the group mob rule.\n\n> \n> > +\n> > +\tfor (DeviceInfo *dev : devices_) {\n> > +\t\tif (dev->busy())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tif (dm.match(dev)) {\n> > +\t\t\tinfo = dev;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn info;\n> > +}\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h\n> > index fb412b8840cb2ffe..cbe17774edb7fcc5 100644\n> > --- a/src/libcamera/include/deviceenumerator.h\n> > +++ b/src/libcamera/include/deviceenumerator.h\n> > @@ -56,6 +56,28 @@ private:\n> > \tbool matchEntities(const std::vector &entities) const;\n> > };\n> >\n> > +class DeviceEnumerator\n> > +{\n> > +public:\n> > +\tvirtual ~DeviceEnumerator();\n> > +\n> > +\tvirtual int init() = 0;\n> > +\tvirtual int enumerate() = 0;\n> > +\n> > +\tDeviceInfo *search(DeviceMatch &dm) const;\n> > +\n> > +protected:\n> > +\tint addDevice(const std::string &devnode);\n> > +\n> > +private:\n> > +\tstd::vector devices_;\n> > +\n> > +\tint readInfo(int fd, struct media_device_info &info);\n> > +\tint readTopology(int fd, std::map &entities);\n> > +\n> > +\tvirtual int lookupDevnode(std::string &devnode, int major, int minor) = 0;\n> > +};\n> > +\n> > } /* namespace libcamera */\n> >\n> > #endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6364600CC\n\tfor ;\n\tFri, 28 Dec 2018 04:23:27 +0100 (CET)","by mail-lf1-x144.google.com with SMTP id a16so13801232lfg.3\n\tfor ;\n\tThu, 27 Dec 2018 19:23:27 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tc2-v6sm8305888ljj.41.2018.12.27.19.23.26\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 27 Dec 2018 19:23:26 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=rMUrEO8dbTcAgowUoN5fIDE/5w+lyQSfwrFcnUff6Q0=;\n\tb=x12DnmlJ/fb0gUGuASWyijtKhvq6YJyCywrea/wHLeS5McsXGVDMBRcKle0fXr+mK3\n\tbFfBfSmGkPqqf30XIe5dbvOrMNsyj5OCrBoxl3h41UrU7Uzg2LaBfvz59DcbxQsB3Xt2\n\tEHkMVKvHPnxowO8jU0+FfX/9w1nArX+arA2nkwhprMTbLUIQzB2nGR5LIP+YyHJqJ4Z7\n\txvUaDL84ehFbab2Yc+hMuALKcZp2i+gY6FXw68cMjnVZ0sJ2yaD9C8IzkCuJAoLn68v6\n\tSoHA9OqX6tG/d8jDyrnXKuf382EIwMUPGU5Z/Gm+rr2E+/xUjdTRb1hZWBS/J6fP1bVH\n\tWoqw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=rMUrEO8dbTcAgowUoN5fIDE/5w+lyQSfwrFcnUff6Q0=;\n\tb=JvSScCb3k6tGHV/zn9uck7GNRMVxutlF5SRD7DXQVBgDdYfhl456ie8iuCtQSeFthV\n\t+LX27kehlyTDbGGpNkff6TUrFpQnET+A2VUy8tqNNRXopR+3szZMGz6hXhCpQOGIr5Sh\n\tSZ7HHP0NC30PpcUfGzgcHltLUyK1KFLDaSv/Fip9+MgF2FeV6jbDgudJkk7ffgI2CqWT\n\tJcck+roF1/gzVqBrvLk3pwYcYj6yf9t6liBkB41smicdrLpPbYbwMrblkEynQv58sACF\n\to+dEDJ/687gRFRY1aRSZRHMAZ6Ncn9IfD7TxP+8uIBrrVxZIEgSbtyQ2A2fH8d2gFH2z\n\tpLSA==","X-Gm-Message-State":"AA+aEWaDRtDhrL1pwrkvOgMP5FJMdx0qBEwo8wiMbC92OnPRbin1765J\n\tbH6FLyCrtQNUwF+fWv4kxqfpjIjFaNg=","X-Google-Smtp-Source":"AFSGD/W6cqWl2WN+ba8KZazIlz8i+42mK4wY+EpKNLGVQ9FUdC4qITtQdEMqG8+JAb+nlutpWTmTKw==","X-Received":"by 2002:a19:1cb:: with SMTP id 194mr12817551lfb.61.1545967407077;\n\tThu, 27 Dec 2018 19:23:27 -0800 (PST)","Date":"Fri, 28 Dec 2018 04:23:25 +0100","From":"Niklas S??derlund ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181228032325.GR19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-6-niklas.soderlund@ragnatech.se>\n\t<20181224112232.GE2364@archlinux>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20181224112232.GE2364@archlinux>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator:\n\tadd DeviceEnumerator class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Dec 2018 03:23:28 -0000"}},{"id":106,"web_url":"https://patchwork.libcamera.org/comment/106/","msgid":"<5950363.ORbLtkCA0S@avalon>","date":"2018-12-28T22:53:22","subject":"Re: [libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator:\n\tadd DeviceEnumerator class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Friday, 28 December 2018 05:23:25 EET Niklas S??derlund wrote:\n> On 2018-12-24 12:22:32 +0100, Jacopo Mondi wrote:\n> > On Sun, Dec 23, 2018 at 12:00:34AM +0100, Niklas S??derlund wrote:\n> > > Provide a DeviceEnumerator base class which enumerates all media devices\n> > > in the system and information about them, resolving V4L2 data structures\n> > > to paths and a method to search in all the enumerated information.\n\nIt's not V4L2 but Media Controller.\n\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > \n> > > src/libcamera/deviceenumerator.cpp | 144 +++++++++++++++++++++++\n> > > src/libcamera/include/deviceenumerator.h | 22 ++++\n> > > 2 files changed, 166 insertions(+)\n> > > \n> > > diff --git a/src/libcamera/deviceenumerator.cpp\n> > > b/src/libcamera/deviceenumerator.cpp index\n> > > 17b6874d229c791c..b2f59017e94d14aa 100644\n> > > --- a/src/libcamera/deviceenumerator.cpp\n> > > +++ b/src/libcamera/deviceenumerator.cpp\n> > > @@ -5,6 +5,10 @@\n> > > * deviceenumerator.cpp - Enumeration and matching\n> > > */\n> > > \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > #include \"deviceenumerator.h\"\n> > > #include \"log.h\"\n> > > \n> > > @@ -129,4 +133,144 @@ bool DeviceMatch::matchEntities(const\n> > > std::vector &entities) const\n> > > \treturn true;\n> > > }\n> > > \n> > > +/* --------------------------------------------------------------------\n> > > + * Enumerator Base\n> > > + */\n> > > +\n> > > +DeviceEnumerator::~DeviceEnumerator()\n> > > +{\n> > > +\tfor (DeviceInfo *dev : devices_) {\n> > > +\t\tif (dev->busy())\n> > > +\t\t\tLOG(Error) << \"Removing device info while still in use\";\n> > > +\n> > > +\t\tdelete dev;\n> > > +\t}\n> > > +}\n> > > +\n> > > +int DeviceEnumerator::addDevice(const std::string &devnode)\n> > > +{\n> > > +\tint fd, ret;\n> > > +\n> > > +\tstruct media_device_info info = {};\n> > > +\tstd::map entities;\n> > > +\n> > > +\tfd = open(devnode.c_str(), O_RDWR);\n> > > +\tif (fd < 0)\n> > > +\t\treturn fd;\n> > \n> > return -errno and printout error with strerror() ?\n> > We should standardize on error handline as much as we could.\n> \n> I agree. What path should we take?\n\nI propose returning a negative error code on error, which would be -errno \nhere. Logging a message would also be useful, but please note that errno could \nbe modified by the message logging, so you need to save the variable first.\n\n\tfd = open(devnode.c_str(), O_RDWR);\n\tif (fd < 0) {\n\t\tret = -errno;\n\t\tLog(Info) << \"Unable to open \" << devnode <<\" (\" << strerror(-ret) << \n\"), skipping\";\n\t\treturn ret;\n\t}\n\n> > > +\n> > > +\tret = readInfo(fd, info);\n> > > +\tif (ret)\n> > > +\t\tgoto out;\n> > > +\n> > > +\tret = readTopology(fd, entities);\n> > > +\tif (ret)\n> > > +\t\tgoto out;\n> > > +\n> > > +\tdevices_.push_back(new DeviceInfo(devnode, info, entities));\n> > > +out:\n> > > +\tclose(fd);\n> > > +\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > +int DeviceEnumerator::readInfo(int fd, struct media_device_info &info)\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\tret = ioctl(fd, MEDIA_IOC_DEVICE_INFO, &info);\n> > > +\tif (ret < 0)\n> > > +\t\treturn -errno;\n> > \n> > I found having strerror printing the error out a good practice.\n> \n> No objection, see comment above.\n> \n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int DeviceEnumerator::readTopology(int fd, std::map > > std::string> &entities)\n> > > +{\n> > > +\tstruct media_v2_topology topology;\n> > > +\tstruct media_v2_entity *ents = NULL;\n> > > +\tstruct media_v2_interface *ifaces = NULL;\n> > > +\tstruct media_v2_link *links = NULL;\n> > > +\tint ret;\n> > > +\n> > > +\twhile (true) {\n> > > +\t\ttopology = {};\n> > > +\n> > > +\t\tret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > > +\t\tif (ret < 0)\n> > > +\t\t\treturn -errno;\n> > > +\n> > > +\t\t__u64 version = topology.topology_version;\n> > > +\n> > > +\t\tents = new media_v2_entity[topology.num_entities]();\n> > \n> > why the ending braces?\n> \n> In C++ they zero the object, think memset().\n> \n> > > +\t\tifaces = new media_v2_interface[topology.num_interfaces]();\n> > > +\t\tlinks = new media_v2_link[topology.num_links]();\n> > > +\t\ttopology.ptr_entities = (__u64)ents;\n> > \n> > C style cast should be avoided even if that's trivial. I have:\n> > \n> > +\tstruct media_v2_entity *_ptr_e =\n> > +\t\t\t\tnew struct media_v2_entity[topology.num_entities];\n> > +\ttopology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);\n> > \n> > I think the nicer one would be:\n> > ents = new media_v2_entity[topology.num_entities];\n> > topology.ptr_entities = reinterpret_cast<__u64>(ents);\n> \n> You might be right.\n> \n> > > +\t\ttopology.ptr_interfaces = (__u64)ifaces;\n> > > +\t\ttopology.ptr_links = (__u64)links;\n> > > +\n> > > +\t\tret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > > +\t\tif (ret < 0) {\n> > > +\t\t\tret = -errno;\n> > > +\t\t\tgoto done;\n> > > +\t\t}\n> > \n> > Again, strerror is nice to have\n> > \n> > > +\n> > > +\t\tif (version == topology.topology_version)\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tdelete[] links;\n> > > +\t\tdelete[] ifaces;\n> > > +\t\tdelete[] ents;\n> > > +\t}\n> > > +\n> > > +\tfor (unsigned int link_id = 0; link_id < topology.num_links;\n> > > link_id++) {\n> > > +\t\tunsigned int iface_id, ent_id;\n> > > +\t\tstd::string devnode;\n> > > +\n> > > +\t\tif ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n> > > MEDIA_LNK_FL_INTERFACE_LINK)\n> > \n> > This is debatable, but if not strictly necessary, let's aim for 80\n> > columns.\n> > \n> > \t\tif ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n> > MEDIA_LNK_FL_INTERFACE_LINK)\n> > \n> > Is equally readable\n> > \n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tfor (iface_id = 0; iface_id < topology.num_interfaces; iface_id+\n+)\n> > > +\t\t\tif (links[link_id].source_id == ifaces[iface_id].id)\n> > > +\t\t\t\tbreak;\n> > > +\n> > > +\t\tfor (ent_id = 0; ent_id < topology.num_entities; ent_id++)\n> > > +\t\t\tif (links[link_id].sink_id == ents[ent_id].id)\n> > > +\t\t\t\tbreak;\n> > > +\n> > > +\t\tif (ent_id >= topology.num_entities || iface_id >=\n> > > topology.num_interfaces)> \n> > \n> > Same here\n> > \n> > \t\tif (ent_id >= topology.num_entities ||\n> > iface_id >= topology.num_interfaces)\n> \n> No strong opinion, with a 120 limit I think this and the thing above is\n> more readable. I will fix as the group judge.\n\nI find Jacopo's suggestion here more readable, but perhaps because I'm used to \nwrap lines at the 80 columns limit. I don't want to force a particular choice \nhere.\n\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tret = lookupDevnode(devnode,\n> > > +\t\t\t\t ifaces[iface_id].devnode.major,\n> > > +\t\t\t\t ifaces[iface_id].devnode.minor);\n> > > +\t\tif (ret)\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tentities[ents[ent_id].name] = devnode;\n> > > +\t}\n> > > +done:\n> > > +\tdelete[] links;\n> > > +\tdelete[] ifaces;\n> > > +\tdelete[] ents;\n> > > +\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > +DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const\n> > > +{\n> > > +\tDeviceInfo *info = NULL;\n> > \n> > In C++ code is more commonly used 'nullptr'\n> > I know... :)\n> \n> No preference, I let the group mob rule.\n> \n> > > +\n> > > +\tfor (DeviceInfo *dev : devices_) {\n> > > +\t\tif (dev->busy())\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tif (dm.match(dev)) {\n> > > +\t\t\tinfo = dev;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\treturn info;\n> > > +}\n> > > +\n> > > \n> > > } /* namespace libcamera */\n> > > \n> > > diff --git a/src/libcamera/include/deviceenumerator.h\n> > > b/src/libcamera/include/deviceenumerator.h index\n> > > fb412b8840cb2ffe..cbe17774edb7fcc5 100644\n> > > --- a/src/libcamera/include/deviceenumerator.h\n> > > +++ b/src/libcamera/include/deviceenumerator.h\n> > > \n> > > @@ -56,6 +56,28 @@ private:\n> > > \tbool matchEntities(const std::vector &entities) const;\n> > > };\n> > > \n> > > +class DeviceEnumerator\n> > > +{\n> > > +public:\n> > > +\tvirtual ~DeviceEnumerator();\n> > > +\n> > > +\tvirtual int init() = 0;\n> > > +\tvirtual int enumerate() = 0;\n> > > +\n> > > +\tDeviceInfo *search(DeviceMatch &dm) const;\n> > > +\n> > > +protected:\n> > > +\tint addDevice(const std::string &devnode);\n> > > +\n> > > +private:\n> > > +\tstd::vector devices_;\n> > > +\n> > > +\tint readInfo(int fd, struct media_device_info &info);\n> > > +\tint readTopology(int fd, std::map\n> > > &entities);\n\nDo you foresee the above two methods being moved to MediaDevice ?\n\n> > > +\tvirtual int lookupDevnode(std::string &devnode, int major, int \nminor)\n> > > = 0;\n> > > +};\n> > > +\n> > > \n> > > } /* namespace libcamera */\n> > > \n> > > #endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF28B60B2E\n\tfor ;\n\tFri, 28 Dec 2018 23:52:26 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:2788:66a:3eb:2624:a446:f4b7:b19d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 40A64DF;\n\tFri, 28 Dec 2018 23:52:26 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546037546;\n\tbh=eq/veUlgezJ9RcExleK4OpdoCk+VEs1phRBbT3q6HI8=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=VjORkaPwhR0i/6oUdJUJ2Yl6e25d9cT+tCGDN54hULAFkwE6aKt15JofAkRv4Qvit\n\t0PXbWe27pXAtkdTTBLcaXC8jh1hECgraCnGa/ezZirqVe7p3tcuMl5TeGez+slOCfQ\n\tRhUS/0NRDscB8hBd+YouO9Yp/ipKqZ6rEzxwLTjw=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Sat, 29 Dec 2018 00:53:22 +0200","Message-ID":"<5950363.ORbLtkCA0S@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181228032325.GR19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181224112232.GE2364@archlinux>\n\t<20181228032325.GR19796@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator:\n\tadd DeviceEnumerator class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Dec 2018 22:52:27 -0000"}},{"id":116,"web_url":"https://patchwork.libcamera.org/comment/116/","msgid":"<20181229012706.GB19796@bigcity.dyn.berto.se>","date":"2018-12-29T01:27:06","subject":"Re: [libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator:\n\tadd DeviceEnumerator class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2018-12-29 00:53:22 +0200, Laurent Pinchart wrote:\n> Hello,\n> \n> On Friday, 28 December 2018 05:23:25 EET Niklas S??derlund wrote:\n> > On 2018-12-24 12:22:32 +0100, Jacopo Mondi wrote:\n> > > On Sun, Dec 23, 2018 at 12:00:34AM +0100, Niklas S??derlund wrote:\n> > > > Provide a DeviceEnumerator base class which enumerates all media devices\n> > > > in the system and information about them, resolving V4L2 data structures\n> > > > to paths and a method to search in all the enumerated information.\n> \n> It's not V4L2 but Media Controller.\n\nThanks.\n\n> \n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > \n> > > > src/libcamera/deviceenumerator.cpp | 144 +++++++++++++++++++++++\n> > > > src/libcamera/include/deviceenumerator.h | 22 ++++\n> > > > 2 files changed, 166 insertions(+)\n> > > > \n> > > > diff --git a/src/libcamera/deviceenumerator.cpp\n> > > > b/src/libcamera/deviceenumerator.cpp index\n> > > > 17b6874d229c791c..b2f59017e94d14aa 100644\n> > > > --- a/src/libcamera/deviceenumerator.cpp\n> > > > +++ b/src/libcamera/deviceenumerator.cpp\n> > > > @@ -5,6 +5,10 @@\n> > > > * deviceenumerator.cpp - Enumeration and matching\n> > > > */\n> > > > \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > #include \"deviceenumerator.h\"\n> > > > #include \"log.h\"\n> > > > \n> > > > @@ -129,4 +133,144 @@ bool DeviceMatch::matchEntities(const\n> > > > std::vector &entities) const\n> > > > \treturn true;\n> > > > }\n> > > > \n> > > > +/* --------------------------------------------------------------------\n> > > > + * Enumerator Base\n> > > > + */\n> > > > +\n> > > > +DeviceEnumerator::~DeviceEnumerator()\n> > > > +{\n> > > > +\tfor (DeviceInfo *dev : devices_) {\n> > > > +\t\tif (dev->busy())\n> > > > +\t\t\tLOG(Error) << \"Removing device info while still in use\";\n> > > > +\n> > > > +\t\tdelete dev;\n> > > > +\t}\n> > > > +}\n> > > > +\n> > > > +int DeviceEnumerator::addDevice(const std::string &devnode)\n> > > > +{\n> > > > +\tint fd, ret;\n> > > > +\n> > > > +\tstruct media_device_info info = {};\n> > > > +\tstd::map entities;\n> > > > +\n> > > > +\tfd = open(devnode.c_str(), O_RDWR);\n> > > > +\tif (fd < 0)\n> > > > +\t\treturn fd;\n> > > \n> > > return -errno and printout error with strerror() ?\n> > > We should standardize on error handline as much as we could.\n> > \n> > I agree. What path should we take?\n> \n> I propose returning a negative error code on error, which would be -errno \n> here. Logging a message would also be useful, but please note that errno could \n> be modified by the message logging, so you need to save the variable first.\n> \n> \tfd = open(devnode.c_str(), O_RDWR);\n> \tif (fd < 0) {\n> \t\tret = -errno;\n> \t\tLog(Info) << \"Unable to open \" << devnode <<\" (\" << strerror(-ret) << \n> \"), skipping\";\n> \t\treturn ret;\n> \t}\n\nMob rule, I have taken this approach where Jacopo pointed out it could \nbe beneficial.\n\n> \n> > > > +\n> > > > +\tret = readInfo(fd, info);\n> > > > +\tif (ret)\n> > > > +\t\tgoto out;\n> > > > +\n> > > > +\tret = readTopology(fd, entities);\n> > > > +\tif (ret)\n> > > > +\t\tgoto out;\n> > > > +\n> > > > +\tdevices_.push_back(new DeviceInfo(devnode, info, entities));\n> > > > +out:\n> > > > +\tclose(fd);\n> > > > +\n> > > > +\treturn ret;\n> > > > +}\n> > > > +\n> > > > +int DeviceEnumerator::readInfo(int fd, struct media_device_info &info)\n> > > > +{\n> > > > +\tint ret;\n> > > > +\n> > > > +\tret = ioctl(fd, MEDIA_IOC_DEVICE_INFO, &info);\n> > > > +\tif (ret < 0)\n> > > > +\t\treturn -errno;\n> > > \n> > > I found having strerror printing the error out a good practice.\n> > \n> > No objection, see comment above.\n> > \n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +int DeviceEnumerator::readTopology(int fd, std::map > > > std::string> &entities)\n> > > > +{\n> > > > +\tstruct media_v2_topology topology;\n> > > > +\tstruct media_v2_entity *ents = NULL;\n> > > > +\tstruct media_v2_interface *ifaces = NULL;\n> > > > +\tstruct media_v2_link *links = NULL;\n> > > > +\tint ret;\n> > > > +\n> > > > +\twhile (true) {\n> > > > +\t\ttopology = {};\n> > > > +\n> > > > +\t\tret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > > > +\t\tif (ret < 0)\n> > > > +\t\t\treturn -errno;\n> > > > +\n> > > > +\t\t__u64 version = topology.topology_version;\n> > > > +\n> > > > +\t\tents = new media_v2_entity[topology.num_entities]();\n> > > \n> > > why the ending braces?\n> > \n> > In C++ they zero the object, think memset().\n> > \n> > > > +\t\tifaces = new media_v2_interface[topology.num_interfaces]();\n> > > > +\t\tlinks = new media_v2_link[topology.num_links]();\n> > > > +\t\ttopology.ptr_entities = (__u64)ents;\n> > > \n> > > C style cast should be avoided even if that's trivial. I have:\n> > > \n> > > +\tstruct media_v2_entity *_ptr_e =\n> > > +\t\t\t\tnew struct media_v2_entity[topology.num_entities];\n> > > +\ttopology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);\n> > > \n> > > I think the nicer one would be:\n> > > ents = new media_v2_entity[topology.num_entities];\n> > > topology.ptr_entities = reinterpret_cast<__u64>(ents);\n> > \n> > You might be right.\n> > \n> > > > +\t\ttopology.ptr_interfaces = (__u64)ifaces;\n> > > > +\t\ttopology.ptr_links = (__u64)links;\n> > > > +\n> > > > +\t\tret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > > > +\t\tif (ret < 0) {\n> > > > +\t\t\tret = -errno;\n> > > > +\t\t\tgoto done;\n> > > > +\t\t}\n> > > \n> > > Again, strerror is nice to have\n> > > \n> > > > +\n> > > > +\t\tif (version == topology.topology_version)\n> > > > +\t\t\tbreak;\n> > > > +\n> > > > +\t\tdelete[] links;\n> > > > +\t\tdelete[] ifaces;\n> > > > +\t\tdelete[] ents;\n> > > > +\t}\n> > > > +\n> > > > +\tfor (unsigned int link_id = 0; link_id < topology.num_links;\n> > > > link_id++) {\n> > > > +\t\tunsigned int iface_id, ent_id;\n> > > > +\t\tstd::string devnode;\n> > > > +\n> > > > +\t\tif ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n> > > > MEDIA_LNK_FL_INTERFACE_LINK)\n> > > \n> > > This is debatable, but if not strictly necessary, let's aim for 80\n> > > columns.\n> > > \n> > > \t\tif ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n> > > MEDIA_LNK_FL_INTERFACE_LINK)\n> > > \n> > > Is equally readable\n> > > \n> > > > +\t\t\tcontinue;\n> > > > +\n> > > > +\t\tfor (iface_id = 0; iface_id < topology.num_interfaces; iface_id+\n> +)\n> > > > +\t\t\tif (links[link_id].source_id == ifaces[iface_id].id)\n> > > > +\t\t\t\tbreak;\n> > > > +\n> > > > +\t\tfor (ent_id = 0; ent_id < topology.num_entities; ent_id++)\n> > > > +\t\t\tif (links[link_id].sink_id == ents[ent_id].id)\n> > > > +\t\t\t\tbreak;\n> > > > +\n> > > > +\t\tif (ent_id >= topology.num_entities || iface_id >=\n> > > > topology.num_interfaces)> \n> > > \n> > > Same here\n> > > \n> > > \t\tif (ent_id >= topology.num_entities ||\n> > > iface_id >= topology.num_interfaces)\n> > \n> > No strong opinion, with a 120 limit I think this and the thing above is\n> > more readable. I will fix as the group judge.\n> \n> I find Jacopo's suggestion here more readable, but perhaps because I'm used to \n> wrap lines at the 80 columns limit. I don't want to force a particular choice \n> here.\n\n80 it is then :-)\n\n> \n> > > > +\t\t\tcontinue;\n> > > > +\n> > > > +\t\tret = lookupDevnode(devnode,\n> > > > +\t\t\t\t ifaces[iface_id].devnode.major,\n> > > > +\t\t\t\t ifaces[iface_id].devnode.minor);\n> > > > +\t\tif (ret)\n> > > > +\t\t\tbreak;\n> > > > +\n> > > > +\t\tentities[ents[ent_id].name] = devnode;\n> > > > +\t}\n> > > > +done:\n> > > > +\tdelete[] links;\n> > > > +\tdelete[] ifaces;\n> > > > +\tdelete[] ents;\n> > > > +\n> > > > +\treturn ret;\n> > > > +}\n> > > > +\n> > > > +DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const\n> > > > +{\n> > > > +\tDeviceInfo *info = NULL;\n> > > \n> > > In C++ code is more commonly used 'nullptr'\n> > > I know... :)\n> > \n> > No preference, I let the group mob rule.\n> > \n> > > > +\n> > > > +\tfor (DeviceInfo *dev : devices_) {\n> > > > +\t\tif (dev->busy())\n> > > > +\t\t\tcontinue;\n> > > > +\n> > > > +\t\tif (dm.match(dev)) {\n> > > > +\t\t\tinfo = dev;\n> > > > +\t\t\tbreak;\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > +\treturn info;\n> > > > +}\n> > > > +\n> > > > \n> > > > } /* namespace libcamera */\n> > > > \n> > > > diff --git a/src/libcamera/include/deviceenumerator.h\n> > > > b/src/libcamera/include/deviceenumerator.h index\n> > > > fb412b8840cb2ffe..cbe17774edb7fcc5 100644\n> > > > --- a/src/libcamera/include/deviceenumerator.h\n> > > > +++ b/src/libcamera/include/deviceenumerator.h\n> > > > \n> > > > @@ -56,6 +56,28 @@ private:\n> > > > \tbool matchEntities(const std::vector &entities) const;\n> > > > };\n> > > > \n> > > > +class DeviceEnumerator\n> > > > +{\n> > > > +public:\n> > > > +\tvirtual ~DeviceEnumerator();\n> > > > +\n> > > > +\tvirtual int init() = 0;\n> > > > +\tvirtual int enumerate() = 0;\n> > > > +\n> > > > +\tDeviceInfo *search(DeviceMatch &dm) const;\n> > > > +\n> > > > +protected:\n> > > > +\tint addDevice(const std::string &devnode);\n> > > > +\n> > > > +private:\n> > > > +\tstd::vector devices_;\n> > > > +\n> > > > +\tint readInfo(int fd, struct media_device_info &info);\n> > > > +\tint readTopology(int fd, std::map\n> > > > &entities);\n> \n> Do you foresee the above two methods being moved to MediaDevice ?\n\nI think so, or adopt Jacopo's implementation. As readTopology() \nequivalent would be needed anyhow for a MediaDevice to get links for \nexample.\n\n> \n> > > > +\tvirtual int lookupDevnode(std::string &devnode, int major, int \n> minor)\n> > > > = 0;\n> > > > +};\n> > > > +\n> > > > \n> > > > } /* namespace libcamera */\n> > > > \n> > > > #endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> \n>","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDF1960B30\n\tfor ;\n\tSat, 29 Dec 2018 02:27:08 +0100 (CET)","by mail-lj1-x244.google.com with SMTP id n18-v6so19919440lji.7\n\tfor ;\n\tFri, 28 Dec 2018 17:27:08 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tr1-v6sm8837807ljb.64.2018.12.28.17.27.07\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 28 Dec 2018 17:27:07 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=dKpeLni6hTQrUEc+M1f1tcb4Rxwvy8JPAzIPq170xlk=;\n\tb=L8r7mL1QDj1v+fkE6uqj7rrrCnn48lhcYCN2M1dwnCJIfFxv8e78F5T5oSraphH+Hs\n\tO6jGkQoI3GN+fuLYEHx2v3/NfYIs+TWLeOcXFvQjtXQzysVYkonRjMY3Z6Nb+Z3vJcMy\n\tft7qEaIPOmxQ4D2mN16gnk5ED+DqgIZ4jD715l400Oa2m8ju7xKtyE7VaZCiUpL8TDrO\n\tVu76KgOjKNz/fk1JSAlnqUsAbmHY+QNfAz2gCN7USYCFUYz4+0GIs2/dvpCCmna3xH96\n\tW8avrhs1XxNF2GjMZzFE7Eqd8RzOqvxSgrGIz4W42LzbQRWka5TAEz/6PgchGokNNPnY\n\tt7Mw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=dKpeLni6hTQrUEc+M1f1tcb4Rxwvy8JPAzIPq170xlk=;\n\tb=osa7wejzFjUAGuxmujuuxP3IqMp4e0zUtqlNhst7CYSbymqns1sEEjRbttogBxm43d\n\t26slWlxGzEe9r60KAaCIuSfMH7wxxsziWNOPWYasF5BWZy8Zdlt97wjpvQx9ztnEGQQe\n\tg2vULyvTthS97EqG8AcAyH4UFTJ1U5LdkGZA7tBi0nMmSVxl++PA8xduVb0OGjAXFnoU\n\tkOIUmr3iNWeoPOivIn3Vw9/efaKwMA0tVg2Lyk1ldFsfTRmxfBSOIGd8hqfEx07iVnfN\n\tSY3bUmon8/2WAtltY0ZLZ4cxjFvVJLqzVoQdiQNo09co1mYrIw7nS65420UWL1iesPB8\n\tqKTA==","X-Gm-Message-State":"AA+aEWY2Zmpbgf656csDLFjr6zLbdQAp1PCZwd1mJqBBXXhCZprUlEj2\n\t4RkGoiqC7v0HY5vwhd3vMrzyEBEL2x0=","X-Google-Smtp-Source":"ALg8bN4pPuH8VggSK2XCsvd4H1oS3YWmBfKD3WrRIdJ9RzpSxdVkHE7DiFPbNfG92Z6VZLRAqQpuow==","X-Received":"by 2002:a2e:681a:: with SMTP id\n\tc26-v6mr16824557lja.175.1546046827982; \n\tFri, 28 Dec 2018 17:27:07 -0800 (PST)","Date":"Sat, 29 Dec 2018 02:27:06 +0100","From":"Niklas S??derlund ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Jacopo Mondi ","Message-ID":"<20181229012706.GB19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181224112232.GE2364@archlinux>\n\t<20181228032325.GR19796@bigcity.dyn.berto.se>\n\t<5950363.ORbLtkCA0S@avalon>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<5950363.ORbLtkCA0S@avalon>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator:\n\tadd DeviceEnumerator class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 29 Dec 2018 01:27:09 -0000"}}]