On 2/14/19 5:33 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> Thank you for the patch.
>
> On Thu, Feb 14, 2019 at 04:43:17PM +0100, Hans Verkuil wrote:
>> Add a --bus-info option to media-ctl which opens the media device
>> that has this bus info string. That makes it possible to open a specific
>> media device without having to know the name of the media device.
>>
>> Similar functionality has been implemented for v4l2-ctl and v4l2-compliance,
>> and for the cec utilities.
>>
>> This allows scripts that no longer need to care about the name of a device
>> node, instead they can find it based on a unique string.
>>
>> Also extend the -d option to support -d0 as a shorthand for /dev/media0 to
>> make it consistent with the other utils.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-ci...@xs4all.nl>
>> ---
>> diff --git a/utils/media-ctl/options.c b/utils/media-ctl/options.c
>> index 16367857..0430b8bc 100644
>> --- a/utils/media-ctl/options.c
>> +++ b/utils/media-ctl/options.c
>> @@ -19,13 +19,18 @@
>> * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> */
>>
>> +#include <ctype.h>
>> +#include <dirent.h>
>> +#include <fcntl.h>
>> #include <getopt.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <string.h>
>> +#include <sys/ioctl.h>
>> #include <unistd.h>
>> #include <v4l2subdev.h>
>>
>> +#include <linux/media.h>
>> #include <linux/videodev2.h>
>>
>> #include "options.h"
>> @@ -42,7 +47,9 @@ static void usage(const char *argv0)
>> unsigned int i;
>>
>> printf("%s [options]\n", argv0);
>> + printf("-b, --bus-info name Use the media device with bus info
>> equal to name\n");
>
> When seeing --bus-info I initially thought it was meant to print bus
> information. I wonder if we could find a name a bit more straightforward
> to guess. Would it be an option to reuse the -d option, first trying to
> open the specified device node, and considering it as a bus-info string
> if the file doesn't exist ?
That was actually my first approach. I'll change this in a v2.
>
>> printf("-d, --device dev Media device name (default: %s)\n",
>> MEDIA_DEVNAME_DEFAULT);
>> + printf(" If <dev> starts with a digit, then
>> /dev/media<dev> is used.\n");
>> printf("-e, --entity name Print the device name associated with
>> the given entity\n");
>> printf("-V, --set-v4l2 v4l2 Comma-separated list of formats to
>> setup\n");
>> printf(" --get-v4l2 pad Print the active format on a given
>> pad\n");
>> @@ -121,6 +128,7 @@ static void usage(const char *argv0)
>> #define OPT_GET_DV 260
>>
>> static struct option opts[] = {
>> + {"bus-info", 1, 0, 'b'},
>> {"device", 1, 0, 'd'},
>> {"entity", 1, 0, 'e'},
>> {"set-format", 1, 0, 'f'},
>> @@ -161,6 +169,51 @@ static void list_known_mbus_formats(void)
>> }
>> }
>>
>> +static const char *find_bus_info(const char *bus_info)
>> +{
>> + static char newdev[300];
>> + struct dirent *ep;
>> + DIR *dp;
>> +
>> + dp = opendir("/dev");
>> + if (dp == NULL)
>> + return NULL;
>> +
>> + while ((ep = readdir(dp))) {
>> + const char *name = ep->d_name;
>> +
>> + if (!memcmp(name, "media", 5) && isdigit(name[5])) {
>> + struct media_device_info mdi;
>> + int ret;
>> + int fd;
>> +
>> + snprintf(newdev, sizeof(newdev), "/dev/%s", name);
>> + fd = open(newdev, O_RDWR);
>
> You can use openat() instead of open() to avoid the snprintf.
Thanks for the tip!
>
>> + if (fd < 0)
>> + continue;
>> + ret = ioctl(fd, MEDIA_IOC_DEVICE_INFO, &mdi);
>> + close(fd);
>> + if (!ret && !strcmp(bus_info, mdi.bus_info)) {
>> + closedir(dp);
>> + return newdev;
>> + }
>> + }
>> + }
>> + closedir(dp);
>> + return NULL;
>
> Should we try to enumerate media devices using libudev, to support
> systems with custom udev rules ?
I think that's overkill. Might be something for the future, though.
>
>> +}
>> +
>> +static const char *make_devname(const char *device)
>> +{
>> + static char newdev[300];
>
> Seems a bit long for concatenating /dev/media and a 3 characters string.
>
>> +
>> + if (device[0] >= '0' && device[0] <= '9' && strlen(device) <= 3) {
>
> if (isdigit(device[0]) && strlen(device) <= 3)
>
> Shouldn't you however check that the whole string parses as a number ?
It's code copied from the other utilities. You can do this, but it's overkill.
I.e. '-d 0p' won't be a valid device node regardless.
>
>> + snprintf(newdev, sizeof(newdev), "/dev/media%s", device);
>> + return newdev;
>> + }
>> + return device;
>> +}
>> +
>> int parse_cmdline(int argc, char **argv)
>> {
>> int opt;
>> @@ -171,11 +224,20 @@ int parse_cmdline(int argc, char **argv)
>> }
>>
>> /* parse options */
>> - while ((opt = getopt_long(argc, argv, "d:e:f:hil:prvV:",
>> + while ((opt = getopt_long(argc, argv, "b:d:e:f:hil:prvV:",
>> opts, NULL)) != -1) {
>> switch (opt) {
>> + case 'b':
>> + media_opts.devname = find_bus_info(optarg);
>> + if (!media_opts.devname) {
>> + fprintf(stderr, "Error: no media device with
>> bus info '%s' found\n",
>> + optarg);
>> + return 1;
>> + }
>> + break;
>> +
>> case 'd':
>> - media_opts.devname = optarg;
>> + media_opts.devname = make_devname(optarg);
>> break;
>>
>> case 'e':
>
Regards,
Hans
