| Message ID | 1337033057-31707-1-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 15.05.2012 00:04, Michael Roth wrote: > Currently, if we fail to open the specified log file (generally due to a > permissions issue), we'll assign NULL to the logfile handle (stderr, > initially) used by the logging routines, which can cause a segfault to > occur when we attempt to report the error before exiting. > > Instead, only re-assign if the open() was successful. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > qemu-ga.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/qemu-ga.c b/qemu-ga.c > index 3a88333..e2725c8 100644 > --- a/qemu-ga.c > +++ b/qemu-ga.c > @@ -681,6 +681,7 @@ int main(int argc, char **argv) > const char *log_filepath = NULL; > const char *pid_filepath = QGA_PIDFILE_DEFAULT; > const char *state_dir = QGA_STATEDIR_DEFAULT; > + FILE *log_file; > #ifdef _WIN32 > const char *service = NULL; > #endif > @@ -836,12 +837,13 @@ int main(int argc, char **argv) > become_daemon(pid_filepath); > } > if (log_filepath) { > - s->log_file = fopen(log_filepath, "a"); > - if (!s->log_file) { > + log_file = fopen(log_filepath, "a"); > + if (!log_file) { > g_critical("unable to open specified log file: %s", > strerror(errno)); > goto out_bad; > } > + s->log_file = log_file; > } > } > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
On Mon, 14 May 2012 17:04:17 -0500 Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > Currently, if we fail to open the specified log file (generally due to a > permissions issue), we'll assign NULL to the logfile handle (stderr, > initially) used by the logging routines, which can cause a segfault to > occur when we attempt to report the error before exiting. > > Instead, only re-assign if the open() was successful. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > qemu-ga.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/qemu-ga.c b/qemu-ga.c > index 3a88333..e2725c8 100644 > --- a/qemu-ga.c > +++ b/qemu-ga.c > @@ -681,6 +681,7 @@ int main(int argc, char **argv) > const char *log_filepath = NULL; > const char *pid_filepath = QGA_PIDFILE_DEFAULT; > const char *state_dir = QGA_STATEDIR_DEFAULT; > + FILE *log_file; > #ifdef _WIN32 > const char *service = NULL; > #endif > @@ -836,12 +837,13 @@ int main(int argc, char **argv) > become_daemon(pid_filepath); > } > if (log_filepath) { > - s->log_file = fopen(log_filepath, "a"); > - if (!s->log_file) { > + log_file = fopen(log_filepath, "a"); > + if (!log_file) { > g_critical("unable to open specified log file: %s", > strerror(errno)); > goto out_bad; > } > + s->log_file = log_file; Is it safe to change the log file this way? Isn't it necessary to go through g_log_set_default_handler() or some other function? > } > } >
On 14 May 2012 23:04, Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > Currently, if we fail to open the specified log file (generally due to a > permissions issue), we'll assign NULL to the logfile handle (stderr, > initially) used by the logging routines, which can cause a segfault to > occur when we attempt to report the error before exiting. > > Instead, only re-assign if the open() was successful. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > qemu-ga.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/qemu-ga.c b/qemu-ga.c > index 3a88333..e2725c8 100644 > --- a/qemu-ga.c > +++ b/qemu-ga.c > @@ -681,6 +681,7 @@ int main(int argc, char **argv) > const char *log_filepath = NULL; > const char *pid_filepath = QGA_PIDFILE_DEFAULT; > const char *state_dir = QGA_STATEDIR_DEFAULT; > + FILE *log_file; > #ifdef _WIN32 > const char *service = NULL; > #endif > @@ -836,12 +837,13 @@ int main(int argc, char **argv) > become_daemon(pid_filepath); > } > if (log_filepath) { > - s->log_file = fopen(log_filepath, "a"); > - if (!s->log_file) { > + log_file = fopen(log_filepath, "a"); > + if (!log_file) { > g_critical("unable to open specified log file: %s", > strerror(errno)); > goto out_bad; > } > + s->log_file = log_file; > } > } It would be nicer to put the log_file variable definition inside the if(), rather than putting it 150 lines earlier in the source file... -- PMM
On Tue, May 15, 2012 at 10:04:32AM -0300, Luiz Capitulino wrote: > On Mon, 14 May 2012 17:04:17 -0500 > Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > > > Currently, if we fail to open the specified log file (generally due to a > > permissions issue), we'll assign NULL to the logfile handle (stderr, > > initially) used by the logging routines, which can cause a segfault to > > occur when we attempt to report the error before exiting. > > > > Instead, only re-assign if the open() was successful. > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > --- > > qemu-ga.c | 6 ++++-- > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/qemu-ga.c b/qemu-ga.c > > index 3a88333..e2725c8 100644 > > --- a/qemu-ga.c > > +++ b/qemu-ga.c > > @@ -681,6 +681,7 @@ int main(int argc, char **argv) > > const char *log_filepath = NULL; > > const char *pid_filepath = QGA_PIDFILE_DEFAULT; > > const char *state_dir = QGA_STATEDIR_DEFAULT; > > + FILE *log_file; > > #ifdef _WIN32 > > const char *service = NULL; > > #endif > > @@ -836,12 +837,13 @@ int main(int argc, char **argv) > > become_daemon(pid_filepath); > > } > > if (log_filepath) { > > - s->log_file = fopen(log_filepath, "a"); > > - if (!s->log_file) { > > + log_file = fopen(log_filepath, "a"); > > + if (!log_file) { > > g_critical("unable to open specified log file: %s", > > strerror(errno)); > > goto out_bad; > > } > > + s->log_file = log_file; > > Is it safe to change the log file this way? Isn't it necessary > to go through g_log_set_default_handler() or some other function? Are you worried about a race condition? g_log_set_default_handler() does do locking, but our log handler is actually unchanged, so the only case it would be needed is if the opaque argument changes, or there were multiple fields changed in the opaque, since that might result it an undefined mix of old/new values. The locking for the actual fields in the opaque is our responsibility, and there's no way to get glib to comply (short of duplicating the opaque and then using g_log_set_default_handler(), since g_messages_lock isn't exposed) But since we're only changing a single field, the FILE* the log handler passes to fprintf(), we get the same level of automicity: a log statement will get stuck either in stderr or the new handle, depending on whether we made this change before or after another caller got to that point in ga_log(). So we should be okay, and aren't violating any glib locking protocols with change. > > > } > > } > > > >
On Tue, May 15, 2012 at 02:32:41PM +0100, Peter Maydell wrote: > On 14 May 2012 23:04, Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > > Currently, if we fail to open the specified log file (generally due to a > > permissions issue), we'll assign NULL to the logfile handle (stderr, > > initially) used by the logging routines, which can cause a segfault to > > occur when we attempt to report the error before exiting. > > > > Instead, only re-assign if the open() was successful. > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > --- > > qemu-ga.c | 6 ++++-- > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/qemu-ga.c b/qemu-ga.c > > index 3a88333..e2725c8 100644 > > --- a/qemu-ga.c > > +++ b/qemu-ga.c > > @@ -681,6 +681,7 @@ int main(int argc, char **argv) > > const char *log_filepath = NULL; > > const char *pid_filepath = QGA_PIDFILE_DEFAULT; > > const char *state_dir = QGA_STATEDIR_DEFAULT; > > + FILE *log_file; > > #ifdef _WIN32 > > const char *service = NULL; > > #endif > > @@ -836,12 +837,13 @@ int main(int argc, char **argv) > > become_daemon(pid_filepath); > > } > > if (log_filepath) { > > - s->log_file = fopen(log_filepath, "a"); > > - if (!s->log_file) { > > + log_file = fopen(log_filepath, "a"); > > + if (!log_file) { > > g_critical("unable to open specified log file: %s", > > strerror(errno)); > > goto out_bad; > > } > > + s->log_file = log_file; > > } > > } > > It would be nicer to put the log_file variable definition > inside the if(), rather than putting it 150 lines earlier > in the source file... Agreed...I've had it in my head for the longest time that declarations at the beginning of functions were QEMU coding style, but looking again I'm not sure where I got that idea. Fixed in tree: https://github.com/mdroth/qemu/commit/6c615ec57e83bf8cc7b1721bcd58c7d1ed93ef65 > > -- PMM >
On 15 May 2012 15:22, Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > On Tue, May 15, 2012 at 02:32:41PM +0100, Peter Maydell wrote: >> On 14 May 2012 23:04, Michael Roth <mdroth@linux.vnet.ibm.com> wrote: >> > Currently, if we fail to open the specified log file (generally due to a >> > permissions issue), we'll assign NULL to the logfile handle (stderr, >> > initially) used by the logging routines, which can cause a segfault to >> > occur when we attempt to report the error before exiting. >> > >> > Instead, only re-assign if the open() was successful. >> > >> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> >> > --- >> > qemu-ga.c | 6 ++++-- >> > 1 files changed, 4 insertions(+), 2 deletions(-) >> > >> > diff --git a/qemu-ga.c b/qemu-ga.c >> > index 3a88333..e2725c8 100644 >> > --- a/qemu-ga.c >> > +++ b/qemu-ga.c >> > @@ -681,6 +681,7 @@ int main(int argc, char **argv) >> > const char *log_filepath = NULL; >> > const char *pid_filepath = QGA_PIDFILE_DEFAULT; >> > const char *state_dir = QGA_STATEDIR_DEFAULT; >> > + FILE *log_file; >> > #ifdef _WIN32 >> > const char *service = NULL; >> > #endif >> > @@ -836,12 +837,13 @@ int main(int argc, char **argv) >> > become_daemon(pid_filepath); >> > } >> > if (log_filepath) { >> > - s->log_file = fopen(log_filepath, "a"); >> > - if (!s->log_file) { >> > + log_file = fopen(log_filepath, "a"); >> > + if (!log_file) { >> > g_critical("unable to open specified log file: %s", >> > strerror(errno)); >> > goto out_bad; >> > } >> > + s->log_file = log_file; >> > } >> > } >> >> It would be nicer to put the log_file variable definition >> inside the if(), rather than putting it 150 lines earlier >> in the source file... > > Agreed...I've had it in my head for the longest time that declarations > at the beginning of functions were QEMU coding style, but looking again > I'm not sure where I got that idea. I think there's a dislike of middle-of-block declarations (C++/C99 style), but beginning of a block is certainly fine. -- PMM
On Tue, 15 May 2012 09:14:13 -0500 Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > On Tue, May 15, 2012 at 10:04:32AM -0300, Luiz Capitulino wrote: > > On Mon, 14 May 2012 17:04:17 -0500 > > Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > > > > > Currently, if we fail to open the specified log file (generally due to a > > > permissions issue), we'll assign NULL to the logfile handle (stderr, > > > initially) used by the logging routines, which can cause a segfault to > > > occur when we attempt to report the error before exiting. > > > > > > Instead, only re-assign if the open() was successful. > > > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > > --- > > > qemu-ga.c | 6 ++++-- > > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/qemu-ga.c b/qemu-ga.c > > > index 3a88333..e2725c8 100644 > > > --- a/qemu-ga.c > > > +++ b/qemu-ga.c > > > @@ -681,6 +681,7 @@ int main(int argc, char **argv) > > > const char *log_filepath = NULL; > > > const char *pid_filepath = QGA_PIDFILE_DEFAULT; > > > const char *state_dir = QGA_STATEDIR_DEFAULT; > > > + FILE *log_file; > > > #ifdef _WIN32 > > > const char *service = NULL; > > > #endif > > > @@ -836,12 +837,13 @@ int main(int argc, char **argv) > > > become_daemon(pid_filepath); > > > } > > > if (log_filepath) { > > > - s->log_file = fopen(log_filepath, "a"); > > > - if (!s->log_file) { > > > + log_file = fopen(log_filepath, "a"); > > > + if (!log_file) { > > > g_critical("unable to open specified log file: %s", > > > strerror(errno)); > > > goto out_bad; > > > } > > > + s->log_file = log_file; > > > > Is it safe to change the log file this way? Isn't it necessary > > to go through g_log_set_default_handler() or some other function? > > Are you worried about a race condition? Actually, I was worried that glib could store the old s->log_file pointer somewhere, but I think I misread its documentation. Now I see that it's only used by ga_log(). I think that doing the log setup before logging anything would be the best solution, but that's a future improvement: Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
diff --git a/qemu-ga.c b/qemu-ga.c index 3a88333..e2725c8 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -681,6 +681,7 @@ int main(int argc, char **argv) const char *log_filepath = NULL; const char *pid_filepath = QGA_PIDFILE_DEFAULT; const char *state_dir = QGA_STATEDIR_DEFAULT; + FILE *log_file; #ifdef _WIN32 const char *service = NULL; #endif @@ -836,12 +837,13 @@ int main(int argc, char **argv) become_daemon(pid_filepath); } if (log_filepath) { - s->log_file = fopen(log_filepath, "a"); - if (!s->log_file) { + log_file = fopen(log_filepath, "a"); + if (!log_file) { g_critical("unable to open specified log file: %s", strerror(errno)); goto out_bad; } + s->log_file = log_file; } }
Currently, if we fail to open the specified log file (generally due to a permissions issue), we'll assign NULL to the logfile handle (stderr, initially) used by the logging routines, which can cause a segfault to occur when we attempt to report the error before exiting. Instead, only re-assign if the open() was successful. Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> --- qemu-ga.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)