On 05/21/2010 07:16 AM, Paolo Smiraglia wrote:
> Hi guys, I'm working on a pre migration checks implementation.
>
> The patch files attached provide a framework to execute
> pre-migration-checks before the domain migration.
First, thanks for the efforts (sometimes, as open source developers, we
get too caught up in our own work, and forget to acknowledge the work of
others; at which point our reviews come across as overly-critical, even
though that is not the intent).
Rather than sending lots of attachments, one per file, it would be nicer
to send one commit (if this is all atomic) or a series of commit files
(incremental atomic changes), and preferably inline to make it easier to
respond to files in email. 'git send-email' can be quite handy in this
regards.
The file HACKING has some more tips for submitting patches that are
easier to review.
>
>
> ================================================================
> 1 - FRAMEWORK OVERVIEW
> ================================================================
> Pre-Migration-Checks are enabled by adding a "--pmc" option in
> "virsh migrate" command
>
> $ virsh migrate --pmc ...
>
> In this way, before the execution of function virDomainMigrate(),
> an array of "checks" are performed. Return value of every check
> set if the migration will continue or not.
How does this fit in with the recent addition of hooks? While you've
done a good job explaining _what_ your patch does, I'm still not sure
you've done a good job of saying _why_ we need it, and how it solves
problems not already solvable with the hooks mechanism.
> Adding new check is very simple! You have to define a check
> funcion as
>
> static int
> pmcCheckFooCheck(virDomainPtr domain,
> virConnectPtr dconn)
> {
> /* do something */
> return CHECK_SUCCESS
> }
>
> then you have to define a "check-hook" as
>
> static virPMCCheck chk3 = {
> .name = "this is fooCheck",
> .run = pmcCheckFooCheck
> };
This requires recompilation, whereas the hooks mechanism allows addition
of a new hook via editing an external file with no recompilation. I'm
still wondering why a builtin mechanism is more extensible than
something that uses an external file.
Caveat - I have not glanced at any of your patch files, neither for
style reviews nor for algorithmic review.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
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