[libvirt] [PATCH] create() and destroy() support for Power Hypervisor
Matthias Bolte
matthias.bolte at googlemail.com
Tue Oct 20 00:08:44 UTC 2009
2009/10/6 Eduardo Otubo <otubo at 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
More information about the libvir-list
mailing list