[libvirt] [PATCH v2 01/12] migration: refactor: get rid of use_params p2p_full

[Nikolay Shirokovskiy]

On 17.09.2015 00:54, John Ferlan wrote:
> 
> FWIW: I figured I'd at least take a look - it's not my area of expertise
> though. I also ran the changes through my Coverity checker. The first
> pass found an issue in patch 10, which seems to be a result of some
> changes in patch 2 and perhaps patch 3...
> 
> 
> On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
>> 'useParams' parameter usage is an example of contol coupling. Most of the work
> 
> s/contol/control
> 
>> inside the function is done differently for different value of this flag except
> 
> s/value of this flag//
>> for the uri check. Lets split this function into 2, one with extensible
> 
> s/2/two
> 
>> parameters set and one with hardcoded parameter set. Common uri check we factor
>> out in different patch for clarity.
> 
> Move this last sentence to the commit message of patch 2...
> 
>>
>> Aim of this patchset is to unify logic for differet parameters representation
>> so finally we merge this split back thru extensible parameters.
> 
> This paragraph could state - this patch begins a series of changes to
> the Peer2Peer API's to utilize a common API with extensible parameters.
> Or it could be stricken or moved to the cover letter...
> 
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>> ---
>>  src/libvirt-domain.c |  129 +++++++++++++++++++++----------------------------
>>  1 files changed, 55 insertions(+), 74 deletions(-)
>>

> 
>>  
>> -/*
>> - * In normal migration, the libvirt client co-ordinates communication
>> - * between the 2 libvirtd instances on source & dest hosts.
>> - *
>> - * In this peer-2-peer migration alternative, the libvirt client
>> - * only talks to the source libvirtd instance. The source libvirtd
>> - * then opens its own connection to the destination and co-ordinates
>> - * migration itself.
>> - *
>> - * If useParams is true, params and nparams contain migration parameters and
>> - * we know it's safe to call the API which supports extensible parameters.
>> - * Otherwise, we have to use xmlin, dname, uri, and bandwidth and pass them
>> - * to the old-style APIs.
>> - */
> 
> Perhaps a hassle, but for now the comments could have moved with their
> respective function... Perhaps the first two paragraphs for the "Plain"
> function with the last "Otherwise" sentence changed slightly to indicate
> an "old style" static parameter listing.
> 
> Then for the "Params" function that last paragraph preceded with a
> reference to the "Plain" function...  Unlike the "Plain" function, this
> function uses an extensible parameter list...

Indeed I'll better keep comments here, but as plain and params are going
to be merged back again and parameters check will be moved inside
the function i'll keep only p2p explanation part. Which will finally
merge with direct function comments too.