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