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@xxxxxxxxxxx> > >>Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> > >>--- > >> 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; -- Kind regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
