Re: [PATCH 11/12] Move virDomain related APIs out of libvirt.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/23/2014 02:17 PM, Eric Blake wrote:
> On 10/22/2014 11:15 AM, Daniel P. Berrange wrote:
>> Introduce a src/libvirt-domain.c file to hold all the
>> methods related to the virDomain type.
>> ---
>>  docs/apibuild.py       |     2 +
>>  po/POTFILES.in         |     1 +
>>  src/Makefile.am        |     2 +
>>  src/libvirt-domain.c   | 11112 ++++++++++++++++++++++++++++++++++++++++++
>>  src/libvirt.c          | 12388 +++--------------------------------------------
>>  src/libvirt_internal.h |     6 +
>>  6 files changed, 11774 insertions(+), 11737 deletions(-)
>>  create mode 100644 src/libvirt-domain.c
> 
> My trick for reviewing the earlier patches via a pre-process comparison
> of lines added vs. lines removed failed on this one; you must have
> reordered some functions or something similar that made the diff messy :(
> 

>> +int
>> +virTypedParameterValidateSet(virConnectPtr conn,
>> +                             virTypedParameterPtr params,
>> +                             int nparams)
> 
> Ah. You left this function in place, but changed it from 'static int' to
> 'int', which in turn caused diff to think you re-arranged its location...
> 

>>  int
>> -virConnectListDomains(virConnectPtr conn, int *ids, int maxids)
>> +virNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
> 
> followed by all sorts of junk like this.

Aha.  You should run 'git config diff.algorithm patience'.  git's
default non-patience algorithm is LOUSY at code motion patches.  Once I
applied your patch, then regenerated it using the patience algorithm, it
became MUCH more readable, to the point that my code motion review trick
became a LOT easier to read.

ACK (but you may STILL want to do the refactoring of
virTypedParameterValidateSet in its own patch).  Here's how I reviewed:

$ diff -u <(git diff --patience HEAD^ | sed -n 's/^\-//p') \
          <(git diff --patience HEAD^ | sed -n 's/^\+//p')
> 
> --- /dev/fd/63	2014-10-23 14:27:58.067126468 -0600
> +++ /dev/fd/62	2014-10-23 14:27:58.067126468 -0600
> @@ -1,9 +1,49 @@
> --- c/docs/apibuild.py
> --- c/po/POTFILES.in
> --- c/src/Makefile.am
> --- /dev/null
> --- c/src/libvirt.c
> +++ w/docs/apibuild.py
> +  "libvirt-domain.c": "Domain interfaces for the libvirt library",
> +  "virTypedParameterValidateSet": "internal function in virtypedparam.c",
> +++ w/po/POTFILES.in
> +src/libvirt-domain.c
> +++ w/src/Makefile.am
> +		libvirt-domain.c 					\
> +		libvirt-domain.c		\
> +++ w/src/libvirt-domain.c
> +/*
> + * libvirt-domain.c: entry points for virDomainPtr APIs
> + *
> + * Copyright (C) 2006-2014 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +#include <sys/stat.h>
> +
>  #include "intprops.h"
> +
> +#include "datatypes.h"
> +#include "viralloc.h"
> +#include "virfile.h"
> +#include "virlog.h"
> +#include "virtypedparam.h"
> +
> +VIR_LOG_INIT("libvirt.domain");
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +
> +/**
>   * virConnectListDomains:
>   * @conn: pointer to the hypervisor connection
>   * @ids: array to collect the list of IDs of active domains
> @@ -2033,48 +2073,6 @@
>  }
>  
>  
> -/* Helper function called to validate incoming client array on any
> - * interface that sets typed parameters in the hypervisor.  */
> -static int
> -virTypedParameterValidateSet(virConnectPtr conn,
> -                             virTypedParameterPtr params,
> -                             int nparams)
> -{
> -    bool string_okay;
> -    size_t i;
> -
> -    string_okay = VIR_DRV_SUPPORTS_FEATURE(conn->driver,
> -                                           conn,
> -                                           VIR_DRV_FEATURE_TYPED_PARAM_STRING);
> -    for (i = 0; i < nparams; i++) {
> -        if (strnlen(params[i].field, VIR_TYPED_PARAM_FIELD_LENGTH) ==
> -            VIR_TYPED_PARAM_FIELD_LENGTH) {
> -            virReportInvalidArg(params,
> -                                _("string parameter name '%.*s' too long"),
> -                                VIR_TYPED_PARAM_FIELD_LENGTH,
> -                                params[i].field);
> -            return -1;
> -        }
> -        if (params[i].type == VIR_TYPED_PARAM_STRING) {
> -            if (string_okay) {
> -                if (!params[i].value.s) {
> -                    virReportInvalidArg(params,
> -                                        _("NULL string parameter '%s'"),
> -                                        params[i].field);
> -                    return -1;
> -                }
> -            } else {
> -                virReportInvalidArg(params,
> -                                    _("string parameter '%s' unsupported"),
> -                                    params[i].field);
> -                return -1;
> -            }
> -        }
> -    }
> -    return 0;
> -}
> -
> -
>  /**
>   * virDomainSetMemoryParameters:
>   * @domain: pointer to domain object
> @@ -6435,12 +6433,6 @@
>  }
>  
>  
> -/************************************************************************
> - *									*
> - *		Handling of defined but not running domains		*
> - *									*
> - ************************************************************************/
> -
>  /**
>   * virDomainDefineXML:
>   * @conn: pointer to the hypervisor connection
> @@ -7938,6 +7930,8 @@
>      virDispatchError(domain->conn);
>      return -1;
>  }
> +
> +
>  /**
>   * virDomainSetMetadata:
>   * @domain: a domain object
> @@ -8395,11 +8389,6 @@
>  
>  
>  /**
> -/*
> - * Domain Event Notification
> - */
> -
> -/**
>   * virConnectDomainEventRegister:
>   * @conn: pointer to the connection
>   * @cb: callback to the function handling domain events
> @@ -8592,6 +8581,7 @@
>  }
>  
>  
> +/**
>   * virDomainGetJobInfo:
>   * @domain: a domain object
>   * @info: pointer to a virDomainJobInfo structure allocated by the user
> @@ -10797,6 +10787,7 @@
>  }
>  
>  
> +
>  /**
>   * virConnectGetDomainCapabilities:
>   * @conn: pointer to the hypervisor connection
> @@ -11128,7 +11119,53 @@
>  
>      VIR_FREE(stats);
>  }
> +++ w/src/libvirt.c
> +/* Helper function called to validate incoming client array on any
> + * interface that sets typed parameters in the hypervisor.  */
> +int
> +virTypedParameterValidateSet(virConnectPtr conn,
> +                             virTypedParameterPtr params,
> +                             int nparams)
> +{
> +    bool string_okay;
> +    size_t i;
>  
> +    string_okay = VIR_DRV_SUPPORTS_FEATURE(conn->driver,
> +                                           conn,
> +                                           VIR_DRV_FEATURE_TYPED_PARAM_STRING);
> +    for (i = 0; i < nparams; i++) {
> +        if (strnlen(params[i].field, VIR_TYPED_PARAM_FIELD_LENGTH) ==
> +            VIR_TYPED_PARAM_FIELD_LENGTH) {
> +            virReportInvalidArg(params,
> +                                _("string parameter name '%.*s' too long"),
> +                                VIR_TYPED_PARAM_FIELD_LENGTH,
> +                                params[i].field);
> +            return -1;
> +        }
> +        if (params[i].type == VIR_TYPED_PARAM_STRING) {
> +            if (string_okay) {
> +                if (!params[i].value.s) {
> +                    virReportInvalidArg(params,
> +                                        _("NULL string parameter '%s'"),
> +                                        params[i].field);
> +                    return -1;
> +                }
> +            } else {
> +                virReportInvalidArg(params,
> +                                    _("string parameter '%s' unsupported"),
> +                                    params[i].field);
> +                return -1;
> +            }
> +        }
> +    }
> +    return 0;
> +}
> +
> +
> +++ w/src/libvirt_internal.h
> +
> +int
> +virTypedParameterValidateSet(virConnectPtr conn,
> +                             virTypedParameterPtr params,
> +                             int nparams);
>  
> -/**
> --- c/src/libvirt_internal.h

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]