[libcamera-devel] [PATCH 1/4] libcamera: mediadevice: Fix graph parsing error handling
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jan 1 22:52:23 CET 2019
Hi Laurent,
On 01/01/2019 21:23, Laurent Pinchart wrote:
> Most errors recorded during graph parsing are logged but not propagated
> to the caller. Fix this and delete objects that are created but not
> successfully added to the graph to avoid memory leaks. As the error code
> returned from the addObject() and populate*() functions doesn't matter
> much, turn them into bool functions.
>
> Additionally, add a way to query whether the media graph was valid, and
> clear objects before populating the graph to avoid leaking them.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
This looks good to me...
A bit risky to go inverting the boolean logic of return values - but as
you've collected all the populate*() calls to a single expression it works.
Also - I think this reads nicely:
> + if (!addObject(entity)) {
as "If we can't addObject() ..."
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> src/libcamera/include/media_device.h | 10 ++--
> src/libcamera/media_device.cpp | 75 +++++++++++++++++-----------
> 2 files changed, 53 insertions(+), 32 deletions(-)
>
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index bca7c9a19471..74a67c81398a 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -28,6 +28,7 @@ public:
> void close();
>
> int populate();
> + bool valid() const { return valid_; }
(nit?) Shouldn't this be down with the getters below and after devnode_ ?
>
> const std::string driver() const { return driver_; }
> const std::string devnode() const { return devnode_; }
> @@ -37,18 +38,19 @@ private:
> std::string driver_;
> std::string devnode_;
> int fd_;
> + bool valid_;
>
> std::map<unsigned int, MediaObject *> objects_;
> MediaObject *object(unsigned int id);
> - int addObject(MediaObject *obj);
> + bool addObject(MediaObject *obj);
> void clear();
>
> std::vector<MediaEntity *> entities_;
> MediaEntity *getEntityByName(const std::string &name);
>
> - void populateEntities(const struct media_v2_topology &topology);
> - int populatePads(const struct media_v2_topology &topology);
> - int populateLinks(const struct media_v2_topology &topology);
> + bool populateEntities(const struct media_v2_topology &topology);
> + bool populatePads(const struct media_v2_topology &topology);
> + bool populateLinks(const struct media_v2_topology &topology);
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index d9a5196ac28c..fd5a31746075 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -56,7 +56,7 @@ namespace libcamera {
> * \param devnode The media device node path
> */
> MediaDevice::MediaDevice(const std::string &devnode)
> - : devnode_(devnode), fd_(-1)
> + : devnode_(devnode), fd_(-1), valid_(false)
> {
> }
>
> @@ -99,6 +99,7 @@ void MediaDevice::clear()
>
> objects_.clear();
> entities_.clear();
> + valid_ = false;
> }
>
> /**
> @@ -163,18 +164,18 @@ void MediaDevice::close()
> * Add a new object to the global objects pool and fail if the object
> * has already been registered.
> */
> -int MediaDevice::addObject(MediaObject *obj)
> +bool MediaDevice::addObject(MediaObject *obj)
> {
>
> if (objects_.find(obj->id()) != objects_.end()) {
> LOG(Error) << "Element with id " << obj->id()
> << " already enumerated.";
> - return -EEXIST;
> + return false;
> }
>
> objects_[obj->id()] = obj;
>
> - return 0;
> + return true;
> }
>
> /*
> @@ -201,7 +202,7 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name)
> return nullptr;
> }
>
> -int MediaDevice::populateLinks(const struct media_v2_topology &topology)
> +bool MediaDevice::populateLinks(const struct media_v2_topology &topology)
> {
> media_v2_link *mediaLinks = reinterpret_cast<media_v2_link *>
> (topology.ptr_links);
> @@ -222,7 +223,7 @@ int MediaDevice::populateLinks(const struct media_v2_topology &topology)
> if (!source) {
> LOG(Error) << "Failed to find pad with id: "
> << source_id;
> - return -ENODEV;
> + return false;
> }
>
> unsigned int sink_id = mediaLinks[i].sink_id;
> @@ -231,20 +232,23 @@ int MediaDevice::populateLinks(const struct media_v2_topology &topology)
> if (!sink) {
> LOG(Error) << "Failed to find pad with id: "
> << sink_id;
> - return -ENODEV;
> + return false;
> }
>
> MediaLink *link = new MediaLink(&mediaLinks[i], source, sink);
> + if (!addObject(link)) {
> + delete link;
> + return false;
> + }
> +
> source->addLink(link);
> sink->addLink(link);
> -
> - addObject(link);
> }
>
> - return 0;
> + return true;
> }
>
> -int MediaDevice::populatePads(const struct media_v2_topology &topology)
> +bool MediaDevice::populatePads(const struct media_v2_topology &topology)
> {
> media_v2_pad *mediaPads = reinterpret_cast<media_v2_pad *>
> (topology.ptr_pads);
> @@ -258,16 +262,19 @@ int MediaDevice::populatePads(const struct media_v2_topology &topology)
> if (!mediaEntity) {
> LOG(Error) << "Failed to find entity with id: "
> << entity_id;
> - return -ENODEV;
> + return false;
> }
>
> MediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity);
> - mediaEntity->addPad(pad);
> + if (!addObject(pad)) {
> + delete pad;
> + return false;
> + }
>
> - addObject(pad);
> + mediaEntity->addPad(pad);
> }
>
> - return 0;
> + return true;
> }
>
> /*
> @@ -275,17 +282,22 @@ int MediaDevice::populatePads(const struct media_v2_topology &topology)
> * reference in the MediaObject global pool and in the global vector of
> * entities.
> */
> -void MediaDevice::populateEntities(const struct media_v2_topology &topology)
> +bool MediaDevice::populateEntities(const struct media_v2_topology &topology)
> {
> media_v2_entity *mediaEntities = reinterpret_cast<media_v2_entity *>
> (topology.ptr_entities);
>
> for (unsigned int i = 0; i < topology.num_entities; ++i) {
> MediaEntity *entity = new MediaEntity(&mediaEntities[i]);
> + if (!addObject(entity)) {
> + delete entity;
> + return false;
> + }
>
> - addObject(entity);
> entities_.push_back(entity);
> }
> +
> + return true;
> }
>
> /**
> @@ -311,6 +323,8 @@ int MediaDevice::populate()
> __u64 version = -1;
> int ret;
>
> + clear();
> +
> /*
> * Keep calling G_TOPOLOGY until the version number stays stable.
> */
> @@ -343,24 +357,29 @@ int MediaDevice::populate()
> }
>
> /* Populate entities, pads and links. */
> - populateEntities(topology);
> -
> - ret = populatePads(topology);
> - if (ret)
> - goto error_free_objs;
> -
> - ret = populateLinks(topology);
> -error_free_objs:
> - if (ret)
> - clear();
> + if (populateEntities(topology) &&
> + populatePads(topology) &&
> + populateLinks(topology))
> + valid_ = true;
>
> delete[] links;
> delete[] ents;
> delete[] pads;
>
> - return ret;
> + if (!valid_) {
> + clear();
> + return -EINVAL;
> + }
> +
> + return 0;
> }
>
> +/**
> + * \fn MediaDevice::valid()
> + * \brief Query whether the media graph is valid
> + * \return true if the media graph is valid, false otherwise
> + */
> +
> /**
> * \var MediaDevice::objects_
> * \brief Global map of media objects (entities, pads, links) keyed by their
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list