[VDPAU] [PATCH] Add VDPAU_DRIVER_PATH support

[Emil Velikov]

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.

Does that make sense or you would prefer if we return an error ?

Thanks
-Emil