2009/10/6 Eduardo Otubo <otubo@linux.vnet.ibm.com>:

Hello all,

This patch includes a lot of changes:

* I changed all references of uuid_db to uuid_table. Bigger name, but this semantic has a better understanding.

* Now we have a little control of UUID generated for each partition. It's based on a table that matches UUID with LPAR's IDs, I keep it in a file that is stored in the managed system (HMC or IVM). Even having isolated functions to manipulate this file I still need to implement a way to lock it and make the operation atomic to avoid corruptions.

* The XML file used in the create() function still must be improved. I used the <disk> tag to make a work around to handle storage pools. Now I ask for some help, how do I use the <pool> tag if there is no reference to it at the virDomainDef structure?

I've been told that libvirt possibly would make a mini-release this week to push some major fixes on Fedora 12. Is there a little possibility to push these new phyp features too? This would be very important for the phyp project, since this functions are not just to manage partitions but manipulate them.

Any comments are always welcome. []'s

Here you go!

@@ -129,17 +136,25 @@ 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; + + phyp_driver->uuid_table = uuid_table; + if ((phyp_driver->caps = phypCapsInit()) == NULL)

You're missing a virReportOOMError here, or report an error in phypCapsInit() if returning NULL from it. You've also forgotten to free the caps in phypClose() via virCapabilitiesFree() and phyp_driver via VIR_FREE.

+ goto failure;

- conn->privateData = uuid_db; + 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);

You're potentially leaking uuid_table->lpars here.

VIR_FREE(connection_data); VIR_FREE(string);

+int phypDiskType(virConnectPtr conn, char *backing_device) { + phyp_driverPtr phyp_driver = conn->privateData; ConnectionData *connection_data = conn->networkPrivateData; LIBSSH2_SESSION *session = connection_data->session; char *cmd; int exit_status = 0; + char *char_ptr; + char *managed_system = conn->uri->path; + int vios_id = phyp_driver->vios_id; + + /* 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';

May be prepare the managed_system name once in the phypOpen() function and store it in the phyp_driver struct, because you're using it in multiple functions.

@@ -1189,9 +1254,12 @@ phypDomainDumpXML(virDomainPtr dom, int flags)

ret = virDomainDefFormat(dom->conn, def, flags);

- err: VIR_FREE(def); return ret; + + err: + VIR_FREE(def);

Don't use VIR_FREE() with virDomainDefPtr. Use virDomainDefFree(def) instead.

+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 = conn->uri->path; + char *char_ptr = NULL; + + /* 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 (VIR_ALLOC(def) < 0) + virReportOOMError(conn);

Don't allocate def here because you're assigning the result of virDomainDefParseString() to it and are leaking the initial allocation.

+ if (VIR_ALLOC(dom) < 0) + virReportOOMError(conn);

Don't allocate dom here but use virGetDomain() later.

+ 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 may be -1 (undefined) if the domain XML doesn't contain an ID. You should generate one and assign it or read it back from the hypervisor after starting the domain.

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

Use virGetDomain() here: dom = virGetDomain(conn, def->name, def->uuid); if (!dom) goto err; dom->id = def->id;

+ if(phypBuildLpar(conn, def) == -1) + goto err; + + if(phypDomainResume(dom) == -1) + goto err; + + return dom; + + err: + VIR_FREE(def); + VIR_FREE(dom);

Don't use VIR_FREE() with virDomainDefPtr or virDomainPtr. Use virDomainDefFree(def) and virUnrefDomain(dom) instead.

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

int -phypRegister(void) +phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) { - virRegisterDriver(&phypDriver); + ConnectionData *connection_data = conn->networkPrivateData; + LIBSSH2_SESSION *session = connection_data->session; + char *managed_system = conn->uri->path; + char *char_ptr = NULL; + char *cmd; + int exit_status = 0; + + /* 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';

As noted before you should refactor the parsing of the path into the phypOpen() function and store the result in the phyp_driver struct.

+ if (virAsprintf + (&cmd, + "mksyscfg -m %s -r lpar --id %d -i name=%s,min_mem=%d,desired_mem=%d,\ + max_mem=%d,desired_procs=%d,virtual_scsi_adapters=%s",

Don't break the string like this, because it includes the indentation into the string. Close the string before the \ and start another string at the next line: "mksyscfg -m %s -r lpar --id %d -i name=%s,min_mem=%d,desired_mem=%d," \ "max_mem=%d,desired_procs=%d,virtual_scsi_adapters=%s"

+ managed_system, def->id, def->name, (int)def->memory, (int)def->memory, + (int)def->maxmem, (int) def->vcpus, def->disks[0]->src) < 0) { + virReportOOMError(conn); + goto err; + } + + char *ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0) { + VIR_ERROR("%s\"%s\"", "Unable to create LPAR. Reason: ", ret); + goto err; + } + + if(phypUUIDTable_AddLpar(conn, def->uuid, def->id) == -1){ + VIR_ERROR("%s", "Unable to add LPAR to the table"); + goto err; + } + + VIR_FREE(cmd); + VIR_FREE(ret); + return 0; + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return -1; +} +

-void -init_uuid_db(virConnectPtr conn) +int +phypUUIDTable_AddLpar(virConnectPtr conn, unsigned char *uuid, int id) { - uuid_dbPtr uuid_db; + phyp_driverPtr phyp_driver = conn->privateData; + uuid_tablePtr uuid_table = phyp_driver->uuid_table; + + uuid_table->nlpars++; + unsigned int i = uuid_table->nlpars; + i--; + + if (VIR_REALLOC_N(uuid_table->lpars, uuid_table->nlpars) < 0) { + virReportOOMError(conn); + goto err; + } + + if (VIR_ALLOC(uuid_table->lpars[i]) < 0) + virReportOOMError(conn);

You're missing a goto err here.

+ uuid_table->lpars[i]->id = id; + if (memmove(uuid_table->lpars[i]->uuid, uuid, VIR_UUID_BUFLEN) == NULL) + goto err; + + if(phypUUIDTable_WriteFile(conn) == -1) + goto err; + + if(phypUUIDTable_Push(conn) == -1) + goto err; + + return 0; + + err: + return -1; +} + +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.

+ if (!(fd = open(local_file, O_RDONLY))) {

0 is a valid file descriptor, open() returns -1 in case of an error.

+ VIR_WARN("%s", "Unable to write information to local file."); + goto err; + } + + /* Creating a new data base and writing to local file */ + if (VIR_ALLOC_N(uuid_table->lpars, uuid_table->nlpars) >= 0) { + for (i = 0; i < uuid_table->nlpars; i++) { + + rc = read(fd, buffer, sizeof(int)); + if (rc == sizeof(int)) { + if (VIR_ALLOC(uuid_table->lpars[i]) < 0) + virReportOOMError(conn); + uuid_table->lpars[i]->id = (*buffer); + } else { + VIR_WARN("%s", + "Unable to read from information to local file."); + goto err; + } + + rc = read(fd, uuid_table->lpars[i]->uuid, VIR_UUID_BUFLEN); + if (rc != VIR_UUID_BUFLEN) { + VIR_WARN("%s", + "Unable to read information to local file."); + goto err; + } + } + } + + close(fd); + goto exit; + + exit: + VIR_FREE(local_file); + return 0; + + err:

You're potentially leaking a file desrciptor here, add a close(fd).

+ VIR_FREE(local_file); + return -1; +} + +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().

+int +phypUUIDTable_Init(virConnectPtr conn) +{ + uuid_tablePtr uuid_table; + phyp_driverPtr phyp_driver; int nids = 0; int *ids = NULL; unsigned int i = 0;

if ((nids = phypNumDomainsGeneric(conn, 2)) == 0) - goto exit; + goto err;

if (VIR_ALLOC_N(ids, nids) < 0) virReportOOMError(conn);

If an allocation fails you should return from this function (e.g. be goto err), otherwise it'll segfault if you try to access the NULL pointer a few lines below.

- if (VIR_ALLOC(uuid_db) < 0) + if (VIR_ALLOC(phyp_driver) < 0) + virReportOOMError(conn); +

This allocation of phyp_driver is useless, because you assign conn->privateData to phyp_driver a few lines below and leak the allocated memory then.

+ if (VIR_ALLOC(uuid_table) < 0) virReportOOMError(conn);

Same for this allocation. You assign phyp_driver->uuid_table to uuid_table a few lines below and leak the allocated memory then.

if (phypListDomainsGeneric(conn, ids, nids, 1) == 0) - goto exit; + goto err; + + phyp_driver = conn->privateData; + uuid_table = phyp_driver->uuid_table; + uuid_table->nlpars = nids; + + /* try to get the table from server */ + if (phypUUIDTable_Pull(conn) == -1) { + /* file not found in the server, creating a new one */ + if (VIR_ALLOC_N(uuid_table->lpars, uuid_table->nlpars) >= 0) { + for (i = 0; i < uuid_table->nlpars; i++) { + char *id_c;

- uuid_db = conn->privateData; - uuid_db->nlpars = nids; + if (VIR_ALLOC(uuid_table->lpars[i]) < 0) + virReportOOMError(conn);

If an allocation fails you should return from this function (e.g. be goto err), otherwise it'll segfault if you try to access the NULL pointer.

+ uuid_table->lpars[i]->id = ids[i];

- if (VIR_ALLOC_N(uuid_db->lpars, uuid_db->nlpars) >= 0) { - for (i = 0; i < uuid_db->nlpars; i++) { - if (VIR_ALLOC(uuid_db->lpars[i]) < 0) - virReportOOMError(conn); - uuid_db->lpars[i]->id = ids[i]; + if (virUUIDGenerate(uuid_table->lpars[i]->uuid) < 0) + VIR_WARN("%s %d", "Unable to generate UUID for domain", + ids[i]);

- if (virUUIDGenerate(uuid_db->lpars[i]->uuid) < 0) - VIR_WARN("%s %d", "Unable to generate UUID for domain", - ids[i]); + VIR_FREE(id_c);

You're freeing an uninitialized char* here, this is bad. Just remove the definition and freeing of id_c, because you don't seem to use it anyway.

+ } } + if (phypUUIDTable_WriteFile(conn) == -1) + goto err; + + if (phypUUIDTable_Push(conn) == -1) + goto err; + } else { + if (phypUUIDTable_ReadFile(conn) == -1) + goto err; + goto exit; } + exit: VIR_FREE(ids); - return; + return 0; + + err: + VIR_FREE(ids); + return -1; }

-static int +int +phypUUIDTable_Push(virConnectPtr conn) +{ + ConnectionData *connection_data = conn->networkPrivateData; + LIBSSH2_SESSION *session = connection_data->session; + LIBSSH2_CHANNEL *channel = NULL; + struct stat local_fileinfo; + char buffer[1024]; + int rc = 0; + FILE *fd; + size_t nread, sent; + char *ptr; + char *remote_file; + char *local_file; + + if (virAsprintf(&remote_file, "/home/hscroot/uuid_table") < 0) {

May be name the file libvirt_uuid_table, to make its origin and purpose more obvious.

+ virReportOOMError(conn); + goto err; + } + + 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".

+ if (stat(local_file, &local_fileinfo) == -1) { + VIR_WARN("%s", "Unable to stat local file.");

Use VIR_WARN0("Unable to stat local file."); instead.

+ goto err; + } + + if (!(fd = fopen(local_file, "rb"))) { + VIR_WARN("%s", "Unable to open local file.");

Use VIR_WARN0("Unable to open local file."); instead.

+ goto err; + } + + do { + channel = + libssh2_scp_send(session, remote_file, + 0x1FF & local_fileinfo.st_mode, + (unsigned long) local_fileinfo.st_size); + + if ((!channel) && (libssh2_session_last_errno(session) != + LIBSSH2_ERROR_EAGAIN)) + goto err; + } while (!channel); + + do { + nread = fread(buffer, 1, sizeof(buffer), fd); + if (nread <= 0) { + /* end of file */ + break; + } + ptr = buffer; + sent = 0; + + do { + /* write the same data over and over, until error or completion */ + rc = libssh2_channel_write(channel, ptr, nread); + if (LIBSSH2_ERROR_EAGAIN == rc) { /* must loop around */ + continue; + } else { + /* rc indicates how many bytes were written this time */ + sent += rc; + }

I assume libssh2_channel_write() can return other error codes beside LIBSSH2_ERROR_EAGAIN and you'll just erroneously treat them as number of written bytes.

+ } while (rc > 0 && sent < nread); + ptr += sent; + nread -= sent; + } while (1); + + goto exit; + + exit: + if (channel) { + libssh2_channel_send_eof(channel); + libssh2_channel_wait_eof(channel); + libssh2_channel_wait_closed(channel); + libssh2_channel_free(channel); + channel = NULL; + } + return 0; + + err: + if (channel) { + libssh2_channel_send_eof(channel); + libssh2_channel_wait_eof(channel); + libssh2_channel_wait_closed(channel); + libssh2_channel_free(channel); + channel = NULL; + } + return -1; +} + +int +phypUUIDTable_Pull(virConnectPtr conn) +{ + ConnectionData *connection_data = conn->networkPrivateData; + LIBSSH2_SESSION *session = connection_data->session; + LIBSSH2_CHANNEL *channel = NULL; + struct stat fileinfo; + char buffer[1024]; + int rc = 0; + int fd; + int got = 0; + int amount = 0; + int spin = 0; + int total = 0; + int sock = 0; + char *remote_file; + char *local_file; + + if (virAsprintf(&remote_file, "/home/hscroot/uuid_table") < 0) { + virReportOOMError(conn); + goto err; + } + + 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".

+ /* Trying to stat the remote file. */ + do { + channel = libssh2_scp_recv(session, remote_file, &fileinfo); + + if (!channel) { + if (libssh2_session_last_errno(session) != + LIBSSH2_ERROR_EAGAIN) { + goto err;; + } else { + waitsocket(sock, session); + } + } + } while (!channel); + + /* Creating a new data base based on remote file */ + if ((fd = creat(local_file, 0755)) == -1) + goto err; + + /* Request a file via SCP */ + while (got < fileinfo.st_size) { + do { + amount = sizeof(buffer); + + if ((fileinfo.st_size - got) < amount) { + amount = fileinfo.st_size - got; + } + + rc = libssh2_channel_read(channel, buffer, amount); + if (rc > 0) { + if (write(fd, buffer, rc) == -1) + VIR_WARN("%s", + "Unable to write information to local file."); + + got += rc; + total += rc; + } + } while (rc > 0); + + if ((rc == LIBSSH2_ERROR_EAGAIN) + && (got < fileinfo.st_size)) { + /* this is due to blocking that would occur otherwise + * so we loop on this condition */ + + spin++;

What's the purpose of spin?

+ waitsocket(sock, session); /* now we wait */ + continue; + } + break; + } + close(fd); + goto exit; + + exit: + if (channel) { + libssh2_channel_send_eof(channel); + libssh2_channel_wait_eof(channel); + libssh2_channel_wait_closed(channel); + libssh2_channel_free(channel); + channel = NULL; + } + return 0; + + err: + if (channel) { + libssh2_channel_send_eof(channel); + libssh2_channel_wait_eof(channel); + libssh2_channel_wait_closed(channel); + libssh2_channel_free(channel); + channel = NULL; + } + return -1; +}

diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h --- a/src/phyp/phyp_driver.h +++ b/src/phyp/phyp_driver.h

+/* This is the main structure of the driver + * */ +typedef struct _phyp_driver phyp_driver_t; +typedef phyp_driver_t *phyp_driverPtr; +struct _phyp_driver { + uuid_tablePtr uuid_table; + virCapsPtr caps; + int vios_id; +};

Whitespace errors (tabs) here, make syntax-check should have complained. Some things that could be improved: - The copy&pasted code for managed_system-from-path-parsing could be refactored and the result cached in the phyp_driver struct. - The UUID table push/pull could be reworked to operate without writing the content to a temporary file first. Matthias