[PATCH 0 of 2] VCPU xml generation
We have, so far, been ignoring the vcpu bit of XML generation, mostly because we treat multiple vcpus as separate devices in the CIM world, but as a single integral value in the libvirt world. This set changes the way the xml generation code is structured to handle batches of devices, and then adds support for generating the VCPU node. It's a little strange, but I decided that allowing a given device class to modify and append the XML being generated for its class was the safest way to handle all the cases without separating things out too much. Comments on this approach are welcomed. I believe that this set will also make ModifyResource in VSMS work for VCPUs as well, since it should be able to generate updated XML with the new vcpu count after such an operation. More testing to determine if additional work will be required needs to be done.
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1200414819 28800 # Node ID f90450901ee0e616bda311a5da2e7d14c570f6fa # Parent b2a79064df2639a6b7ade2f2bbcb21af9c66a267 Change xmlgen core to handle batches of devices in preparation for the next patch to fix vcpu xml generation Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r b2a79064df26 -r f90450901ee0 libxkutil/xmlgen.c --- a/libxkutil/xmlgen.c Mon Jan 14 13:01:22 2008 -0800 +++ b/libxkutil/xmlgen.c Tue Jan 15 08:33:39 2008 -0800 @@ -93,6 +93,26 @@ static char *tagify(char *tagname, char return result; } +static int astrcat(char **dest, char *source) +{ + char *tmp; + int ret; + + if (*dest) { + ret = asprintf(&tmp, "%s%s", *dest, source); + if (ret == -1) + return 0; + } else { + tmp = strdup(source); + } + + free(*dest); + + *dest = tmp; + + return 1; +} + static char *disk_block_xml(const char *path, const char *vdev) { char *xml; @@ -129,26 +149,34 @@ static char *disk_file_xml(const char *p return xml; } -static char *disk_to_xml(struct disk_device *disk) -{ +static bool disk_to_xml(char **xml, struct virt_device *dev) +{ + char *_xml = NULL; + struct disk_device *disk = &dev->dev.disk; + if (disk->disk_type == DISK_PHY) - return disk_block_xml(disk->source, disk->virtual_dev); + _xml = disk_block_xml(disk->source, disk->virtual_dev); else if (disk->disk_type == DISK_FILE) /* If it's not a block device, we assume a file, which should be a reasonable fail-safe */ - return disk_file_xml(disk->source, disk->virtual_dev); - else - return strdup("<!-- Unknown disk type -->\n"); -} - -static char *net_to_xml(struct net_device *net) -{ - int ret; - char *xml; - + _xml = disk_file_xml(disk->source, disk->virtual_dev); + else + return false; + + astrcat(xml, _xml); + free(_xml); + + return true; +} + +static bool net_to_xml(char **xml, struct virt_device *dev) +{ + int ret; + char *_xml; char *script = "vif-bridge"; - - ret = asprintf(&xml, + struct net_device *net = &dev->dev.net; + + ret = asprintf(&_xml, "<interface type='%s'>\n" " <mac address='%s'/>\n" " <script path='%s'/>\n" @@ -158,22 +186,28 @@ static char *net_to_xml(struct net_devic script); if (ret == -1) - xml = NULL; - - return xml; -} - -static char *proc_to_xml(struct vcpu_device *proc) -{ - return strdup(""); -} - -static char *mem_to_xml(struct mem_device *mem) -{ - int ret; - char *xml; - - ret = asprintf(&xml, + return false; + else + astrcat(xml, _xml); + + free(_xml); + + return true; +} + +static bool vcpu_to_xml(char **xml, struct virt_device *dev) +{ + astrcat(xml, "<vcpu>1</vcpu>\n"); + return true; +} + +static bool mem_to_xml(char **xml, struct virt_device *dev) +{ + int ret; + char *_xml; + struct mem_device *mem = &dev->dev.mem; + + ret = asprintf(&_xml, "<currentMemory>%" PRIu64 "</currentMemory>\n" "<memory>%" PRIu64 "</memory>\n", mem->size, @@ -181,91 +215,107 @@ static char *mem_to_xml(struct mem_devic if (ret == 1) - xml = NULL; - - return xml; -} - -static char *emu_to_xml(struct emu_device *emu) -{ - int ret; - char *xml; - - ret = asprintf(&xml, + return false; + else + astrcat(xml, _xml); + + free(_xml); + + return true; +} + +static bool emu_to_xml(char **xml, struct virt_device *dev) +{ + int ret; + char *_xml; + struct emu_device *emu = &dev->dev.emu; + + ret = asprintf(&_xml, "<emulator>%s</emulator>\n", emu->path); if (ret == -1) - xml = NULL; - - return xml; -} - -static char *graphics_to_xml(struct graphics_device *graphics) -{ - int ret; - char *xml; - - ret = asprintf(&xml, + return false; + else + astrcat(xml, _xml); + + free(_xml); + + return true; +} + +static bool graphics_to_xml(char **xml, struct virt_device *dev) +{ + int ret; + char *_xml; + struct graphics_device *graphics = &dev->dev.graphics; + + ret = asprintf(&_xml, "<graphics type='%s' port='%s'/>\n", graphics->type, graphics->port); if (ret == -1) - xml = NULL; - - return xml; + return false; + else + astrcat(xml, _xml); + + free(_xml); + + return true; +} + +static bool concat_devxml(char **xml, + struct virt_device *list, + int count, + bool (*func)(char **, struct virt_device *)) +{ + char *_xml = NULL; + int i; + + for (i = 0; i < count; i++) { + func(&_xml, &list[i]); + } + + astrcat(xml, _xml); + free(_xml); + + return true; } char *device_to_xml(struct virt_device *dev) { - switch (dev->type) { + char *xml = NULL; + int type = dev->type; + bool (*func)(char **, struct virt_device *); + + switch (type) { + case VIRT_DEV_DISK: + func = disk_to_xml; + break; + case VIRT_DEV_VCPU: + func = vcpu_to_xml; + break; case VIRT_DEV_NET: - return net_to_xml(&dev->dev.net); - case VIRT_DEV_DISK: - return disk_to_xml(&dev->dev.disk); + func = net_to_xml; + break; case VIRT_DEV_MEM: - return mem_to_xml(&dev->dev.mem); - case VIRT_DEV_VCPU: - return proc_to_xml(&dev->dev.vcpu); + func = mem_to_xml; + break; case VIRT_DEV_EMU: - return emu_to_xml(&dev->dev.emu); + func = emu_to_xml; + break; case VIRT_DEV_GRAPHICS: - return graphics_to_xml(&dev->dev.graphics); + func = graphics_to_xml; + break; default: return NULL; - }; -} - -static int astrcat(char **dest, char *source) -{ - char *tmp; - int ret; - - ret = asprintf(&tmp, "%s%s", *dest, source); - if (ret == -1) - return 0; - - free(*dest); - - *dest = tmp; - - return 1; -} - -static int concat_devxml(char ** xml, struct virt_device *list, int count) -{ - int i; - - for (i = 0; i < count; i++) { - char *devxml; - - devxml = device_to_xml(&list[i]); - if (devxml) { - astrcat(xml, devxml); - free(devxml); - } } - return count; + if (concat_devxml(&xml, dev, 1, func)) + return xml; + + free(xml); + + return NULL; } static char *system_xml(struct domain *domain) @@ -458,17 +508,35 @@ char *system_to_xml(struct domain *domin uuid_unparse(uuid, uuidstr); } - concat_devxml(&devxml, dominfo->dev_net, dominfo->dev_net_ct); - concat_devxml(&devxml, dominfo->dev_disk, dominfo->dev_disk_ct); + concat_devxml(&devxml, + dominfo->dev_net, + dominfo->dev_net_ct, + net_to_xml); + concat_devxml(&devxml, + dominfo->dev_disk, + dominfo->dev_disk_ct, + disk_to_xml); if (dominfo->dev_emu) - concat_devxml(&devxml, dominfo->dev_emu, 1); + concat_devxml(&devxml, + dominfo->dev_emu, + 1, + emu_to_xml); if (dominfo->dev_graphics) - concat_devxml(&devxml, dominfo->dev_graphics, 1); - - concat_devxml(&sysdevxml, dominfo->dev_mem, dominfo->dev_mem_ct); - concat_devxml(&sysdevxml, dominfo->dev_vcpu, dominfo->dev_vcpu_ct); + concat_devxml(&devxml, + dominfo->dev_graphics, + 1, + graphics_to_xml); + + concat_devxml(&sysdevxml, + dominfo->dev_mem, + dominfo->dev_mem_ct, + mem_to_xml); + concat_devxml(&sysdevxml, + dominfo->dev_vcpu, + dominfo->dev_vcpu_ct, + vcpu_to_xml); sysxml = system_xml(dominfo); osxml = os_xml(dominfo);
Dan Smith wrote:
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1200414819 28800 # Node ID f90450901ee0e616bda311a5da2e7d14c570f6fa # Parent b2a79064df2639a6b7ade2f2bbcb21af9c66a267 Change xmlgen core to handle batches of devices in preparation for the next patch to fix vcpu xml generation
+static bool mem_to_xml(char **xml, struct virt_device *dev) +{ + int ret; + char *_xml; + struct mem_device *mem = &dev->dev.mem; + + ret = asprintf(&_xml, "<currentMemory>%" PRIu64 "</currentMemory>\n" "<memory>%" PRIu64 "</memory>\n", mem->size, @@ -181,91 +215,107 @@ static char *mem_to_xml(struct mem_devic
if (ret == 1) - xml = NULL;
I think this should be if (ret == -1) here.
- return count; + if (concat_devxml(&xml, dev, 1, func)) + return xml; No complaint, just a question - why is the count being hardcoded to 1 here?
-- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
KR> I think this should be if (ret == -1) here. Yep, thanks. KR> No complaint, just a question - why is the count being hardcoded KR> to 1 here? Because I'm passing a single device instead of an array of devices. The device_to_xml() function is used elsewhere to convert a single device to xml, so this just acts as a type-detecting, list-faking, wrapper to the concat_devxml() function. Since the concat_devxml() function has a strange syntax and depends on internal-to-that-file functions, it seems better to have an external interface that is a bit more sane :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
KR> I think this should be if (ret == -1) here.
Yep, thanks.
KR> No complaint, just a question - why is the count being hardcoded KR> to 1 here?
Because I'm passing a single device instead of an array of devices. The device_to_xml() function is used elsewhere to convert a single device to xml, so this just acts as a type-detecting, list-faking, wrapper to the concat_devxml() function. Since the concat_devxml() function has a strange syntax and depends on internal-to-that-file functions, it seems better to have an external interface that is a bit more sane :)
Ah, yes. This makes more sense now. Thanks! -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1200415539 28800 # Node ID 0339dbd4e4898d409cc4a276a0197e71aec99937 # Parent f90450901ee0e616bda311a5da2e7d14c570f6fa Add VCPU XML generation Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r f90450901ee0 -r 0339dbd4e489 libxkutil/xmlgen.c --- a/libxkutil/xmlgen.c Tue Jan 15 08:33:39 2008 -0800 +++ b/libxkutil/xmlgen.c Tue Jan 15 08:45:39 2008 -0800 @@ -197,8 +197,21 @@ static bool net_to_xml(char **xml, struc static bool vcpu_to_xml(char **xml, struct virt_device *dev) { - astrcat(xml, "<vcpu>1</vcpu>\n"); - return true; + int count; + int ret; + + if (*xml == NULL) { + ret = asprintf(xml, "<vcpu>1</vcpu>"); + return ret != -1; + } + + if (sscanf(*xml, "<vcpu>%i</vcpu>\n", &count) != 1) + return false; + + free(*xml); + ret = asprintf(xml, "<vcpu>%i</vcpu>\n", count + 1); + + return ret != -1; } static bool mem_to_xml(char **xml, struct virt_device *dev)
Dan Smith wrote:
We have, so far, been ignoring the vcpu bit of XML generation, mostly because we treat multiple vcpus as separate devices in the CIM world, but as a single integral value in the libvirt world. This set changes the way the xml generation code is structured to handle batches of devices, and then adds support for generating the VCPU node.
It's a little strange, but I decided that allowing a given device class to modify and append the XML being generated for its class was the safest way to handle all the cases without separating things out too much. Comments on this approach are welcomed.
I believe that this set will also make ModifyResource in VSMS work for VCPUs as well, since it should be able to generate updated XML with the new vcpu count after such an operation. More testing to determine if additional work will be required needs to be done.
I'm unopposed in principle, and the implementation in these patches seems sound, so I would put my vote in the oh-so-confident position of, "Sounds like a good idea to me," while still leaving plenty of room to say, "Oh I was pretty sure there was a problem all along," if Heidi sees a CIM issue here. We're in a bit of a tricky position either way when we have to bridge the two concepts of how to treat VCPUs, so we might as well keep as much of it right in with the device class as we can. That way, if we find a problem, it won't be spread all over the place; we'll know exactly what we need to change. -- -Jay
participants (3)
-
Dan Smith -
Jay Gagnon -
Kaitlin Rupert