[Libvirt-cim] [PATCH 1/4] VSSD: Add properties for arch and machine
Wenchao Xia
xiawenc at linux.vnet.ibm.com
Mon Aug 19 08:35:18 UTC 2013
于 2013-8-19 11:31, Wenchao Xia 写道:
> 于 2013-8-15 22:48, Viktor Mihajlovski 写道:
>> For architectures like s390 the machine type is relevant for
>> the proper guest construction. We add the necessary properties
>> to the schema and the C structures and the necessary code
>> for CIM-to-libvirt mapping.
>>
>> While doing this I noticed that the union fields in os_info
>> were set by means of XML parsing which doesn't take into account
>> that certain fields are depending on the virtualization type.
> I think this is a issue. Could u split this patch into two:
> 1 consider virt type for os_info, bugfix.
> 2 add xml-domain-VSSD mapping for properties machine and arch.
>
> Thus will make commit history clear and easier to review.
>
>> This could lead both to memory overwrites and memory leaks.
>> Fixed by using temporary variables and type-based setting of fields
>>
>> Signed-off-by: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
>> ---
>> libxkutil/device_parsing.c | 85
>> ++++++++++++++++++++++-------
>> libxkutil/device_parsing.h | 2 +
>> libxkutil/xmlgen.c | 6 ++
>> schema/VSSD.mof | 6 ++
>> src/Virt_VSSD.c | 9 +++
>> src/Virt_VirtualSystemManagementService.c | 14 +++++
>> 6 files changed, 101 insertions(+), 21 deletions(-)
>>
>> diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c
>> index ffdf682..df7a87a 100644
>> --- a/libxkutil/device_parsing.c
>> +++ b/libxkutil/device_parsing.c
>> @@ -1077,23 +1077,41 @@ int parse_fq_devid(const char *devid, char
>> **host, char **device)
>> return 1;
>> }
>>
>> +static void cleanup_bootlist(char **blist, unsigned blist_ct)
>> +{
>> + while (blist_ct > 0) {
>> + free(blist[--blist_ct]);
>> + }
>> + free(blist);
>> +}
>> +
>> static int parse_os(struct domain *dominfo, xmlNode *os)
>> {
>> xmlNode *child;
>> char **blist = NULL;
>> unsigned bl_size = 0;
>> + char *arch = NULL;
>> + char *machine = NULL;
>> + char *kernel = NULL;
>> + char *initrd = NULL;
>> + char *cmdline = NULL;
>> + char *loader = NULL;
>> + char *boot = NULL;
>> + char *init = NULL;
>>
>> for (child = os->children; child != NULL; child =
>> child->next) {
>> - if (XSTREQ(child->name, "type"))
>> + if (XSTREQ(child->name, "type")) {
>> STRPROP(dominfo, os_info.pv.type, child);
>> - else if (XSTREQ(child->name, "kernel"))
>> - STRPROP(dominfo, os_info.pv.kernel, child);
>> + arch = get_attr_value(child, "arch");
>> + machine = get_attr_value(child, "machine");
>> + } else if (XSTREQ(child->name, "kernel"))
>> + kernel = get_node_content(child);
>> else if (XSTREQ(child->name, "initrd"))
>> - STRPROP(dominfo, os_info.pv.initrd, child);
>> + initrd = get_node_content(child);
>> else if (XSTREQ(child->name, "cmdline"))
>> - STRPROP(dominfo, os_info.pv.cmdline, child);
>> + cmdline = get_node_content(child);
>> else if (XSTREQ(child->name, "loader"))
>> - STRPROP(dominfo, os_info.fv.loader, child);
>> + loader = get_node_content(child);
>> else if (XSTREQ(child->name, "boot")) {
>> char **tmp_list = NULL;
>>
>> @@ -1111,7 +1129,7 @@ static int parse_os(struct domain *dominfo,
>> xmlNode *os)
>> blist[bl_size] = get_attr_value(child, "dev");
>> bl_size++;
>> } else if (XSTREQ(child->name, "init"))
>> - STRPROP(dominfo, os_info.lxc.init, child);
>> + init = get_node_content(child);
>> }
>>
>> if ((STREQC(dominfo->os_info.fv.type, "hvm")) &&
>> @@ -1128,17 +1146,45 @@ static int parse_os(struct domain *dominfo,
>> xmlNode *os)
>> else
>> dominfo->type = -1;
>>
>> - if (STREQC(dominfo->os_info.fv.type, "hvm")) {
>> + switch (dominfo->type) {
>> + case DOMAIN_XENFV:
>> + case DOMAIN_KVM:
>> + case DOMAIN_QEMU:
>> + dominfo->os_info.fv.loader = loader;
>> + dominfo->os_info.fv.arch = arch;
>> + dominfo->os_info.fv.machine = machine;
> "machine = NULL;" is missing? Otherwise dominfo->os_info.fv.machine
> point to a value which is freed later.
>
Sorry I missed that it is in the following lines, so this is not a
problem.
>> dominfo->os_info.fv.bootlist_ct = bl_size;
>> dominfo->os_info.fv.bootlist = blist;
>> - } else {
>> - int i;
>> -
>> - for (i = 0; i < bl_size; i++)
>> - free(blist[i]);
>> - free(blist);
>> + loader = NULL;
>> + arch = NULL;
>> + machine = NULL;
>> + blist = NULL;
>> + bl_size = 0;
>> + break;
>> + case DOMAIN_XENPV:
>> + dominfo->os_info.pv.kernel = kernel;
>> + dominfo->os_info.pv.initrd = initrd;
>> + dominfo->os_info.pv.cmdline = cmdline;
>> + kernel = NULL;
>> + initrd = NULL;
>> + cmdline = NULL;
>> + break;
>> + case DOMAIN_LXC:
>> + dominfo->os_info.lxc.init = init;
>> + init = NULL;
>> + break;
>> + default:
>> + break;
>> }
>>
>> + free(arch);
>> + free(machine);
>> + free(kernel);
>> + free(initrd);
>> + free(cmdline);
>> + free(boot);
>> + free(init);
>> + cleanup_bootlist(blist, bl_size);
>> return 1;
>> }
>>
>> @@ -1334,15 +1380,12 @@ void cleanup_dominfo(struct domain **dominfo)
>> free(dom->os_info.pv.cmdline);
>> } else if ((dom->type == DOMAIN_XENFV) ||
>> (dom->type == DOMAIN_KVM) || (dom->type ==
>> DOMAIN_QEMU)) {
>> - int i;
>> -
>> free(dom->os_info.fv.type);
>> free(dom->os_info.fv.loader);
>> -
>> - for (i = 0; i < dom->os_info.fv.bootlist_ct; i++) {
>> - free(dom->os_info.fv.bootlist[i]);
>> - }
>> - free(dom->os_info.fv.bootlist);
>> + free(dom->os_info.fv.arch);
>> + free(dom->os_info.fv.machine);
>> + cleanup_bootlist(dom->os_info.fv.bootlist,
>> + dom->os_info.fv.bootlist_ct);
>> } else if (dom->type == DOMAIN_LXC) {
>> free(dom->os_info.lxc.type);
>> free(dom->os_info.lxc.init);
>> diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h
>> index 2b6d3d1..379d48c 100644
>> --- a/libxkutil/device_parsing.h
>> +++ b/libxkutil/device_parsing.h
>> @@ -136,6 +136,8 @@ struct pv_os_info {
>>
>> struct fv_os_info {
>> char *type; /* Should always be 'hvm' */
>> + char *arch;
>> + char *machine;
>> char *loader;
>> unsigned bootlist_ct;
>> char **bootlist;
>> diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c
>> index 4287d42..f19830f 100644
>> --- a/libxkutil/xmlgen.c
>> +++ b/libxkutil/xmlgen.c
>> @@ -802,6 +802,12 @@ static char *_kvm_os_xml(xmlNodePtr root, struct
>> domain *domain)
>> if (tmp == NULL)
>> return XML_ERROR;
>>
>> + if (os->arch)
>> + xmlNewProp(tmp, BAD_CAST "arch", BAD_CAST os->arch);
>> +
>> + if (os->machine)
>> + xmlNewProp(tmp, BAD_CAST "machine", BAD_CAST
>> os->machine);
>> +
>> ret = _fv_bootlist_xml(root, os);
>> if (ret == 0)
>> return XML_ERROR;
>> diff --git a/schema/VSSD.mof b/schema/VSSD.mof
>> index 0359d67..2734d8e 100644
>> --- a/schema/VSSD.mof
>> +++ b/schema/VSSD.mof
>> @@ -48,6 +48,12 @@ class KVM_VirtualSystemSettingData :
>> Virt_VirtualSystemSettingData
>> [Description ("The emulator the guest should use during runtime.")]
>> string Emulator;
>>
>> + [Description ("The guest's architecture.")]
>> + string Arch;
>> +
>> + [Description ("The guest's machine type")]
>> + string Machine;
>> +
>> };
> I haven't check DMTF docs, but wonder if there are existing DMTF file
> point out where this property should belong. If no, I think put it
> in VSSD is OK.
>
>>
>> [Description (
>> diff --git a/src/Virt_VSSD.c b/src/Virt_VSSD.c
>> index 3363b38..67e56aa 100644
>> --- a/src/Virt_VSSD.c
>> +++ b/src/Virt_VSSD.c
>> @@ -121,6 +121,15 @@ static CMPIStatus _set_fv_prop(const CMPIBroker
>> *broker,
>> goto out;
>> }
>>
>> + if (dominfo->os_info.fv.arch != NULL)
>> + CMSetProperty(inst, "Arch",
>> + (CMPIValue *)dominfo->os_info.fv.arch,
>> + CMPI_chars);
>> +
>> + if (dominfo->os_info.fv.machine != NULL)
>> + CMSetProperty(inst, "Machine",
>> + (CMPIValue *)dominfo->os_info.fv.machine,
>> + CMPI_chars);
>> out:
>> return s;
>> }
>> diff --git a/src/Virt_VirtualSystemManagementService.c
>> b/src/Virt_VirtualSystemManagementService.c
>> index 8ced2d6..3df878f 100644
>> --- a/src/Virt_VirtualSystemManagementService.c
>> +++ b/src/Virt_VirtualSystemManagementService.c
>> @@ -543,6 +543,20 @@ static int fv_vssd_to_domain(CMPIInstance *inst,
>> if (!fv_set_emulator(domain, val))
>> return 0;
>>
>> + free(domain->os_info.fv.arch);
>> + ret = cu_get_str_prop(inst, "Arch", &val);
>> + if (ret == CMPI_RC_OK)
>> + domain->os_info.fv.arch = strdup(val);
>> + else
>> + domain->os_info.fv.arch = NULL;
>> +
>> + free(domain->os_info.fv.machine);
>> + ret = cu_get_str_prop(inst, "Machine", &val);
>> + if (ret == CMPI_RC_OK)
>> + domain->os_info.fv.machine = strdup(val);
>> + else
>> + domain->os_info.fv.machine = NULL;
>> +
>> return 1;
>> }
>>
>
>
--
Best Regards
Wenchao Xia
More information about the Libvirt-cim
mailing list