[vlc-devel] [PATCH] Support for alternative libv4l2 library (userspace drivers)

[Rémi Denis-Courmont]

Le 2014-08-01 19:07, Markus Rechberger a écrit :
> libv4l2 accesses device nodes, our one communicates via unix domain
> sockets to a local server the v4l2 API is fully compatible but the
> interface is different of course. I just mean that the API calls are
> fully compatible. The underlying architecture is completely 
> different.

As far as I understand, v4l2_fd_open() does not really care what type 
of file descriptor you feed it. As long as the file path can be opened 
in read/write mode, it should work just fine. Of course a socket cannot 
be opened, at least not with open/openat, but that is an internal 
implementation issue. You can still get the application to pass a 
regular file or a named pipe descriptor to v4l2_fd_open() and then 
switch to a socket in the V4L plugin initialization.

>> +        int fd;
>> +        char *buf=NULL;
>> +        int buflen=0;
>> +        int nread;
>> +        char rbuf[10240];
>> +        int rv;
>> +        struct stat statbuf;
>> +
>> +        const char *sockpath="@/de/sundtek/mediasocket";
>> +
>> +        /* FreeBSD, Solaris, MacOSX */
>> +        rv = stat("/tmp/.mediasocket", &statbuf);
>> +        if (rv == 0)
>> +                return 0;
>>
>> Looks very much like a TOCTOU race.
>>
>> Ditto for the procfs check.
>>
>> +
>> +        fd = open("/proc/net/unix", O_RDONLY);
>>
>> Missing close-on-exec.
>>
>
> close-on-exec is handled in the library already.

Uh? open() does not set close-on-exec by default, and it would be a 
serious problem if the run-time did it, since it would break those 
programs that do need to preserve file descriptors across exec().

>> +                        free(buf);
>> +                       close(fd);
>> +                        return 0;
>> +                }
>> +               close(fd);
>> +                free(buf);
>> +        }
>> +        return -1;
>> +}
>> +
>> +static int v4l2_net_open (char *path) {
>> +    void *h;
>> +    int fd;
>> +
>> +    h = dlopen ("libmcsimple.so", RTLD_LAZY | RTLD_LOCAL);
>> +    if (h == NULL) {
>> +       /* this is the default driver location */
>> +       h = dlopen("/opt/lib/libmcsimple.so", RTLD_LAZY | 
>> RTLD_LOCAL);
>>
>> From a quick glance, that library is not reentrant.
>>
>
> the library has locking inside to avoid race conditions.

I have to doubt that. For one thing, a quick objdump shows strerror() 
calls and strerror() is not reentrant at least not on GNU/Linux.

>> +       if (h == NULL)
>> +            goto fallback;
>> +    }
>> +    /* no need for that one */
>> +    v4l2_open_direct = dlsym (h, "net_open");
>> +    v4l2_close = dlsym (h, "net_close");
>> +    v4l2_ioctl = dlsym (h, "net_ioctl");
>> +    v4l2_read = dlsym (h, "__net_read");
>> +    v4l2_mmap = dlsym (h, "net_mmap");
>> +    v4l2_munmap = dlsym (h, "net_munmap");
>>
>> This breaks reentrancy and the memory model.
>> The reast of the assumes that the back-end library is constant 
>> throughout
>> the lifetime of the process.
>>
>
> I see what you mean here, I'll do some further tests.
> But I guess it should be more dynamic and just be checked when the
> user tries to open a node.

In general, we do not support updating VLC underlying libraries live; 
we just keep the old library version in memory. I cannot conceive any 
other corner case.

Unloading and reloading the library everytime would be a waste, not to 
mention the increased risk of memory leaks (should the library have any 
static data).

> Aside of that I was wondering about the pthread_once inside the
> original source, can you elaborate what the purpose
> of that is instead of doing those things directly without 
> pthread_once?

Reentrancy is the whole point. While unlikely, nothing (else) truly 
prevents two concurrent calls.

-- 
Rémi Denis-Courmont