Skip to content

Conversation

@derekschrock
Copy link
Contributor

@derekschrock derekschrock commented Jan 7, 2023

For some window managers (fvwm2 and fvwm3) if the X server isn't
running and has output it's possible for the window manager to fail or
reconfigure randr incorrectly.

With xrdp-waitfox:

  • Install xrdp-waitfox to the BIN dir.
  • sesman will run xrdp-waitfox as the logged in user.
  • Set an alarm to exit after 30 seconds.
  • Try to open env DISPLAY value's display (10 seconds).
  • Test for RandR extension.
  • Wait for outputs to appear (10 seconds).

Fixes #2436

Copy link
Member

@matt335672 matt335672 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this Derek.

for (n = 1; n <= wait; ++n)
{
dpy = XOpenDisplay(display);
printf("Opening display %s. Attempt %d of %d\n", display, n, wait);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using printf here raises an interesting question about some of the wrapper functions in common/os_calls.h.

Other bits of xrdp using printf() rather than g_printf()

Personally I can't see that we gain anything by using g_printf() over printf(). I think we should (over time) be removing the wrappers for standard C functions and simply use the <stdio.h> (etc) includes.

@metalefty - what's your take on this? Should we start a discussion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding printf(), I personally think I would like to quit using wrapper function and use printf() directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'll start a discussion about this.

printf("Opened display %s\n", display);
break;
}
sleep(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sleep() isn't a standard C function - can we use g_sleep(1000) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See d7d0866

display = getenv("DISPLAY");

signal(SIGALRM, alarm_handler);
alarm(ALARM_WAIT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the other signals these should probably be wrapped.

I'd suggest writing a function which does both the timer and setting the handler, as this could be moved to Windows later (not that this is likely to happen).

How does this look?

unsigned int g_set_alarm(void (*func)(int), unsigned int secs);
/*****************************************************************************/
/* does not work in win32 */
unsigned int
g_set_alarm(void (*func)(int), unsigned int secs)
{
#if defined(_WIN32)
    return 0;
#else
    /* Cancel any previous alarm to prevent a race */
    unsigned int rv = alarm(0);
    signal(SIGALRM, func);
    (void)alarm(secs);
    return rv;
#endif
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 8c44ae0 and d7d0866

void
alarm_handler(int signal_num)
{
printf("Unable to find RandR outputs after %d seconds\n", ALARM_WAIT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't use printf() in a signal handler. See signal-safety(7)

Given this is unlikely to be used on Windows, here's a suggested alternative.

/* Avoid printf() in signal handler (see signal-safety(7)) */
const char msg[] = "Timed out waiting for RandR outputs\n";
g_file_write(1, msg, g_strlen(msg));

}
XRRFreeScreenResources(res);
}
sleep(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See d7d0866

@matt335672
Copy link
Member

As far as the printf/g_printf discussion goes, this is effectively resolved. See #2499. We'll stick with printf() here I think.

For some window managers (fvwm2 and fvwm3) if the X server isn't
running and has output it's possible for the window manager to fail or
reconfigure randr incorrectly.

With xrdp-waitfox:
 - Install xrdp-waitfox to the BIN dir.
 - sesman will run xrdp-waitfox as the logged in user.
 - Set an alarm to exit after 30 seconds.
 - Try to open env DISPLAY value's display (10 seconds).
 - Test for RandR extension.
 - Wait for outputs to appear (10 seconds).
@matt335672
Copy link
Member

Thanks Derek.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't start window managers until there's an ouptut

3 participants