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