Re: [libvirt] [PATCH v5 5/5] qemu: Implement the oncrash events in processWatchdogEvent

On 06/05/2013 03:55 AM, Chen Fan wrote: > Through the watchdog actions, we can implement the docoredump func, > we rewrite the processWatchdogEvent function to serval independent functions, > so we move the previous implementation of the destroy and restart code to here. > then the code looks like easy. > --- > src/qemu/qemu_driver.c | 197 +++++++++++++++++++++++++++++++++++++----------- > src/qemu/qemu_process.c | 118 +++++++++++++---------------- > src/qemu/qemu_process.h | 2 + > 3 files changed, 208 insertions(+), 109 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 4a76f14..4743539 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -3441,76 +3441,183 @@ cleanup: > return ret; > } > > +static int > +qemuProcessWatchdogCrashEvent(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + int watchDogAction) Indentation is off. Just as Dan complained on 4/5, guest panic and watchdog events are NOT the same; if you are going to have both call into common code, then the common code needs to be named something that fits the scenario correctly. > + > + if (isReset) { > + qemuProcessShutdownOrReboot(driver, vm); > + return 0; > + } > + > + /* If isReset, then do follow. */ This comment doesn't make sense with the earlier code; if isReset, then we've already exited. I'd just delete the comment, as it doesn't add anything. > +static int > +qemuProcessWatchdogDumpEvent(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + int watchDogAction, > + unsigned int flags) > +{ > + int ret = -1; > + char *dumpfile = NULL; > + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); > + > + if (virAsprintf(&dumpfile, "%s/%s-%u", > + cfg->autoDumpPath, > + vm->def->name, > + (unsigned int)time(NULL)) < 0) { This risks truncation of a 64-bit result from time(). > + case VIR_DOMAIN_WATCHDOG_ACTION_CRASHDUMP_DESTROY: > + case VIR_DOMAIN_WATCHDOG_ACTION_CRASHDUMP_RESET: > + { > + unsigned int flags = VIR_DUMP_MEMORY_ONLY; > > - if (virAsprintf(&dumpfile, "%s/%s-%u", > - cfg->autoDumpPath, > - wdEvent->vm->def->name, > - (unsigned int)time(NULL)) < 0) { then again, you are just doing code motion from earlier bad code. If you are going to do code motion, it's best to separate the refactoring into one commit, and then the new use of the refactored code in the next commit, rather than trying to combine the two steps into one commit. That is, it is easier for a reviewer to see that all you did in the first commit was pull things into their own function, rather than trying to figure out which part of the commit is new or movement. > +++ b/src/qemu/qemu_process.c > @@ -615,7 +615,7 @@ cleanup: > } > > > -static void > +void > qemuProcessShutdownOrReboot(virQEMUDriverPtr driver, > virDomainObjPtr vm) > { > @@ -816,6 +816,27 @@ qemuProcessHandleRTCChange(qemuMonitorPtr mon ATTRIBUTE_UNUSED, > return 0; > } > > +static void sendWatchDogEvent(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + int action) Indentation is off. Although we aren't very consistent in existing functions, our style for new functions is: static void sendWatchDogEvent(virQEMUDriverPtr driver, virDomainObjPtr vm, int action) My review was rather cursory, so there may be another round of meat to fix. In general, I'm grateful that you are working on adding this feature, and hope that it is in better shape by the time we are ready for freeze for 1.0.7 :)