-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add xrdp-waitforx to wait for X to start with RandR outputs #2492
Conversation
There was a problem hiding this 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
waitforx/waitforx.c
Outdated
printf("Opened display %s\n", display); | ||
break; | ||
} | ||
sleep(1); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See d7d0866
waitforx/waitforx.c
Outdated
display = getenv("DISPLAY"); | ||
|
||
signal(SIGALRM, alarm_handler); | ||
alarm(ALARM_WAIT); |
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitforx/waitforx.c
Outdated
void | ||
alarm_handler(int signal_num) | ||
{ | ||
printf("Unable to find RandR outputs after %d seconds\n", ALARM_WAIT); |
There was a problem hiding this comment.
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));
waitforx/waitforx.c
Outdated
} | ||
XRRFreeScreenResources(res); | ||
} | ||
sleep(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See d7d0866
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).
d7d0866
to
cb39b84
Compare
Thanks Derek. |
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:
Fixes #2436