1. Patchew
  2. Libvirt
  3. [PATCH 0/2] vbox: Two simple fixes
  4. View patch

[PATCH 2/2] vbox: Fix starting domains

Michal Privoznik posted 2 patches 3 years ago
[PATCH 2/2] vbox: Fix starting domains
Posted by Michal Privoznik 3 years ago 
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@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);
+        VIR_FREE(displayutf8);
+        VIR_FREE(guiDisplay);
 
         VBOX_UTF8_TO_UTF16("gui", &sessionType);
     }
-- 
2.39.1
Re: [PATCH 2/2] vbox: Fix starting domains
Posted by Martin Kletzander 3 years ago 
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@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.

>+        VIR_FREE(displayutf8);
>+        VIR_FREE(guiDisplay);
>
>         VBOX_UTF8_TO_UTF16("gui", &sessionType);
>     }
>-- 
>2.39.1
>
Re: [PATCH 2/2] vbox: Fix starting domains
Posted by Michal Prívozník 3 years ago 
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@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.

Michal
Re: [PATCH 2/2] vbox: Fix starting domains
Posted by Martin Kletzander 3 years ago 
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@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?

>Michal
>
Re: [PATCH 2/2] vbox: Fix starting domains
Posted by Michal Prívozník 3 years ago 
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@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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.