Re: [PATCH 2/2] vbox: Fix starting domains

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(a)redhat.com&gt; >>> --- >>> 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#L... But in any case, doesn't the block under sdlDisplay conditional need the same treatment?