[PATCH 2/2] vbox: Fix starting domains

[Michal Prívozník]

On 1/23/23 15:22, Martin Kletzander wrote:
> On Mon, Jan 23, 2023 at 02:35:40PM +0100, Michal Prívozník wrote:
>> On 1/23/23 10:45, Martin Kletzander wrote:
>>> On Mon, Jan 23, 2023 at 10:31:53AM +0100, Michal Privoznik wrote:
>>>> When starting a VirtualBox domain, we try to guess which frontend
>>>> to use. While the whole algorithm looks a bit outdated, it may
>>>> happen that we tell VirtualBox to use "gui" frontend, but not
>>>> which DISPLAY= to use.
>>>>
>>>> I haven't found any documentation on the algorithm we use, but if
>>>> I make us fallback onto DISPLAY=:0 when no other configuration is
>>>> found then I'm able to start my guests just fine.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>> ---
>>>> src/vbox/vbox_common.c | 13 ++++++-------
>>>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
>>>> index bd77641d39..5269f9b23f 100644
>>>> --- a/src/vbox/vbox_common.c
>>>> +++ b/src/vbox/vbox_common.c
>>>> @@ -2121,13 +2121,12 @@ vboxStartMachine(virDomainPtr dom, int
>>>> maxDomID, IMachine *machine, vboxIID *iid
>>>>     VBOX_UTF8_FREE(valueDisplayUtf8);
>>>>
>>>>     if (guiPresent) {
>>>> -        if (guiDisplay) {
>>>> -            char *displayutf8;
>>>> -            displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay);
>>>> -            VBOX_UTF8_TO_UTF16(displayutf8, &env);
>>>> -            VIR_FREE(displayutf8);
>>>> -            VIR_FREE(guiDisplay);
>>>> -        }
>>>> +        char *displayutf8;
>>>> +
>>>> +        displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay ?
>>>> guiDisplay : ":0");
>>>> +        VBOX_UTF8_TO_UTF16(displayutf8, &env);
>>>
>>> This might get overwritten when using SDL couple lines below.  I suggest
>>> you default to :0 only if none other option remains.  It feels dirty
>>> just guessing the display number, but let's say that's something we'll
>>> have to live with in the vbox driver.
>>
>> I'm failing to see how guiPresent and sdlPresent can be set at the same
>> time. But I guess I can join those two cases together, sure.
>>
> 
> Sorry, I missed that, I'm probably too cautious, although there are some
> other places where we're trying to be equally cautious,
> e.g. vboxAttachDisplay:
> 
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/vbox/vbox_common.c#L1651
> 
> But in any case, doesn't the block under sdlDisplay conditional need the
> same treatment?

Yeah, that's why I've sent v2:

https://listman.redhat.com/archives/libvir-list/2023-January/237296.html

Michal