Kaitlin Rupert wrote:
Richard Maciel wrote:
> # HG changeset patch
> # User Richard Maciel <rmaciel(a)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(a)linux.vnet.ibm.com