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

[Eduardo Otubo]

New patch, more fixes.

Matthias Bolte wrote:
>> +static virDomainPtr
>> +phypDomainCreateAndStart(virConnectPtr conn,
>> +                         const char *xml,
>> +                         unsigned int flags ATTRIBUTE_UNUSED)
>> +{
>> +
>> +    ConnectionData *connection_data = conn->networkPrivateData;
>> +    LIBSSH2_SESSION *session = connection_data->session;
>> +    virDomainDefPtr def = NULL;
>> +    virDomainPtr dom = NULL;
>> +    phyp_driverPtr phyp_driver = conn->privateData;
>> +    uuid_tablePtr uuid_table = phyp_driver->uuid_table;
>> +    lparPtr *lpars = uuid_table->lpars;
>> +    unsigned int i = 0;
>> +    char *managed_system = phyp_driver->managed_system;
>> +
>> +    if (!(def = virDomainDefParseString(conn, phyp_driver->caps, xml,
>> +                                        VIR_DOMAIN_XML_INACTIVE)))
>> +        goto err;
>> +
>> +    /* checking if this name already exists on this system */
>> +    if (phypGetLparID(session, managed_system, def->name, conn) == -1) {
>> +        VIR_WARN("%s", "LPAR name already exists.");
>> +        goto err;
>> +    }
>> +
>> +    /* checking if ID or UUID already exists on this system */
>> +    for (i = 0; i < uuid_table->nlpars; i++) {
>> +        if (lpars[i]->id == def->id || lpars[i]->uuid == def->uuid) {
>> +            VIR_WARN("%s", "LPAR ID or UUID already exists.");
>> +            goto err;
>> +        }
>> +    }
> 
> def->id is always -1 here because you're parsing the domain XML with
> the VIR_DOMAIN_XML_INACTIVE flag set.
> 
>> +    dom->conn = conn;
>> +    dom->name = def->name;
>> +    dom->id = def->id;
>> +    memmove(dom->uuid, def->uuid, VIR_UUID_BUFLEN);
> 
> You're accessing a NULL pointer here. Just remove this 4 lines of
> code, because you're setting dom a line below anyway.
> 
>> +    if ((dom = virGetDomain(conn, def->name, def->uuid)) == NULL)
>> +        goto err;
> 
> def->id is -1 here. As I understand phypBuildLpar() it calls mksyscfg
> to actually define a new LPAR with a given ID. You use def->id for
> this. You're either defining all new LPARs with ID -1 or I
> misunderstand how this method is working.

I was thinking I could just avoid handling the ID at all here, HMC does 
it by itself. So, skiping ID and just dealing with the LPAR name.

> 
>> +    if (phypBuildLpar(conn, def) == -1)
>> +        goto err;
>> +
>> +    if (phypDomainResume(dom) == -1)
>> +        goto err;
>> +
>> +    return dom;
>> +
>> +  err:
>> +    virDomainDefFree(def);
>> +    VIR_FREE(dom);
>> +    return NULL;
>> +}
>> +
> 
> [...]
>> -void
>> -init_uuid_db(virConnectPtr conn)
>> +int
>> +phypUUIDTable_WriteFile(virConnectPtr conn)
>>  {
>> -    uuid_dbPtr uuid_db;
>> +    phyp_driverPtr phyp_driver = conn->privateData;
>> +    uuid_tablePtr uuid_table = phyp_driver->uuid_table;
>> +    unsigned int i = 0;
>> +    int fd = -1;
>> +    char local_file[] = "./uuid_table";
>> +
>> +    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_ERROR("%s", "Unable to write information to local file.");
>> +
>> +        if (write(fd, uuid_table->lpars[i]->uuid, VIR_UUID_BUFLEN) == -1)
>> +            VIR_ERROR("%s", "Unable to write information to local file.");
>> +    }
> 
> You should goto err if a write fails, because a single failed write
> will corrupt the while table.
> 
> You should instead check for
> 
> write(...) != sizeof(uuid_table->lpars[i]->id) and
> write(...) != VIR_UUID_BUFLEN
> 
> because write() may write less bytes than requested.
> 
>> +    close(fd);
>> +    return 0;
>> +
>> +  err:
>> +    close(fd);
>> +    return -1;
>> +}
>> +
> 
> You fixed most issues in this version of the patch, but some memory
> leaks are still there.
> 

All the other things are fixed.
Thanks for all the comments.
[]'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: 51444 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091103/2d5a38e3/attachment-0001.bin>