In fact, 'pos' is always -1, this reason is because qemuProcessStart function assigns -1 to 'pos' variable then call qemuProcessWaitForMonitor,

meanwhile, qemuProcessAttach function also call qemuProcessWaitForMonitor and directly pass -1 as an argument, so if (pos != -1) statement can't been run for ever,

it also means we can't allocate memory to 'buf' variable, that is, 'buf' is a initial value NULL, however, the function qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf)) will be called on 'cleanup' section, null pointer passed as an argument. * src/qemu/qemu_process.c: avoid null pointer passed as an argument to a 'nonnull' parameter.

On 08/04/2011 02:07 PM, Eric Blake wrote: > > We definitely have a bug here, but this is not the right fix. The bug is > that the cleanup: label is trying to read from logfd if the vm crashed, > without having opened logfd in the qemuProcessAttach case. > > I think the more appropriate patch is this: > > diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c > index 8508ff6..1eea45f 100644 > --- i/src/qemu/qemu_process.c > +++ w/src/qemu/qemu_process.c > @@ -1214,7 +1214,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* > driver, > cleanup: > virHashFree(paths); > > - if (kill(vm->pid, 0) == -1 && errno == ESRCH) { > + if (pos != -1 && kill(vm->pid, 0) == -1 && errno == ESRCH) { > /* VM is dead, any other error raised in the interim is probably > * not as important as the qemu cmdline output */ > qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf)); Also squash this in - no need to free buf twice. diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 1eea45f..a37f709 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1221,16 +1221,14 @@ cleanup: qemuReportError(VIR_ERR_INTERNAL_ERROR, _("process exited while connecting to monitor: %s"), buf); ret = -1; } closelog: - VIR_FREE(buf); - if (VIR_CLOSE(logfd) < 0) { char ebuf[1024]; VIR_WARN("Unable to close logfile: %s", virStrerror(errno, ebuf, sizeof ebuf)); } VIR_FREE(buf);

On 08/04/2011 09:31 AM, Alex Jia wrote: > In fact, 'pos' is always -1, this reason is because qemuProcessStart > function > assigns -1 to 'pos' variable then call qemuProcessWaitForMonitor, Actually, qemuProcessStart always calls qemuProcessWaitForMonitor with non-negative pos (pos was set by lseek()ing to the end of the log file). > meanwhile, > qemuProcessAttach function also call qemuProcessWaitForMonitor and > directly > pass -1 as an argument, so if (pos != -1) statement can't been run > for ever, Rather, this merely means that the if (pos != -1) statement wasn't run when called by qemuProcessAttach. > it also means we can't allocate memory to 'buf' variable, that is, > 'buf' is > a initial value NULL, however, the function > qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf)) will be called > on 'cleanup' section, null pointer passed as an argument. > > * src/qemu/qemu_process.c: avoid null pointer passed as an argument to a > 'nonnull' parameter. We definitely have a bug here, but this is not the right fix. The bug is that the cleanup: label is trying to read from logfd if the vm crashed, without having opened logfd in the qemuProcessAttach case. I think the more appropriate patch is this: diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 8508ff6..1eea45f 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1214,7 +1214,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver, cleanup: virHashFree(paths); - if (kill(vm->pid, 0) == -1 && errno == ESRCH) { + if (pos != -1 && kill(vm->pid, 0) == -1 && errno == ESRCH) { /* VM is dead, any other error raised in the interim is probably * not as important as the qemu cmdline output */ qemuProcessReadLogFD(logfd, buf, buf_size, strlen(buf));