[Libvirt-cim] [PATCH 2 of 2] Add boot order support
Kaitlin Rupert
kaitlin at linux.vnet.ibm.com
Wed May 13 17:15:57 UTC 2009
Richard Maciel wrote:
> # HG changeset patch
> # User Richard Maciel <rmaciel at 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 at linux.vnet.ibm.com
More information about the Libvirt-cim
mailing list