On 06/27/2012 09:18 AM, Osier Yang wrote:
On 2012年06月25日 19:35, Shradha Shah wrote:
Refactoring existing code without causing any functional changes to prepare for new code. This patch makes the code reusable.
Signed-off-by: Shradha Shah<sshah@solarflare.com> --- src/Makefile.am | 7 ++- src/conf/device_conf.c | 135 ++++++++++++++++++++++++++++++++++++++++++ src/conf/device_conf.h | 65 ++++++++++++++++++++ src/conf/domain_conf.c | 114 ++++------------------------------- src/conf/domain_conf.h | 25 +------- src/libvirt_private.syms | 10 ++- src/qemu/qemu_command.c | 13 ++-- src/qemu/qemu_hotplug.c | 7 +- src/qemu/qemu_monitor.c | 14 ++-- src/qemu/qemu_monitor.h | 17 +++--- src/qemu/qemu_monitor_json.c | 14 ++-- src/qemu/qemu_monitor_json.h | 14 ++-- src/qemu/qemu_monitor_text.c | 16 +++--- src/qemu/qemu_monitor_text.h | 14 ++-- src/xen/xend_internal.c | 3 +- 15 files changed, 288 insertions(+), 180 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index e40909b..009c4e5 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -199,6 +199,9 @@ CONSOLE_CONF_SOURCES = \ DOMAIN_LIST_SOURCES = \ conf/virdomainlist.c conf/virdomainlist.h
+DEVICE_CONF_SOURCES = \ + conf/device_conf.c conf/device_conf.h + CONF_SOURCES = \ $(NETDEV_CONF_SOURCES) \ $(DOMAIN_CONF_SOURCES) \ @@ -212,7 +215,8 @@ CONF_SOURCES = \ $(SECRET_CONF_SOURCES) \ $(CPU_CONF_SOURCES) \ $(CONSOLE_CONF_SOURCES) \ - $(DOMAIN_LIST_SOURCES) + $(DOMAIN_LIST_SOURCES) \ + $(DEVICE_CONF_SOURCES)
# The remote RPC driver, covering domains, storage, networks, etc REMOTE_DRIVER_GENERATED = \ @@ -1525,6 +1529,7 @@ libvirt_lxc_SOURCES = \ $(ENCRYPTION_CONF_SOURCES) \ $(NETDEV_CONF_SOURCES) \ $(DOMAIN_CONF_SOURCES) \ + $(DEVICE_CONF_SOURCES) \ $(SECRET_CONF_SOURCES) \ $(CPU_CONF_SOURCES) \ $(SECURITY_DRIVER_SOURCES) \ diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c new file mode 100644 index 0000000..af21aad --- /dev/null +++ b/src/conf/device_conf.c @@ -0,0 +1,135 @@ +/* + * device_conf.h: device XML handling + * + * Copyright (C) 2006-2012 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, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Shradha Shah<sshah@solarflare.com> + */ + +#include<config.h> +#include "virterror_internal.h" +#include "datatypes.h" +#include "memory.h" +#include "xml.h" +#include "uuid.h" +#include "util.h" +#include "buf.h" +#include "conf/device_conf.h" + +#define VIR_FROM_THIS VIR_FROM_DEVICE
VIR_FROM_DEVICE is not defined, and isn't it too big to use 'device'? I see only pci device address parsing and formating code actually. And most of the device conf codes are in domain_conf.[ch].
I think I suggested "device_conf.[ch]" in my review of Shradha's previous series. The reason is that we may need to break out the parsing/formatting of other types of devices in the future.
+ +#define virDeviceReportError(code, ...) \ + virReportErrorHelper(VIR_FROM_DOMAIN, code, __FILE__, \
Copy & Paste mistake (VIR_DOMAIN_DOMAIN)
I'm wondering if it's neccessary to refactor the codes like this. As it just split the codes from domain_conf.[ch] into new files, no more reusability as far as I can see. The only exception is the new virDevicePCIAddressFormat, but it can be in domain_conf.[ch] too. Any special reason you want to have 2 new files?
The immediate use is that the data structures will also be used in network_conf.h, and some of the functions will be called from network_conf.c, and I don't think it's clean to have network_conf.c calling into domain_conf.c, or to have network_conf.h #including domain_conf.h. The alternative mode of operation (just use the functions in their current location) could lead to a jumbled mess - consider if someone in the future decided that (in a move mirroring what's happened here) an object defined in network_conf.h was also useful in domain_conf.[hc] - you would then have a situation where domain_conf.h needed to #include network_conf.h, but network_conf.h needed to #include domain_conf.h. Better to keep a hierarchical organization and avoid circular references.