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

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