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