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

[Laine Stump]

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 at 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 at 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.