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