[libvirt] [PATCH] create() and destroy() support for Power Hypervisor

[Eduardo Otubo]

Hello Matthias,

First of all, I would like to thank you for all the comments you made. I 
tried to fix all those things up. But I still have some points to say:

Matthias Bolte wrote:
  >> +virCapsPtr
>> +phypCapsInit(void)
>> +{
>> +    struct utsname utsname;
>> +    virCapsPtr caps;
>> +    virCapsGuestPtr guest;
>> +
>> +    uname(&utsname);
>> +
>> +    if ((caps = virCapabilitiesNew(utsname.machine, 0, 0)) == NULL)
>> +        goto no_memory;
>> +
>> +    /* Some machines have problematic NUMA toplogy causing
>> +     * unexpected failures. We don't want to break the QEMU
>> +     * driver in this scenario, so log errors & carry on
>> +     */
>> +    if (nodeCapsInitNUMA(caps) < 0) {
>> +        virCapabilitiesFreeNUMAInfo(caps);
>> +        VIR_WARN0
>> +            ("Failed to query host NUMA topology, disabling NUMA capabilities");
>> +    }
>> +
>> +    /* XXX shouldn't 'borrow' KVM's prefix */
>> +    virCapabilitiesSetMacPrefix(caps, (unsigned char[]) {
>> +                                0x52, 0x54, 0x00});
>> +
>> +    if ((guest = virCapabilitiesAddGuest(caps,
>> +                                         "linux",
>> +                                         utsname.machine,
>> +                                         sizeof(int) == 4 ? 32 : 8,
>> +                                         NULL, NULL, 0, NULL)) == NULL)
>> +        goto no_memory;
>> +
>> +    if (virCapabilitiesAddGuestDomain(guest,
>> +                                      "phyp", NULL, NULL, 0, NULL) == NULL)
>> +        goto no_memory;
>> +
>> +    return caps;
>> +
>> +  no_memory:
>> +    virCapabilitiesFree(caps);
>> +    return NULL;
>> +}
> 
> As I understand the mechanics of this driver, the driver is designed
> to operator from a client machine. The caps should describe the
> capabilities of the hypervisor machine, but this functions initializes
> the caps based on the machine where the driver is running.

Yep, you're right. I really need to think a better way to gather remote 
information about hypervisor. I missunderstood this concept, but I 
surely will have a fix for next the patch.

>> +
>> +int
>> +phypUUIDTable_ReadFile(virConnectPtr conn)
>> +{
>> +    phyp_driverPtr phyp_driver = conn->privateData;
>> +    uuid_tablePtr uuid_table = phyp_driver->uuid_table;
>> +    unsigned int i = 0;
>> +    int fd = -1;
>> +    char *local_file;
>> +    int rc = 0;
>> +    char buffer[1024];
>> +
>> +    if (virAsprintf(&local_file, "./uuid_table") < 0) {
>> +        virReportOOMError(conn);
>> +        goto err;
>> +    }
> 
> virAsprintf for a fixed string is a bit of overkill, just use const
> char *local_file = "./uuid_table".
> 
> In addition IMHO using a file in the current working directory for
> temporary purpose isn't a good idea. Use a temporary path returned by
> mktemp(), or even better try to solve the problem without using a
> temporary file.

I agree with you. But this was the fastest way I found to get this part 
functional. I'll come up with a better solution in the next patch.

>> +int
>> +phypUUIDTable_WriteFile(virConnectPtr conn)
>> +{
>> +    phyp_driverPtr phyp_driver = conn->privateData;
>> +    uuid_tablePtr uuid_table = phyp_driver->uuid_table;
>> +    unsigned int i = 0;
>> +    int fd = -1;
>> +    char *local_file;
>> +
>> +    if (virAsprintf(&local_file, "./uuid_table") < 0) {
>> +        virReportOOMError(conn);
>> +        goto err;
>> +    }
> 
> See comment in phypUUIDTable_ReadFile() about virAsprintf for a static
> string etc.
> 
>> +    if ((fd = creat(local_file, 0755)) == -1)
>> +        goto err;
>> +
>> +    for (i = 0; i < uuid_table->nlpars; i++) {
>> +        if (write(fd, &uuid_table->lpars[i]->id, sizeof(uuid_table->lpars[i]->id)) == -1)
>> +            VIR_WARN("%s", "Unable to write information to local file.");
>> +
>> +        if (write(fd, uuid_table->lpars[i]->uuid, VIR_UUID_BUFLEN) == -1)
>> +            VIR_WARN("%s", "Unable to write information to local file.");
> 
> Failed or incomplete writes shouldn't just warnings, they must be
> error, because they corrupt the file.
> 
> Your binary file format is based on offsets. At offset 0 the first ID
> is located, at offset 4 the first UUID, at offset 20 the next ID etc.
> If you miss to write the appropriate number of bytes per item you'll
> fail to read the data back in correctly.
> 
>> +    }
>> +    close(fd);
>> +    goto exit;
>> +
>> +  exit:
>> +    VIR_FREE(local_file);
>> +    return 0;
>> +
>> +  err:
>> +    VIR_FREE(local_file);
>> +    return -1;
>> +}
> 
> It seems you just write the UUID table to a file to read it back in
> again in phypUUIDTable_Push(). I would suggest to remove the detour
> using the temporary file, but instead calculate the total size of the
> data (uuid_table->nlpars * (sizeof (int) + VIR_UUID_BUFLEN)) for
> libssh2_scp_send() and write the data directly via
> libssh2_channel_write(). Basically merge phypUUIDTable_WriteFile()
> into phypUUIDTable_Push() and phypUUIDTable_ReadFile() into
> phypUUIDTable_Pull().

This also applies to above comment, I'll come up with a better way to 
handle this.

Change log:
  * Now we have CPU management! Now I assume the user will know how to 
operate a HMC/IVM IBM system. This saves a lot of (useless) coding time 
for me :)
  * I solved all the other issues Matthias pointed.

Next steps:
  * Find a better way to handle the uuid_table files without the need of 
use local temp files.
  * Storage management.

Any comments are always welcome. The comments made for this patch will 
be fixed in time for 0.7.3 version release.

[]'s

-- 
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo at linux.vnet.ibm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: phyp_driver.patch
Type: text/x-patch
Size: 50225 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091030/6db30b76/attachment-0001.bin>