[VDPAU] [PATCH] Add VDPAU_DRIVER_PATH support

[Emil Velikov]

On 13/03/14 20:46, Aaron Plattner wrote:
> On 03/13/2014 01:23 PM, Emil Velikov wrote:
>> On 13/03/14 15:42, Stephen Warren wrote:
>>> Emil Velikov wrote at Monday, March 10, 2014 2:13 PM:
>>>> Allow the user to specify the location of the backend driver,
>>>> via the VDPAU_DRIVER_PATH environment variable. This allows
>>>> easier testing of VDPAU backends without the need to rebuild
>>>> libvdpau.
>>>>
>>>> Inspired by LIBGL_DRIVERS_PATH from mesa.
>>>
>>> Acked-by: Stephen Warren <swarren at nvidia.com>
>>>
>>> Just one question:
>>>
>>>> diff --git a/include/vdpau/vdpau_x11.h b/include/vdpau/vdpau_x11.h
>>>
>>>> +    if (geteuid() == getuid()) {
>>>> +        /* don't allow setuid apps to use VDPAU_DRIVER_PATH */
>>>> +        vdpau_driver_path = getenv("VDPAU_DRIVER_PATH");
>>>> +        if (vdpau_driver_path &&
>>>> +            snprintf(vdpau_driver_lib, sizeof(vdpau_driver_lib),
>>>> +                     DRIVER_LIB_FORMAT, vdpau_driver_path, vdpau_driver) <
>>>> +                sizeof(vdpau_driver_lib)) {
>>>> +            _vdp_driver_dll = dlopen(vdpau_driver_lib, RTLD_NOW |
>>>> RTLD_GLOBAL);
>>>
>>> If the snprintf() there fails, we just ignore it...
>>>
>>>> +    /* Fallback to VDPAU_MODULEDIR when VDPAU_DRIVER_PATH is not set,
>>>> +     * or if we fail to create the driver path/dlopen the library. */
>>>> +    if (!_vdp_driver_dll)
>>>> +        if (snprintf(vdpau_driver_lib, sizeof(vdpau_driver_lib),
>>>> +                     DRIVER_LIB_FORMAT, VDPAU_MODULEDIR, vdpau_driver) >=
>>>> +                sizeof(vdpau_driver_lib)) {
>>>> +            fprintf(stderr, "Failed to construct driver path: path too
>>>> long\n");
>>>> +            if (vdpau_driver_dri2) {
>>>> +                XFree(vdpau_driver_dri2);
>>>> +                vdpau_driver_dri2 = NULL;
>>>> +            }
>>>> +            _VDP_ERROR_BREAKPOINT();
>>>> +            return VDP_STATUS_NO_IMPLEMENTATION;
>>>
>>> ... whereas here, we actively return an error.
>>>
>>> Shouldn't those two paths be consistent; either each should return an error
>>> (probably best?), or perhaps both should print a warning message, and then
>>> the function should return an error at the end if (!_vdp_driver_dll)?
>>>
>> Interesting, I though I've covered this in an earlier email but I cannot
>> seem to find it. Either way here it goes:
>>
>> I have deliberately tried not to change the original functionality - if
>> for whatever reason we fail to load the getenv library we just fall back
>> to the original methods. Think of unsuspecting user has
>> VDPAU_DRIVER_PATH set in a way that snprint fails, while the original
>> methods would succeed.
>> With that said, a fprintf() would not hurt as a notification for the
>> failed snprintf, but I would not want to add premature return here.
> 
> I think that's fine.  I'll try to find some time to look at this patch soon,
> and we can discuss changing the failure behavior in a separate thread.
> 
> Thanks for working on this, Emil!
> 
Humble ping.