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

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

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>

#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';

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);

@@ -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; }

@@ -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; + +}

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

+ dom->conn = conn; + dom->name = def->name; + dom->id = def->id; + memmove(dom->uuid, def->uuid, VIR_UUID_BUFLEN);

+ if ((dom = virGetDomain(conn, def->name, def->uuid)) == NULL) + goto err;

+ 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."); + }

+ close(fd); + return 0; + + err: + close(fd); + return -1; +} +