On 15 May 2012 15:22, Michael Roth <mdr...@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 <mdr...@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 <mdr...@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
