[VDPAU] [PATCH] Add VDPAU_DRIVER_PATH support

[Aaron Plattner]

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!

-- 
Aaron