[Libvirt-cim] [PATCH 2 of 2] Add boot order support

[Kaitlin Rupert]

>>> -                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 at linux.vnet.ibm.com