Skip to content
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

Merged
merged 3 commits into from
Feb 13, 2023

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