Thx for your detailed reviewer, I will make correction as you pointed
out.
On Wed, 2013-06-05 at 16:54 -0600, Eric Blake wrote:
> 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 :)
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list