[media-ctl][PATCH] libmediactl: engage udev to get devname

[media-ctl][PATCH] libmediactl: engage udev to get devname

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 configure.in    |   10 ++++++++
 src/Makefile.am |    2 +
 src/media.c     |   66 ++++++++++++++++++++++++++----------------------------
 3 files changed, 44 insertions(+), 34 deletions(-)

diff --git a/configure.in b/configure.in
index fd4c70c..63432ba 100644
--- a/configure.in
+++ b/configure.in
@@ -12,6 +12,16 @@ AC_PROG_CC
 AC_PROG_LIBTOOL
 
 # Checks for libraries.
+PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes, have_libudev=no)
+
+if test x$have_libudev = xyes; then
+    LIBUDEV_CFLAGS="$lbudev_CFLAGS"
+    LIBUDEV_LIBS="$libudev_LIBS"
+    AC_SUBST(LIBUDEV_CFLAGS)
+    AC_SUBST(LIBUDEV_LIBS)
+else
+    AC_MSG_ERROR([libudev is required])
+fi
 
 # Kernel headers path.
 AC_ARG_WITH(kernel-headers,
diff --git a/src/Makefile.am b/src/Makefile.am
index 267ea83..52628d2 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -5,6 +5,8 @@ mediactl_includedir=$(includedir)/mediactl
 mediactl_include_HEADERS = media.h subdev.h
 
 bin_PROGRAMS = media-ctl
+media_ctl_CFLAGS = $(LIBUDEV_CFLAGS)
+media_ctl_LDFLAGS = $(LIBUDEV_LIBS)
 media_ctl_SOURCES = main.c options.c options.h tools.h
 media_ctl_LDADD = libmediactl.la libv4l2subdev.la
 
diff --git a/src/media.c b/src/media.c
index e3cab86..000d750 100644
--- a/src/media.c
+++ b/src/media.c
@@ -31,6 +31,8 @@
 #include <linux/videodev2.h>
 #include <linux/media.h>
 
+#include <libudev.h>
+
 #include "media.h"
 #include "tools.h"
 
@@ -247,15 +249,20 @@ static int media_enum_links(struct media_device *media)
 
 static int media_enum_entities(struct media_device *media)
 {
+	struct udev *udev;
+	dev_t devnum;
+	struct udev_device *device;
 	struct media_entity *entity;
-	struct stat devstat;
 	unsigned int size;
-	char devname[32];
-	char sysname[32];
-	char target[1024];
-	char *p;
+	const char *p;
 	__u32 id;
-	int ret;
+	int ret = 0;
+
+	udev = udev_new();
+	if (udev == NULL) {
+		printf("unable to allocate memory for context\n");
+		return -ENOMEM;
+	}
 
 	for (id = 0; ; id = entity->info.id) {
 		size = (media->entities_count + 1) * sizeof(*media->entities);
@@ -268,9 +275,9 @@ static int media_enum_entities(struct media_device *media)
 
 		ret = ioctl(media->fd, MEDIA_IOC_ENUM_ENTITIES, &entity->info);
 		if (ret < 0) {
-			if (errno == EINVAL)
-				break;
-			return -errno;
+			if (errno != EINVAL)
+				ret = -errno;
+			break;
 		}
 
 		/* Number of links (for outbound links) plus number of pads (for
@@ -281,8 +288,10 @@ static int media_enum_entities(struct media_device *media)
 
 		entity->pads = malloc(entity->info.pads * sizeof(*entity->pads));
 		entity->links = malloc(entity->max_links * sizeof(*entity->links));
-		if (entity->pads == NULL || entity->links == NULL)
-			return -ENOMEM;
+		if (entity->pads == NULL || entity->links == NULL) {
+			ret = -ENOMEM;
+			break;
+		}
 
 		media->entities_count++;
 
@@ -291,32 +300,21 @@ static int media_enum_entities(struct media_device *media)
 		    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
 			continue;
 
-		sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
-			entity->info.v4l.minor);
-		ret = readlink(sysname, target, sizeof(target));
-		if (ret < 0)
-			continue;
-
-		target[ret] = '\0';
-		p = strrchr(target, '/');
-		if (p == NULL)
-			continue;
-
-		sprintf(devname, "/dev/%s", p + 1);
-		ret = stat(devname, &devstat);
-		if (ret < 0)
-			continue;
+		devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
+		printf("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);
+			if (p)
+				snprintf(entity->devname, sizeof(entity->devname),
+					 "%s", p);
+		}
 
-		/* Sanity check: udev might have reordered the device nodes.
-		 * Make sure the major/minor match. We should really use
-		 * libudev.
-		 */
-		if (major(devstat.st_rdev) == entity->info.v4l.major &&
-		    minor(devstat.st_rdev) == entity->info.v4l.minor)
-			strcpy(entity->devname, devname);
+		udev_device_unref(device);
 	}
 
-	return 0;
+	udev_unref(udev);
+	return ret;
 }
 
 struct media_device *media_open(const char *name, int verbose)
-- 
1.7.5.4

Re: [media-ctl][PATCH] libmediactl: engage udev to get devname

[Laurent Pinchart]

Hi Andy,

Thank you for the patch.

What about making it a configuration option to still support systems that 
don't provide libudev ? We could keep the current behaviour for those.

On Monday 15 August 2011 14:20:34 Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  configure.in    |   10 ++++++++
>  src/Makefile.am |    2 +
>  src/media.c     |   66
> ++++++++++++++++++++++++++---------------------------- 3 files changed, 44
> insertions(+), 34 deletions(-)
> 
> diff --git a/configure.in b/configure.in
> index fd4c70c..63432ba 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -12,6 +12,16 @@ AC_PROG_CC
>  AC_PROG_LIBTOOL
> 
>  # Checks for libraries.
> +PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes, have_libudev=no)
> +
> +if test x$have_libudev = xyes; then
> +    LIBUDEV_CFLAGS="$lbudev_CFLAGS"
> +    LIBUDEV_LIBS="$libudev_LIBS"
> +    AC_SUBST(LIBUDEV_CFLAGS)
> +    AC_SUBST(LIBUDEV_LIBS)
> +else
> +    AC_MSG_ERROR([libudev is required])
> +fi
> 
>  # Kernel headers path.
>  AC_ARG_WITH(kernel-headers,
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 267ea83..52628d2 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -5,6 +5,8 @@ mediactl_includedir=$(includedir)/mediactl
>  mediactl_include_HEADERS = media.h subdev.h
> 
>  bin_PROGRAMS = media-ctl
> +media_ctl_CFLAGS = $(LIBUDEV_CFLAGS)
> +media_ctl_LDFLAGS = $(LIBUDEV_LIBS)
>  media_ctl_SOURCES = main.c options.c options.h tools.h
>  media_ctl_LDADD = libmediactl.la libv4l2subdev.la
> 
> diff --git a/src/media.c b/src/media.c
> index e3cab86..000d750 100644
> --- a/src/media.c
> +++ b/src/media.c
> @@ -31,6 +31,8 @@
>  #include <linux/videodev2.h>
>  #include <linux/media.h>
> 
> +#include <libudev.h>
> +
>  #include "media.h"
>  #include "tools.h"
> 
> @@ -247,15 +249,20 @@ static int media_enum_links(struct media_device
> *media)
> 
>  static int media_enum_entities(struct media_device *media)
>  {
> +	struct udev *udev;
> +	dev_t devnum;
> +	struct udev_device *device;
>  	struct media_entity *entity;
> -	struct stat devstat;
>  	unsigned int size;
> -	char devname[32];
> -	char sysname[32];
> -	char target[1024];
> -	char *p;
> +	const char *p;
>  	__u32 id;
> -	int ret;
> +	int ret = 0;
> +
> +	udev = udev_new();
> +	if (udev == NULL) {
> +		printf("unable to allocate memory for context\n");
> +		return -ENOMEM;
> +	}
> 
>  	for (id = 0; ; id = entity->info.id) {
>  		size = (media->entities_count + 1) * sizeof(*media->entities);
> @@ -268,9 +275,9 @@ static int media_enum_entities(struct media_device
> *media)
> 
>  		ret = ioctl(media->fd, MEDIA_IOC_ENUM_ENTITIES, &entity->info);
>  		if (ret < 0) {
> -			if (errno == EINVAL)
> -				break;
> -			return -errno;
> +			if (errno != EINVAL)
> +				ret = -errno;
> +			break;
>  		}
> 
>  		/* Number of links (for outbound links) plus number of pads (for
> @@ -281,8 +288,10 @@ static int media_enum_entities(struct media_device
> *media)
> 
>  		entity->pads = malloc(entity->info.pads * sizeof(*entity->pads));
>  		entity->links = malloc(entity->max_links * sizeof(*entity->links));
> -		if (entity->pads == NULL || entity->links == NULL)
> -			return -ENOMEM;
> +		if (entity->pads == NULL || entity->links == NULL) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> 
>  		media->entities_count++;
> 
> @@ -291,32 +300,21 @@ static int media_enum_entities(struct media_device
> *media) media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
>  			continue;
> 
> -		sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
> -			entity->info.v4l.minor);
> -		ret = readlink(sysname, target, sizeof(target));
> -		if (ret < 0)
> -			continue;
> -
> -		target[ret] = '\0';
> -		p = strrchr(target, '/');
> -		if (p == NULL)
> -			continue;
> -
> -		sprintf(devname, "/dev/%s", p + 1);
> -		ret = stat(devname, &devstat);
> -		if (ret < 0)
> -			continue;
> +		devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
> +		printf("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);
> +			if (p)
> +				snprintf(entity->devname, sizeof(entity->devname),
> +					 "%s", p);
> +		}
> 
> -		/* Sanity check: udev might have reordered the device nodes.
> -		 * Make sure the major/minor match. We should really use
> -		 * libudev.
> -		 */
> -		if (major(devstat.st_rdev) == entity->info.v4l.major &&
> -		    minor(devstat.st_rdev) == entity->info.v4l.minor)
> -			strcpy(entity->devname, devname);
> +		udev_device_unref(device);
>  	}
> 
> -	return 0;
> +	udev_unref(udev);
> +	return ret;
>  }
> 
>  struct media_device *media_open(const char *name, int verbose)

-- 
Regards,

Laurent Pinchart

Re: [media-ctl][PATCH] libmediactl: engage udev to get devname

[Andy Shevchenko]

On Mon, 2011-08-15 at 16:52 +0200, Laurent Pinchart wrote: 
> Hi Andy,
> 
> Thank you for the patch.
> 
> What about making it a configuration option to still support systems that 
> don't provide libudev ? We could keep the current behaviour for those.
Good point.
Will do.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[media-ctl][PATCHv2 1/4] libmediactl: restruct error path

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/media.c b/src/media.c
index e3cab86..050289e 100644
--- a/src/media.c
+++ b/src/media.c
@@ -255,7 +255,7 @@ static int media_enum_entities(struct media_device *media)
 	char target[1024];
 	char *p;
 	__u32 id;
-	int ret;
+	int ret = 0;
 
 	for (id = 0; ; id = entity->info.id) {
 		size = (media->entities_count + 1) * sizeof(*media->entities);
@@ -268,9 +268,9 @@ static int media_enum_entities(struct media_device *media)
 
 		ret = ioctl(media->fd, MEDIA_IOC_ENUM_ENTITIES, &entity->info);
 		if (ret < 0) {
-			if (errno == EINVAL)
-				break;
-			return -errno;
+			if (errno != EINVAL)
+				ret = -errno;
+			break;
 		}
 
 		/* Number of links (for outbound links) plus number of pads (for
@@ -281,8 +281,10 @@ static int media_enum_entities(struct media_device *media)
 
 		entity->pads = malloc(entity->info.pads * sizeof(*entity->pads));
 		entity->links = malloc(entity->max_links * sizeof(*entity->links));
-		if (entity->pads == NULL || entity->links == NULL)
-			return -ENOMEM;
+		if (entity->pads == NULL || entity->links == NULL) {
+			ret = -ENOMEM;
+			break;
+		}
 
 		media->entities_count++;
 
@@ -316,7 +318,7 @@ static int media_enum_entities(struct media_device *media)
 			strcpy(entity->devname, devname);
 	}
 
-	return 0;
+	return ret;
 }
 
 struct media_device *media_open(const char *name, int verbose)
-- 
1.7.5.4

[media-ctl][PATCHv2 2/4] libmediactl: split media_get_devname from media_enum_entities

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |   69 +++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/src/media.c b/src/media.c
index 050289e..fc05a86 100644
--- a/src/media.c
+++ b/src/media.c
@@ -245,15 +245,50 @@ static int media_enum_links(struct media_device *media)
 	return ret;
 }
 
-static int media_enum_entities(struct media_device *media)
+static int media_get_devname(struct media_entity *entity)
 {
-	struct media_entity *entity;
 	struct stat devstat;
-	unsigned int size;
 	char devname[32];
 	char sysname[32];
 	char target[1024];
 	char *p;
+	int ret;
+
+	if (media_entity_type(entity) != MEDIA_ENT_T_DEVNODE &&
+	    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
+		return 0;
+
+	sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
+		entity->info.v4l.minor);
+	ret = readlink(sysname, target, sizeof(target));
+	if (ret < 0)
+		return -errno;
+
+	target[ret] = '\0';
+	p = strrchr(target, '/');
+	if (p == NULL)
+		return -EINVAL;
+
+	sprintf(devname, "/dev/%s", p + 1);
+	ret = stat(devname, &devstat);
+	if (ret < 0)
+		return -errno;
+
+	/* Sanity check: udev might have reordered the device nodes.
+	 * Make sure the major/minor match. We should really use
+	 * libudev.
+	 */
+	if (major(devstat.st_rdev) == entity->info.v4l.major &&
+	    minor(devstat.st_rdev) == entity->info.v4l.minor)
+		strcpy(entity->devname, devname);
+
+	return 0;
+}
+
+static int media_enum_entities(struct media_device *media)
+{
+	struct media_entity *entity;
+	unsigned int size;
 	__u32 id;
 	int ret = 0;
 
@@ -289,33 +324,7 @@ static int media_enum_entities(struct media_device *media)
 		media->entities_count++;
 
 		/* Find the corresponding device name. */
-		if (media_entity_type(entity) != MEDIA_ENT_T_DEVNODE &&
-		    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
-			continue;
-
-		sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
-			entity->info.v4l.minor);
-		ret = readlink(sysname, target, sizeof(target));
-		if (ret < 0)
-			continue;
-
-		target[ret] = '\0';
-		p = strrchr(target, '/');
-		if (p == NULL)
-			continue;
-
-		sprintf(devname, "/dev/%s", p + 1);
-		ret = stat(devname, &devstat);
-		if (ret < 0)
-			continue;
-
-		/* Sanity check: udev might have reordered the device nodes.
-		 * Make sure the major/minor match. We should really use
-		 * libudev.
-		 */
-		if (major(devstat.st_rdev) == entity->info.v4l.major &&
-		    minor(devstat.st_rdev) == entity->info.v4l.minor)
-			strcpy(entity->devname, devname);
+		media_get_devname(entity);
 	}
 
 	return ret;
-- 
1.7.5.4

[media-ctl][PATCHv2 3/4] libmediactl: use udev conditionally to get a devname

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 configure.in    |   22 ++++++++++++++++++++++
 src/Makefile.am |    2 ++
 src/media.c     |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/configure.in b/configure.in
index fd4c70c..45e0663 100644
--- a/configure.in
+++ b/configure.in
@@ -13,6 +13,28 @@ AC_PROG_LIBTOOL
 
 # Checks for libraries.
 
+AC_ARG_WITH([libudev],
+    AS_HELP_STRING([--without-libudev],
+        [Ignore presence of libudev and disable it]))
+
+AS_IF([test "x$with_libudev" != "xno"],
+    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes, have_libudev=no)],
+    [have_libudev=no])
+
+AS_IF([test "x$have_libudev" = "xyes"],
+    [
+        AC_DEFINE([HAVE_LIBUDEV], [], [Use libudev])
+        LIBUDEV_CFLAGS="$lbudev_CFLAGS"
+        LIBUDEV_LIBS="$libudev_LIBS"
+        AC_SUBST(LIBUDEV_CFLAGS)
+        AC_SUBST(LIBUDEV_LIBS)
+    ],
+    [AS_IF([test "x$with_libudev" = "xyes"],
+        [AC_MSG_ERROR([libudev requested but not found])
+    ])
+])
+
+
 # Kernel headers path.
 AC_ARG_WITH(kernel-headers,
     [AC_HELP_STRING([--with-kernel-headers=DIR],
diff --git a/src/Makefile.am b/src/Makefile.am
index 267ea83..52628d2 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -5,6 +5,8 @@ mediactl_includedir=$(includedir)/mediactl
 mediactl_include_HEADERS = media.h subdev.h
 
 bin_PROGRAMS = media-ctl
+media_ctl_CFLAGS = $(LIBUDEV_CFLAGS)
+media_ctl_LDFLAGS = $(LIBUDEV_LIBS)
 media_ctl_SOURCES = main.c options.c options.h tools.h
 media_ctl_LDADD = libmediactl.la libv4l2subdev.la
 
diff --git a/src/media.c b/src/media.c
index fc05a86..e159526 100644
--- a/src/media.c
+++ b/src/media.c
@@ -17,6 +17,8 @@
  * with this program; if not, write to the Free Software Foundation, Inc.,
  */
 
+#include "config.h"
+
 #include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ -31,6 +33,10 @@
 #include <linux/videodev2.h>
 #include <linux/media.h>
 
+#ifdef HAVE_LIBUDEV
+#include <libudev.h>
+#endif	/* HAVE_LIBUDEV */
+
 #include "media.h"
 #include "tools.h"
 
@@ -245,6 +251,37 @@ static int media_enum_links(struct media_device *media)
 	return ret;
 }
 
+#ifdef HAVE_LIBUDEV
+
+static struct udev *udev;
+
+static int media_get_devname(struct media_entity *entity)
+{
+	dev_t devnum;
+	struct udev_device *device;
+	const char *p;
+	int ret = -ENODEV;
+
+	if (media_entity_type(entity) != MEDIA_ENT_T_DEVNODE &&
+	    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
+		return 0;
+
+	devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
+	printf("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);
+		if (p)
+			snprintf(entity->devname, sizeof(entity->devname), "%s", p);
+		ret = 0;
+	}
+
+	udev_device_unref(device);
+	return ret;
+}
+
+#else	/* HAVE_LIBUDEV */
+
 static int media_get_devname(struct media_entity *entity)
 {
 	struct stat devstat;
@@ -284,6 +321,7 @@ static int media_get_devname(struct media_entity *entity)
 
 	return 0;
 }
+#endif	/* HAVE_LIBUDEV */
 
 static int media_enum_entities(struct media_device *media)
 {
@@ -292,6 +330,14 @@ static int media_enum_entities(struct media_device *media)
 	__u32 id;
 	int ret = 0;
 
+#ifdef HAVE_LIBUDEV
+	udev = udev_new();
+	if (udev == NULL) {
+		printf("unable to allocate memory for context\n");
+		return -ENOMEM;
+	}
+#endif	/* HAVE_LIBUDEV */
+
 	for (id = 0; ; id = entity->info.id) {
 		size = (media->entities_count + 1) * sizeof(*media->entities);
 		media->entities = realloc(media->entities, size);
@@ -327,6 +373,10 @@ static int media_enum_entities(struct media_device *media)
 		media_get_devname(entity);
 	}
 
+#ifdef HAVE_LIBUDEV
+	udev_unref(udev);
+	udev = NULL;
+#endif
 	return ret;
 }
 
-- 
1.7.5.4

[media-ctl][PATCHv2 4/4] libmediactl: pass verbose to media_get_devname

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/media.c b/src/media.c
index e159526..e276df5 100644
--- a/src/media.c
+++ b/src/media.c
@@ -255,7 +255,7 @@ static int media_enum_links(struct media_device *media)
 
 static struct udev *udev;
 
-static int media_get_devname(struct media_entity *entity)
+static int media_get_devname(struct media_entity *entity, int verbose)
 {
 	dev_t devnum;
 	struct udev_device *device;
@@ -267,7 +267,8 @@ static int media_get_devname(struct media_entity *entity)
 		return 0;
 
 	devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
-	printf("looking up device: %u:%u\n", major(devnum), minor(devnum));
+	if (verbose)
+		printf("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);
@@ -282,7 +283,7 @@ static int media_get_devname(struct media_entity *entity)
 
 #else	/* HAVE_LIBUDEV */
 
-static int media_get_devname(struct media_entity *entity)
+static int media_get_devname(struct media_entity *entity, int verbose)
 {
 	struct stat devstat;
 	char devname[32];
@@ -323,7 +324,7 @@ static int media_get_devname(struct media_entity *entity)
 }
 #endif	/* HAVE_LIBUDEV */
 
-static int media_enum_entities(struct media_device *media)
+static int media_enum_entities(struct media_device *media, int verbose)
 {
 	struct media_entity *entity;
 	unsigned int size;
@@ -370,7 +371,7 @@ static int media_enum_entities(struct media_device *media)
 		media->entities_count++;
 
 		/* Find the corresponding device name. */
-		media_get_devname(entity);
+		media_get_devname(entity, verbose);
 	}
 
 #ifdef HAVE_LIBUDEV
@@ -404,7 +405,7 @@ struct media_device *media_open(const char *name, int verbose)
 	if (verbose)
 		printf("Enumerating entities\n");
 
-	ret = media_enum_entities(media);
+	ret = media_enum_entities(media, verbose);
 	if (ret < 0) {
 		printf("%s: Unable to enumerate entities for device %s (%s)\n",
 			__func__, name, strerror(-ret));
-- 
1.7.5.4

Re: [media-ctl][PATCHv2 3/4] libmediactl: use udev conditionally to get a devname

[Laurent Pinchart]

Hi Andy,

Thanks for the patch, and sorry for the late reply.

On Tuesday 16 August 2011 12:28:04 Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  configure.in    |   22 ++++++++++++++++++++++
>  src/Makefile.am |    2 ++
>  src/media.c     |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 74 insertions(+), 0 deletions(-)
> 
> diff --git a/configure.in b/configure.in
> index fd4c70c..45e0663 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -13,6 +13,28 @@ AC_PROG_LIBTOOL
> 
>  # Checks for libraries.
> 
> +AC_ARG_WITH([libudev],
> +    AS_HELP_STRING([--without-libudev],
> +        [Ignore presence of libudev and disable it]))
> +
> +AS_IF([test "x$with_libudev" != "xno"],
> +    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes,
> have_libudev=no)], +    [have_libudev=no])
> +
> +AS_IF([test "x$have_libudev" = "xyes"],
> +    [
> +        AC_DEFINE([HAVE_LIBUDEV], [], [Use libudev])
> +        LIBUDEV_CFLAGS="$lbudev_CFLAGS"
> +        LIBUDEV_LIBS="$libudev_LIBS"
> +        AC_SUBST(LIBUDEV_CFLAGS)
> +        AC_SUBST(LIBUDEV_LIBS)
> +    ],
> +    [AS_IF([test "x$with_libudev" = "xyes"],
> +        [AC_MSG_ERROR([libudev requested but not found])
> +    ])
> +])
> +
> +
>  # Kernel headers path.
>  AC_ARG_WITH(kernel-headers,
>      [AC_HELP_STRING([--with-kernel-headers=DIR],
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 267ea83..52628d2 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -5,6 +5,8 @@ mediactl_includedir=$(includedir)/mediactl
>  mediactl_include_HEADERS = media.h subdev.h
> 
>  bin_PROGRAMS = media-ctl
> +media_ctl_CFLAGS = $(LIBUDEV_CFLAGS)
> +media_ctl_LDFLAGS = $(LIBUDEV_LIBS)
>  media_ctl_SOURCES = main.c options.c options.h tools.h
>  media_ctl_LDADD = libmediactl.la libv4l2subdev.la
> 
> diff --git a/src/media.c b/src/media.c
> index fc05a86..e159526 100644
> --- a/src/media.c
> +++ b/src/media.c
> @@ -17,6 +17,8 @@
>   * with this program; if not, write to the Free Software Foundation, Inc.,
>   */
> 
> +#include "config.h"
> +
>  #include <sys/ioctl.h>
>  #include <sys/stat.h>
>  #include <sys/types.h>
> @@ -31,6 +33,10 @@
>  #include <linux/videodev2.h>
>  #include <linux/media.h>
> 
> +#ifdef HAVE_LIBUDEV
> +#include <libudev.h>
> +#endif	/* HAVE_LIBUDEV */
> +
>  #include "media.h"
>  #include "tools.h"
> 
> @@ -245,6 +251,37 @@ static int media_enum_links(struct media_device
> *media) return ret;
>  }
> 
> +#ifdef HAVE_LIBUDEV
> +
> +static struct udev *udev;

I would move this to media_enum_entities() and pass the variable explicitly to 
media_get_devname(). I will make the change unless you want to resubmit the 
patch yourself.

> +
> +static int media_get_devname(struct media_entity *entity)
> +{
> +	dev_t devnum;
> +	struct udev_device *device;
> +	const char *p;
> +	int ret = -ENODEV;
> +
> +	if (media_entity_type(entity) != MEDIA_ENT_T_DEVNODE &&
> +	    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> +		return 0;
> +
> +	devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
> +	printf("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);
> +		if (p)
> +			snprintf(entity->devname, sizeof(entity->devname), "%s", p);
> +		ret = 0;
> +	}
> +
> +	udev_device_unref(device);
> +	return ret;
> +}
> +
> +#else	/* HAVE_LIBUDEV */
> +
>  static int media_get_devname(struct media_entity *entity)
>  {
>  	struct stat devstat;
> @@ -284,6 +321,7 @@ static int media_get_devname(struct media_entity
> *entity)
> 
>  	return 0;
>  }
> +#endif	/* HAVE_LIBUDEV */
> 
>  static int media_enum_entities(struct media_device *media)
>  {
> @@ -292,6 +330,14 @@ static int media_enum_entities(struct media_device
> *media) __u32 id;
>  	int ret = 0;
> 
> +#ifdef HAVE_LIBUDEV
> +	udev = udev_new();
> +	if (udev == NULL) {
> +		printf("unable to allocate memory for context\n");
> +		return -ENOMEM;
> +	}
> +#endif	/* HAVE_LIBUDEV */
> +

If the code is compiled with libudev support enabled, but the target system 
has no udev, runtime failures will be experienced. Do you think it would be 
useful to fall back to the sysfs-based approach in that case ? Do you know 
which udev-related call would then fail ?

>  	for (id = 0; ; id = entity->info.id) {
>  		size = (media->entities_count + 1) * sizeof(*media->entities);
>  		media->entities = realloc(media->entities, size);
> @@ -327,6 +373,10 @@ static int media_enum_entities(struct media_device
> *media) media_get_devname(entity);
>  	}
> 
> +#ifdef HAVE_LIBUDEV
> +	udev_unref(udev);
> +	udev = NULL;
> +#endif
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart

Re: [media-ctl][PATCHv2 3/4] libmediactl: use udev conditionally to get a devname

[Laurent Pinchart]

Hi Andy,

On Tuesday 16 August 2011 12:28:04 Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  configure.in    |   22 ++++++++++++++++++++++
>  src/Makefile.am |    2 ++
>  src/media.c     |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 74 insertions(+), 0 deletions(-)
> 
> diff --git a/configure.in b/configure.in
> index fd4c70c..45e0663 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -13,6 +13,28 @@ AC_PROG_LIBTOOL
> 
>  # Checks for libraries.
> 
> +AC_ARG_WITH([libudev],
> +    AS_HELP_STRING([--without-libudev],
> +        [Ignore presence of libudev and disable it]))
> +
> +AS_IF([test "x$with_libudev" != "xno"],
> +    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes,
> have_libudev=no)],
> +    [have_libudev=no])

I don't think this works when cross-compiling.

> +
> +AS_IF([test "x$have_libudev" = "xyes"],
> +    [
> +        AC_DEFINE([HAVE_LIBUDEV], [], [Use libudev])
> +        LIBUDEV_CFLAGS="$lbudev_CFLAGS"
> +        LIBUDEV_LIBS="$libudev_LIBS"
> +        AC_SUBST(LIBUDEV_CFLAGS)
> +        AC_SUBST(LIBUDEV_LIBS)
> +    ],
> +    [AS_IF([test "x$with_libudev" = "xyes"],
> +        [AC_MSG_ERROR([libudev requested but not found])
> +    ])
> +])
> +
> +
>  # Kernel headers path.
>  AC_ARG_WITH(kernel-headers,
>      [AC_HELP_STRING([--with-kernel-headers=DIR],

-- 
Regards,

Laurent Pinchart

Re: [media-ctl][PATCH] libmediactl: engage udev to get devname

[L. Hanisch]

Hi,

Am 15.08.2011 14:20, schrieb Andy Shevchenko:
> Signed-off-by: Andy Shevchenko<andriy.shevchenko@linux.intel.com>
> ---
>   configure.in    |   10 ++++++++
>   src/Makefile.am |    2 +
>   src/media.c     |   66 ++++++++++++++++++++++++++----------------------------
>   3 files changed, 44 insertions(+), 34 deletions(-)
>
> diff --git a/configure.in b/configure.in
> index fd4c70c..63432ba 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -12,6 +12,16 @@ AC_PROG_CC
>   AC_PROG_LIBTOOL
>
>   # Checks for libraries.
> +PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes, have_libudev=no)
> +
> +if test x$have_libudev = xyes; then
> +    LIBUDEV_CFLAGS="$lbudev_CFLAGS"

  Just reading it, shouldn't it be
  LIBUDEV_CFLAGS="$libudev_CFLAGS"

Regards,
Lars.

> +    LIBUDEV_LIBS="$libudev_LIBS"
> +    AC_SUBST(LIBUDEV_CFLAGS)
> +    AC_SUBST(LIBUDEV_LIBS)
> +else
> +    AC_MSG_ERROR([libudev is required])
> +fi
>
>   # Kernel headers path.
>   AC_ARG_WITH(kernel-headers,
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 267ea83..52628d2 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -5,6 +5,8 @@ mediactl_includedir=$(includedir)/mediactl
>   mediactl_include_HEADERS = media.h subdev.h
>
>   bin_PROGRAMS = media-ctl
> +media_ctl_CFLAGS = $(LIBUDEV_CFLAGS)
> +media_ctl_LDFLAGS = $(LIBUDEV_LIBS)
>   media_ctl_SOURCES = main.c options.c options.h tools.h
>   media_ctl_LDADD = libmediactl.la libv4l2subdev.la
>
> diff --git a/src/media.c b/src/media.c
> index e3cab86..000d750 100644
> --- a/src/media.c
> +++ b/src/media.c
> @@ -31,6 +31,8 @@
>   #include<linux/videodev2.h>
>   #include<linux/media.h>
>
> +#include<libudev.h>
> +
>   #include "media.h"
>   #include "tools.h"
>
> @@ -247,15 +249,20 @@ static int media_enum_links(struct media_device *media)
>
>   static int media_enum_entities(struct media_device *media)
>   {
> +	struct udev *udev;
> +	dev_t devnum;
> +	struct udev_device *device;
>   	struct media_entity *entity;
> -	struct stat devstat;
>   	unsigned int size;
> -	char devname[32];
> -	char sysname[32];
> -	char target[1024];
> -	char *p;
> +	const char *p;
>   	__u32 id;
> -	int ret;
> +	int ret = 0;
> +
> +	udev = udev_new();
> +	if (udev == NULL) {
> +		printf("unable to allocate memory for context\n");
> +		return -ENOMEM;
> +	}
>
>   	for (id = 0; ; id = entity->info.id) {
>   		size = (media->entities_count + 1) * sizeof(*media->entities);
> @@ -268,9 +275,9 @@ static int media_enum_entities(struct media_device *media)
>
>   		ret = ioctl(media->fd, MEDIA_IOC_ENUM_ENTITIES,&entity->info);
>   		if (ret<  0) {
> -			if (errno == EINVAL)
> -				break;
> -			return -errno;
> +			if (errno != EINVAL)
> +				ret = -errno;
> +			break;
>   		}
>
>   		/* Number of links (for outbound links) plus number of pads (for
> @@ -281,8 +288,10 @@ static int media_enum_entities(struct media_device *media)
>
>   		entity->pads = malloc(entity->info.pads * sizeof(*entity->pads));
>   		entity->links = malloc(entity->max_links * sizeof(*entity->links));
> -		if (entity->pads == NULL || entity->links == NULL)
> -			return -ENOMEM;
> +		if (entity->pads == NULL || entity->links == NULL) {
> +			ret = -ENOMEM;
> +			break;
> +		}
>
>   		media->entities_count++;
>
> @@ -291,32 +300,21 @@ static int media_enum_entities(struct media_device *media)
>   		    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
>   			continue;
>
> -		sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
> -			entity->info.v4l.minor);
> -		ret = readlink(sysname, target, sizeof(target));
> -		if (ret<  0)
> -			continue;
> -
> -		target[ret] = '\0';
> -		p = strrchr(target, '/');
> -		if (p == NULL)
> -			continue;
> -
> -		sprintf(devname, "/dev/%s", p + 1);
> -		ret = stat(devname,&devstat);
> -		if (ret<  0)
> -			continue;
> +		devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
> +		printf("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);
> +			if (p)
> +				snprintf(entity->devname, sizeof(entity->devname),
> +					 "%s", p);
> +		}
>
> -		/* Sanity check: udev might have reordered the device nodes.
> -		 * Make sure the major/minor match. We should really use
> -		 * libudev.
> -		 */
> -		if (major(devstat.st_rdev) == entity->info.v4l.major&&
> -		    minor(devstat.st_rdev) == entity->info.v4l.minor)
> -			strcpy(entity->devname, devname);
> +		udev_device_unref(device);
>   	}
>
> -	return 0;
> +	udev_unref(udev);
> +	return ret;
>   }
>
>   struct media_device *media_open(const char *name, int verbose)

[media-ctl][PATCHv3 1/3] libmediactl: restruct error path

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/media.c b/src/media.c
index e3cab86..050289e 100644
--- a/src/media.c
+++ b/src/media.c
@@ -255,7 +255,7 @@ static int media_enum_entities(struct media_device *media)
 	char target[1024];
 	char *p;
 	__u32 id;
-	int ret;
+	int ret = 0;
 
 	for (id = 0; ; id = entity->info.id) {
 		size = (media->entities_count + 1) * sizeof(*media->entities);
@@ -268,9 +268,9 @@ static int media_enum_entities(struct media_device *media)
 
 		ret = ioctl(media->fd, MEDIA_IOC_ENUM_ENTITIES, &entity->info);
 		if (ret < 0) {
-			if (errno == EINVAL)
-				break;
-			return -errno;
+			if (errno != EINVAL)
+				ret = -errno;
+			break;
 		}
 
 		/* Number of links (for outbound links) plus number of pads (for
@@ -281,8 +281,10 @@ static int media_enum_entities(struct media_device *media)
 
 		entity->pads = malloc(entity->info.pads * sizeof(*entity->pads));
 		entity->links = malloc(entity->max_links * sizeof(*entity->links));
-		if (entity->pads == NULL || entity->links == NULL)
-			return -ENOMEM;
+		if (entity->pads == NULL || entity->links == NULL) {
+			ret = -ENOMEM;
+			break;
+		}
 
 		media->entities_count++;
 
@@ -316,7 +318,7 @@ static int media_enum_entities(struct media_device *media)
 			strcpy(entity->devname, devname);
 	}
 
-	return 0;
+	return ret;
 }
 
 struct media_device *media_open(const char *name, int verbose)
-- 
1.7.5.4

[media-ctl][PATCHv3 2/3] libmediactl: split media_get_devname_sysfs from media_enum_entities

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |   61 +++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/src/media.c b/src/media.c
index 050289e..5d3ff7c 100644
--- a/src/media.c
+++ b/src/media.c
@@ -245,15 +245,46 @@ static int media_enum_links(struct media_device *media)
 	return ret;
 }
 
-static int media_enum_entities(struct media_device *media)
+static int media_get_devname_sysfs(struct media_entity *entity)
 {
-	struct media_entity *entity;
 	struct stat devstat;
-	unsigned int size;
 	char devname[32];
 	char sysname[32];
 	char target[1024];
 	char *p;
+	int ret;
+
+	sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
+		entity->info.v4l.minor);
+	ret = readlink(sysname, target, sizeof(target));
+	if (ret < 0)
+		return -errno;
+
+	target[ret] = '\0';
+	p = strrchr(target, '/');
+	if (p == NULL)
+		return -EINVAL;
+
+	sprintf(devname, "/dev/%s", p + 1);
+	ret = stat(devname, &devstat);
+	if (ret < 0)
+		return -errno;
+
+	/* Sanity check: udev might have reordered the device nodes.
+	 * Make sure the major/minor match. We should really use
+	 * libudev.
+	 */
+	if (major(devstat.st_rdev) == entity->info.v4l.major &&
+	    minor(devstat.st_rdev) == entity->info.v4l.minor)
+		strcpy(entity->devname, devname);
+
+	return 0;
+}
+
+static int media_enum_entities(struct media_device *media)
+{
+	struct media_entity *entity;
+	unsigned int size;
 	__u32 id;
 	int ret = 0;
 
@@ -293,29 +324,7 @@ static int media_enum_entities(struct media_device *media)
 		    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
 			continue;
 
-		sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
-			entity->info.v4l.minor);
-		ret = readlink(sysname, target, sizeof(target));
-		if (ret < 0)
-			continue;
-
-		target[ret] = '\0';
-		p = strrchr(target, '/');
-		if (p == NULL)
-			continue;
-
-		sprintf(devname, "/dev/%s", p + 1);
-		ret = stat(devname, &devstat);
-		if (ret < 0)
-			continue;
-
-		/* Sanity check: udev might have reordered the device nodes.
-		 * Make sure the major/minor match. We should really use
-		 * libudev.
-		 */
-		if (major(devstat.st_rdev) == entity->info.v4l.major &&
-		    minor(devstat.st_rdev) == entity->info.v4l.minor)
-			strcpy(entity->devname, devname);
+		media_get_devname_sysfs(entity);
 	}
 
 	return ret;
-- 
1.7.5.4

[media-ctl][PATCHv3 3/3] libmediactl: get the device name via udev

[Andy Shevchenko]

If configured with --with-libudev, the libmediactl is built with libudev
support. It allows to get the device name in right way in the modern linux
systems.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 configure.in    |   22 ++++++++++++++++
 src/Makefile.am |    2 +
 src/media.c     |   73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/media.h     |    1 +
 4 files changed, 98 insertions(+), 0 deletions(-)

diff --git a/configure.in b/configure.in
index fd4c70c..2f31831 100644
--- a/configure.in
+++ b/configure.in
@@ -13,6 +13,28 @@ AC_PROG_LIBTOOL
 
 # Checks for libraries.
 
+AC_ARG_WITH([libudev],
+    AS_HELP_STRING([--without-libudev],
+        [Ignore presence of libudev and disable it]))
+
+AS_IF([test "x$with_libudev" != "xno"],
+    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes, have_libudev=no)],
+    [have_libudev=no])
+
+AS_IF([test "x$have_libudev" = "xyes"],
+    [
+        AC_DEFINE([HAVE_LIBUDEV], [], [Use libudev])
+        LIBUDEV_CFLAGS="$libudev_CFLAGS"
+        LIBUDEV_LIBS="$libudev_LIBS"
+        AC_SUBST(LIBUDEV_CFLAGS)
+        AC_SUBST(LIBUDEV_LIBS)
+    ],
+    [AS_IF([test "x$with_libudev" = "xyes"],
+        [AC_MSG_ERROR([libudev requested but not found])
+    ])
+])
+
+
 # Kernel headers path.
 AC_ARG_WITH(kernel-headers,
     [AC_HELP_STRING([--with-kernel-headers=DIR],
diff --git a/src/Makefile.am b/src/Makefile.am
index 267ea83..52628d2 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -5,6 +5,8 @@ mediactl_includedir=$(includedir)/mediactl
 mediactl_include_HEADERS = media.h subdev.h
 
 bin_PROGRAMS = media-ctl
+media_ctl_CFLAGS = $(LIBUDEV_CFLAGS)
+media_ctl_LDFLAGS = $(LIBUDEV_LIBS)
 media_ctl_SOURCES = main.c options.c options.h tools.h
 media_ctl_LDADD = libmediactl.la libv4l2subdev.la
 
diff --git a/src/media.c b/src/media.c
index 5d3ff7c..dae649a 100644
--- a/src/media.c
+++ b/src/media.c
@@ -17,6 +17,8 @@
  * with this program; if not, write to the Free Software Foundation, Inc.,
  */
 
+#include "config.h"
+
 #include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ -245,6 +247,64 @@ static int media_enum_links(struct media_device *media)
 	return ret;
 }
 
+#ifdef HAVE_LIBUDEV
+
+#include <libudev.h>
+
+static inline int media_udev_open(struct media_device *media)
+{
+	media->priv = udev_new();
+	if (media->priv == NULL)
+		return -ENOMEM;
+	return 0;
+}
+
+static inline void media_udev_close(struct media_device *media)
+{
+	udev_unref(media->priv);
+}
+
+static int media_get_devname_udev(struct media_device *media,
+		struct media_entity *entity)
+{
+	int ret = -ENODEV;
+	struct udev *udev = media->priv;
+	dev_t devnum;
+	struct udev_device *device;
+	const char *p;
+
+	if (udev == NULL)
+		return -EINVAL;
+
+	devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
+	printf("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);
+		if (p)
+			snprintf(entity->devname, sizeof(entity->devname), "%s", p);
+		ret = 0;
+	}
+
+	udev_device_unref(device);
+
+	return ret;
+}
+
+#else	/* HAVE_LIBUDEV */
+
+static inline int media_udev_open(struct media_device *media) { return 0; }
+
+static inline void media_udev_close(struct media_device *media) { }
+
+static inline int media_get_devname_udev(struct media_device *media,
+		struct media_entity *entity)
+{
+	return -ENOTSUP;
+}
+
+#endif	/* HAVE_LIBUDEV */
+
 static int media_get_devname_sysfs(struct media_entity *entity)
 {
 	struct stat devstat;
@@ -324,6 +384,11 @@ static int media_enum_entities(struct media_device *media)
 		    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
 			continue;
 
+		/* Try to get the device name via udev */
+		if (!media_get_devname_udev(media, entity))
+			continue;
+
+		/* Fall back to get the device name via sysfs */
 		media_get_devname_sysfs(entity);
 	}
 
@@ -351,6 +416,13 @@ struct media_device *media_open(const char *name, int verbose)
 		return NULL;
 	}
 
+	ret = media_udev_open(media);
+	if (ret < 0) {
+		printf("%s: Can't get udev context\n", __func__);
+		media_close(media);
+		return NULL;
+	}
+
 	if (verbose)
 		printf("Enumerating entities\n");
 
@@ -395,6 +467,7 @@ void media_close(struct media_device *media)
 	}
 
 	free(media->entities);
+	media_udev_close(media);
 	free(media);
 }
 
diff --git a/src/media.h b/src/media.h
index b91a2ac..4201451 100644
--- a/src/media.h
+++ b/src/media.h
@@ -54,6 +54,7 @@ struct media_device {
 	struct media_entity *entities;
 	unsigned int entities_count;
 	__u32 padding[6];
+	void *priv;
 };
 
 /**
-- 
1.7.5.4

Re: [media-ctl][PATCHv2 3/4] libmediactl: use udev conditionally to get a devname

[Andy Shevchenko]

On Tue, 2011-08-30 at 21:14 +0200, Laurent Pinchart wrote: 
> > +AC_ARG_WITH([libudev],
> > +    AS_HELP_STRING([--without-libudev],
> > +        [Ignore presence of libudev and disable it]))
> > +
> > +AS_IF([test "x$with_libudev" != "xno"],
> > +    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes,
> > have_libudev=no)],
> > +    [have_libudev=no])
> 
> I don't think this works when cross-compiling.
Do you mean pkg-config call?
Its manual tells us about PKG_CONFIG_SYSROOT_DIR which might be helpful.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [media-ctl][PATCHv2 3/4] libmediactl: use udev conditionally to get a devname

[Laurent Pinchart]

Hi Andy,

On Friday 02 September 2011 10:42:06 Andy Shevchenko wrote:
> On Tue, 2011-08-30 at 21:14 +0200, Laurent Pinchart wrote:
> > > +AC_ARG_WITH([libudev],
> > > +    AS_HELP_STRING([--without-libudev],
> > > +        [Ignore presence of libudev and disable it]))
> > > +
> > > +AS_IF([test "x$with_libudev" != "xno"],
> > > +    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes,
> > > have_libudev=no)],
> > > +    [have_libudev=no])
> > 
> > I don't think this works when cross-compiling.
> 
> Do you mean pkg-config call?
> Its manual tells us about PKG_CONFIG_SYSROOT_DIR which might be helpful.

If I don't set that, pkg-config seems to pick libudev from the host and 
consider that libudev is available. Compilation then fails.

As most users cross-compile libmediactl, I would like to avoid this situation. 
Requiring the user to set PKG_CONFIG_SYSROOT_DIR to habe libudev support is 
fine, but I would like the build to succeed out of the box when cross-
compiling without libudev support. Is that possible ?

-- 
Regards,

Laurent Pinchart

Re: [media-ctl][PATCHv2 3/4] libmediactl: use udev conditionally to get a devname

[Andy Shevchenko]

On Fri, 2011-09-02 at 13:17 +0200, Laurent Pinchart wrote: 
> Hi Andy,
> 
> On Friday 02 September 2011 10:42:06 Andy Shevchenko wrote:
> > On Tue, 2011-08-30 at 21:14 +0200, Laurent Pinchart wrote:
> > > > +AC_ARG_WITH([libudev],
> > > > +    AS_HELP_STRING([--without-libudev],
> > > > +        [Ignore presence of libudev and disable it]))
> > > > +
> > > > +AS_IF([test "x$with_libudev" != "xno"],
> > > > +    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes,
> > > > have_libudev=no)],
> > > > +    [have_libudev=no])
> > > 
> > > I don't think this works when cross-compiling.
> > 
> > Do you mean pkg-config call?
> > Its manual tells us about PKG_CONFIG_SYSROOT_DIR which might be helpful.
> 
> If I don't set that, pkg-config seems to pick libudev from the host and 
> consider that libudev is available. Compilation then fails.
> 
> As most users cross-compile libmediactl, I would like to avoid this situation. 
> Requiring the user to set PKG_CONFIG_SYSROOT_DIR to habe libudev support is 
> fine, but I would like the build to succeed out of the box when cross-
> compiling without libudev support. Is that possible ?
with my patch you run ./configure --without-libudev
Would you like opposite: like ./configure (means w/o
libudev), ./configure --with-libudev otherwise?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [media-ctl][PATCHv2 3/4] libmediactl: use udev conditionally to get a devname

[Laurent Pinchart]

Hi Andy,

On Friday 02 September 2011 13:21:43 Andy Shevchenko wrote:
> On Fri, 2011-09-02 at 13:17 +0200, Laurent Pinchart wrote:
> > On Friday 02 September 2011 10:42:06 Andy Shevchenko wrote:
> > > On Tue, 2011-08-30 at 21:14 +0200, Laurent Pinchart wrote:
> > > > > +AC_ARG_WITH([libudev],
> > > > > +    AS_HELP_STRING([--without-libudev],
> > > > > +        [Ignore presence of libudev and disable it]))
> > > > > +
> > > > > +AS_IF([test "x$with_libudev" != "xno"],
> > > > > +    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes,
> > > > > have_libudev=no)],
> > > > > +    [have_libudev=no])
> > > > 
> > > > I don't think this works when cross-compiling.
> > > 
> > > Do you mean pkg-config call?
> > > Its manual tells us about PKG_CONFIG_SYSROOT_DIR which might be
> > > helpful.
> > 
> > If I don't set that, pkg-config seems to pick libudev from the host and
> > consider that libudev is available. Compilation then fails.
> > 
> > As most users cross-compile libmediactl, I would like to avoid this
> > situation. Requiring the user to set PKG_CONFIG_SYSROOT_DIR to habe
> > libudev support is fine, but I would like the build to succeed out of
> > the box when cross- compiling without libudev support. Is that possible
> > ?
> 
> with my patch you run ./configure --without-libudev
> Would you like opposite: like ./configure (means w/o
> libudev), ./configure --with-libudev otherwise?

I think that would be better, otherwise I will receive lots of support request 
from people who try to cross-compile libmediactl and suddenly get a failure.

-- 
Regards,

Laurent Pinchart

[media-ctl][PATCHv4 1/3] libmediactl: restruct error path

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/media.c b/src/media.c
index e3cab86..050289e 100644
--- a/src/media.c
+++ b/src/media.c
@@ -255,7 +255,7 @@ static int media_enum_entities(struct media_device *media)
 	char target[1024];
 	char *p;
 	__u32 id;
-	int ret;
+	int ret = 0;
 
 	for (id = 0; ; id = entity->info.id) {
 		size = (media->entities_count + 1) * sizeof(*media->entities);
@@ -268,9 +268,9 @@ static int media_enum_entities(struct media_device *media)
 
 		ret = ioctl(media->fd, MEDIA_IOC_ENUM_ENTITIES, &entity->info);
 		if (ret < 0) {
-			if (errno == EINVAL)
-				break;
-			return -errno;
+			if (errno != EINVAL)
+				ret = -errno;
+			break;
 		}
 
 		/* Number of links (for outbound links) plus number of pads (for
@@ -281,8 +281,10 @@ static int media_enum_entities(struct media_device *media)
 
 		entity->pads = malloc(entity->info.pads * sizeof(*entity->pads));
 		entity->links = malloc(entity->max_links * sizeof(*entity->links));
-		if (entity->pads == NULL || entity->links == NULL)
-			return -ENOMEM;
+		if (entity->pads == NULL || entity->links == NULL) {
+			ret = -ENOMEM;
+			break;
+		}
 
 		media->entities_count++;
 
@@ -316,7 +318,7 @@ static int media_enum_entities(struct media_device *media)
 			strcpy(entity->devname, devname);
 	}
 
-	return 0;
+	return ret;
 }
 
 struct media_device *media_open(const char *name, int verbose)
-- 
1.7.5.4

[media-ctl][PATCHv4 2/3] libmediactl: split media_get_devname_sysfs from media_enum_entities

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |   61 +++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/src/media.c b/src/media.c
index 050289e..5d3ff7c 100644
--- a/src/media.c
+++ b/src/media.c
@@ -245,15 +245,46 @@ static int media_enum_links(struct media_device *media)
 	return ret;
 }
 
-static int media_enum_entities(struct media_device *media)
+static int media_get_devname_sysfs(struct media_entity *entity)
 {
-	struct media_entity *entity;
 	struct stat devstat;
-	unsigned int size;
 	char devname[32];
 	char sysname[32];
 	char target[1024];
 	char *p;
+	int ret;
+
+	sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
+		entity->info.v4l.minor);
+	ret = readlink(sysname, target, sizeof(target));
+	if (ret < 0)
+		return -errno;
+
+	target[ret] = '\0';
+	p = strrchr(target, '/');
+	if (p == NULL)
+		return -EINVAL;
+
+	sprintf(devname, "/dev/%s", p + 1);
+	ret = stat(devname, &devstat);
+	if (ret < 0)
+		return -errno;
+
+	/* Sanity check: udev might have reordered the device nodes.
+	 * Make sure the major/minor match. We should really use
+	 * libudev.
+	 */
+	if (major(devstat.st_rdev) == entity->info.v4l.major &&
+	    minor(devstat.st_rdev) == entity->info.v4l.minor)
+		strcpy(entity->devname, devname);
+
+	return 0;
+}
+
+static int media_enum_entities(struct media_device *media)
+{
+	struct media_entity *entity;
+	unsigned int size;
 	__u32 id;
 	int ret = 0;
 
@@ -293,29 +324,7 @@ static int media_enum_entities(struct media_device *media)
 		    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
 			continue;
 
-		sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
-			entity->info.v4l.minor);
-		ret = readlink(sysname, target, sizeof(target));
-		if (ret < 0)
-			continue;
-
-		target[ret] = '\0';
-		p = strrchr(target, '/');
-		if (p == NULL)
-			continue;
-
-		sprintf(devname, "/dev/%s", p + 1);
-		ret = stat(devname, &devstat);
-		if (ret < 0)
-			continue;
-
-		/* Sanity check: udev might have reordered the device nodes.
-		 * Make sure the major/minor match. We should really use
-		 * libudev.
-		 */
-		if (major(devstat.st_rdev) == entity->info.v4l.major &&
-		    minor(devstat.st_rdev) == entity->info.v4l.minor)
-			strcpy(entity->devname, devname);
+		media_get_devname_sysfs(entity);
 	}
 
 	return ret;
-- 
1.7.5.4

[media-ctl][PATCHv4 3/3] libmediactl: get the device name via udev

[Andy Shevchenko]

If configured with --with-libudev, the libmediactl is built with libudev
support. It allows to get the device name in right way in the modern linux
systems.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 configure.in    |   22 ++++++++++++++++
 src/Makefile.am |    2 +
 src/media.c     |   73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/media.h     |    1 +
 4 files changed, 98 insertions(+), 0 deletions(-)

diff --git a/configure.in b/configure.in
index fd4c70c..983023e 100644
--- a/configure.in
+++ b/configure.in
@@ -13,6 +13,28 @@ AC_PROG_LIBTOOL
 
 # Checks for libraries.
 
+AC_ARG_WITH([libudev],
+    AS_HELP_STRING([--with-libudev],
+        [Enable libudev to detect a device name]))
+
+AS_IF([test "x$with_libudev" = "xyes"],
+    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes, have_libudev=no)],
+    [have_libudev=no])
+
+AS_IF([test "x$have_libudev" = "xyes"],
+    [
+        AC_DEFINE([HAVE_LIBUDEV], [], [Use libudev])
+        LIBUDEV_CFLAGS="$libudev_CFLAGS"
+        LIBUDEV_LIBS="$libudev_LIBS"
+        AC_SUBST(LIBUDEV_CFLAGS)
+        AC_SUBST(LIBUDEV_LIBS)
+    ],
+    [AS_IF([test "x$with_libudev" = "xyes"],
+        [AC_MSG_ERROR([libudev requested but not found])
+    ])
+])
+
+
 # Kernel headers path.
 AC_ARG_WITH(kernel-headers,
     [AC_HELP_STRING([--with-kernel-headers=DIR],
diff --git a/src/Makefile.am b/src/Makefile.am
index 267ea83..52628d2 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -5,6 +5,8 @@ mediactl_includedir=$(includedir)/mediactl
 mediactl_include_HEADERS = media.h subdev.h
 
 bin_PROGRAMS = media-ctl
+media_ctl_CFLAGS = $(LIBUDEV_CFLAGS)
+media_ctl_LDFLAGS = $(LIBUDEV_LIBS)
 media_ctl_SOURCES = main.c options.c options.h tools.h
 media_ctl_LDADD = libmediactl.la libv4l2subdev.la
 
diff --git a/src/media.c b/src/media.c
index 5d3ff7c..dae649a 100644
--- a/src/media.c
+++ b/src/media.c
@@ -17,6 +17,8 @@
  * with this program; if not, write to the Free Software Foundation, Inc.,
  */
 
+#include "config.h"
+
 #include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ -245,6 +247,64 @@ static int media_enum_links(struct media_device *media)
 	return ret;
 }
 
+#ifdef HAVE_LIBUDEV
+
+#include <libudev.h>
+
+static inline int media_udev_open(struct media_device *media)
+{
+	media->priv = udev_new();
+	if (media->priv == NULL)
+		return -ENOMEM;
+	return 0;
+}
+
+static inline void media_udev_close(struct media_device *media)
+{
+	udev_unref(media->priv);
+}
+
+static int media_get_devname_udev(struct media_device *media,
+		struct media_entity *entity)
+{
+	int ret = -ENODEV;
+	struct udev *udev = media->priv;
+	dev_t devnum;
+	struct udev_device *device;
+	const char *p;
+
+	if (udev == NULL)
+		return -EINVAL;
+
+	devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
+	printf("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);
+		if (p)
+			snprintf(entity->devname, sizeof(entity->devname), "%s", p);
+		ret = 0;
+	}
+
+	udev_device_unref(device);
+
+	return ret;
+}
+
+#else	/* HAVE_LIBUDEV */
+
+static inline int media_udev_open(struct media_device *media) { return 0; }
+
+static inline void media_udev_close(struct media_device *media) { }
+
+static inline int media_get_devname_udev(struct media_device *media,
+		struct media_entity *entity)
+{
+	return -ENOTSUP;
+}
+
+#endif	/* HAVE_LIBUDEV */
+
 static int media_get_devname_sysfs(struct media_entity *entity)
 {
 	struct stat devstat;
@@ -324,6 +384,11 @@ static int media_enum_entities(struct media_device *media)
 		    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
 			continue;
 
+		/* Try to get the device name via udev */
+		if (!media_get_devname_udev(media, entity))
+			continue;
+
+		/* Fall back to get the device name via sysfs */
 		media_get_devname_sysfs(entity);
 	}
 
@@ -351,6 +416,13 @@ struct media_device *media_open(const char *name, int verbose)
 		return NULL;
 	}
 
+	ret = media_udev_open(media);
+	if (ret < 0) {
+		printf("%s: Can't get udev context\n", __func__);
+		media_close(media);
+		return NULL;
+	}
+
 	if (verbose)
 		printf("Enumerating entities\n");
 
@@ -395,6 +467,7 @@ void media_close(struct media_device *media)
 	}
 
 	free(media->entities);
+	media_udev_close(media);
 	free(media);
 }
 
diff --git a/src/media.h b/src/media.h
index b91a2ac..4201451 100644
--- a/src/media.h
+++ b/src/media.h
@@ -54,6 +54,7 @@ struct media_device {
 	struct media_entity *entities;
 	unsigned int entities_count;
 	__u32 padding[6];
+	void *priv;
 };
 
 /**
-- 
1.7.5.4

Re: [media-ctl][PATCHv4 3/3] libmediactl: get the device name via udev

[Laurent Pinchart]

Hi Andy,

On Friday 02 September 2011 15:09:28 Andy Shevchenko wrote:
> If configured with --with-libudev, the libmediactl is built with libudev
> support. It allows to get the device name in right way in the modern linux
> systems.

Thanks for the patch. We're nearly there :-)

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  configure.in    |   22 ++++++++++++++++
>  src/Makefile.am |    2 +
>  src/media.c     |   73
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/media.h     | 
>   1 +
>  4 files changed, 98 insertions(+), 0 deletions(-)
> 
> diff --git a/configure.in b/configure.in
> index fd4c70c..983023e 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -13,6 +13,28 @@ AC_PROG_LIBTOOL
> 
>  # Checks for libraries.
> 
> +AC_ARG_WITH([libudev],
> +    AS_HELP_STRING([--with-libudev],
> +        [Enable libudev to detect a device name]))
> +
> +AS_IF([test "x$with_libudev" = "xyes"],
> +    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes,
> have_libudev=no)], +    [have_libudev=no])
> +
> +AS_IF([test "x$have_libudev" = "xyes"],
> +    [
> +        AC_DEFINE([HAVE_LIBUDEV], [], [Use libudev])
> +        LIBUDEV_CFLAGS="$libudev_CFLAGS"
> +        LIBUDEV_LIBS="$libudev_LIBS"
> +        AC_SUBST(LIBUDEV_CFLAGS)
> +        AC_SUBST(LIBUDEV_LIBS)
> +    ],
> +    [AS_IF([test "x$with_libudev" = "xyes"],
> +        [AC_MSG_ERROR([libudev requested but not found])
> +    ])
> +])
> +
> +
>  # Kernel headers path.
>  AC_ARG_WITH(kernel-headers,
>      [AC_HELP_STRING([--with-kernel-headers=DIR],
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 267ea83..52628d2 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -5,6 +5,8 @@ mediactl_includedir=$(includedir)/mediactl
>  mediactl_include_HEADERS = media.h subdev.h
> 
>  bin_PROGRAMS = media-ctl
> +media_ctl_CFLAGS = $(LIBUDEV_CFLAGS)
> +media_ctl_LDFLAGS = $(LIBUDEV_LIBS)
>  media_ctl_SOURCES = main.c options.c options.h tools.h
>  media_ctl_LDADD = libmediactl.la libv4l2subdev.la
> 
> diff --git a/src/media.c b/src/media.c
> index 5d3ff7c..dae649a 100644
> --- a/src/media.c
> +++ b/src/media.c
> @@ -17,6 +17,8 @@
>   * with this program; if not, write to the Free Software Foundation, Inc.,
>   */
> 
> +#include "config.h"
> +
>  #include <sys/ioctl.h>
>  #include <sys/stat.h>
>  #include <sys/types.h>
> @@ -245,6 +247,64 @@ static int media_enum_links(struct media_device
> *media) return ret;
>  }
> 
> +#ifdef HAVE_LIBUDEV
> +
> +#include <libudev.h>
> +
> +static inline int media_udev_open(struct media_device *media)
> +{
> +	media->priv = udev_new();
> +	if (media->priv == NULL)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +static inline void media_udev_close(struct media_device *media)
> +{
> +	udev_unref(media->priv);
> +}
> +
> +static int media_get_devname_udev(struct media_device *media,
> +		struct media_entity *entity)
> +{
> +	int ret = -ENODEV;
> +	struct udev *udev = media->priv;
> +	dev_t devnum;
> +	struct udev_device *device;
> +	const char *p;
> +
> +	if (udev == NULL)
> +		return -EINVAL;
> +
> +	devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
> +	printf("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);
> +		if (p)
> +			snprintf(entity->devname, sizeof(entity->devname), "%s", p);
> +		ret = 0;
> +	}
> +
> +	udev_device_unref(device);
> +
> +	return ret;
> +}
> +
> +#else	/* HAVE_LIBUDEV */
> +
> +static inline int media_udev_open(struct media_device *media) { return 0;
> } +
> +static inline void media_udev_close(struct media_device *media) { }
> +
> +static inline int media_get_devname_udev(struct media_device *media,
> +		struct media_entity *entity)
> +{
> +	return -ENOTSUP;
> +}
> +
> +#endif	/* HAVE_LIBUDEV */
> +
>  static int media_get_devname_sysfs(struct media_entity *entity)
>  {
>  	struct stat devstat;
> @@ -324,6 +384,11 @@ static int media_enum_entities(struct media_device
> *media) media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
>  			continue;
> 
> +		/* Try to get the device name via udev */
> +		if (!media_get_devname_udev(media, entity))
> +			continue;
> +
> +		/* Fall back to get the device name via sysfs */
>  		media_get_devname_sysfs(entity);
>  	}
> 
> @@ -351,6 +416,13 @@ struct media_device *media_open(const char *name, int
> verbose) return NULL;
>  	}
> 
> +	ret = media_udev_open(media);
> +	if (ret < 0) {
> +		printf("%s: Can't get udev context\n", __func__);
> +		media_close(media);
> +		return NULL;
> +	}
> +
>  	if (verbose)
>  		printf("Enumerating entities\n");
> 
> @@ -395,6 +467,7 @@ void media_close(struct media_device *media)
>  	}
> 
>  	free(media->entities);
> +	media_udev_close(media);
>  	free(media);
>  }
> 
> diff --git a/src/media.h b/src/media.h
> index b91a2ac..4201451 100644
> --- a/src/media.h
> +++ b/src/media.h
> @@ -54,6 +54,7 @@ struct media_device {
>  	struct media_entity *entities;
>  	unsigned int entities_count;
>  	__u32 padding[6];
> +	void *priv;
>  };
> 
>  /**

This will break binary compatibility if an application creates a struct 
media_device instances itself. On the other hand applications are not supposed 
to do that.

As the struct udev pointer is only used internally, what about passing it 
around between functions explicitly instead ?

-- 
Regards,

Laurent Pinchart

Re: [media-ctl][PATCHv4 3/3] libmediactl: get the device name via udev

[Andy Shevchenko]

On Mon, 2011-09-05 at 12:31 +0200, Laurent Pinchart wrote: 
> Hi Andy,
> 
> On Friday 02 September 2011 15:09:28 Andy Shevchenko wrote:
> > If configured with --with-libudev, the libmediactl is built with libudev
> > support. It allows to get the device name in right way in the modern linux
> > systems.
> 
> Thanks for the patch. We're nearly there :-)
I see.

> This will break binary compatibility if an application creates a struct 
> media_device instances itself. On the other hand applications are not supposed 
> to do that.
> 
> As the struct udev pointer is only used internally, what about passing it 
> around between functions explicitly instead ?
That we will break the API in media_close().
Might be I am a blind, but I can't see the way how to do both 1) don't
provide static global variable and 2) don't break the API/ABI.

So, I'll send 5th version of the patchset with API breakage.
Hope that what you could accept as a final version.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [media-ctl][PATCHv4 3/3] libmediactl: get the device name via udev

[Laurent Pinchart]

Hi Andy,

On Monday 05 September 2011 16:48:34 Andy Shevchenko wrote:
> On Mon, 2011-09-05 at 12:31 +0200, Laurent Pinchart wrote:
> > Hi Andy,
> > 
> > On Friday 02 September 2011 15:09:28 Andy Shevchenko wrote:
> > > If configured with --with-libudev, the libmediactl is built with
> > > libudev support. It allows to get the device name in right way in the
> > > modern linux systems.
> > 
> > Thanks for the patch. We're nearly there :-)
> 
> I see.
> 
> > This will break binary compatibility if an application creates a struct
> > media_device instances itself. On the other hand applications are not
> > supposed to do that.
> > 
> > As the struct udev pointer is only used internally, what about passing it
> > around between functions explicitly instead ?
> 
> That we will break the API in media_close().
> Might be I am a blind, but I can't see the way how to do both 1) don't
> provide static global variable and 2) don't break the API/ABI.

What about passing the udev pointer explictly to media_enum_entities() (which 
is static), and calling media_udev_close() in media_open() after the 
media_enum_entities() call ?

> So, I'll send 5th version of the patchset with API breakage.
> Hope that what you could accept as a final version.

Sorry for making it so difficult :-/

-- 
Regards,

Laurent Pinchart

[media-ctl][PATCHv5 1/5] libmediactl: restruct error path

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/media.c b/src/media.c
index e3cab86..050289e 100644
--- a/src/media.c
+++ b/src/media.c
@@ -255,7 +255,7 @@ static int media_enum_entities(struct media_device *media)
 	char target[1024];
 	char *p;
 	__u32 id;
-	int ret;
+	int ret = 0;
 
 	for (id = 0; ; id = entity->info.id) {
 		size = (media->entities_count + 1) * sizeof(*media->entities);
@@ -268,9 +268,9 @@ static int media_enum_entities(struct media_device *media)
 
 		ret = ioctl(media->fd, MEDIA_IOC_ENUM_ENTITIES, &entity->info);
 		if (ret < 0) {
-			if (errno == EINVAL)
-				break;
-			return -errno;
+			if (errno != EINVAL)
+				ret = -errno;
+			break;
 		}
 
 		/* Number of links (for outbound links) plus number of pads (for
@@ -281,8 +281,10 @@ static int media_enum_entities(struct media_device *media)
 
 		entity->pads = malloc(entity->info.pads * sizeof(*entity->pads));
 		entity->links = malloc(entity->max_links * sizeof(*entity->links));
-		if (entity->pads == NULL || entity->links == NULL)
-			return -ENOMEM;
+		if (entity->pads == NULL || entity->links == NULL) {
+			ret = -ENOMEM;
+			break;
+		}
 
 		media->entities_count++;
 
@@ -316,7 +318,7 @@ static int media_enum_entities(struct media_device *media)
 			strcpy(entity->devname, devname);
 	}
 
-	return 0;
+	return ret;
 }
 
 struct media_device *media_open(const char *name, int verbose)
-- 
1.7.5.4

[media-ctl][PATCHv5 2/5] libmediactl: split media_get_devname_sysfs from media_enum_entities

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |   61 +++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/src/media.c b/src/media.c
index 050289e..5d3ff7c 100644
--- a/src/media.c
+++ b/src/media.c
@@ -245,15 +245,46 @@ static int media_enum_links(struct media_device *media)
 	return ret;
 }
 
-static int media_enum_entities(struct media_device *media)
+static int media_get_devname_sysfs(struct media_entity *entity)
 {
-	struct media_entity *entity;
 	struct stat devstat;
-	unsigned int size;
 	char devname[32];
 	char sysname[32];
 	char target[1024];
 	char *p;
+	int ret;
+
+	sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
+		entity->info.v4l.minor);
+	ret = readlink(sysname, target, sizeof(target));
+	if (ret < 0)
+		return -errno;
+
+	target[ret] = '\0';
+	p = strrchr(target, '/');
+	if (p == NULL)
+		return -EINVAL;
+
+	sprintf(devname, "/dev/%s", p + 1);
+	ret = stat(devname, &devstat);
+	if (ret < 0)
+		return -errno;
+
+	/* Sanity check: udev might have reordered the device nodes.
+	 * Make sure the major/minor match. We should really use
+	 * libudev.
+	 */
+	if (major(devstat.st_rdev) == entity->info.v4l.major &&
+	    minor(devstat.st_rdev) == entity->info.v4l.minor)
+		strcpy(entity->devname, devname);
+
+	return 0;
+}
+
+static int media_enum_entities(struct media_device *media)
+{
+	struct media_entity *entity;
+	unsigned int size;
 	__u32 id;
 	int ret = 0;
 
@@ -293,29 +324,7 @@ static int media_enum_entities(struct media_device *media)
 		    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
 			continue;
 
-		sprintf(sysname, "/sys/dev/char/%u:%u", entity->info.v4l.major,
-			entity->info.v4l.minor);
-		ret = readlink(sysname, target, sizeof(target));
-		if (ret < 0)
-			continue;
-
-		target[ret] = '\0';
-		p = strrchr(target, '/');
-		if (p == NULL)
-			continue;
-
-		sprintf(devname, "/dev/%s", p + 1);
-		ret = stat(devname, &devstat);
-		if (ret < 0)
-			continue;
-
-		/* Sanity check: udev might have reordered the device nodes.
-		 * Make sure the major/minor match. We should really use
-		 * libudev.
-		 */
-		if (major(devstat.st_rdev) == entity->info.v4l.major &&
-		    minor(devstat.st_rdev) == entity->info.v4l.minor)
-			strcpy(entity->devname, devname);
+		media_get_devname_sysfs(entity);
 	}
 
 	return ret;
-- 
1.7.5.4

[media-ctl][PATCHv5 3/5] libmediactl: get the device name via udev

[Andy Shevchenko]

If configured with --with-libudev, the libmediactl is built with libudev
support. It allows to get the device name in right way in the modern linux
systems.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 configure.in    |   22 +++++++++++
 src/Makefile.am |    2 +
 src/media.c     |  105 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 127 insertions(+), 2 deletions(-)

diff --git a/configure.in b/configure.in
index fd4c70c..983023e 100644
--- a/configure.in
+++ b/configure.in
@@ -13,6 +13,28 @@ AC_PROG_LIBTOOL
 
 # Checks for libraries.
 
+AC_ARG_WITH([libudev],
+    AS_HELP_STRING([--with-libudev],
+        [Enable libudev to detect a device name]))
+
+AS_IF([test "x$with_libudev" = "xyes"],
+    [PKG_CHECK_MODULES(libudev, libudev, have_libudev=yes, have_libudev=no)],
+    [have_libudev=no])
+
+AS_IF([test "x$have_libudev" = "xyes"],
+    [
+        AC_DEFINE([HAVE_LIBUDEV], [], [Use libudev])
+        LIBUDEV_CFLAGS="$libudev_CFLAGS"
+        LIBUDEV_LIBS="$libudev_LIBS"
+        AC_SUBST(LIBUDEV_CFLAGS)
+        AC_SUBST(LIBUDEV_LIBS)
+    ],
+    [AS_IF([test "x$with_libudev" = "xyes"],
+        [AC_MSG_ERROR([libudev requested but not found])
+    ])
+])
+
+
 # Kernel headers path.
 AC_ARG_WITH(kernel-headers,
     [AC_HELP_STRING([--with-kernel-headers=DIR],
diff --git a/src/Makefile.am b/src/Makefile.am
index 267ea83..52628d2 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -5,6 +5,8 @@ mediactl_includedir=$(includedir)/mediactl
 mediactl_include_HEADERS = media.h subdev.h
 
 bin_PROGRAMS = media-ctl
+media_ctl_CFLAGS = $(LIBUDEV_CFLAGS)
+media_ctl_LDFLAGS = $(LIBUDEV_LIBS)
 media_ctl_SOURCES = main.c options.c options.h tools.h
 media_ctl_LDADD = libmediactl.la libv4l2subdev.la
 
diff --git a/src/media.c b/src/media.c
index 5d3ff7c..657b6c4 100644
--- a/src/media.c
+++ b/src/media.c
@@ -17,6 +17,8 @@
  * with this program; if not, write to the Free Software Foundation, Inc.,
  */
 
+#include "config.h"
+
 #include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ -245,6 +247,71 @@ static int media_enum_links(struct media_device *media)
 	return ret;
 }
 
+struct media_private {
+	struct media_device *media;
+	int verbose;
+	void *priv;
+};
+
+#ifdef HAVE_LIBUDEV
+
+#include <libudev.h>
+
+static inline int media_udev_open(struct media_private *priv)
+{
+	priv->priv = udev_new();
+	if (priv->priv == NULL)
+		return -ENOMEM;
+	return 0;
+}
+
+static inline void media_udev_close(struct media_private *priv)
+{
+	udev_unref(priv->priv);
+}
+
+static int media_get_devname_udev(struct media_private *priv,
+		struct media_entity *entity)
+{
+	struct udev *udev = priv->priv;
+	dev_t devnum;
+	struct udev_device *device;
+	const char *p;
+	int ret = -ENODEV;
+
+	if (udev == NULL)
+		return -EINVAL;
+
+	devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
+	if (priv->verbose)
+		printf("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);
+		if (p)
+			snprintf(entity->devname, sizeof(entity->devname), "%s", p);
+		ret = 0;
+	}
+
+	udev_device_unref(device);
+
+	return ret;
+}
+
+#else	/* HAVE_LIBUDEV */
+
+static inline int media_udev_open(struct media_private *priv) { return 0; }
+
+static inline void media_udev_close(struct media_private *priv) { }
+
+static inline int media_get_devname_udev(struct media_private *priv,
+		struct media_entity *entity)
+{
+	return -ENOTSUP;
+}
+
+#endif	/* HAVE_LIBUDEV */
+
 static int media_get_devname_sysfs(struct media_entity *entity)
 {
 	struct stat devstat;
@@ -281,8 +348,9 @@ static int media_get_devname_sysfs(struct media_entity *entity)
 	return 0;
 }
 
-static int media_enum_entities(struct media_device *media)
+static int media_enum_entities(struct media_private *priv)
 {
+	struct media_device *media = priv->media;
 	struct media_entity *entity;
 	unsigned int size;
 	__u32 id;
@@ -324,6 +392,11 @@ static int media_enum_entities(struct media_device *media)
 		    media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
 			continue;
 
+		/* Try to get the device name via udev */
+		if (!media_get_devname_udev(priv, entity))
+			continue;
+
+		/* Fall back to get the device name via sysfs */
 		media_get_devname_sysfs(entity);
 	}
 
@@ -333,6 +406,7 @@ static int media_enum_entities(struct media_device *media)
 struct media_device *media_open(const char *name, int verbose)
 {
 	struct media_device *media;
+	struct media_private *priv;
 	int ret;
 
 	media = malloc(sizeof(*media));
@@ -351,13 +425,39 @@ struct media_device *media_open(const char *name, int verbose)
 		return NULL;
 	}
 
+	priv = malloc(sizeof(*priv));
+	if (priv == NULL) {
+		printf("%s: unable to allocate memory\n", __func__);
+		media_close(media);
+		return NULL;
+	}
+	memset(priv, 0, sizeof(*priv));
+
+	/* Fill the private structure */
+	priv->media = media;
+	priv->verbose = verbose;
+
+	ret = media_udev_open(priv);
+	if (ret < 0) {
+		printf("%s: Can't get udev context\n", __func__);
+		free(priv);
+		media_close(media);
+		return NULL;
+	}
+
 	if (verbose)
 		printf("Enumerating entities\n");
 
-	ret = media_enum_entities(media);
+	ret = media_enum_entities(priv);
+
+	/* We should close the udev independently of return value of
+	 * media_enum_entities. */
+	media_udev_close(priv);
+
 	if (ret < 0) {
 		printf("%s: Unable to enumerate entities for device %s (%s)\n",
 			__func__, name, strerror(-ret));
+		free(priv);
 		media_close(media);
 		return NULL;
 	}
@@ -371,6 +471,7 @@ struct media_device *media_open(const char *name, int verbose)
 	if (ret < 0) {
 		printf("%s: Unable to enumerate pads and linksfor device %s\n",
 			__func__, name);
+		free(priv);
 		media_close(media);
 		return NULL;
 	}
-- 
1.7.5.4

[media-ctl][PATCHv5 4/5] libmediactl: simplify code by introducing close_and_free inliner

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/media.c b/src/media.c
index 657b6c4..6c03369 100644
--- a/src/media.c
+++ b/src/media.c
@@ -403,6 +403,12 @@ static int media_enum_entities(struct media_private *priv)
 	return ret;
 }
 
+static inline void close_and_free(struct media_private *priv)
+{
+	free(priv);
+	media_close(priv->media);
+}
+
 struct media_device *media_open(const char *name, int verbose)
 {
 	struct media_device *media;
@@ -440,8 +446,7 @@ struct media_device *media_open(const char *name, int verbose)
 	ret = media_udev_open(priv);
 	if (ret < 0) {
 		printf("%s: Can't get udev context\n", __func__);
-		free(priv);
-		media_close(media);
+		close_and_free(priv);
 		return NULL;
 	}
 
@@ -457,8 +462,7 @@ struct media_device *media_open(const char *name, int verbose)
 	if (ret < 0) {
 		printf("%s: Unable to enumerate entities for device %s (%s)\n",
 			__func__, name, strerror(-ret));
-		free(priv);
-		media_close(media);
+		close_and_free(priv);
 		return NULL;
 	}
 
@@ -471,8 +475,7 @@ struct media_device *media_open(const char *name, int verbose)
 	if (ret < 0) {
 		printf("%s: Unable to enumerate pads and linksfor device %s\n",
 			__func__, name);
-		free(priv);
-		media_close(media);
+		close_and_free(priv);
 		return NULL;
 	}
 
-- 
1.7.5.4

[media-ctl][PATCHv5 5/5] libmediactl: get rid of memset via using calloc

[Andy Shevchenko]

The code snippet
	x = malloc(sizeof(*x));
	memset(x, 0, sizeof(*x));
could be easily changed to
	x = calloc(1, sizeof(*x));

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 src/media.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/media.c b/src/media.c
index 6c03369..38ebaac 100644
--- a/src/media.c
+++ b/src/media.c
@@ -415,12 +415,11 @@ struct media_device *media_open(const char *name, int verbose)
 	struct media_private *priv;
 	int ret;
 
-	media = malloc(sizeof(*media));
+	media = calloc(1, sizeof(*media));
 	if (media == NULL) {
 		printf("%s: unable to allocate memory\n", __func__);
 		return NULL;
 	}
-	memset(media, 0, sizeof(*media));
 
 	if (verbose)
 		printf("Opening media device %s\n", name);
@@ -431,13 +430,12 @@ struct media_device *media_open(const char *name, int verbose)
 		return NULL;
 	}
 
-	priv = malloc(sizeof(*priv));
+	priv = calloc(1, sizeof(*priv));
 	if (priv == NULL) {
 		printf("%s: unable to allocate memory\n", __func__);
 		media_close(media);
 		return NULL;
 	}
-	memset(priv, 0, sizeof(*priv));
 
 	/* Fill the private structure */
 	priv->media = media;
-- 
1.7.5.4

Re: [media-ctl][PATCHv4 3/3] libmediactl: get the device name via udev

[Andy Shevchenko]

On Mon, 2011-09-05 at 16:57 +0200, Laurent Pinchart wrote: 
> > > This will break binary compatibility if an application creates a struct
> > > media_device instances itself. On the other hand applications are not
> > > supposed to do that.
> > > 
> > > As the struct udev pointer is only used internally, what about passing it
> > > around between functions explicitly instead ?
> > 
> > That we will break the API in media_close().
> > Might be I am a blind, but I can't see the way how to do both 1) don't
> > provide static global variable and 2) don't break the API/ABI.
> 
> What about passing the udev pointer explictly to media_enum_entities() (which 
> is static), and calling media_udev_close() in media_open() after the 
> media_enum_entities() call ?
I sent the patch series that incorporates your last comments.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [media-ctl][PATCHv5 1/5] libmediactl: restruct error path

[Laurent Pinchart]

Hi Andy,

Thank you for the patches.

I've slightly modified 1/5 and 3/5 (the first one returned -1 from 
media_enum_entities(), which made media-ctl stop with a failure message) and 
pushed the result to the repository.

I've also added another patch to fix autoconf malloc/realloc tests when cross-
compiling that resulted in a compilation failure.

-- 
Regards,

Laurent Pinchart

Re: [media-ctl][PATCHv5 1/5] libmediactl: restruct error path

[Andy Shevchenko]

On Tue, 2011-09-06 at 12:25 +0200, Laurent Pinchart wrote: 
> I've slightly modified 1/5 and 3/5 (the first one returned -1 from 
> media_enum_entities(), which made media-ctl stop with a failure message) and 
> pushed the result to the repository.
Okay. I looked at them.
One minor comment: udef_unref is aware of NULL.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [media-ctl][PATCHv5 1/5] libmediactl: restruct error path

[Andy Shevchenko]

On Tue, 2011-09-06 at 13:46 +0300, Andy Shevchenko wrote: 
> On Tue, 2011-09-06 at 12:25 +0200, Laurent Pinchart wrote: 
> > I've slightly modified 1/5 and 3/5 (the first one returned -1 from 
> > media_enum_entities(), which made media-ctl stop with a failure message) and 
> > pushed the result to the repository.
> Okay. I looked at them.
> One minor comment: udef_unref is aware of NULL.
Ah, and another. I don't get why you split snprintf() to that suboptimal
strncpy + x[sizeof(x)-1] = 0?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [media-ctl][PATCHv5 1/5] libmediactl: restruct error path

[Laurent Pinchart]

Hi Andy,

On Tuesday 06 September 2011 12:50:25 Andy Shevchenko wrote:
> On Tue, 2011-09-06 at 13:46 +0300, Andy Shevchenko wrote:
> > On Tue, 2011-09-06 at 12:25 +0200, Laurent Pinchart wrote:
> > > I've slightly modified 1/5 and 3/5 (the first one returned -1 from
> > > media_enum_entities(), which made media-ctl stop with a failure
> > > message) and pushed the result to the repository.
> > 
> > Okay. I looked at them.
> > One minor comment: udef_unref is aware of NULL.

I wasn't aware of that, thanks.

> Ah, and another. I don't get why you split snprintf() to that suboptimal
> strncpy + x[sizeof(x)-1] = 0?

snprintf needs to parse the format argument, is strncpy really suboptimal ?

-- 
Regards,

Laurent Pinchart