Re: [PATCH 01/15] mediactl: Introduce v4l2_subdev structure
Hi Sakari, On 03/21/2016 12:39 AM, Sakari Ailus wrote: Hi Jacek, On Thu, Feb 18, 2016 at 03:15:32PM +0100, Jacek Anaszewski wrote: Hi Sakari, Thanks for the review. On 02/12/2016 01:42 PM, Sakari Ailus wrote: Hi Jacek, Thanks for continuing this work! And my apologies for reviewing only now... please see the comments below. Jacek Anaszewski wrote: Add struct v4l2_subdev - a representation of the v4l2 sub-device, related to the media entity. Add field 'sd', the pointer to the newly introduced structure, to the struct media_entity and move 'fd' property from struct media entity to struct v4l2_subdev. Avoid accessing sub-device file descriptor from libmediactl and make the v4l2_subdev_open capable of creating the v4l2_subdev if the 'sd' pointer is uninitialized. Signed-off-by: Jacek AnaszewskiAcked-by: Kyungmin Park --- utils/media-ctl/libmediactl.c |4 -- utils/media-ctl/libv4l2subdev.c | 82 +++ utils/media-ctl/mediactl-priv.h |5 ++- utils/media-ctl/v4l2subdev.h| 38 ++ 4 files changed, 107 insertions(+), 22 deletions(-) diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c index 4a82d24..7e98440 100644 --- a/utils/media-ctl/libmediactl.c +++ b/utils/media-ctl/libmediactl.c @@ -525,7 +525,6 @@ static int media_enum_entities(struct media_device *media) entity = >entities[media->entities_count]; memset(entity, 0, sizeof(*entity)); - entity->fd = -1; entity->info.id = id | MEDIA_ENT_ID_FLAG_NEXT; entity->media = media; @@ -719,8 +718,6 @@ void media_device_unref(struct media_device *media) free(entity->pads); free(entity->links); - if (entity->fd != -1) - close(entity->fd); } free(media->entities); @@ -747,7 +744,6 @@ int media_device_add_entity(struct media_device *media, entity = >entities[media->entities_count - 1]; memset(entity, 0, sizeof *entity); - entity->fd = -1; entity->media = media; strncpy(entity->devname, devnode, sizeof entity->devname); entity->devname[sizeof entity->devname - 1] = '\0'; diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c index 33c1ee6..3977ce5 100644 --- a/utils/media-ctl/libv4l2subdev.c +++ b/utils/media-ctl/libv4l2subdev.c @@ -39,13 +39,61 @@ #include "tools.h" #include "v4l2subdev.h" +int v4l2_subdev_create(struct media_entity *entity) +{ + if (entity->sd) + return 0; + + entity->sd = calloc(1, sizeof(*entity->sd)); + if (entity->sd == NULL) + return -ENOMEM; + + entity->sd->fd = -1; + + return 0; +} + +int v4l2_subdev_create_with_fd(struct media_entity *entity, int fd) +{ + int ret; + + if (entity->sd) + return -EEXIST; + + ret = v4l2_subdev_create(entity); + if (ret < 0) + return ret; + + entity->sd->fd = fd; + + return 0; +} + +void v4l2_subdev_release(struct media_entity *entity, bool close_fd) +{ + if (entity->sd == NULL) + return; + + if (close_fd) + v4l2_subdev_close(entity); + + free(entity->sd->v4l2_control_redir); + free(entity->sd); +} + int v4l2_subdev_open(struct media_entity *entity) { - if (entity->fd != -1) + int ret; + + ret = v4l2_subdev_create(entity); The current users of v4l2_subdev_open() in libv4l2subdev do not explicitly close the sub-devices they open; thus calling v4l2_subdev_create() here creates a memory leak. Currently in my use cases there is no memory leak since I assumed that the one who instantiates struct media_device should take care of releasing it properly. I added v4l2_subdev_open_pipeline() and v4l2_subdev_release_pipeline() API that is called on plugin init and close respectively. I'm referring to the use of the libv4l2subdev API as it's documented; the media-ctl test program which also serves as a good example on the API. Any sub-device IOCTL wrapper function will call v4l2_subdev_open() which stores the file descriptor returned by open(2) to struct media_entity.fd. v4l2_subdev_close() is not called explicitly. This is currently not required. The file handle is not leaked, as it is closed by media_device_unref() in libmediactl. This patch allocates memory for each sub-device in v4l2_subdev_create() which is in turn called from v4l2_subdev_open(). As the calls to v4l2_subdev_close() (which would release memory) are lacking, the memory is leaked. Probably it would be good to remove v4l2_subdev_open from v4l2_subdev_* prefixed API and return error if sd property of passed struct media_entity is not initialized. The purpose of the libv4l2subdev library is to make accessing sub-devices easier, and tossing that responsibility to
Re: [PATCH 01/15] mediactl: Introduce v4l2_subdev structure
Hi Jacek, On Thu, Feb 18, 2016 at 03:15:32PM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > Thanks for the review. > > On 02/12/2016 01:42 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >Thanks for continuing this work! And my apologies for reviewing only > >now... please see the comments below. > > > >Jacek Anaszewski wrote: > >>Add struct v4l2_subdev - a representation of the v4l2 sub-device, > >>related to the media entity. Add field 'sd', the pointer to > >>the newly introduced structure, to the struct media_entity > >>and move 'fd' property from struct media entity to struct v4l2_subdev. > >>Avoid accessing sub-device file descriptor from libmediactl and > >>make the v4l2_subdev_open capable of creating the v4l2_subdev > >>if the 'sd' pointer is uninitialized. > >> > >>Signed-off-by: Jacek Anaszewski> >>Acked-by: Kyungmin Park > >>--- > >> utils/media-ctl/libmediactl.c |4 -- > >> utils/media-ctl/libv4l2subdev.c | 82 > >> +++ > >> utils/media-ctl/mediactl-priv.h |5 ++- > >> utils/media-ctl/v4l2subdev.h| 38 ++ > >> 4 files changed, 107 insertions(+), 22 deletions(-) > >> > >>diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c > >>index 4a82d24..7e98440 100644 > >>--- a/utils/media-ctl/libmediactl.c > >>+++ b/utils/media-ctl/libmediactl.c > >>@@ -525,7 +525,6 @@ static int media_enum_entities(struct media_device > >>*media) > >> > >>entity = >entities[media->entities_count]; > >>memset(entity, 0, sizeof(*entity)); > >>- entity->fd = -1; > >>entity->info.id = id | MEDIA_ENT_ID_FLAG_NEXT; > >>entity->media = media; > >> > >>@@ -719,8 +718,6 @@ void media_device_unref(struct media_device *media) > >> > >>free(entity->pads); > >>free(entity->links); > >>- if (entity->fd != -1) > >>- close(entity->fd); > >>} > >> > >>free(media->entities); > >>@@ -747,7 +744,6 @@ int media_device_add_entity(struct media_device *media, > >>entity = >entities[media->entities_count - 1]; > >>memset(entity, 0, sizeof *entity); > >> > >>- entity->fd = -1; > >>entity->media = media; > >>strncpy(entity->devname, devnode, sizeof entity->devname); > >>entity->devname[sizeof entity->devname - 1] = '\0'; > >>diff --git a/utils/media-ctl/libv4l2subdev.c > >>b/utils/media-ctl/libv4l2subdev.c > >>index 33c1ee6..3977ce5 100644 > >>--- a/utils/media-ctl/libv4l2subdev.c > >>+++ b/utils/media-ctl/libv4l2subdev.c > >>@@ -39,13 +39,61 @@ > >> #include "tools.h" > >> #include "v4l2subdev.h" > >> > >>+int v4l2_subdev_create(struct media_entity *entity) > >>+{ > >>+ if (entity->sd) > >>+ return 0; > >>+ > >>+ entity->sd = calloc(1, sizeof(*entity->sd)); > >>+ if (entity->sd == NULL) > >>+ return -ENOMEM; > >>+ > >>+ entity->sd->fd = -1; > >>+ > >>+ return 0; > >>+} > >>+ > >>+int v4l2_subdev_create_with_fd(struct media_entity *entity, int fd) > >>+{ > >>+ int ret; > >>+ > >>+ if (entity->sd) > >>+ return -EEXIST; > >>+ > >>+ ret = v4l2_subdev_create(entity); > >>+ if (ret < 0) > >>+ return ret; > >>+ > >>+ entity->sd->fd = fd; > >>+ > >>+ return 0; > >>+} > >>+ > >>+void v4l2_subdev_release(struct media_entity *entity, bool close_fd) > >>+{ > >>+ if (entity->sd == NULL) > >>+ return; > >>+ > >>+ if (close_fd) > >>+ v4l2_subdev_close(entity); > >>+ > >>+ free(entity->sd->v4l2_control_redir); > >>+ free(entity->sd); > >>+} > >>+ > >> int v4l2_subdev_open(struct media_entity *entity) > >> { > >>- if (entity->fd != -1) > >>+ int ret; > >>+ > >>+ ret = v4l2_subdev_create(entity); > > > >The current users of v4l2_subdev_open() in libv4l2subdev do not > >explicitly close the sub-devices they open; thus calling > >v4l2_subdev_create() here creates a memory leak. > > Currently in my use cases there is no memory leak since I assumed > that the one who instantiates struct media_device should take > care of releasing it properly. I added v4l2_subdev_open_pipeline() > and v4l2_subdev_release_pipeline() API that is called on plugin > init and close respectively. I'm referring to the use of the libv4l2subdev API as it's documented; the media-ctl test program which also serves as a good example on the API. Any sub-device IOCTL wrapper function will call v4l2_subdev_open() which stores the file descriptor returned by open(2) to struct media_entity.fd. v4l2_subdev_close() is not called explicitly. This is currently not required. The file handle is not leaked, as it is closed by media_device_unref() in libmediactl. This patch allocates memory for each sub-device in v4l2_subdev_create() which is in turn called from v4l2_subdev_open(). As the calls to v4l2_subdev_close() (which would release memory) are lacking, the memory is leaked. > > Probably it would be good to