[PATCH 0 of 2] [RFC] Add boot order support for full virtualization environments
# HG changeset patch # User Richard Maciel <rmaciel@linux.vnet.ibm.com> # Date 1242071235 10800 # Node ID c0bd6c9a2c0084398784bb1ae36649bd3400e36c # Parent 5608b9455cd32fccbc324cd540c509d7230a113f This fix the generation of the <boot> tag on fully-virtualizable Xen guests. Right now it is generated with the boot device as a the value of the node (e.g. <boot>hd</boot>) However, the boot device must be a property of the node (e.g. <boot dev='hd'/>) Signed-off-by: Richard Maciel <rmaciel@linux.vnet.ibm.com> diff -r 5608b9455cd3 -r c0bd6c9a2c00 libxkutil/xmlgen.c --- a/libxkutil/xmlgen.c Mon Apr 27 17:05:48 2009 -0700 +++ b/libxkutil/xmlgen.c Mon May 11 16:47:15 2009 -0300 @@ -457,10 +457,12 @@ if (tmp == NULL) return XML_ERROR; - tmp = xmlNewChild(root, NULL, BAD_CAST "boot", BAD_CAST os->boot); + tmp = xmlNewChild(root, NULL, BAD_CAST "boot", NULL); if (tmp == NULL) return XML_ERROR; + xmlNewProp(tmp, BAD_CAST "dev", BAD_CAST os->boot); + tmp = xmlNewChild(root, NULL, BAD_CAST "features", NULL); xmlNewChild(tmp, NULL, BAD_CAST "pae", NULL); xmlNewChild(tmp, NULL, BAD_CAST "acpi", NULL);
# HG changeset patch # User Richard Maciel <rmaciel@linux.vnet.ibm.com> # Date 1242218025 10800 # Node ID b188a6d5bfea59ab1aae26be4b62817a5a414f4e # Parent c0bd6c9a2c0084398784bb1ae36649bd3400e36c Add boot order support Signed-off-by: Richard Maciel <rmaciel@linux.vnet.ibm.com> diff -r c0bd6c9a2c00 -r b188a6d5bfea libxkutil/device_parsing.c --- a/libxkutil/device_parsing.c Mon May 11 16:47:15 2009 -0300 +++ b/libxkutil/device_parsing.c Wed May 13 09:33:45 2009 -0300 @@ -810,6 +810,9 @@ static int parse_os(struct domain *dominfo, xmlNode *os) { xmlNode *child; + char **blist = NULL; + char **tmp_list = NULL; + unsigned bl_size = 0; for (child = os->children; child != NULL; child = child->next) { if (XSTREQ(child->name, "type")) @@ -822,10 +825,23 @@ STRPROP(dominfo, os_info.pv.cmdline, child); else if (XSTREQ(child->name, "loader")) STRPROP(dominfo, os_info.fv.loader, child); - else if (XSTREQ(child->name, "boot")) - dominfo->os_info.fv.boot = get_attr_value(child, - "dev"); - else if (XSTREQ(child->name, "init")) + else if (XSTREQ(child->name, "boot")) { + //dominfo->os_info.fv.boot = get_attr_value(child, + //"dev"); + bl_size++; + + tmp_list = (char **)realloc(blist, + bl_size * sizeof(char *)); + if (tmp_list == NULL) { + // Nothing you can do. Just go on. + CU_DEBUG("Could not alloc space for " + "boot device"); + bl_size--; + continue; + } + + blist[bl_size - 1] = get_attr_value(child, "dev"); + } else if (XSTREQ(child->name, "init")) STRPROP(dominfo, os_info.lxc.init, child); } @@ -843,6 +859,9 @@ else dominfo->type = -1; + dominfo->os_info.fv.bootlist = blist; + dominfo->os_info.fv.bootlist_size = bl_size; + return 1; } @@ -1001,9 +1020,15 @@ 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); - free(dom->os_info.fv.boot); + + for (i = 0; i < dom->os_info.fv.bootlist_size; i++) { + free(dom->os_info.fv.bootlist[i]); + } + free(dom->os_info.fv.bootlist); } else if (dom->type == DOMAIN_LXC) { free(dom->os_info.lxc.type); free(dom->os_info.lxc.init); diff -r c0bd6c9a2c00 -r b188a6d5bfea libxkutil/device_parsing.h --- a/libxkutil/device_parsing.h Mon May 11 16:47:15 2009 -0300 +++ b/libxkutil/device_parsing.h Wed May 13 09:33:45 2009 -0300 @@ -99,7 +99,9 @@ struct fv_os_info { char *type; /* Should always be 'hvm' */ char *loader; - char *boot; + unsigned bootlist_size; + char **bootlist; + //char *boot; }; struct lxc_os_info { diff -r c0bd6c9a2c00 -r b188a6d5bfea libxkutil/xml_parse_test.c --- a/libxkutil/xml_parse_test.c Mon May 11 16:47:15 2009 -0300 +++ b/libxkutil/xml_parse_test.c Wed May 13 09:33:45 2009 -0300 @@ -28,6 +28,7 @@ static void print_os(struct domain *dom, FILE *d) { + int i; if (dom->type == DOMAIN_XENPV) { print_value(d, "Domain Type", "Xen PV"); @@ -39,13 +40,18 @@ print_value(d, "Domain Type", "Xen FV"); print_value(d, "Type", dom->os_info.fv.type); print_value(d, "Loader", dom->os_info.fv.loader); - print_value(d, "Boot", dom->os_info.fv.boot); + for (i = 0; i < dom->os_info.fv.bootlist_size; i++) { + print_value(d, "Boot", dom->os_info.fv.bootlist[i]); + } } else if ((dom->type == DOMAIN_KVM) || (dom->type == DOMAIN_QEMU)) { print_value(d, "Domain Type", "KVM/QEMU"); print_value(d, "Type", dom->os_info.fv.type); print_value(d, "Loader", dom->os_info.fv.loader); - print_value(d, "Boot", dom->os_info.fv.boot); + + for (i = 0; i < dom->os_info.fv.bootlist_size; i++) { + print_value(d, "Boot", dom->os_info.fv.bootlist[i]); + } } else if (dom->type == DOMAIN_LXC) { print_value(d, "Init", dom->os_info.lxc.init); } else { diff -r c0bd6c9a2c00 -r b188a6d5bfea libxkutil/xmlgen.c --- a/libxkutil/xmlgen.c Mon May 11 16:47:15 2009 -0300 +++ b/libxkutil/xmlgen.c Wed May 13 09:33:45 2009 -0300 @@ -439,6 +439,7 @@ { struct fv_os_info *os = &domain->os_info.fv; xmlNodePtr tmp; + unsigned i; if (os->type == NULL) os->type = strdup("hvm"); @@ -446,8 +447,13 @@ if (os->loader == NULL) os->loader = strdup("/usr/lib/xen/boot/hvmloader"); - if (os->boot == NULL) - os->boot = strdup("hd"); + //if (os->boot == NULL) + // os->boot = strdup("hd"); + if (os->bootlist_size == 0) { + os->bootlist_size = 1; + os->bootlist = (char **)calloc(1, sizeof(char *)); + os->bootlist[0] = strdup("hd"); + } tmp = xmlNewChild(root, NULL, BAD_CAST "type", BAD_CAST os->type); if (tmp == NULL) @@ -457,11 +463,13 @@ if (tmp == NULL) return XML_ERROR; - tmp = xmlNewChild(root, NULL, BAD_CAST "boot", NULL); - if (tmp == NULL) - return XML_ERROR; + for (i = 0; i < os->bootlist_size; i++) { + tmp = xmlNewChild(root, NULL, BAD_CAST "boot", NULL); + if (tmp == NULL) + return XML_ERROR; - xmlNewProp(tmp, BAD_CAST "dev", BAD_CAST os->boot); + xmlNewProp(tmp, BAD_CAST "dev", BAD_CAST os->bootlist[i]); + } tmp = xmlNewChild(root, NULL, BAD_CAST "features", NULL); xmlNewChild(tmp, NULL, BAD_CAST "pae", NULL); @@ -475,21 +483,28 @@ { struct fv_os_info *os = &domain->os_info.fv; xmlNodePtr tmp; + unsigned i; if (os->type == NULL) os->type = strdup("hvm"); - if (os->boot == NULL) - os->boot = strdup("hd"); + if (os->bootlist_size == 0) { + os->bootlist_size = 1; + os->bootlist = (char **)calloc(1, sizeof(char *)); + os->bootlist[0] = strdup("hd"); + } tmp = xmlNewChild(root, NULL, BAD_CAST "type", BAD_CAST os->type); if (tmp == NULL) return XML_ERROR; - tmp = xmlNewChild(root, NULL, BAD_CAST "boot", NULL); - if (tmp == NULL) - return XML_ERROR; - xmlNewProp(tmp, BAD_CAST "dev", BAD_CAST os->boot); + for (i = 0; i < os->bootlist_size; i++) { + tmp = xmlNewChild(root, NULL, BAD_CAST "boot", NULL); + if (tmp == NULL) + return XML_ERROR; + + xmlNewProp(tmp, BAD_CAST "dev", BAD_CAST os->bootlist[i]); + } return NULL; } diff -r c0bd6c9a2c00 -r b188a6d5bfea schema/VSSD.mof --- a/schema/VSSD.mof Mon May 11 16:47:15 2009 -0300 +++ b/schema/VSSD.mof Wed May 13 09:33:45 2009 -0300 @@ -27,7 +27,7 @@ [Description ("The device to boot from when in fully-virtualized mode." "One of hd,fd,cdrom.")] - string BootDevice; + string BootDevices[]; [Description ("The emulator the guest should use during runtime.")] string Emulator; @@ -42,8 +42,8 @@ class KVM_VirtualSystemSettingData : Virt_VirtualSystemSettingData { - [Description ("The device to boot from. One of hd,fd,cdrom.")] - string BootDevice; + [Description ("The list of devices to boot from. hd,fd,cdrom.")] + string BootDevices[]; [Description ("The emulator the guest should use during runtime.")] string Emulator; diff -r c0bd6c9a2c00 -r b188a6d5bfea src/Virt_VSSD.c --- a/src/Virt_VSSD.c Mon May 11 16:47:15 2009 -0300 +++ b/src/Virt_VSSD.c Wed May 13 09:33:45 2009 -0300 @@ -42,16 +42,50 @@ CMPIInstance *inst) { bool fv = true; + CMPIArray *array; if (dominfo->type == DOMAIN_XENFV) CMSetProperty(inst, "IsFullVirt", (CMPIValue *)&fv, CMPI_boolean); - if (dominfo->os_info.fv.boot != NULL) - CMSetProperty(inst, - "BootDevice", - (CMPIValue *)dominfo->os_info.fv.boot, - CMPI_chars); + if (dominfo->os_info.fv.bootlist_size > 0) { + CMPICount i; + CMPICount bl_size; + CMPIStatus s; + + bl_size = (CMPICount)dominfo->os_info.fv.bootlist_size; + + array = CMNewArray(_BROKER, + bl_size, + CMPI_string, + &s); + + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error creating BootDevice list"); + + for (i = 0; i < bl_size; i++) { + CMPIString *cm_str; + + cm_str = CMNewString(_BROKER, + (const char *)dominfo->os_info.fv.bootlist[i], + &s); + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error adding item to BootDevice " + "list"); + } + + s = CMSetProperty(inst, + "BootDevices", + (CMPIValue *)array, + CMPI_stringA); + + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error setting BootDevices property"); + + //CMSetProperty(inst, + // "BootDevices", + // (CMPIValue *)dominfo->os_info.fv.boot); + } } static void _set_pv_prop(struct domain *dominfo, diff -r c0bd6c9a2c00 -r b188a6d5bfea src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c Mon May 11 16:47:15 2009 -0300 +++ b/src/Virt_VirtualSystemManagementService.c Wed May 13 09:33:45 2009 -0300 @@ -202,7 +202,13 @@ const char *pfx) { int ret; + CMPICount i; const char *val; + CMPIArray *bootlist; + CMPIStatus s; + CMPIData boot_elem; + char **tmp_str_arr; + if (STREQC(pfx, "KVM")) { if (system_has_kvm(pfx)) @@ -216,12 +222,75 @@ return 0; } - ret = cu_get_str_prop(inst, "BootDevice", &val); - if (ret != CMPI_RC_OK) - val = "hd"; + for (i = 0; i < domain->os_info.fv.bootlist_size; i++) + free(domain->os_info.fv.bootlist[i]); - free(domain->os_info.fv.boot); - domain->os_info.fv.boot = strdup(val); + ret = cu_get_array_prop(inst, "BootDevices", &bootlist); + + if (ret == CMPI_RC_OK) { + CMPICount bl_size; + + bl_size = CMGetArrayCount(bootlist, &s); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Invalid BootDevice array size"); + return 0; + } + + tmp_str_arr = (char **)realloc(domain->os_info.fv.bootlist, + bl_size * sizeof(char *)); + + if (tmp_str_arr == NULL) { + CU_DEBUG("Could not alloc BootDevices array"); + return 0; + } + + for (i = 0; i < bl_size; i++) { + CMPIString *cmpi_str; + const char *str; + + boot_elem = CMGetArrayElementAt(bootlist, + i, + NULL); + + if (CMIsNullValue(boot_elem)) { + CU_DEBUG("Null BootDevice"); + return 0; + } + + cmpi_str = boot_elem.value.string; + + free(domain->os_info.fv.bootlist[i]); + CU_DEBUG("Freed item from bootlist"); + + str = cmpi_str->ft->getCharPtr(cmpi_str, &s); + + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Could not extract char pointer from " + "CMPIArray"); + } + + tmp_str_arr[i] = strdup(str); + } + domain->os_info.fv.bootlist_size = bl_size; + domain->os_info.fv.bootlist = tmp_str_arr; + + } else { + + CU_DEBUG("Failed to get BootDevices property"); + + tmp_str_arr = (char **)realloc(domain->os_info.fv.bootlist, + sizeof(char *)); + if (tmp_str_arr == NULL) + return 0; + + tmp_str_arr[0] = strdup("hd"); + + domain->os_info.fv.bootlist = tmp_str_arr; + domain->os_info.fv.bootlist_size = 1; + } + + //free(domain->os_info.fv.boot); + //domain->os_info.fv.boot = strdup(val); ret = cu_get_str_prop(inst, "Emulator", &val); if (ret != CMPI_RC_OK)
Richard Maciel wrote:
# HG changeset patch # User Richard Maciel <rmaciel@linux.vnet.ibm.com> # Date 1242218025 10800 # Node ID b188a6d5bfea59ab1aae26be4b62817a5a414f4e # Parent c0bd6c9a2c0084398784bb1ae36649bd3400e36c Add boot order support
This patch is quite long and difficult to read. Can you break this up into smaller patches?
@@ -822,10 +825,23 @@ STRPROP(dominfo, os_info.pv.cmdline, child); else if (XSTREQ(child->name, "loader")) STRPROP(dominfo, os_info.fv.loader, child); - else if (XSTREQ(child->name, "boot")) - dominfo->os_info.fv.boot = get_attr_value(child, - "dev"); - else if (XSTREQ(child->name, "init")) + else if (XSTREQ(child->name, "boot")) { + //dominfo->os_info.fv.boot = get_attr_value(child, + //"dev"); + bl_size++; + + tmp_list = (char **)realloc(blist, + bl_size * sizeof(char *));
tmp_list isn't needed here - you aren't using it anywhere else. Just assign the the realloc back to blist.
+ if (tmp_list == NULL) { + // Nothing you can do. Just go on. + CU_DEBUG("Could not alloc space for " + "boot device"); + bl_size--;
Instead of incrementing prior to the realloc(), just call realloc() with (bl_size + 1). If the realloc() is successful, increment bl_size after the assignment below.
+ continue; + } + + blist[bl_size - 1] = get_attr_value(child, "dev");
So you would increment bl_size here.
diff -r c0bd6c9a2c00 -r b188a6d5bfea libxkutil/device_parsing.h --- a/libxkutil/device_parsing.h Mon May 11 16:47:15 2009 -0300 +++ b/libxkutil/device_parsing.h Wed May 13 09:33:45 2009 -0300 @@ -99,7 +99,9 @@ struct fv_os_info { char *type; /* Should always be 'hvm' */ char *loader; - char *boot; + unsigned bootlist_size;
I would call this bootlist_cnt - it'll parallel the dev_disk_ct (etc) fields of the domain struct
+ char **bootlist; + //char *boot;
Remove commented out line.
diff -r c0bd6c9a2c00 -r b188a6d5bfea libxkutil/xmlgen.c --- a/libxkutil/xmlgen.c Mon May 11 16:47:15 2009 -0300 +++ b/libxkutil/xmlgen.c Wed May 13 09:33:45 2009 -0300 @@ -439,6 +439,7 @@ { struct fv_os_info *os = &domain->os_info.fv; xmlNodePtr tmp; + unsigned i;
if (os->type == NULL) os->type = strdup("hvm"); @@ -446,8 +447,13 @@ if (os->loader == NULL) os->loader = strdup("/usr/lib/xen/boot/hvmloader");
- if (os->boot == NULL) - os->boot = strdup("hd"); + //if (os->boot == NULL) + // os->boot = strdup("hd");
Remove commented out lines.
+ if (os->bootlist_size == 0) { + os->bootlist_size = 1; + os->bootlist = (char **)calloc(1, sizeof(char *)); + os->bootlist[0] = strdup("hd"); + }
libvirt will set a default for us, but it's good to add something just in case.
diff -r c0bd6c9a2c00 -r b188a6d5bfea src/Virt_VSSD.c --- a/src/Virt_VSSD.c Mon May 11 16:47:15 2009 -0300 +++ b/src/Virt_VSSD.c Wed May 13 09:33:45 2009 -0300 @@ -42,16 +42,50 @@ CMPIInstance *inst) { bool fv = true; + CMPIArray *array;
if (dominfo->type == DOMAIN_XENFV) CMSetProperty(inst, "IsFullVirt", (CMPIValue *)&fv, CMPI_boolean);
- if (dominfo->os_info.fv.boot != NULL) - CMSetProperty(inst, - "BootDevice", - (CMPIValue *)dominfo->os_info.fv.boot, - CMPI_chars); + if (dominfo->os_info.fv.bootlist_size > 0) { + CMPICount i; + CMPICount bl_size; + CMPIStatus s; + + bl_size = (CMPICount)dominfo->os_info.fv.bootlist_size; + + array = CMNewArray(_BROKER, + bl_size, + CMPI_string, + &s); + + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error creating BootDevice list");
You should return here - if you fail to create the array, you can't add elements to it.
+ + for (i = 0; i < bl_size; i++) { + CMPIString *cm_str; + + cm_str = CMNewString(_BROKER, + (const char *)dominfo->os_info.fv.bootlist[i], + &s);
You need to set the elements of the array. See Virt_VirtualSystemManagementCapabilities.c (or other providers that need to set an array).
+ if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error adding item to BootDevice " + "list");
The debug message here is misleading - the call to CMNewString() doesn't add the string to the array. You'll need to call CMSetArrayElementAt() for that. Also, if you encounter an error here, you need to return an error.
+ } + + s = CMSetProperty(inst, + "BootDevices", + (CMPIValue *)array,
This needs to be &array.
+ CMPI_stringA); + + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error setting BootDevices property"); + + //CMSetProperty(inst, + // "BootDevices", + // (CMPIValue *)dominfo->os_info.fv.boot);
Remove commented out lines.
+ } }
static void _set_pv_prop(struct domain *dominfo, diff -r c0bd6c9a2c00 -r b188a6d5bfea src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c Mon May 11 16:47:15 2009 -0300 +++ b/src/Virt_VirtualSystemManagementService.c Wed May 13 09:33:45 2009 -0300 @@ -202,7 +202,13 @@ const char *pfx) { int ret; + CMPICount i; const char *val; + CMPIArray *bootlist; + CMPIStatus s; + CMPIData boot_elem; + char **tmp_str_arr; +
if (STREQC(pfx, "KVM")) { if (system_has_kvm(pfx)) @@ -216,12 +222,75 @@ return 0; }
- ret = cu_get_str_prop(inst, "BootDevice", &val); - if (ret != CMPI_RC_OK) - val = "hd"; + for (i = 0; i < domain->os_info.fv.bootlist_size; i++) + free(domain->os_info.fv.bootlist[i]);
- free(domain->os_info.fv.boot); - domain->os_info.fv.boot = strdup(val); + ret = cu_get_array_prop(inst, "BootDevices", &bootlist); + + if (ret == CMPI_RC_OK) { + CMPICount bl_size; + + bl_size = CMGetArrayCount(bootlist, &s); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Invalid BootDevice array size"); + return 0; + } + + tmp_str_arr = (char **)realloc(domain->os_info.fv.bootlist, + bl_size * sizeof(char *)); + + if (tmp_str_arr == NULL) { + CU_DEBUG("Could not alloc BootDevices array"); + return 0; + } + + for (i = 0; i < bl_size; i++) { + CMPIString *cmpi_str; + const char *str; + + boot_elem = CMGetArrayElementAt(bootlist, + i, + NULL); + + if (CMIsNullValue(boot_elem)) { + CU_DEBUG("Null BootDevice"); + return 0; + } + + cmpi_str = boot_elem.value.string;
You'll want to make sure that boot_elem isn't null. You can use CMIsNullObject().
+ + free(domain->os_info.fv.bootlist[i]); + CU_DEBUG("Freed item from bootlist"); + + str = cmpi_str->ft->getCharPtr(cmpi_str, &s);
Instead of doing this, you can use CMGetCharPtr() to pull the string from the CMPIData object. If you use CMGetCharPtr(), you won't need the cmpi_str variable.
+ + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Could not extract char pointer from " + "CMPIArray");
You'll want to return an error here.
+ } + + tmp_str_arr[i] = strdup(str); + } + domain->os_info.fv.bootlist_size = bl_size; + domain->os_info.fv.bootlist = tmp_str_arr; + + } else { + + CU_DEBUG("Failed to get BootDevices property"); + + tmp_str_arr = (char **)realloc(domain->os_info.fv.bootlist, + sizeof(char *)); + if (tmp_str_arr == NULL) + return 0; + + tmp_str_arr[0] = strdup("hd"); + + domain->os_info.fv.bootlist = tmp_str_arr; + domain->os_info.fv.bootlist_size = 1;
In xmlgen, you already set a default boot device. So we probably only need to do this once. You can remove this or remove the bit in xmlgen.
+ }
This whole block is quite large - I would make the boot order bits a separate function.
+ + //free(domain->os_info.fv.boot); + //domain->os_info.fv.boot = strdup(val);
Remove commented lines.
ret = cu_get_str_prop(inst, "Emulator", &val); if (ret != CMPI_RC_OK)
-- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
Kaitlin Rupert wrote:
Richard Maciel wrote:
# HG changeset patch # User Richard Maciel <rmaciel@linux.vnet.ibm.com> # Date 1242218025 10800 # Node ID b188a6d5bfea59ab1aae26be4b62817a5a414f4e # Parent c0bd6c9a2c0084398784bb1ae36649bd3400e36c Add boot order support
This patch is quite long and difficult to read. Can you break this up into smaller patches?
@@ -822,10 +825,23 @@ STRPROP(dominfo, os_info.pv.cmdline, child); else if (XSTREQ(child->name, "loader")) STRPROP(dominfo, os_info.fv.loader, child); - else if (XSTREQ(child->name, "boot")) - dominfo->os_info.fv.boot = get_attr_value(child, - "dev"); - else if (XSTREQ(child->name, "init")) + else if (XSTREQ(child->name, "boot")) { + //dominfo->os_info.fv.boot = get_attr_value(child, + //"dev"); + bl_size++; + + tmp_list = (char **)realloc(blist, + bl_size * sizeof(char *));
tmp_list isn't needed here - you aren't using it anywhere else. Just assign the the realloc back to blist.
Well, if realloc returns a null pointer I lose my pointer to the array which makes me deliver a NULL-pointer array with non-zero size.
+ if (tmp_list == NULL) { + // Nothing you can do. Just go on. + CU_DEBUG("Could not alloc space for " + "boot device"); + bl_size--;
Instead of incrementing prior to the realloc(), just call realloc() with (bl_size + 1). If the realloc() is successful, increment bl_size after the assignment below.
+ continue; + } + + blist[bl_size - 1] = get_attr_value(child, "dev");
So you would increment bl_size here.
diff -r c0bd6c9a2c00 -r b188a6d5bfea libxkutil/device_parsing.h --- a/libxkutil/device_parsing.h Mon May 11 16:47:15 2009 -0300 +++ b/libxkutil/device_parsing.h Wed May 13 09:33:45 2009 -0300 @@ -99,7 +99,9 @@ struct fv_os_info { char *type; /* Should always be 'hvm' */ char *loader; - char *boot; + unsigned bootlist_size;
I would call this bootlist_cnt - it'll parallel the dev_disk_ct (etc) fields of the domain struct
Well, the size suffix is better, IMO. But if you think it's better to follow the name pattern I'll fix it.
+ char **bootlist; + //char *boot;
Remove commented out line.
diff -r c0bd6c9a2c00 -r b188a6d5bfea libxkutil/xmlgen.c --- a/libxkutil/xmlgen.c Mon May 11 16:47:15 2009 -0300 +++ b/libxkutil/xmlgen.c Wed May 13 09:33:45 2009 -0300 @@ -439,6 +439,7 @@ { struct fv_os_info *os = &domain->os_info.fv; xmlNodePtr tmp; + unsigned i;
if (os->type == NULL) os->type = strdup("hvm"); @@ -446,8 +447,13 @@ if (os->loader == NULL) os->loader = strdup("/usr/lib/xen/boot/hvmloader");
- if (os->boot == NULL) - os->boot = strdup("hd"); + //if (os->boot == NULL) + // os->boot = strdup("hd");
Remove commented out lines.
Leave my comments alone!
+ if (os->bootlist_size == 0) { + os->bootlist_size = 1; + os->bootlist = (char **)calloc(1, sizeof(char *)); + os->bootlist[0] = strdup("hd"); + }
libvirt will set a default for us, but it's good to add something just in case.
I don't get it. How can libvirt set a default? Will it automagically provide an array with a default value?
diff -r c0bd6c9a2c00 -r b188a6d5bfea src/Virt_VSSD.c --- a/src/Virt_VSSD.c Mon May 11 16:47:15 2009 -0300 +++ b/src/Virt_VSSD.c Wed May 13 09:33:45 2009 -0300 @@ -42,16 +42,50 @@ CMPIInstance *inst) { bool fv = true; + CMPIArray *array;
if (dominfo->type == DOMAIN_XENFV) CMSetProperty(inst, "IsFullVirt", (CMPIValue *)&fv, CMPI_boolean);
- if (dominfo->os_info.fv.boot != NULL) - CMSetProperty(inst, - "BootDevice", - (CMPIValue *)dominfo->os_info.fv.boot, - CMPI_chars); + if (dominfo->os_info.fv.bootlist_size > 0) { + CMPICount i; + CMPICount bl_size; + CMPIStatus s; + + bl_size = (CMPICount)dominfo->os_info.fv.bootlist_size; + + array = CMNewArray(_BROKER, + bl_size, + CMPI_string, + &s); + + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error creating BootDevice list");
You should return here - if you fail to create the array, you can't add elements to it.
+ + for (i = 0; i < bl_size; i++) { + CMPIString *cm_str; + + cm_str = CMNewString(_BROKER, + (const char *)dominfo->os_info.fv.bootlist[i], + &s);
You need to set the elements of the array. See Virt_VirtualSystemManagementCapabilities.c (or other providers that need to set an array).
Yes, I guess that would eliminate my segfault problem. :-( Ok. I just don't believe I actually forgot to set the elements of the array. I think YOU somehow changed my code to make me look silly! And I'll prove sending the original co... nevermind.
+ if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error adding item to BootDevice " + "list");
The debug message here is misleading - the call to CMNewString() doesn't add the string to the array. You'll need to call CMSetArrayElementAt() for that.
Also, if you encounter an error here, you need to return an error.
+ } + + s = CMSetProperty(inst, + "BootDevices", + (CMPIValue *)array,
This needs to be &array.
Uh?! CMNewArray returns a pointer to CMPIArray.
+ CMPI_stringA); + + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error setting BootDevices property"); + + //CMSetProperty(inst, + // "BootDevices", + // (CMPIValue *)dominfo->os_info.fv.boot);
Remove commented out lines.
+ } }
static void _set_pv_prop(struct domain *dominfo, diff -r c0bd6c9a2c00 -r b188a6d5bfea src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c Mon May 11 16:47:15 2009 -0300 +++ b/src/Virt_VirtualSystemManagementService.c Wed May 13 09:33:45 2009 -0300 @@ -202,7 +202,13 @@ const char *pfx) { int ret; + CMPICount i; const char *val; + CMPIArray *bootlist; + CMPIStatus s; + CMPIData boot_elem; + char **tmp_str_arr; +
if (STREQC(pfx, "KVM")) { if (system_has_kvm(pfx)) @@ -216,12 +222,75 @@ return 0; }
- ret = cu_get_str_prop(inst, "BootDevice", &val); - if (ret != CMPI_RC_OK) - val = "hd"; + for (i = 0; i < domain->os_info.fv.bootlist_size; i++) + free(domain->os_info.fv.bootlist[i]);
- free(domain->os_info.fv.boot); - domain->os_info.fv.boot = strdup(val); + ret = cu_get_array_prop(inst, "BootDevices", &bootlist); + + if (ret == CMPI_RC_OK) { + CMPICount bl_size; + + bl_size = CMGetArrayCount(bootlist, &s); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Invalid BootDevice array size"); + return 0; + } + + tmp_str_arr = (char **)realloc(domain->os_info.fv.bootlist, + bl_size * sizeof(char *)); + + if (tmp_str_arr == NULL) { + CU_DEBUG("Could not alloc BootDevices array"); + return 0; + } + + for (i = 0; i < bl_size; i++) { + CMPIString *cmpi_str; + const char *str; + + boot_elem = CMGetArrayElementAt(bootlist, + i, + NULL); + + if (CMIsNullValue(boot_elem)) { + CU_DEBUG("Null BootDevice"); + return 0; + } + + cmpi_str = boot_elem.value.string;
You'll want to make sure that boot_elem isn't null. You can use CMIsNullObject().
Should I use both functions or only CMIsNullObject?
+ + free(domain->os_info.fv.bootlist[i]); + CU_DEBUG("Freed item from bootlist"); + + str = cmpi_str->ft->getCharPtr(cmpi_str, &s);
Instead of doing this, you can use CMGetCharPtr() to pull the string from the CMPIData object. If you use CMGetCharPtr(), you won't need the cmpi_str variable.
Oh, CMGetCharPtr is a macro! Nice!
+ + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Could not extract char pointer from " + "CMPIArray");
You'll want to return an error here.
+ } + + tmp_str_arr[i] = strdup(str); + } + domain->os_info.fv.bootlist_size = bl_size; + domain->os_info.fv.bootlist = tmp_str_arr; + + } else { + + CU_DEBUG("Failed to get BootDevices property"); + + tmp_str_arr = (char **)realloc(domain->os_info.fv.bootlist, + sizeof(char *)); + if (tmp_str_arr == NULL) + return 0; + + tmp_str_arr[0] = strdup("hd"); + + domain->os_info.fv.bootlist = tmp_str_arr; + domain->os_info.fv.bootlist_size = 1;
In xmlgen, you already set a default boot device. So we probably only need to do this once. You can remove this or remove the bit in xmlgen.
Does it always execute the code in xmlgen after this function?
+ }
This whole block is quite large - I would make the boot order bits a separate function.
+ + //free(domain->os_info.fv.boot); + //domain->os_info.fv.boot = strdup(val);
Remove commented lines.
ret = cu_get_str_prop(inst, "Emulator", &val); if (ret != CMPI_RC_OK)
-- Richard Maciel, MSc IBM Linux Technology Center rmaciel@linux.vnet.ibm.com
- else if (XSTREQ(child->name, "init")) + else if (XSTREQ(child->name, "boot")) { + //dominfo->os_info.fv.boot = get_attr_value(child, + //"dev"); + bl_size++; + + tmp_list = (char **)realloc(blist, + bl_size * sizeof(char *));
tmp_list isn't needed here - you aren't using it anywhere else. Just assign the the realloc back to blist.
Well, if realloc returns a null pointer I lose my pointer to the array which makes me deliver a NULL-pointer array with non-zero size.
Fair point. You'll want to be sure to use tmp_list when you do the assignment.
+ if (tmp_list == NULL) { + // Nothing you can do. Just go on. + CU_DEBUG("Could not alloc space for " + "boot device"); + bl_size--;
Instead of incrementing prior to the realloc(), just call realloc() with (bl_size + 1). If the realloc() is successful, increment bl_size after the assignment below.
+ continue; + } + + blist[bl_size - 1] = get_attr_value(child, "dev");
Instead of blist, you'd need to use tmp_list here.
So you would increment bl_size here.
Leave my comments alone!
+ if (os->bootlist_size == 0) { + os->bootlist_size = 1; + os->bootlist = (char **)calloc(1, sizeof(char *)); + os->bootlist[0] = strdup("hd"); + }
libvirt will set a default for us, but it's good to add something just in case.
I don't get it. How can libvirt set a default? Will it automagically provide an array with a default value?
libvirt will add a boot tag to the guest XML when the guest is defined if one isn't supplied. I haven't verified in the Xen case, but it definitely does this for KVM.
+ bl_size = (CMPICount)dominfo->os_info.fv.bootlist_size; + + array = CMNewArray(_BROKER, + bl_size, + CMPI_string, + &s); + + if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error creating BootDevice list");
You should return here - if you fail to create the array, you can't add elements to it.
+ + for (i = 0; i < bl_size; i++) { + CMPIString *cm_str; + + cm_str = CMNewString(_BROKER, + (const char *)dominfo->os_info.fv.bootlist[i], + &s);
You need to set the elements of the array. See Virt_VirtualSystemManagementCapabilities.c (or other providers that need to set an array).
Yes, I guess that would eliminate my segfault problem. :-( Ok. I just don't believe I actually forgot to set the elements of the array. I think YOU somehow changed my code to make me look silly! And I'll prove sending the original co... nevermind.
=)
+ if (s.rc != CMPI_RC_OK) + CU_DEBUG("Error adding item to BootDevice " + "list");
The debug message here is misleading - the call to CMNewString() doesn't add the string to the array. You'll need to call CMSetArrayElementAt() for that.
Also, if you encounter an error here, you need to return an error.
+ } + + s = CMSetProperty(inst, + "BootDevices", + (CMPIValue *)array,
This needs to be &array.
Uh?! CMNewArray returns a pointer to CMPIArray.
Right. But then you are casting the CMPIArray pointer as a CMPIValue pointer.
+ if (tmp_str_arr == NULL) { + CU_DEBUG("Could not alloc BootDevices array"); + return 0; + } + + for (i = 0; i < bl_size; i++) { + CMPIString *cmpi_str; + const char *str; + + boot_elem = CMGetArrayElementAt(bootlist, + i, + NULL); + + if (CMIsNullValue(boot_elem)) { + CU_DEBUG("Null BootDevice"); + return 0; + } + + cmpi_str = boot_elem.value.string;
You'll want to make sure that boot_elem isn't null. You can use CMIsNullObject().
Should I use both functions or only CMIsNullObject?
Oh! I missed the call to CMIsNullValue(). That check should be fine.
+ + CU_DEBUG("Failed to get BootDevices property"); + + tmp_str_arr = (char **)realloc(domain->os_info.fv.bootlist, + sizeof(char *)); + if (tmp_str_arr == NULL) + return 0; + + tmp_str_arr[0] = strdup("hd"); + + domain->os_info.fv.bootlist = tmp_str_arr; + domain->os_info.fv.bootlist_size = 1;
In xmlgen, you already set a default boot device. So we probably only need to do this once. You can remove this or remove the bit in xmlgen.
Does it always execute the code in xmlgen after this function?
Yes. Here's the general flow: 1) We pull the data from the CIM attributes and store them in the domain struct. 2) VSMS eventually calls system_to_xml() passing in the domain struct 3) xmlgen generates an XML for the guest based on the info in the domain struct 4) VSMS calls virDomainDefineXML() and passed in the XML we've generated -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
participants (2)
-
Kaitlin Rupert -
Richard Maciel