[vbox-dev] vgdrvIoCtl_CancelAllWaitEvents inconsistent behavior ?

[Michael Thayer]

Hello Hans,

05.07.2017 11:18, Hans de Goede wrote:
[Discussion of inconsistent wait event cancelling code in VBoxGuest.]
> I've been looking at the userspace code myself and this bit:
> 
> src/VBox/Additions/x11/VBoxClient/seamless.cpp:
> 
> int SeamlessMain::selfTest()
> {
>     int rc = VERR_INTERNAL_ERROR;
>     const char *pcszStage;
> 
>     LogRelFlowFunc(("\n"));
>     do {
>         pcszStage = "Testing event loop cancellation";
>         VbglR3InterruptEventWaits();
>         if (RT_FAILURE(VbglR3WaitEvent(VMMDEV_EVENT_VALID_EVENT_MASK, 0,
> NULL)))
>             break;
>         if (   VbglR3WaitEvent(VMMDEV_EVENT_VALID_EVENT_MASK, 0, NULL)
>             != VERR_TIMEOUT)
>             break;
>         rc = VINF_SUCCESS;
>     } while(0);
>     if (RT_FAILURE(rc))
>         LogRel(("VBoxClient (seamless): self test failed.  Stage:
> \"%s\"\n",
>                 pcszStage));
>     return rc;
> }
> 
> Relies on the current behavior that a cancel while there are no waiters
> results in the next wait succeeding (without reporting any events), so
> I will mimick that behavior when moving this code over to linux
> waitqueues.

Actually it seems to me that it relies on the fact that the 0ms time out
is checked before the interruption.  At some point we (I) used
VbglR3InterruptEventWaits() in the seamless code, but I removed it when
I cleaned up the code as it is not nice.  I realised while searching for
uses of INTERRUPT_WAITEVENTS that I had forgotten to remove this pretty
pointless test.  I plan to deprecate calling WAITEVENT in a session
after INTERRUPT_WAITEVENTS has been called to make the matter moot and
to change WAITEVENT to enforce this.

Which brings us back to the question of ABI stability.  If something
like this were to pop up after you had released your code, I would
expect to handle it by announcing that it is deprecated and changing the
ABI later, say in the next major release.  Does that seem reasonable to
you, or too short?  And on the flip side, given that you have not yet
released, it seems like a good time for you to suggest similar clean-ups
too.

>> Of course, in practice there is always the risk of accidental
>> dependencies on current behaviour in all parts of the kernel-user
>> interface, but in the end it was your choice to try to duplicate an
>> interface created by several people independently, often under time
>> pressure, which has evolved more or less in parallel to the matching
>> user-space and host parts.  I would still recommend using our code as it
>> is in Fedora without submitting it upstream, and investing any
>> additional time you have for the task in helping improve our code.
> 
> I believe that there is value for Fedora, VirtualBox and our users in
> having this code upstream. The current code really is not suitable for
> upstream. I already have an idea of what the cleaned up code will look
> like and I think when you see it you will agree that it is an improvement
> over the current vboxguest kernel module code. The downside is that this
> improvement will come at the cost of it being Linux specific.
> 
>> 04.07.2017 16:57, Michael Thayer wrote:
>> Maybe I should have written that part in private mail.  If so, very
> sorry!
> 
> No problem / no worries, I'm fine with having this discussion in the open.
[...]I'm glad of that!  I tend not to continue the discussion, as I
think I have said what I wanted to, though I am open for anything you
still wish to add.  As mentioned above, suggestions for cleaning up our
ABI and code, including ones which might reduce the difference between
our code and yours are welcome and will probably benefit both of us.
When I get back to merging your vboxvideo updates I want to look at the
possibility of making our shared code match yours modulo a script to
adjust coding style (not yet sure if that will work).  The two versions
of vboxguest will definitely never get that close, but we can probably
still do a lot.

Regards
Michael
-- 
Michael Thayer | VirtualBox engineer
ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstraße 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V.
Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister
der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher