[vlc-devel] [PATCH 1/2] interrupt: restore interrupted to the killed state
Rémi Denis-Courmont
remi at remlab.net
Thu Oct 24 19:02:14 CEST 2019
That sounds like a design bug in the password manager API. Checking which manager to use shouldn't need interrupt handling, and showing a dialog should happen after module probing is already finished.
Le 24 octobre 2019 16:12:30 GMT+03:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>There is the same problem with keystore modules. 2 (secret and kwallet)
>are using the i11e API. If you abort the first one, the second one will
>wait.
>
>There is likely the same issue with every modules that use the i11e API
>from their probe function.
>
>On Thu, Oct 24, 2019, at 14:14, Thomas Guillem wrote:
>>
>> On Thu, Oct 24, 2019, at 13:52, Rémi Denis-Courmont wrote:
>>> Hi,
>>>
>>> It is the same bug to the extent that:
>>> - there should not be more than one access module per network URI
>scheme, and
>> Why ? There is no documentation about that.
>>
>>> - it does not relate to the interrupt framework.
>>>
>>> It is not the same to the extent that the sad HTTP situation exists
>under the grand father clause. I already cut the number of HTTP
>handlers from original three (HTTP 1.1, HTTP 1.0, MMSH) to two (new and
>old), and just now suggested how to get to just one. SMB never had more
>than one handler in any release.
>>
>> libsmb2 is merged in VLC 3.0 now. Android and iOS ports will need
>these 2 modules for a long time.
>>
>>>
>>> Le 24 octobre 2019 14:39:14 GMT+03:00, Thomas Guillem
><thomas at gllm.fr> a écrit :
>>>>
>>>> On Thu, Oct 24, 2019, at 13:36, Rémi Denis-Courmont wrote:
>>>>> That's easy: drop the http alias from the old module (and rename
>it Icecast).
>>>>
>>>> We have the same issue with smb2 and libdsm.
>>>>
>>>>> This bug has nothing to do with user abort; we shouldn't retry to
>connect to an unresponsive IP address.
>>>>
>>>> "we shouldn't retry to connect to an unresponsive IP address." that
>is exactly what I'm trying to do.
>>>>
>>>>>
>>>>> Le 24 octobre 2019 13:47:02 GMT+03:00, Thomas Guillem
><thomas at gllm.fr> a écrit :
>>>>>> I have an other use case:
>>>>>>
>>>>>> 1/ Open a http stream, server is not responding, the open
>function of the http2 module is waiting
>>>>>> 2/ The user cancel if after some time, the http2 open module is
>interrupted
>>>>>> 3/ The old http module is probed and will also wait
>indefinitely. The user can't cancel it anymore.
>>>>>>
>>>>>> For me, it's clearly a bug, and a possible fix should be
>backported to VLC 3.0 too.
>>>>>>
>>>>>> I agree that my 2 attempts to fix it are not good
>>>>>>
>>>>>> 1/ Interrupt patch: rejected by you, I'll revert it
>>>>>> 2/ module probe patch: rejected by you. And indeed, breaking
>module loading even when the user cancel a media is not a perfect
>solution. For example it could prevent to load a module that will just
>save some contexts.
>>>>>>
>>>>>> I will work on an other solution
>>>>>>
>>>>>> On Thu, Oct 24, 2019, at 11:59, Rémi Denis-Courmont wrote:
>>>>>>> I don't agree with the problem statement. What it describes is
>intended behaviour, not a bug.
>>>>>>>
>>>>>>> Le 24 octobre 2019 12:46:46 GMT+03:00, Thomas Guillem
><thomas at gllm.fr> a écrit :
>>>>>>>>
>>>>>>>> On Thu, Oct 24, 2019, at 11:20, Rémi Denis-Courmont wrote:
>>>>>>>>> You effectively reverted to the old broken logic from before
>the interrupt system, and disregarding documented API semantics,
>wrecking many weeks of work from me. Is what you did.
>>>>>>>>
>>>>>>>> Yes I made a mistake, I should have waited longer and/or ask
>again for review before pushing it.
>>>>>>>> I did not realize that my patch would cause such a mess. I'm
>sorry for that.
>>>>>>>>
>>>>>>>> Is my alternate solution acceptable for you ?
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Le 24 octobre 2019 10:02:17 GMT+03:00, Thomas Guillem
><thomas at gllm.fr> a écrit :
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Wed, Oct 23, 2019, at 21:25, Remi Denis-Courmont wrote:
>>>>>>>>>>> Le 2019-10-22 17:47, Thomas Guillem a écrit :
>>>>>>>>>>>> If a module is interrupted while waiting in a vlc_*_i11e*()
>function,
>>>>>>>>>>>> the i11e
>>>>>>>>>>>> function will return an error after restoring the
>interrupted state
>>>>>>>>>>>> (from
>>>>>>>>>>>> vlc_interrupt_finish()). This error will be carefully
>handled by the
>>>>>>>>>>>> module
>>>>>>>>>>>> that will close itself. If a second module is in the probe
>list, any
>>>>>>>>>>>> call to a
>>>>>>>>>>>> vlc_*_i11e*() function will act as not interrupted. This
>cause any
>>>>>>>>>>>> following
>>>>>>>>>>>> modules in the probe list to ignore this interrupted state.
>>>>>>>>>>>>
>>>>>>>>>>>> To fix this issue, vlc_interrupt_finish() will now restore
>the
>>>>>>>>>>>> interrupted
>>>>>>>>>>>> state to the killed state. src/misc/interrupt.c | 2 +-
>>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/src/misc/interrupt.c b/src/misc/interrupt.c
>>>>>>>>>>>> index d2a04a72b3c..54810892fc1 100644
>>>>>>>>>>>> --- a/src/misc/interrupt.c
>>>>>>>>>>>> +++ b/src/misc/interrupt.c
>>>>>>>>>>>> @@ -151,7 +151,7 @@ static int
>vlc_interrupt_finish(vlc_interrupt_t
>>>>>>>>>>>> *ctx)
>>>>>>>>>>>> if (ctx->interrupted)
>>>>>>>>>>>> {
>>>>>>>>>>>> ret = EINTR;
>>>>>>>>>>>> - ctx->interrupted = false;
>>>>>>>>>>>> + ctx->interrupted = atomic_load(&ctx->killed);
>>>>>>>>>>> This looks wrong. The interruption is only there to signal
>the change in
>>>>>>>>>>> status. It's not supposed to fire again if the status is not
>changed.
>>>>>>>>>>
>>>>>>>>>> I sent this patch to open a discussion, I pushed the next
>day, maybe way too early, because vlc for iOS really need it in order
>to fix dialogs that are shown after the media is killed.
>>>>>>>>>>
>>>>>>>>>> It's OK I can revert.
>>>>>>>>>>
>>>>>>>>>> But I would like to discuss about the other way to fix the
>issue mentioned by this patch.
>>>>>>>>>> The only other fix I see if to abort the module probing
>(between 2 probes) if vlc_killed().
>>>>>>>>>>
>>>>>>>>>> >
>>>>>>>>>>>> }
>>>>>>>>>>>> vlc_mutex_unlock(&ctx->lock);
>>>>>>>>>>>> return ret;
>>>>>>>>>>> --
>>>>>>>>>>> Rémi Denis-Courmontvlc-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
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> 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
>>>>>>
>>>>>
>>>>> --
>>>>> 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
>>>>
>>>
>>> --
>>> 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é.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20191024/44250311/attachment.html>
More information about the vlc-devel
mailing list