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@xxxxxxxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxx> >> + */ >> + >> +#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. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list