| Message ID | 1476282922-11544-5-git-send-email-j.anaszewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jacek, Thanks for the patchset. On Wed, Oct 12, 2016 at 04:35:19PM +0200, Jacek Anaszewski wrote: > Add helper functions that allow for easy instantiation of media_device > object basing on whether the media device contains v4l2 subdev with > given file descriptor. Doesn't this work with video nodes as well? That's what you seem to be using it for later on. And I think that's actually more useful. The existing implementation uses udev to look up devices. Could you use libudev device enumeration API to find the media devices, and fall back to sysfs if udev doesn't work? There seems to be a reasonable-looking example here: <URL:http://stackoverflow.com/questions/25361042/how-to-list-usb-mass-storage-devices-programatically-using-libudev-in-linux> > > Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > utils/media-ctl/libmediactl.c | 131 +++++++++++++++++++++++++++++++++++++++++- > utils/media-ctl/mediactl.h | 27 +++++++++ > 2 files changed, 156 insertions(+), 2 deletions(-) > > diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c > index 155b65f..d347a40 100644 > --- a/utils/media-ctl/libmediactl.c > +++ b/utils/media-ctl/libmediactl.c > @@ -27,6 +27,7 @@ > #include <sys/sysmacros.h> > > #include <ctype.h> > +#include <dirent.h> > #include <errno.h> > #include <fcntl.h> > #include <stdbool.h> > @@ -440,8 +441,9 @@ static int media_get_devname_udev(struct udev *udev, > return -EINVAL; > > devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor); > - media_dbg(entity->media, "looking up device: %u:%u\n", > - major(devnum), minor(devnum)); > + if (entity->media) > + media_dbg(entity->media, "looking up device: %u:%u\n", > + major(devnum), minor(devnum)); > device = udev_device_new_from_devnum(udev, 'c', devnum); > if (device) { > p = udev_device_get_devnode(device); > @@ -523,6 +525,7 @@ static int media_get_devname_sysfs(struct media_entity *entity) > return 0; > } > > + Unrelated change. > static int media_enum_entities(struct media_device *media) > { > struct media_entity *entity; > @@ -707,6 +710,92 @@ struct media_device *media_device_new(const char *devnode) > return media; > } > > +struct media_device *media_device_new_by_subdev_fd(int fd, struct media_entity **fd_entity) > +{ > + char video_devname[32], device_dir_path[256], media_dev_path[256], media_major_minor[10]; > + struct media_device *media = NULL; > + struct dirent *entry; > + struct media_entity tmp_entity; > + DIR *device_dir; > + struct udev *udev; > + char *p; > + int ret, i; > + > + if (fd_entity == NULL) > + return NULL; > + > + ret = media_get_devname_by_fd(fd, video_devname); > + if (ret < 0) > + return NULL; > + > + p = strrchr(video_devname, '/'); > + if (p == NULL) > + return NULL; > + > + ret = media_udev_open(&udev); > + if (ret < 0) > + return NULL; > + > + sprintf(device_dir_path, "/sys/class/video4linux/%s/device/", p + 1); > + > + device_dir = opendir(device_dir_path); > + if (device_dir == NULL) > + return NULL; > + > + while ((entry = readdir(device_dir))) { > + if (strncmp(entry->d_name, "media", 4)) Why 4? And isn't entry->d_name nul-terminated, so you could use strcmp()? > + continue; > + > + sprintf(media_dev_path, "%s%s/dev", device_dir_path, entry->d_name); > + > + fd = open(media_dev_path, O_RDONLY); > + if (fd < 0) > + continue; > + > + ret = read(fd, media_major_minor, sizeof(media_major_minor)); > + if (ret < 0) > + continue; > + > + sscanf(media_major_minor, "%d:%d", &tmp_entity.info.dev.major, &tmp_entity.info.dev.minor); This would be better split on two lines. > + > + /* Try to get the device name via udev */ > + if (media_get_devname_udev(udev, &tmp_entity)) { > + /* Fall back to get the device name via sysfs */ > + if (media_get_devname_sysfs(&tmp_entity)) > + continue; > + } > + > + media = media_device_new(tmp_entity.devname); > + if (media == NULL) > + continue; > + > + ret = media_device_enumerate(media); > + if (ret < 0) { > + media_dbg(media, "Failed to enumerate %s (%d)\n", > + tmp_entity.devname, ret); > + media_device_unref(media); > + media = NULL; > + continue; > + } > + > + /* Get the entity associated with given fd */ > + for (i = 0; i < media->entities_count; i++) { > + struct media_entity *entity = &media->entities[i]; > + > + if (!strcmp(entity->devname, video_devname)) { > + *fd_entity = &media->entities[i]; > + break; > + } > + } What if you exit the loop without finding the entity you were looking for? > + > + break; > + } > + > + media_udev_close(udev); > + > + return media; > +} > + > struct media_device *media_device_new_emulated(struct media_device_info *info) > { > struct media_device *media; > @@ -748,6 +837,44 @@ void media_device_unref(struct media_device *media) > free(media); > } > > +int media_get_devname_by_fd(int fd, char *node_name) > +{ > + struct udev *udev; > + struct media_entity tmp_entity; > + struct stat stat; > + int ret, ret_udev; > + > + if (node_name == NULL) > + return -EINVAL; > + > + ret = fstat(fd, &stat); > + if (ret < 0) > + return -errno; > + > + tmp_entity.info.v4l.major = MAJOR(stat.st_rdev); > + tmp_entity.info.v4l.minor = MINOR(stat.st_rdev); > + > + ret_udev = media_udev_open(&udev); > + if (ret_udev < 0) > + printf("Can't get udev context\n"); > + > + /* Try to get the device name via udev */ > + ret = media_get_devname_udev(udev, &tmp_entity); > + if (!ret) > + goto out; > + > + ret = media_get_devname_sysfs(&tmp_entity); > + if (ret < 0) > + goto err_get_devname; > + > +out: > + strncpy(node_name, tmp_entity.devname, sizeof(tmp_entity.devname)); This seems risky. How does the caller know the maximum size? I'd simply allocate the string, and document the caller is responsible for releasing it. > +err_get_devname: > + if (!ret_udev) > + media_udev_close(udev); > + return ret; > +} > + > int media_device_add_entity(struct media_device *media, > const struct media_entity_desc *desc, > const char *devnode) > diff --git a/utils/media-ctl/mediactl.h b/utils/media-ctl/mediactl.h > index b1f33cd..580a25a 100644 > --- a/utils/media-ctl/mediactl.h > +++ b/utils/media-ctl/mediactl.h > @@ -76,6 +76,21 @@ struct media_device *media_device_new(const char *devnode); > struct media_device *media_device_new_emulated(struct media_device_info *info); > > /** > + * @brief Create a new media device contatning entity associated with v4l2 subdev fd. > + * @param fd - file descriptor of a v4l2 subdev. > + * @param fd_entity - media entity associated with the v4l2 subdev. > + * > + * Create a representation of the media device referenced by the v4l2-subdev. > + * The media device instance is initialized with enumerated entities and links. > + * > + * Media devices are reference-counted, see media_device_ref() and > + * media_device_unref() for more information. > + * > + * @return A pointer to the new media device or NULL if error occurred. > + */ > +struct media_device *media_device_new_by_subdev_fd(int fd, struct media_entity **fd_entity); I'd drop the "subdev_" part as both V4L2 device nodes and sub-devices work. If you wish to keep this V4L2 specific, I suggest ...by_v4l2_fd(). > + > +/** > * @brief Take a reference to the device. > * @param media - device instance. > * > @@ -231,6 +246,18 @@ const struct media_link *media_entity_get_link(struct media_entity *entity, > const char *media_entity_get_devname(struct media_entity *entity); > > /** > + * @brief Get the device node name by its file descriptor > + * @param fd - file descriptor of a device. > + * @param node_name - output device node name string. > + * > + * This function returns the full path and name to the device node corresponding > + * to the given file descriptor. > + * > + * @return 0 on success, or a negative error code on failure. > + */ > +int media_get_devname_by_fd(int fd, char *node_name); > + > +/** > * @brief Get the type of an entity. > * @param entity - the entity. > *
Hi Sakari, Thanks for the review. On 11/24/2016 01:17 PM, Sakari Ailus wrote: > Hi Jacek, > > Thanks for the patchset. > > On Wed, Oct 12, 2016 at 04:35:19PM +0200, Jacek Anaszewski wrote: >> Add helper functions that allow for easy instantiation of media_device >> object basing on whether the media device contains v4l2 subdev with >> given file descriptor. > > Doesn't this work with video nodes as well? That's what you seem to be using > it for later on. And I think that's actually more useful. Exactly, thanks for spotting this. s/v4l2 subdev/video device opened/ > > The existing implementation uses udev to look up devices. Could you use > libudev device enumeration API to find the media devices, and fall back to > sysfs if udev doesn't work? There seems to be a reasonable-looking example > here: > > <URL:http://stackoverflow.com/questions/25361042/how-to-list-usb-mass-storage-devices-programatically-using-libudev-in-linux> I'll check that, thanks. >> >> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> >> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> utils/media-ctl/libmediactl.c | 131 +++++++++++++++++++++++++++++++++++++++++- >> utils/media-ctl/mediactl.h | 27 +++++++++ >> 2 files changed, 156 insertions(+), 2 deletions(-) >> >> diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c >> index 155b65f..d347a40 100644 >> --- a/utils/media-ctl/libmediactl.c >> +++ b/utils/media-ctl/libmediactl.c >> @@ -27,6 +27,7 @@ >> #include <sys/sysmacros.h> >> >> #include <ctype.h> >> +#include <dirent.h> >> #include <errno.h> >> #include <fcntl.h> >> #include <stdbool.h> >> @@ -440,8 +441,9 @@ static int media_get_devname_udev(struct udev *udev, >> return -EINVAL; >> >> devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor); >> - media_dbg(entity->media, "looking up device: %u:%u\n", >> - major(devnum), minor(devnum)); >> + if (entity->media) >> + media_dbg(entity->media, "looking up device: %u:%u\n", >> + major(devnum), minor(devnum)); >> device = udev_device_new_from_devnum(udev, 'c', devnum); >> if (device) { >> p = udev_device_get_devnode(device); >> @@ -523,6 +525,7 @@ static int media_get_devname_sysfs(struct media_entity *entity) >> return 0; >> } >> >> + > > Unrelated change. > >> static int media_enum_entities(struct media_device *media) >> { >> struct media_entity *entity; >> @@ -707,6 +710,92 @@ struct media_device *media_device_new(const char *devnode) >> return media; >> } >> >> +struct media_device *media_device_new_by_subdev_fd(int fd, struct media_entity **fd_entity) >> +{ >> + char video_devname[32], device_dir_path[256], media_dev_path[256], media_major_minor[10]; >> + struct media_device *media = NULL; >> + struct dirent *entry; >> + struct media_entity tmp_entity; >> + DIR *device_dir; >> + struct udev *udev; >> + char *p; >> + int ret, i; >> + >> + if (fd_entity == NULL) >> + return NULL; >> + >> + ret = media_get_devname_by_fd(fd, video_devname); >> + if (ret < 0) >> + return NULL; >> + >> + p = strrchr(video_devname, '/'); >> + if (p == NULL) >> + return NULL; >> + >> + ret = media_udev_open(&udev); >> + if (ret < 0) >> + return NULL; >> + >> + sprintf(device_dir_path, "/sys/class/video4linux/%s/device/", p + 1); >> + >> + device_dir = opendir(device_dir_path); >> + if (device_dir == NULL) >> + return NULL; >> + >> + while ((entry = readdir(device_dir))) { >> + if (strncmp(entry->d_name, "media", 4)) > > Why 4? And isn't entry->d_name nul-terminated, so you could use strcmp()? Media devices, as other devices, have numerical postfix, which is not of our interest. >> + continue; >> + >> + sprintf(media_dev_path, "%s%s/dev", device_dir_path, entry->d_name); >> + >> + fd = open(media_dev_path, O_RDONLY); >> + if (fd < 0) >> + continue; >> + >> + ret = read(fd, media_major_minor, sizeof(media_major_minor)); >> + if (ret < 0) >> + continue; >> + >> + sscanf(media_major_minor, "%d:%d", &tmp_entity.info.dev.major, &tmp_entity.info.dev.minor); > > This would be better split on two lines. OK. >> + >> + /* Try to get the device name via udev */ >> + if (media_get_devname_udev(udev, &tmp_entity)) { >> + /* Fall back to get the device name via sysfs */ >> + if (media_get_devname_sysfs(&tmp_entity)) >> + continue; >> + } >> + >> + media = media_device_new(tmp_entity.devname); >> + if (media == NULL) >> + continue; >> + >> + ret = media_device_enumerate(media); >> + if (ret < 0) { >> + media_dbg(media, "Failed to enumerate %s (%d)\n", >> + tmp_entity.devname, ret); >> + media_device_unref(media); >> + media = NULL; >> + continue; >> + } >> + >> + /* Get the entity associated with given fd */ >> + for (i = 0; i < media->entities_count; i++) { >> + struct media_entity *entity = &media->entities[i]; >> + >> + if (!strcmp(entity->devname, video_devname)) { >> + *fd_entity = &media->entities[i]; >> + break; >> + } >> + } > > What if you exit the loop without finding the entity you were looking for? Ah, right, this case is unhandled. Adding below condition should cover that: if (i == media->entities_count) media = NULL; >> + >> + break; This break should be removed and the one in the inner for loop above should be replaced with goto here. Are you OK with that? >> + } >> + >> + media_udev_close(udev); >> + >> + return media; >> +} >> + >> struct media_device *media_device_new_emulated(struct media_device_info *info) >> { >> struct media_device *media; >> @@ -748,6 +837,44 @@ void media_device_unref(struct media_device *media) >> free(media); >> } >> >> +int media_get_devname_by_fd(int fd, char *node_name) >> +{ >> + struct udev *udev; >> + struct media_entity tmp_entity; >> + struct stat stat; >> + int ret, ret_udev; >> + >> + if (node_name == NULL) >> + return -EINVAL; >> + >> + ret = fstat(fd, &stat); >> + if (ret < 0) >> + return -errno; >> + >> + tmp_entity.info.v4l.major = MAJOR(stat.st_rdev); >> + tmp_entity.info.v4l.minor = MINOR(stat.st_rdev); >> + >> + ret_udev = media_udev_open(&udev); >> + if (ret_udev < 0) >> + printf("Can't get udev context\n"); >> + >> + /* Try to get the device name via udev */ >> + ret = media_get_devname_udev(udev, &tmp_entity); >> + if (!ret) >> + goto out; >> + >> + ret = media_get_devname_sysfs(&tmp_entity); >> + if (ret < 0) >> + goto err_get_devname; >> + >> +out: >> + strncpy(node_name, tmp_entity.devname, sizeof(tmp_entity.devname)); > > This seems risky. How does the caller know the maximum size? I'd simply > allocate the string, and document the caller is responsible for releasing > it. OK. >> +err_get_devname: >> + if (!ret_udev) >> + media_udev_close(udev); >> + return ret; >> +} >> + >> int media_device_add_entity(struct media_device *media, >> const struct media_entity_desc *desc, >> const char *devnode) >> diff --git a/utils/media-ctl/mediactl.h b/utils/media-ctl/mediactl.h >> index b1f33cd..580a25a 100644 >> --- a/utils/media-ctl/mediactl.h >> +++ b/utils/media-ctl/mediactl.h >> @@ -76,6 +76,21 @@ struct media_device *media_device_new(const char *devnode); >> struct media_device *media_device_new_emulated(struct media_device_info *info); >> >> /** >> + * @brief Create a new media device contatning entity associated with v4l2 subdev fd. >> + * @param fd - file descriptor of a v4l2 subdev. >> + * @param fd_entity - media entity associated with the v4l2 subdev. >> + * >> + * Create a representation of the media device referenced by the v4l2-subdev. >> + * The media device instance is initialized with enumerated entities and links. >> + * >> + * Media devices are reference-counted, see media_device_ref() and >> + * media_device_unref() for more information. >> + * >> + * @return A pointer to the new media device or NULL if error occurred. >> + */ >> +struct media_device *media_device_new_by_subdev_fd(int fd, struct media_entity **fd_entity); > > I'd drop the "subdev_" part as both V4L2 device nodes and sub-devices work. > > If you wish to keep this V4L2 specific, I suggest ...by_v4l2_fd(). Right, I will rename it. >> + >> +/** >> * @brief Take a reference to the device. >> * @param media - device instance. >> * >> @@ -231,6 +246,18 @@ const struct media_link *media_entity_get_link(struct media_entity *entity, >> const char *media_entity_get_devname(struct media_entity *entity); >> >> /** >> + * @brief Get the device node name by its file descriptor >> + * @param fd - file descriptor of a device. >> + * @param node_name - output device node name string. >> + * >> + * This function returns the full path and name to the device node corresponding >> + * to the given file descriptor. >> + * >> + * @return 0 on success, or a negative error code on failure. >> + */ >> +int media_get_devname_by_fd(int fd, char *node_name); >> + >> +/** >> * @brief Get the type of an entity. >> * @param entity - the entity. >> * >
Hi Jacek, On Thu, Nov 24, 2016 at 02:50:39PM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > Thanks for the review. > > On 11/24/2016 01:17 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >Thanks for the patchset. > > > >On Wed, Oct 12, 2016 at 04:35:19PM +0200, Jacek Anaszewski wrote: > >>Add helper functions that allow for easy instantiation of media_device > >>object basing on whether the media device contains v4l2 subdev with > >>given file descriptor. > > > >Doesn't this work with video nodes as well? That's what you seem to be using > >it for later on. And I think that's actually more useful. > > Exactly, thanks for spotting this. > > s/v4l2 subdev/video device opened/ > > > > >The existing implementation uses udev to look up devices. Could you use > >libudev device enumeration API to find the media devices, and fall back to > >sysfs if udev doesn't work? There seems to be a reasonable-looking example > >here: > > > ><URL:http://stackoverflow.com/questions/25361042/how-to-list-usb-mass-storage-devices-programatically-using-libudev-in-linux> > > I'll check that, thanks. > > >> > >>Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> > >>Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > >>--- > >> utils/media-ctl/libmediactl.c | 131 +++++++++++++++++++++++++++++++++++++++++- > >> utils/media-ctl/mediactl.h | 27 +++++++++ > >> 2 files changed, 156 insertions(+), 2 deletions(-) > >> > >>diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c > >>index 155b65f..d347a40 100644 > >>--- a/utils/media-ctl/libmediactl.c > >>+++ b/utils/media-ctl/libmediactl.c > >>@@ -27,6 +27,7 @@ > >> #include <sys/sysmacros.h> > >> > >> #include <ctype.h> > >>+#include <dirent.h> > >> #include <errno.h> > >> #include <fcntl.h> > >> #include <stdbool.h> > >>@@ -440,8 +441,9 @@ static int media_get_devname_udev(struct udev *udev, > >> return -EINVAL; > >> > >> devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor); > >>- media_dbg(entity->media, "looking up device: %u:%u\n", > >>- major(devnum), minor(devnum)); > >>+ if (entity->media) > >>+ media_dbg(entity->media, "looking up device: %u:%u\n", > >>+ major(devnum), minor(devnum)); > >> device = udev_device_new_from_devnum(udev, 'c', devnum); > >> if (device) { > >> p = udev_device_get_devnode(device); > >>@@ -523,6 +525,7 @@ static int media_get_devname_sysfs(struct media_entity *entity) > >> return 0; > >> } > >> > >>+ > > > >Unrelated change. > > > >> static int media_enum_entities(struct media_device *media) > >> { > >> struct media_entity *entity; > >>@@ -707,6 +710,92 @@ struct media_device *media_device_new(const char *devnode) > >> return media; > >> } > >> > >>+struct media_device *media_device_new_by_subdev_fd(int fd, struct media_entity **fd_entity) > >>+{ > >>+ char video_devname[32], device_dir_path[256], media_dev_path[256], media_major_minor[10]; > >>+ struct media_device *media = NULL; > >>+ struct dirent *entry; > >>+ struct media_entity tmp_entity; > >>+ DIR *device_dir; > >>+ struct udev *udev; > >>+ char *p; > >>+ int ret, i; > >>+ > >>+ if (fd_entity == NULL) > >>+ return NULL; > >>+ > >>+ ret = media_get_devname_by_fd(fd, video_devname); > >>+ if (ret < 0) > >>+ return NULL; > >>+ > >>+ p = strrchr(video_devname, '/'); > >>+ if (p == NULL) > >>+ return NULL; > >>+ > >>+ ret = media_udev_open(&udev); > >>+ if (ret < 0) > >>+ return NULL; > >>+ > >>+ sprintf(device_dir_path, "/sys/class/video4linux/%s/device/", p + 1); > >>+ > >>+ device_dir = opendir(device_dir_path); > >>+ if (device_dir == NULL) > >>+ return NULL; > >>+ > >>+ while ((entry = readdir(device_dir))) { > >>+ if (strncmp(entry->d_name, "media", 4)) > > > >Why 4? And isn't entry->d_name nul-terminated, so you could use strcmp()? > > Media devices, as other devices, have numerical postfix, which is > not of our interest. Right. But still 5 would be the right number as we should also check the last "a". > > >>+ continue; > >>+ > >>+ sprintf(media_dev_path, "%s%s/dev", device_dir_path, entry->d_name); > >>+ > >>+ fd = open(media_dev_path, O_RDONLY); > >>+ if (fd < 0) > >>+ continue; > >>+ > >>+ ret = read(fd, media_major_minor, sizeof(media_major_minor)); > >>+ if (ret < 0) > >>+ continue; > >>+ > >>+ sscanf(media_major_minor, "%d:%d", &tmp_entity.info.dev.major, &tmp_entity.info.dev.minor); > > > >This would be better split on two lines. > > OK. > > >>+ > >>+ /* Try to get the device name via udev */ > >>+ if (media_get_devname_udev(udev, &tmp_entity)) { > >>+ /* Fall back to get the device name via sysfs */ > >>+ if (media_get_devname_sysfs(&tmp_entity)) > >>+ continue; > >>+ } > >>+ > >>+ media = media_device_new(tmp_entity.devname); > >>+ if (media == NULL) > >>+ continue; > >>+ > >>+ ret = media_device_enumerate(media); > >>+ if (ret < 0) { > >>+ media_dbg(media, "Failed to enumerate %s (%d)\n", > >>+ tmp_entity.devname, ret); > >>+ media_device_unref(media); > >>+ media = NULL; > >>+ continue; > >>+ } > >>+ > >>+ /* Get the entity associated with given fd */ > >>+ for (i = 0; i < media->entities_count; i++) { > >>+ struct media_entity *entity = &media->entities[i]; > >>+ > >>+ if (!strcmp(entity->devname, video_devname)) { > >>+ *fd_entity = &media->entities[i]; > >>+ break; > >>+ } > >>+ } > > > >What if you exit the loop without finding the entity you were looking for? > > Ah, right, this case is unhandled. > > Adding below condition should cover that: > > if (i == media->entities_count) > media = NULL; and media_device_unref()? You could have a label for handling that at the end of the loop basic block so you could implement handling of that just once to avoid such issues in the future. > > >>+ > >>+ break; > > This break should be removed and the one in the inner for loop above > should be replaced with goto here. Are you OK with that? Um, yeah. There are indeed two loops. In Perl you could get out nicely but in C we have to do something else. Two labels perhaps? > > >>+ } > >>+ > >>+ media_udev_close(udev); > >>+ > >>+ return media; > >>+} > >>+ > >> struct media_device *media_device_new_emulated(struct media_device_info *info) > >> { > >> struct media_device *media;
On 11/24/2016 03:32 PM, Sakari Ailus wrote: [...] >>>> + sprintf(device_dir_path, "/sys/class/video4linux/%s/device/", p + 1); >>>> + >>>> + device_dir = opendir(device_dir_path); >>>> + if (device_dir == NULL) >>>> + return NULL; >>>> + >>>> + while ((entry = readdir(device_dir))) { >>>> + if (strncmp(entry->d_name, "media", 4)) >>> >>> Why 4? And isn't entry->d_name nul-terminated, so you could use strcmp()? >> >> Media devices, as other devices, have numerical postfix, which is >> not of our interest. > > Right. But still 5 would be the right number as we should also check the > last "a". Of course, this needs to be fixed, thanks.
Hi Sakari, On 11/24/2016 01:17 PM, Sakari Ailus wrote: > Hi Jacek, > > Thanks for the patchset. > > On Wed, Oct 12, 2016 at 04:35:19PM +0200, Jacek Anaszewski wrote: >> Add helper functions that allow for easy instantiation of media_device >> object basing on whether the media device contains v4l2 subdev with >> given file descriptor. > > Doesn't this work with video nodes as well? That's what you seem to be using > it for later on. And I think that's actually more useful. > > The existing implementation uses udev to look up devices. Could you use > libudev device enumeration API to find the media devices, and fall back to > sysfs if udev doesn't work? There seems to be a reasonable-looking example > here: > > <URL:http://stackoverflow.com/questions/25361042/how-to-list-usb-mass-storage-devices-programatically-using-libudev-in-linux> Actually I am calling media_get_devname_udev() at first and falling back to sysfs similarly as it is accomplished in media_enum_entities(). Is there any specific reason for which I should use libudev device enumeration API in media_device_new_by_subdev_fd()? Best regards, Jacek Anaszewski >> >> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> >> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> utils/media-ctl/libmediactl.c | 131 +++++++++++++++++++++++++++++++++++++++++- >> utils/media-ctl/mediactl.h | 27 +++++++++ >> 2 files changed, 156 insertions(+), 2 deletions(-) >> >> diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c >> index 155b65f..d347a40 100644 >> --- a/utils/media-ctl/libmediactl.c >> +++ b/utils/media-ctl/libmediactl.c >> @@ -27,6 +27,7 @@ >> #include <sys/sysmacros.h> >> >> #include <ctype.h> >> +#include <dirent.h> >> #include <errno.h> >> #include <fcntl.h> >> #include <stdbool.h> >> @@ -440,8 +441,9 @@ static int media_get_devname_udev(struct udev *udev, >> return -EINVAL; >> >> devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor); >> - media_dbg(entity->media, "looking up device: %u:%u\n", >> - major(devnum), minor(devnum)); >> + if (entity->media) >> + media_dbg(entity->media, "looking up device: %u:%u\n", >> + major(devnum), minor(devnum)); >> device = udev_device_new_from_devnum(udev, 'c', devnum); >> if (device) { >> p = udev_device_get_devnode(device); >> @@ -523,6 +525,7 @@ static int media_get_devname_sysfs(struct media_entity *entity) >> return 0; >> } >> >> + > > Unrelated change. > >> static int media_enum_entities(struct media_device *media) >> { >> struct media_entity *entity; >> @@ -707,6 +710,92 @@ struct media_device *media_device_new(const char *devnode) >> return media; >> } >> >> +struct media_device *media_device_new_by_subdev_fd(int fd, struct media_entity **fd_entity) >> +{ >> + char video_devname[32], device_dir_path[256], media_dev_path[256], media_major_minor[10]; >> + struct media_device *media = NULL; >> + struct dirent *entry; >> + struct media_entity tmp_entity; >> + DIR *device_dir; >> + struct udev *udev; >> + char *p; >> + int ret, i; >> + >> + if (fd_entity == NULL) >> + return NULL; >> + >> + ret = media_get_devname_by_fd(fd, video_devname); >> + if (ret < 0) >> + return NULL; >> + >> + p = strrchr(video_devname, '/'); >> + if (p == NULL) >> + return NULL; >> + >> + ret = media_udev_open(&udev); >> + if (ret < 0) >> + return NULL; >> + >> + sprintf(device_dir_path, "/sys/class/video4linux/%s/device/", p + 1); >> + >> + device_dir = opendir(device_dir_path); >> + if (device_dir == NULL) >> + return NULL; >> + >> + while ((entry = readdir(device_dir))) { >> + if (strncmp(entry->d_name, "media", 4)) > > Why 4? And isn't entry->d_name nul-terminated, so you could use strcmp()? > >> + continue; >> + >> + sprintf(media_dev_path, "%s%s/dev", device_dir_path, entry->d_name); >> + >> + fd = open(media_dev_path, O_RDONLY); >> + if (fd < 0) >> + continue; >> + >> + ret = read(fd, media_major_minor, sizeof(media_major_minor)); >> + if (ret < 0) >> + continue; >> + >> + sscanf(media_major_minor, "%d:%d", &tmp_entity.info.dev.major, &tmp_entity.info.dev.minor); > > This would be better split on two lines. > >> + >> + /* Try to get the device name via udev */ >> + if (media_get_devname_udev(udev, &tmp_entity)) { >> + /* Fall back to get the device name via sysfs */ >> + if (media_get_devname_sysfs(&tmp_entity)) >> + continue; >> + } >> + >> + media = media_device_new(tmp_entity.devname); >> + if (media == NULL) >> + continue; >> + >> + ret = media_device_enumerate(media); >> + if (ret < 0) { >> + media_dbg(media, "Failed to enumerate %s (%d)\n", >> + tmp_entity.devname, ret); >> + media_device_unref(media); >> + media = NULL; >> + continue; >> + } >> + >> + /* Get the entity associated with given fd */ >> + for (i = 0; i < media->entities_count; i++) { >> + struct media_entity *entity = &media->entities[i]; >> + >> + if (!strcmp(entity->devname, video_devname)) { >> + *fd_entity = &media->entities[i]; >> + break; >> + } >> + } > > What if you exit the loop without finding the entity you were looking for? > >> + >> + break; >> + } >> + >> + media_udev_close(udev); >> + >> + return media; >> +} >> + >> struct media_device *media_device_new_emulated(struct media_device_info *info) >> { >> struct media_device *media; >> @@ -748,6 +837,44 @@ void media_device_unref(struct media_device *media) >> free(media); >> } >> >> +int media_get_devname_by_fd(int fd, char *node_name) >> +{ >> + struct udev *udev; >> + struct media_entity tmp_entity; >> + struct stat stat; >> + int ret, ret_udev; >> + >> + if (node_name == NULL) >> + return -EINVAL; >> + >> + ret = fstat(fd, &stat); >> + if (ret < 0) >> + return -errno; >> + >> + tmp_entity.info.v4l.major = MAJOR(stat.st_rdev); >> + tmp_entity.info.v4l.minor = MINOR(stat.st_rdev); >> + >> + ret_udev = media_udev_open(&udev); >> + if (ret_udev < 0) >> + printf("Can't get udev context\n"); >> + >> + /* Try to get the device name via udev */ >> + ret = media_get_devname_udev(udev, &tmp_entity); >> + if (!ret) >> + goto out; >> + >> + ret = media_get_devname_sysfs(&tmp_entity); >> + if (ret < 0) >> + goto err_get_devname; >> + >> +out: >> + strncpy(node_name, tmp_entity.devname, sizeof(tmp_entity.devname)); > > This seems risky. How does the caller know the maximum size? I'd simply > allocate the string, and document the caller is responsible for releasing > it. > >> +err_get_devname: >> + if (!ret_udev) >> + media_udev_close(udev); >> + return ret; >> +} >> + >> int media_device_add_entity(struct media_device *media, >> const struct media_entity_desc *desc, >> const char *devnode) >> diff --git a/utils/media-ctl/mediactl.h b/utils/media-ctl/mediactl.h >> index b1f33cd..580a25a 100644 >> --- a/utils/media-ctl/mediactl.h >> +++ b/utils/media-ctl/mediactl.h >> @@ -76,6 +76,21 @@ struct media_device *media_device_new(const char *devnode); >> struct media_device *media_device_new_emulated(struct media_device_info *info); >> >> /** >> + * @brief Create a new media device contatning entity associated with v4l2 subdev fd. >> + * @param fd - file descriptor of a v4l2 subdev. >> + * @param fd_entity - media entity associated with the v4l2 subdev. >> + * >> + * Create a representation of the media device referenced by the v4l2-subdev. >> + * The media device instance is initialized with enumerated entities and links. >> + * >> + * Media devices are reference-counted, see media_device_ref() and >> + * media_device_unref() for more information. >> + * >> + * @return A pointer to the new media device or NULL if error occurred. >> + */ >> +struct media_device *media_device_new_by_subdev_fd(int fd, struct media_entity **fd_entity); > > I'd drop the "subdev_" part as both V4L2 device nodes and sub-devices work. > > If you wish to keep this V4L2 specific, I suggest ...by_v4l2_fd(). > >> + >> +/** >> * @brief Take a reference to the device. >> * @param media - device instance. >> * >> @@ -231,6 +246,18 @@ const struct media_link *media_entity_get_link(struct media_entity *entity, >> const char *media_entity_get_devname(struct media_entity *entity); >> >> /** >> + * @brief Get the device node name by its file descriptor >> + * @param fd - file descriptor of a device. >> + * @param node_name - output device node name string. >> + * >> + * This function returns the full path and name to the device node corresponding >> + * to the given file descriptor. >> + * >> + * @return 0 on success, or a negative error code on failure. >> + */ >> +int media_get_devname_by_fd(int fd, char *node_name); >> + >> +/** >> * @brief Get the type of an entity. >> * @param entity - the entity. >> * > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacek, On Thu, Dec 08, 2016 at 11:04:20PM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > On 11/24/2016 01:17 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >Thanks for the patchset. > > > >On Wed, Oct 12, 2016 at 04:35:19PM +0200, Jacek Anaszewski wrote: > >>Add helper functions that allow for easy instantiation of media_device > >>object basing on whether the media device contains v4l2 subdev with > >>given file descriptor. > > > >Doesn't this work with video nodes as well? That's what you seem to be using > >it for later on. And I think that's actually more useful. > > > >The existing implementation uses udev to look up devices. Could you use > >libudev device enumeration API to find the media devices, and fall back to > >sysfs if udev doesn't work? There seems to be a reasonable-looking example > >here: > > > ><URL:http://stackoverflow.com/questions/25361042/how-to-list-usb-mass-storage-devices-programatically-using-libudev-in-linux> > > Actually I am calling media_get_devname_udev() at first and falling back > to sysfs similarly as it is accomplished in media_enum_entities(). > Is there any specific reason for which I should use libudev device > enumeration API in media_device_new_by_subdev_fd()? Yes. You rely on the API udev provides; the sysfs implementation is just a fallback in case udev isn't available in the system. I guess it'd mostly work but, for instance, you assume sysfs is found under /sys. The sysfs itself isn't one of the most stable APIs either. Udev is a simply better option when it's there.
Hhi Sakari, On 12/09/2016 12:05 AM, Sakari Ailus wrote: > Hi Jacek, > > On Thu, Dec 08, 2016 at 11:04:20PM +0100, Jacek Anaszewski wrote: >> Hi Sakari, >> >> On 11/24/2016 01:17 PM, Sakari Ailus wrote: >>> Hi Jacek, >>> >>> Thanks for the patchset. >>> >>> On Wed, Oct 12, 2016 at 04:35:19PM +0200, Jacek Anaszewski wrote: >>>> Add helper functions that allow for easy instantiation of media_device >>>> object basing on whether the media device contains v4l2 subdev with >>>> given file descriptor. >>> >>> Doesn't this work with video nodes as well? That's what you seem to be using >>> it for later on. And I think that's actually more useful. >>> >>> The existing implementation uses udev to look up devices. Could you use >>> libudev device enumeration API to find the media devices, and fall back to >>> sysfs if udev doesn't work? There seems to be a reasonable-looking example >>> here: >>> >>> <URL:http://stackoverflow.com/questions/25361042/how-to-list-usb-mass-storage-devices-programatically-using-libudev-in-linux> >> >> Actually I am calling media_get_devname_udev() at first and falling back >> to sysfs similarly as it is accomplished in media_enum_entities(). >> Is there any specific reason for which I should use libudev device >> enumeration API in media_device_new_by_subdev_fd()? > > Yes. You rely on the API udev provides; the sysfs implementation is just a > fallback in case udev isn't available in the system. I guess it'd mostly > work but, for instance, you assume sysfs is found under /sys. The sysfs > itself isn't one of the most stable APIs either. Udev is a simply better > option when it's there. Thanks for clarifying that. I'll check the libudev device enumeration API then.
diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c index 155b65f..d347a40 100644 --- a/utils/media-ctl/libmediactl.c +++ b/utils/media-ctl/libmediactl.c @@ -27,6 +27,7 @@ #include <sys/sysmacros.h> #include <ctype.h> +#include <dirent.h> #include <errno.h> #include <fcntl.h> #include <stdbool.h> @@ -440,8 +441,9 @@ static int media_get_devname_udev(struct udev *udev, return -EINVAL; devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor); - media_dbg(entity->media, "looking up device: %u:%u\n", - major(devnum), minor(devnum)); + if (entity->media) + media_dbg(entity->media, "looking up device: %u:%u\n", + major(devnum), minor(devnum)); device = udev_device_new_from_devnum(udev, 'c', devnum); if (device) { p = udev_device_get_devnode(device); @@ -523,6 +525,7 @@ static int media_get_devname_sysfs(struct media_entity *entity) return 0; } + static int media_enum_entities(struct media_device *media) { struct media_entity *entity; @@ -707,6 +710,92 @@ struct media_device *media_device_new(const char *devnode) return media; } +struct media_device *media_device_new_by_subdev_fd(int fd, struct media_entity **fd_entity) +{ + char video_devname[32], device_dir_path[256], media_dev_path[256], media_major_minor[10]; + struct media_device *media = NULL; + struct dirent *entry; + struct media_entity tmp_entity; + DIR *device_dir; + struct udev *udev; + char *p; + int ret, i; + + if (fd_entity == NULL) + return NULL; + + ret = media_get_devname_by_fd(fd, video_devname); + if (ret < 0) + return NULL; + + p = strrchr(video_devname, '/'); + if (p == NULL) + return NULL; + + ret = media_udev_open(&udev); + if (ret < 0) + return NULL; + + sprintf(device_dir_path, "/sys/class/video4linux/%s/device/", p + 1); + + device_dir = opendir(device_dir_path); + if (device_dir == NULL) + return NULL; + + while ((entry = readdir(device_dir))) { + if (strncmp(entry->d_name, "media", 4)) + continue; + + sprintf(media_dev_path, "%s%s/dev", device_dir_path, entry->d_name); + + fd = open(media_dev_path, O_RDONLY); + if (fd < 0) + continue; + + ret = read(fd, media_major_minor, sizeof(media_major_minor)); + if (ret < 0) + continue; + + sscanf(media_major_minor, "%d:%d", &tmp_entity.info.dev.major, &tmp_entity.info.dev.minor); + + /* Try to get the device name via udev */ + if (media_get_devname_udev(udev, &tmp_entity)) { + /* Fall back to get the device name via sysfs */ + if (media_get_devname_sysfs(&tmp_entity)) + continue; + } + + media = media_device_new(tmp_entity.devname); + if (media == NULL) + continue; + + ret = media_device_enumerate(media); + if (ret < 0) { + media_dbg(media, "Failed to enumerate %s (%d)\n", + tmp_entity.devname, ret); + media_device_unref(media); + media = NULL; + continue; + } + + /* Get the entity associated with given fd */ + for (i = 0; i < media->entities_count; i++) { + struct media_entity *entity = &media->entities[i]; + + if (!strcmp(entity->devname, video_devname)) { + *fd_entity = &media->entities[i]; + break; + } + } + + break; + } + + media_udev_close(udev); + + return media; +} + struct media_device *media_device_new_emulated(struct media_device_info *info) { struct media_device *media; @@ -748,6 +837,44 @@ void media_device_unref(struct media_device *media) free(media); } +int media_get_devname_by_fd(int fd, char *node_name) +{ + struct udev *udev; + struct media_entity tmp_entity; + struct stat stat; + int ret, ret_udev; + + if (node_name == NULL) + return -EINVAL; + + ret = fstat(fd, &stat); + if (ret < 0) + return -errno; + + tmp_entity.info.v4l.major = MAJOR(stat.st_rdev); + tmp_entity.info.v4l.minor = MINOR(stat.st_rdev); + + ret_udev = media_udev_open(&udev); + if (ret_udev < 0) + printf("Can't get udev context\n"); + + /* Try to get the device name via udev */ + ret = media_get_devname_udev(udev, &tmp_entity); + if (!ret) + goto out; + + ret = media_get_devname_sysfs(&tmp_entity); + if (ret < 0) + goto err_get_devname; + +out: + strncpy(node_name, tmp_entity.devname, sizeof(tmp_entity.devname)); +err_get_devname: + if (!ret_udev) + media_udev_close(udev); + return ret; +} + int media_device_add_entity(struct media_device *media, const struct media_entity_desc *desc, const char *devnode) diff --git a/utils/media-ctl/mediactl.h b/utils/media-ctl/mediactl.h index b1f33cd..580a25a 100644 --- a/utils/media-ctl/mediactl.h +++ b/utils/media-ctl/mediactl.h @@ -76,6 +76,21 @@ struct media_device *media_device_new(const char *devnode); struct media_device *media_device_new_emulated(struct media_device_info *info); /** + * @brief Create a new media device contatning entity associated with v4l2 subdev fd. + * @param fd - file descriptor of a v4l2 subdev. + * @param fd_entity - media entity associated with the v4l2 subdev. + * + * Create a representation of the media device referenced by the v4l2-subdev. + * The media device instance is initialized with enumerated entities and links. + * + * Media devices are reference-counted, see media_device_ref() and + * media_device_unref() for more information. + * + * @return A pointer to the new media device or NULL if error occurred. + */ +struct media_device *media_device_new_by_subdev_fd(int fd, struct media_entity **fd_entity); + +/** * @brief Take a reference to the device. * @param media - device instance. * @@ -231,6 +246,18 @@ const struct media_link *media_entity_get_link(struct media_entity *entity, const char *media_entity_get_devname(struct media_entity *entity); /** + * @brief Get the device node name by its file descriptor + * @param fd - file descriptor of a device. + * @param node_name - output device node name string. + * + * This function returns the full path and name to the device node corresponding + * to the given file descriptor. + * + * @return 0 on success, or a negative error code on failure. + */ +int media_get_devname_by_fd(int fd, char *node_name); + +/** * @brief Get the type of an entity. * @param entity - the entity. *