[vlc-devel] [PATCH 2/6] modules: check plugin version
Steve Lhomme
robux4 at ycbcr.xyz
Fri Jul 19 14:12:51 CEST 2019
On 2019-07-19 13:42, Rémi Denis-Courmont wrote:
> Hi,
>
> Uh string comparison is linear and if it comes to that, this compares one fewer byte that before. This is completely irrelevant anyway, because the cost of relocating the loaded module will be literally thousands of times slower than the version check.
>
> If anything, the patch improves diagnostic by printing the plugin version in the error message. Nothing else really changes from a functional standpoint.
OK, because my next question was going to be: what do we gain from this
change ? This is a good improvement. It won't work when loading plugins
from before this change though.
> Le 19 juillet 2019 13:08:43 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>> On 2019-07-19 11:33, Rémi Denis-Courmont wrote:
>>> And vlc_dlsym does a string match. Exact same logic for the version
>> check.
>>
>> I don't deny that. But there is an extra string comparison in your
>> code.
>>
>>> In practice, this fails miserably. Either the plugin will not load
>> because of missing symbols (most common), or we'll crash or corrupt the
>> process before we can dlsym(). No change there.
>>
>> Having a generic entry point has its benefits. But that also mean we
>> have stronger backward/forward compatibility checks. For example I
>> sometimes have old plugins lying around in my build folder. If the
>> version doesn't match it won't be a problem in the current code. It
>> will
>> realize the new entry point is not there and won't go further. In the
>> new system it will actually load it and call the version callback.
>>
>>> Le 19 juillet 2019 12:25:02 GMT+03:00, Steve Lhomme
>> <robux4 at ycbcr.xyz> a écrit :
>>>> On 2019-07-19 11:15, Rémi Denis-Courmont wrote:
>>>>> Hi,
>>>>>
>>>>> It was a string with a string comparison before (inside vlc_dlsym).
>>>> It still is a string comparison.
>>>>
>>>> In the current code we do:
>>>> - vlc_dlopen
>>>> - vlc_dlsym to load the callback that has the version in its name
>>>>
>>>> In the new code we do:
>>>> - vlc_dlopen
>>>> - vlc_dlsym to load the callback with a generic name
>>>> - strcmp to check the version of the module
>>>>
>>>>> Of course it could be changed to an integer, but then the meta
>> export
>>>> macro won't be usable, so it will need more custom code.
>>>>>
>>>>> Le 19 juillet 2019 10:04:48 GMT+03:00, Steve Lhomme
>>>> <robux4 at ycbcr.xyz> a écrit :
>>>>>> On 2019-07-18 21:29, Rémi Denis-Courmont wrote:
>>>>>>> ---
>>>>>>> src/modules/bank.c | 59
>>>>>> +++++++++++++++++++++++++++++++++++-----------
>>>>>>> 1 file changed, 45 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/modules/bank.c b/src/modules/bank.c
>>>>>>> index 0b551a91bc..f6f5ae1a85 100644
>>>>>>> --- a/src/modules/bank.c
>>>>>>> +++ b/src/modules/bank.c
>>>>>>> @@ -193,6 +193,49 @@ static void module_InitStaticModules(void) {
>> }
>>>>>>> #endif
>>>>>>>
>>>>>>> #ifdef HAVE_DYNAMIC_PLUGINS
>>>>>>> +static const char *module_GetVersion(void *handle)
>>>>>>> +{
>>>>>>> + const char *(*get_api_version)(void);
>>>>>>> +
>>>>>>> + get_api_version = vlc_dlsym(handle,
>> "vlc_entry_api_version");
>>>>>>> + if (get_api_version == NULL)
>>>>>>> + return NULL;
>>>>>>> +
>>>>>>> + return get_api_version();
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void *module_Open(struct vlc_logger *log,
>>>>>>> + const char *path, bool fast)
>>>>>>> +{
>>>>>>> + void *handle = vlc_dlopen(path, fast);
>>>>>>> + if (handle == NULL)
>>>>>>> + {
>>>>>>> + char *errmsg = vlc_dlerror();
>>>>>>> +
>>>>>>> + vlc_error(log, "cannot load plug-in %s: %s", path,
>>>>>>> + errmsg ? errmsg : "unknown error");
>>>>>>> + free(errmsg);
>>>>>>> + return NULL;
>>>>>>> + }
>>>>>>> +
>>>>>>> + const char *str = module_GetVersion(handle);
>>>>>>> + if (str == NULL) {
>>>>>>> + vlc_error(log, "cannot load plug-in %s: %s", path,
>>>>>>> + "unknown version or not a plug-in");
>>>>>>> +error:
>>>>>>> + vlc_dlclose(handle);
>>>>>>> + return NULL;
>>>>>>> + }
>>>>>>> +
>>>>>>> + if (strcmp(str, VLC_API_VERSION_STRING)) {
>>>>>>
>>>>>> Couldn't we use an integer ? Speed-wise this is going backward
>> from
>>>>>> have
>>>>>> no extra check needed because the name we loaded already had the
>>>> proper
>>>>>>
>>>>>> version, which was a feature not a bug.
>>>>>>
>>>>>>> + vlc_error(log, "cannot load plug-in %s: unsupported
>>>> version
>>>>>> %s", path,
>>>>>>> + str);
>>>>>>> + goto error;
>>>>>>> + }
>>>>>>> +
>>>>>>> + return handle;
>>>>>>> +}
>>>>>>> +
>>>>>>> static const char vlc_entry_name[] = "vlc_entry"
>> MODULE_SUFFIX;
>>>>>>>
>>>>>>> /**
>>>>>>> @@ -207,15 +250,9 @@ static const char vlc_entry_name[] =
>>>> "vlc_entry"
>>>>>> MODULE_SUFFIX;
>>>>>>> static vlc_plugin_t *module_InitDynamic(vlc_object_t *obj,
>> const
>>>>>> char *path,
>>>>>>> bool fast)
>>>>>>> {
>>>>>>> - void *handle = vlc_dlopen(path, fast);
>>>>>>> + void *handle = module_Open(obj->logger, path, fast);
>>>>>>> if (handle == NULL)
>>>>>>> - {
>>>>>>> - char *errmsg = vlc_dlerror();
>>>>>>> - msg_Err(obj, "cannot load plug-in %s: %s", path,
>>>>>>> - errmsg ? errmsg : "unknown error");
>>>>>>> - free(errmsg);
>>>>>>> return NULL;
>>>>>>> - }
>>>>>>>
>>>>>>> /* Try to resolve the symbol */
>>>>>>> vlc_plugin_cb entry = vlc_dlsym(handle, vlc_entry_name);
>>>>>>> @@ -507,15 +544,9 @@ int module_Map(struct vlc_logger *log,
>>>>>> vlc_plugin_t *plugin)
>>>>>>> /* Try to load the plug-in (without locks, so read-only)
>> */
>>>>>>> assert(plugin->abspath != NULL);
>>>>>>>
>>>>>>> - void *handle = vlc_dlopen(plugin->abspath, false);
>>>>>>> + void *handle = module_Open(log, plugin->abspath, false);
>>>>>>> if (handle == NULL)
>>>>>>> - {
>>>>>>> - char *errmsg = vlc_dlerror();
>>>>>>> - vlc_error(log, "cannot load plug-in %s: %s",
>>>>>>> - plugin->abspath, errmsg ? errmsg : "unknown
>>>>>> error");
>>>>>>> - free(errmsg);
>>>>>>> return -1;
>>>>>>> - }
>>>>>>>
>>>>>>> vlc_plugin_cb entry = vlc_dlsym(handle, vlc_entry_name);
>>>>>>> if (entry == NULL)
>>>>>>> --
>>>>>>> 2.22.0
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> vlc-devel mailing list
>>>>>>> To unsubscribe or modify your subscription options:
>>>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>>>>
>>>>>> _______________________________________________
>>>>>> vlc-devel mailing list
>>>>>> To unsubscribe or modify your subscription options:
>>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>>
>>>>> --
>>>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>>>> excuser ma brièveté.
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> vlc-devel mailing list
>>>>> To unsubscribe or modify your subscription options:
>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>>
>>>> _______________________________________________
>>>> vlc-devel mailing list
>>>> To unsubscribe or modify your subscription options:
>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>
>>> --
>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>> excuser ma brièveté.
>>>
>>>
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>
> --
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list