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

[chenfan]

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 :)
>