[{"id":160,"web_url":"https://patchwork.libcamera.org/comment/160/","msgid":"<539a808d-3f24-3fc2-e025-08e7325690a1@ideasonboard.com>","date":"2019-01-01T21:52:23","subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: mediadevice: Fix graph\n\tparsing error handling","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 01/01/2019 21:23, Laurent Pinchart wrote:\n> Most errors recorded during graph parsing are logged but not propagated\n> to the caller. Fix this and delete objects that are created but not\n> successfully added to the graph to avoid memory leaks. As the error code\n> returned from the addObject() and populate*() functions doesn't matter\n> much, turn them into bool functions.\n> \n> Additionally, add a way to query whether the media graph was valid, and\n> clear objects before populating the graph to avoid leaking them.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nThis looks good to me...\n\nA bit risky to go inverting the boolean logic of return values - but as\nyou've collected all the populate*() calls to a single expression it works.\n\n\nAlso - I think this reads nicely:\n\n> +\t\tif (!addObject(entity)) {\n\nas \"If we can't addObject() ...\"\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n\n> ---\n>  src/libcamera/include/media_device.h | 10 ++--\n>  src/libcamera/media_device.cpp       | 75 +++++++++++++++++-----------\n>  2 files changed, 53 insertions(+), 32 deletions(-)\n> \n> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h\n> index bca7c9a19471..74a67c81398a 100644\n> --- a/src/libcamera/include/media_device.h\n> +++ b/src/libcamera/include/media_device.h\n> @@ -28,6 +28,7 @@ public:\n>  \tvoid close();\n>  \n>  \tint populate();\n> +\tbool valid() const { return valid_; }\n\n(nit?) Shouldn't this be down with the getters below and after devnode_ ?\n\n>  \n>  \tconst std::string driver() const { return driver_; }\n>  \tconst std::string devnode() const { return devnode_; }\n> @@ -37,18 +38,19 @@ private:\n>  \tstd::string driver_;\n>  \tstd::string devnode_;\n>  \tint fd_;\n> +\tbool valid_;\n>  \n>  \tstd::map<unsigned int, MediaObject *> objects_;\n>  \tMediaObject *object(unsigned int id);\n> -\tint addObject(MediaObject *obj);\n> +\tbool addObject(MediaObject *obj);\n>  \tvoid clear();\n>  \n>  \tstd::vector<MediaEntity *> entities_;\n>  \tMediaEntity *getEntityByName(const std::string &name);\n>  \n> -\tvoid populateEntities(const struct media_v2_topology &topology);\n> -\tint populatePads(const struct media_v2_topology &topology);\n> -\tint populateLinks(const struct media_v2_topology &topology);\n> +\tbool populateEntities(const struct media_v2_topology &topology);\n> +\tbool populatePads(const struct media_v2_topology &topology);\n> +\tbool populateLinks(const struct media_v2_topology &topology);\n>  };\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index d9a5196ac28c..fd5a31746075 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -56,7 +56,7 @@ namespace libcamera {\n>   * \\param devnode The media device node path\n>   */\n>  MediaDevice::MediaDevice(const std::string &devnode)\n> -\t: devnode_(devnode), fd_(-1)\n> +\t: devnode_(devnode), fd_(-1), valid_(false)\n>  {\n>  }\n>  \n> @@ -99,6 +99,7 @@ void MediaDevice::clear()\n>  \n>  \tobjects_.clear();\n>  \tentities_.clear();\n> +\tvalid_ = false;\n>  }\n>  \n>  /**\n> @@ -163,18 +164,18 @@ void MediaDevice::close()\n>   * Add a new object to the global objects pool and fail if the object\n>   * has already been registered.\n>   */\n> -int MediaDevice::addObject(MediaObject *obj)\n> +bool MediaDevice::addObject(MediaObject *obj)\n>  {\n>  \n>  \tif (objects_.find(obj->id()) != objects_.end()) {\n>  \t\tLOG(Error) << \"Element with id \" << obj->id()\n>  \t\t\t   << \" already enumerated.\";\n> -\t\treturn -EEXIST;\n> +\t\treturn false;\n>  \t}\n>  \n>  \tobjects_[obj->id()] = obj;\n>  \n> -\treturn 0;\n> +\treturn true;\n>  }\n>  \n>  /*\n> @@ -201,7 +202,7 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name)\n>  \treturn nullptr;\n>  }\n>  \n> -int MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> +bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n>  {\n>  \tmedia_v2_link *mediaLinks = reinterpret_cast<media_v2_link *>\n>  \t\t\t\t    (topology.ptr_links);\n> @@ -222,7 +223,7 @@ int MediaDevice::populateLinks(const struct media_v2_topology &topology)\n>  \t\tif (!source) {\n>  \t\t\tLOG(Error) << \"Failed to find pad with id: \"\n>  \t\t\t\t   << source_id;\n> -\t\t\treturn -ENODEV;\n> +\t\t\treturn false;\n>  \t\t}\n>  \n>  \t\tunsigned int sink_id = mediaLinks[i].sink_id;\n> @@ -231,20 +232,23 @@ int MediaDevice::populateLinks(const struct media_v2_topology &topology)\n>  \t\tif (!sink) {\n>  \t\t\tLOG(Error) << \"Failed to find pad with id: \"\n>  \t\t\t\t   << sink_id;\n> -\t\t\treturn -ENODEV;\n> +\t\t\treturn false;\n>  \t\t}\n>  \n>  \t\tMediaLink *link = new MediaLink(&mediaLinks[i], source, sink);\n> +\t\tif (!addObject(link)) {\n> +\t\t\tdelete link;\n> +\t\t\treturn false;\n> +\t\t}\n> +\n>  \t\tsource->addLink(link);\n>  \t\tsink->addLink(link);\n> -\n> -\t\taddObject(link);\n>  \t}\n>  \n> -\treturn 0;\n> +\treturn true;\n>  }\n>  \n> -int MediaDevice::populatePads(const struct media_v2_topology &topology)\n> +bool MediaDevice::populatePads(const struct media_v2_topology &topology)\n>  {\n>  \tmedia_v2_pad *mediaPads = reinterpret_cast<media_v2_pad *>\n>  \t\t\t\t  (topology.ptr_pads);\n> @@ -258,16 +262,19 @@ int MediaDevice::populatePads(const struct media_v2_topology &topology)\n>  \t\tif (!mediaEntity) {\n>  \t\t\tLOG(Error) << \"Failed to find entity with id: \"\n>  \t\t\t\t   << entity_id;\n> -\t\t\treturn -ENODEV;\n> +\t\t\treturn false;\n>  \t\t}\n>  \n>  \t\tMediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity);\n> -\t\tmediaEntity->addPad(pad);\n> +\t\tif (!addObject(pad)) {\n> +\t\t\tdelete pad;\n> +\t\t\treturn false;\n> +\t\t}\n>  \n> -\t\taddObject(pad);\n> +\t\tmediaEntity->addPad(pad);\n>  \t}\n>  \n> -\treturn 0;\n> +\treturn true;\n>  }\n>  \n>  /*\n> @@ -275,17 +282,22 @@ int MediaDevice::populatePads(const struct media_v2_topology &topology)\n>   * reference in the MediaObject global pool and in the global vector of\n>   * entities.\n>   */\n> -void MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> +bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n>  {\n>  \tmedia_v2_entity *mediaEntities = reinterpret_cast<media_v2_entity *>\n>  \t\t\t\t\t (topology.ptr_entities);\n>  \n>  \tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n>  \t\tMediaEntity *entity = new MediaEntity(&mediaEntities[i]);\n> +\t\tif (!addObject(entity)) {\n> +\t\t\tdelete entity;\n> +\t\t\treturn false;\n> +\t\t}\n>  \n> -\t\taddObject(entity);\n>  \t\tentities_.push_back(entity);\n>  \t}\n> +\n> +\treturn true;\n>  }\n>  \n>  /**\n> @@ -311,6 +323,8 @@ int MediaDevice::populate()\n>  \t__u64 version = -1;\n>  \tint ret;\n>  \n> +\tclear();\n> +\n>  \t/*\n>  \t * Keep calling G_TOPOLOGY until the version number stays stable.\n>  \t */\n> @@ -343,24 +357,29 @@ int MediaDevice::populate()\n>  \t}\n>  \n>  \t/* Populate entities, pads and links. */\n> -\tpopulateEntities(topology);\n> -\n> -\tret = populatePads(topology);\n> -\tif (ret)\n> -\t\tgoto error_free_objs;\n> -\n> -\tret = populateLinks(topology);\n> -error_free_objs:\n> -\tif (ret)\n> -\t\tclear();\n> +\tif (populateEntities(topology) &&\n> +\t    populatePads(topology) &&\n> +\t    populateLinks(topology))\n> +\t\tvalid_ = true;\n>  \n>  \tdelete[] links;\n>  \tdelete[] ents;\n>  \tdelete[] pads;\n>  \n> -\treturn ret;\n> +\tif (!valid_) {\n> +\t\tclear();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n>  }\n>  \n> +/**\n> + * \\fn MediaDevice::valid()\n> + * \\brief Query whether the media graph is valid\n> + * \\return true if the media graph is valid, false otherwise\n> + */\n> +\n>  /**\n>   * \\var MediaDevice::objects_\n>   * \\brief Global map of media objects (entities, pads, links) keyed by their\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16A0F60B31\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  1 Jan 2019 22:52:27 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69F0C505;\n\tTue,  1 Jan 2019 22:52:26 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546379546;\n\tbh=qc+TkKYlM5G0V20bqazZP/i6CKTT1cD6fENHTb0QPtM=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=BGoPgaLFZ74tDOgOrt2q3hgiK0E68E93UcCDZxPJpxGXU7FKzA8TmlGTCDawtJoWS\n\tB0muvS8UuImhalIl70+kw6RB2dRtWTwrGIkYNkr2LtJSiRuuLVmROu65/J+0E8z868\n\tdv28WKw1HiFg6b2NqywkS2AljsdCVFAZV+akeNvc=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20190101212328.18361-1-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<539a808d-3f24-3fc2-e025-08e7325690a1@ideasonboard.com>","Date":"Tue, 1 Jan 2019 21:52:23 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20190101212328.18361-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: mediadevice: Fix graph\n\tparsing error handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 01 Jan 2019 21:52:27 -0000"}},{"id":168,"web_url":"https://patchwork.libcamera.org/comment/168/","msgid":"<20190102091523.d4f7slf4bssqyzi2@uno.localdomain>","date":"2019-01-02T09:15:23","subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: mediadevice: Fix graph\n\tparsing error handling","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n   thanks for the patches\n\nA bit late, as this has been pushed already, but for the record.\n\nOn Tue, Jan 01, 2019 at 11:23:25PM +0200, Laurent Pinchart wrote:\n> Most errors recorded during graph parsing are logged but not propagated\n> to the caller. Fix this and delete objects that are created but not\n> successfully added to the graph to avoid memory leaks. As the error code\n> returned from the addObject() and populate*() functions doesn't matter\n> much, turn them into bool functions.\n>\n\nThanks, originally addObject() had void return value, I then returned\nand error code, but ignored it in the caller. Quite silly.\n\n> Additionally, add a way to query whether the media graph was valid, and\n> clear objects before populating the graph to avoid leaking them.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  src/libcamera/include/media_device.h | 10 ++--\n>  src/libcamera/media_device.cpp       | 75 +++++++++++++++++-----------\n>  2 files changed, 53 insertions(+), 32 deletions(-)\n>\n> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h\n> index bca7c9a19471..74a67c81398a 100644\n> --- a/src/libcamera/include/media_device.h\n> +++ b/src/libcamera/include/media_device.h\n> @@ -28,6 +28,7 @@ public:\n>  \tvoid close();\n>\n>  \tint populate();\n> +\tbool valid() const { return valid_; }\n>\n>  \tconst std::string driver() const { return driver_; }\n>  \tconst std::string devnode() const { return devnode_; }\n> @@ -37,18 +38,19 @@ private:\n>  \tstd::string driver_;\n>  \tstd::string devnode_;\n>  \tint fd_;\n> +\tbool valid_;\n>\n>  \tstd::map<unsigned int, MediaObject *> objects_;\n>  \tMediaObject *object(unsigned int id);\n> -\tint addObject(MediaObject *obj);\n> +\tbool addObject(MediaObject *obj);\n>  \tvoid clear();\n>\n>  \tstd::vector<MediaEntity *> entities_;\n>  \tMediaEntity *getEntityByName(const std::string &name);\n>\n> -\tvoid populateEntities(const struct media_v2_topology &topology);\n> -\tint populatePads(const struct media_v2_topology &topology);\n> -\tint populateLinks(const struct media_v2_topology &topology);\n> +\tbool populateEntities(const struct media_v2_topology &topology);\n> +\tbool populatePads(const struct media_v2_topology &topology);\n> +\tbool populateLinks(const struct media_v2_topology &topology);\n>  };\n>\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index d9a5196ac28c..fd5a31746075 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -56,7 +56,7 @@ namespace libcamera {\n>   * \\param devnode The media device node path\n>   */\n>  MediaDevice::MediaDevice(const std::string &devnode)\n> -\t: devnode_(devnode), fd_(-1)\n> +\t: devnode_(devnode), fd_(-1), valid_(false)\n>  {\n>  }\n>\n> @@ -99,6 +99,7 @@ void MediaDevice::clear()\n>\n>  \tobjects_.clear();\n>  \tentities_.clear();\n> +\tvalid_ = false;\n>  }\n>\n>  /**\n> @@ -163,18 +164,18 @@ void MediaDevice::close()\n>   * Add a new object to the global objects pool and fail if the object\n>   * has already been registered.\n>   */\n> -int MediaDevice::addObject(MediaObject *obj)\n> +bool MediaDevice::addObject(MediaObject *obj)\n>  {\n>\n>  \tif (objects_.find(obj->id()) != objects_.end()) {\n>  \t\tLOG(Error) << \"Element with id \" << obj->id()\n>  \t\t\t   << \" already enumerated.\";\n> -\t\treturn -EEXIST;\n> +\t\treturn false;\n>  \t}\n>\n>  \tobjects_[obj->id()] = obj;\n>\n> -\treturn 0;\n> +\treturn true;\n>  }\n>\n>  /*\n> @@ -201,7 +202,7 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name)\n>  \treturn nullptr;\n>  }\n>\n> -int MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> +bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n>  {\n>  \tmedia_v2_link *mediaLinks = reinterpret_cast<media_v2_link *>\n>  \t\t\t\t    (topology.ptr_links);\n> @@ -222,7 +223,7 @@ int MediaDevice::populateLinks(const struct media_v2_topology &topology)\n>  \t\tif (!source) {\n>  \t\t\tLOG(Error) << \"Failed to find pad with id: \"\n>  \t\t\t\t   << source_id;\n> -\t\t\treturn -ENODEV;\n> +\t\t\treturn false;\n>  \t\t}\n>\n>  \t\tunsigned int sink_id = mediaLinks[i].sink_id;\n> @@ -231,20 +232,23 @@ int MediaDevice::populateLinks(const struct media_v2_topology &topology)\n>  \t\tif (!sink) {\n>  \t\t\tLOG(Error) << \"Failed to find pad with id: \"\n>  \t\t\t\t   << sink_id;\n> -\t\t\treturn -ENODEV;\n> +\t\t\treturn false;\n>  \t\t}\n>\n>  \t\tMediaLink *link = new MediaLink(&mediaLinks[i], source, sink);\n> +\t\tif (!addObject(link)) {\n> +\t\t\tdelete link;\n> +\t\t\treturn false;\n> +\t\t}\n> +\n>  \t\tsource->addLink(link);\n>  \t\tsink->addLink(link);\n> -\n> -\t\taddObject(link);\n>  \t}\n>\n> -\treturn 0;\n> +\treturn true;\n>  }\n>\n> -int MediaDevice::populatePads(const struct media_v2_topology &topology)\n> +bool MediaDevice::populatePads(const struct media_v2_topology &topology)\n>  {\n>  \tmedia_v2_pad *mediaPads = reinterpret_cast<media_v2_pad *>\n>  \t\t\t\t  (topology.ptr_pads);\n> @@ -258,16 +262,19 @@ int MediaDevice::populatePads(const struct media_v2_topology &topology)\n>  \t\tif (!mediaEntity) {\n>  \t\t\tLOG(Error) << \"Failed to find entity with id: \"\n>  \t\t\t\t   << entity_id;\n> -\t\t\treturn -ENODEV;\n> +\t\t\treturn false;\n>  \t\t}\n>\n>  \t\tMediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity);\n> -\t\tmediaEntity->addPad(pad);\n> +\t\tif (!addObject(pad)) {\n> +\t\t\tdelete pad;\n> +\t\t\treturn false;\n> +\t\t}\n>\n> -\t\taddObject(pad);\n> +\t\tmediaEntity->addPad(pad);\n>  \t}\n>\n> -\treturn 0;\n> +\treturn true;\n>  }\n>\n>  /*\n> @@ -275,17 +282,22 @@ int MediaDevice::populatePads(const struct media_v2_topology &topology)\n>   * reference in the MediaObject global pool and in the global vector of\n>   * entities.\n>   */\n> -void MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> +bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n>  {\n>  \tmedia_v2_entity *mediaEntities = reinterpret_cast<media_v2_entity *>\n>  \t\t\t\t\t (topology.ptr_entities);\n>\n>  \tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n>  \t\tMediaEntity *entity = new MediaEntity(&mediaEntities[i]);\n> +\t\tif (!addObject(entity)) {\n> +\t\t\tdelete entity;\n> +\t\t\treturn false;\n> +\t\t}\n>\n> -\t\taddObject(entity);\n>  \t\tentities_.push_back(entity);\n>  \t}\n> +\n> +\treturn true;\n>  }\n>\n>  /**\n> @@ -311,6 +323,8 @@ int MediaDevice::populate()\n>  \t__u64 version = -1;\n>  \tint ret;\n>\n> +\tclear();\n> +\n>  \t/*\n>  \t * Keep calling G_TOPOLOGY until the version number stays stable.\n>  \t */\n> @@ -343,24 +357,29 @@ int MediaDevice::populate()\n>  \t}\n>\n>  \t/* Populate entities, pads and links. */\n> -\tpopulateEntities(topology);\n> -\n> -\tret = populatePads(topology);\n> -\tif (ret)\n> -\t\tgoto error_free_objs;\n> -\n> -\tret = populateLinks(topology);\n> -error_free_objs:\n> -\tif (ret)\n> -\t\tclear();\n> +\tif (populateEntities(topology) &&\n> +\t    populatePads(topology) &&\n> +\t    populateLinks(topology))\n> +\t\tvalid_ = true;\n>\n>  \tdelete[] links;\n>  \tdelete[] ents;\n>  \tdelete[] pads;\n>\n> -\treturn ret;\n> +\tif (!valid_) {\n> +\t\tclear();\n> +\t\treturn -EINVAL;\n> +\t}\n}\n>\n> +/**\n> + * \\fn MediaDevice::valid()\n> + * \\brief Query whether the media graph is valid\n\n\\brief Query whether the media graph has been populated and is valid\n\nI feel the useful information here is that the graph is populated and\nits media objects can be accessed. I would convey that in documenting\nthe method.\n\nThanks\n   j\n\n> + * \\return true if the media graph is valid, false otherwise\n> + */\n> +\n>  /**\n>   * \\var MediaDevice::objects_\n>   * \\brief Global map of media objects (entities, pads, links) keyed by their\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 11F1B60B30\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  2 Jan 2019 10:15:24 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 9C675C0014;\n\tWed,  2 Jan 2019 09:15:23 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 2 Jan 2019 10:15:23 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190102091523.d4f7slf4bssqyzi2@uno.localdomain>","References":"<20190101212328.18361-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"673z74x6ibmuru66\"","Content-Disposition":"inline","In-Reply-To":"<20190101212328.18361-1-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: mediadevice: Fix graph\n\tparsing error handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 02 Jan 2019 09:15:24 -0000"}}]