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

Support and document SIGHUP for xrdp-sesman #2416

Merged
merged 1 commit into from
Nov 9, 2022

Conversation

matt335672
Copy link
Member

Fixes #2414

@yxzzx - can you take a look at this please?

@yxzzx
Copy link

yxzzx commented Nov 8, 2022

Isn't it a little bit asymmetric to do the kill with "xrdp-sesman --kill" and the reload with "kill -HUP $MAINPID". I think it would be cleaner to either remove the "--kill" code from sesman or add a "--reload" option to sesman. But that's probably just me being my usual obsessive-compulsive self...

Anyway, I think your commit will be doing the job just fine. Thanks for the fix and the documentation.

@matt335672
Copy link
Member Author

Hi @yxzzx

Thanks for the swift reply.

No - that's a very good point. It absolutely should be consistent. Leave it with me.

Adds a --reload switch to sesman and plumbs this in
to systemctl reload xrdp-sesman.service
Copy link
Member

@metalefty metalefty left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

BTW, is the log file reopened after reloading?

@matt335672
Copy link
Member Author

The code for a log restart was added for 2484928 :-

xrdp/common/log.c

Lines 727 to 782 in 4ff968b

/**
* Restarts the logging.
*
* The logging file is never changed, as it is common in xrdp to share a
* log file between parents and children. The end result would be
* confusing for the user.
*/
static enum logReturns
log_restart_from_param(const struct log_config *lc)
{
enum logReturns rv = LOG_GENERAL_ERROR;
if (g_staticLogConfig == NULL)
{
log_message(LOG_LEVEL_ALWAYS, "Log not already initialized");
}
else if (lc == NULL)
{
g_writeln("lc to log_start_from_param is NULL");
}
else
{
if (g_staticLogConfig->fd >= 0 &&
g_strcmp(g_staticLogConfig->log_file, lc->log_file) != 0)
{
log_message(LOG_LEVEL_WARNING,
"Unable to change log file name from %s to %s",
g_staticLogConfig->log_file,
lc->log_file);
}
/* Reconfigure syslog logging, allowing for a program_name change */
if (g_staticLogConfig->enable_syslog)
{
closelog();
}
if (lc->enable_syslog)
{
openlog(lc->program_name, LOG_CONS | LOG_PID, LOG_DAEMON);
}
/* Copy over simple values... */
#ifdef LOG_ENABLE_THREAD
g_staticLogConfig->log_lock = lc->log_lock;
g_staticLogConfig->log_lock_attr = lc->log_lock_attr;
#endif
g_staticLogConfig->program_name = lc->program_name;
g_staticLogConfig->enable_pid = lc->enable_pid;
g_staticLogConfig->dump_on_start = lc->dump_on_start;
/* ... and the log levels */
internal_log_config_copy_levels(g_staticLogConfig, lc);
rv = LOG_STARTUP_OK;
}
return rv;
}

Since the log file is shared, we never change it, as we've got no way of telling any child processes that the log file has changed. If the user tries to change it we log a warning and carry on using the old one.

That's the current behaviour anyway.

@matt335672 matt335672 merged commit 9940f2f into neutrinolabs:devel Nov 9, 2022
@matt335672 matt335672 deleted the reload_sesman branch November 9, 2022 16:06
@danielperna84
Copy link

Will this also affect #1317 ?

@matt335672
Copy link
Member Author

@danielperna84 - thanks for the heads-up.

As far as I can tell, #1317 isn't actually a problem. More information would be required to figure out exactly what was going on there.

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.

instfiles/xrdp-sesman.service.in has no ExecReload entry
4 participants