[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



2009/10/30 Eduardo Otubo <otubo linux vnet ibm com>:
> 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.

Okay.

>>> +
>>> +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.

Okay.

>>> +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
>

> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index ef465ed..5b5cb3c 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
[...]
> @@ -39,6 +40,9 @@
>  #include <arpa/inet.h>
>  #include <sys/socket.h>
>  #include <netdb.h>
> +#include <fcntl.h>
> +#include <sys/utsname.h>
> +#include <conf/domain_event.h>

Better include this as #include "domain_event.h".

>  #include "internal.h"
>  #include "util.h"
> @@ -51,15 +55,12 @@
>  #include "virterror_internal.h"
>  #include "uuid.h"
>  #include "domain_conf.h"
> +#include "nodeinfo.h"
>
>  #include "phyp_driver.h"
>
>  #define VIR_FROM_THIS VIR_FROM_PHYP
>
> -#define PHYP_CMD_DEBUG VIR_DEBUG("COMMAND:%s\n",cmd);
> -
> -static int escape_specialcharacters(char *src, char *dst, size_t dstlen);
> -
>  /*
>   * URI: phyp://user [hmc|ivm]/managed_system
>   * */
> @@ -72,8 +73,23 @@ phypOpen(virConnectPtr conn,
>      ConnectionData *connection_data = NULL;
>      char *string;
>      size_t len = 0;
> -    uuid_dbPtr uuid_db = NULL;
>      int internal_socket;
> +    uuid_tablePtr uuid_table = NULL;
> +    phyp_driverPtr phyp_driver = NULL;
> +    char *char_ptr;
> +    char *managed_system = conn->uri->path;
> +
> +    /* need to shift one byte in order to remove the first "/" of URI component */
> +    if (managed_system[0] == '/')
> +        managed_system++;
> +
> +    /* here we are handling only the first component of the path,
> +     * so skipping the second:
> +     * */
> +    char_ptr = strchr(managed_system, '/');
> +
> +    if (char_ptr)
> +        *char_ptr = '\0';

You're accessing conn->uri->path without checking if it's NULL. Also
you're altering the original conn->uri->path and some lines below you
pass it to escape_specialcharacters() but don't actually use the
string variable anymore. Is escape_specialcharacters() and the call to
it just unused old code now?

In addition you should work with a copy of conn->uri->path instead of
altering it directly. If you change this don't forget to free the copy
in phypClose().

>      if (!conn || !conn->uri)
>          return VIR_DRV_OPEN_DECLINED;
> @@ -95,7 +111,12 @@ phypOpen(virConnectPtr conn,
>          return VIR_DRV_OPEN_ERROR;
>      }
>
> -    if (VIR_ALLOC(uuid_db) < 0) {
> +    if (VIR_ALLOC(phyp_driver) < 0) {
> +        virReportOOMError(conn);
> +        goto failure;
> +    }
> +
> +    if (VIR_ALLOC(uuid_table) < 0) {
>          virReportOOMError(conn);
>          goto failure;
>      }
> @@ -129,17 +150,29 @@ phypOpen(virConnectPtr conn,
>      connection_data->session = session;
>      connection_data->auth = auth;
>
> -    uuid_db->nlpars = 0;
> -    uuid_db->lpars = NULL;
> +    uuid_table->nlpars = 0;
> +    uuid_table->lpars = NULL;
>
> -    conn->privateData = uuid_db;
> +    phyp_driver->managed_system = managed_system;
> +    phyp_driver->uuid_table = uuid_table;
> +    if ((phyp_driver->caps = phypCapsInit()) == NULL) {
> +        virReportOOMError(conn);
> +        goto failure;
> +    }
> +
> +    conn->privateData = phyp_driver;
>      conn->networkPrivateData = connection_data;
> -    init_uuid_db(conn);
> +    if (phypUUIDTable_Init(conn) == -1)
> +        goto failure;
> +
> +    if ((phyp_driver->vios_id = phypGetVIOSPartitionID(conn)) == -1)
> +        goto failure;
>
>      return VIR_DRV_OPEN_SUCCESS;
>
>    failure:
> -    VIR_FREE(uuid_db);
> +    VIR_FREE(uuid_table);
> +    VIR_FREE(uuid_table->lpars);
>      VIR_FREE(connection_data);
>      VIR_FREE(string);

You should free phyp_driver and phyp_driver->caps here too.

> @@ -150,11 +183,14 @@ static int
>  phypClose(virConnectPtr conn)
>  {
>      ConnectionData *connection_data = conn->networkPrivateData;
> +    phyp_driverPtr phyp_driver = conn->privateData;
>      LIBSSH2_SESSION *session = connection_data->session;
>
>      libssh2_session_disconnect(session, "Disconnecting...");
>      libssh2_session_free(session);
>
> +    virCapabilitiesFree(phyp_driver->caps);
> +    VIR_FREE(phyp_driver);
>      VIR_FREE(connection_data);
>      return 0;
>  }

You should free phyp_driver->uuid_table and
phyp_driver->uuid_table->lpars here too.

[...]
> @@ -1306,6 +1299,215 @@ phypDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
>          VIR_WARN("%s", "Unable to determine domain's CPU.");
>
>      return 0;
> +}
> +
> +static int
> +phypDomainDestroy(virDomainPtr dom)
> +{
> +    ConnectionData *connection_data = dom->conn->networkPrivateData;
> +    phyp_driverPtr phyp_driver = dom->conn->privateData;
> +    LIBSSH2_SESSION *session = connection_data->session;
> +    char *managed_system = phyp_driver->managed_system;
> +    int exit_status = 0;
> +    char *cmd;
> +
> +    if (virAsprintf
> +        (&cmd,
> +         "rmsyscfg -m %s -r lpar --id %d", managed_system, dom->id) < 0) {
> +        virReportOOMError(dom->conn);
> +        goto err;
> +    }
> +
> +    char *ret = phypExec(session, cmd, &exit_status, dom->conn);
> +
> +    if (exit_status < 0)
> +        goto err;
> +
> +    if (phypUUIDTable_RemLpar(dom->conn, dom->id) == -1)
> +        goto err;
> +
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return 0;
> +
> +  err:
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return 0;
> +
> +}

You're returning 0 in both cases, you should return -1 in the error case.

> +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.

> +    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.

Matthias


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]