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 :)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list