Re: [libvirt] [PATCH 1/6 v2] Prerequisite Patch. virDomainDevicePCIAddress and respective functions moved to a new file called conf/device_conf.ch

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&lt;sshah(a)solarflare.com&gt; > --- > 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&lt;sshah(a)solarflare.com&gt; > + */ > + > +#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].

> + > +#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?