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

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

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