2009/10/6 Eduardo Otubo <otubo(a)linux.vnet.ibm.com&gt;: Here you go! 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. You're potentially leaking uuid_table->lpars here. 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. Don't use VIR_FREE() with virDomainDefPtr. Use virDomainDefFree(def) instead. Don't allocate def here because you're assigning the result of virDomainDefParseString() to it and are leaking the initial allocation. Don't allocate dom here but use virGetDomain() later. 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. Use virGetDomain() here: dom = virGetDomain(conn, def->name, def->uuid); if (!dom) goto err; dom->id = def->id; Don't use VIR_FREE() with virDomainDefPtr or virDomainPtr. Use virDomainDefFree(def) and virUnrefDomain(dom) instead. 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. As noted before you should refactor the parsing of the path into the phypOpen() function and store the result in the phyp_driver struct. 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" You're missing a goto err here. 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. 0 is a valid file descriptor, open() returns -1 in case of an error. You're potentially leaking a file desrciptor here, add a close(fd). See comment in phypUUIDTable_ReadFile() about virAsprintf for a static string etc. 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. 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(). 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. 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. Same for this allocation. You assign phyp_driver->uuid_table to uuid_table a few lines below and leak the allocated memory then. 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. 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. May be name the file libvirt_uuid_table, to make its origin and purpose more obvious. virAsprintf for a fixed string is a bit of overkill, just use const char *local_file = "./uuid_table". Use VIR_WARN0("Unable to stat local file."); instead. Use VIR_WARN0("Unable to open local file."); instead. 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) { virAsprintf for a fixed string is a bit of overkill, just use const char *local_file = "./uuid_table". What's the purpose of spin? 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

2009/10/30 Eduardo Otubo <otubo(a)linux.vnet.ibm.com&gt;: Okay. Okay. [...] Better include this as #include "domain_event.h". 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(). You should free phyp_driver and phyp_driver->caps here too. You should free phyp_driver->uuid_table and phyp_driver->uuid_table->lpars here too. [...] You're returning 0 in both cases, you should return -1 in the error case. def->id is always -1 here because you're parsing the domain XML with the VIR_DOMAIN_XML_INACTIVE flag set. You're accessing a NULL pointer here. Just remove this 4 lines of code, because you're setting dom a line below anyway. 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. [...] 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. You fixed most issues in this version of the patch, but some memory leaks are still there. Matthias