2009/10/30 Eduardo Otubo <otubo@xxxxxxxxxxxxxxxxxx>:
> Hello Matthias,
>
> First of all, I would like to thank you for all the comments you made. I
> tried to fix all those things up. But I still have some points to say:
>
> Matthias Bolte wrote:
> >> +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.
>
> Yep, you're right. I really need to think a better way to gather remote
> information about hypervisor. I missunderstood this concept, but I surely
> will have a fix for next the patch.
Okay.
>>> +
>>> +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.
>
> I agree with you. But this was the fastest way I found to get this part
> functional. I'll come up with a better solution in the next patch.
Okay.
>>> +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().
>
> This also applies to above comment, I'll come up with a better way to handle
> this.
>
> Change log:
> * Now we have CPU management! Now I assume the user will know how to
> operate a HMC/IVM IBM system. This saves a lot of (useless) coding time for
> me :)
> * I solved all the other issues Matthias pointed.
>
> Next steps:
> * Find a better way to handle the uuid_table files without the need of use
> local temp files.
> * Storage management.
>
> Any comments are always welcome. The comments made for this patch will be
> fixed in time for 0.7.3 version release.
>
> []'s
>
> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index ef465ed..5b5cb3c 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
[...]
> @@ -39,6 +40,9 @@
> #include <arpa/inet.h>
> #include <sys/socket.h>
> #include <netdb.h>
> +#include <fcntl.h>
> +#include <sys/utsname.h>
> +#include <conf/domain_event.h>
Better include this as #include "domain_event.h".
> #include "internal.h"
> #include "util.h"
> @@ -51,15 +55,12 @@
> #include "virterror_internal.h"
> #include "uuid.h"
> #include "domain_conf.h"
> +#include "nodeinfo.h"
>
> #include "phyp_driver.h"
>
> #define VIR_FROM_THIS VIR_FROM_PHYP
>
> -#define PHYP_CMD_DEBUG VIR_DEBUG("COMMAND:%s\n",cmd);
> -
> -static int escape_specialcharacters(char *src, char *dst, size_t dstlen);
> -
> /*
> * URI: phyp://user@[hmc|ivm]/managed_system
> * */
> @@ -72,8 +73,23 @@ phypOpen(virConnectPtr conn,
> ConnectionData *connection_data = NULL;
> char *string;
> size_t len = 0;
> - uuid_dbPtr uuid_db = NULL;
> int internal_socket;
> + uuid_tablePtr uuid_table = NULL;
> + phyp_driverPtr phyp_driver = NULL;
> + char *char_ptr;
> + char *managed_system = conn->uri->path;
> +
> + /* 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';
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().
> if (!conn || !conn->uri)
> return VIR_DRV_OPEN_DECLINED;
> @@ -95,7 +111,12 @@ phypOpen(virConnectPtr conn,
> return VIR_DRV_OPEN_ERROR;
> }
>
> - if (VIR_ALLOC(uuid_db) < 0) {
> + if (VIR_ALLOC(phyp_driver) < 0) {
> + virReportOOMError(conn);
> + goto failure;
> + }
> +
> + if (VIR_ALLOC(uuid_table) < 0) {
> virReportOOMError(conn);
> goto failure;
> }
> @@ -129,17 +150,29 @@ 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;
>
> - conn->privateData = uuid_db;
> + phyp_driver->managed_system = managed_system;
> + phyp_driver->uuid_table = uuid_table;
> + if ((phyp_driver->caps = phypCapsInit()) == NULL) {
> + virReportOOMError(conn);
> + goto failure;
> + }
> +
> + 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);
> + VIR_FREE(uuid_table->lpars);
> VIR_FREE(connection_data);
> VIR_FREE(string);
You should free phyp_driver and phyp_driver->caps here too.
> @@ -150,11 +183,14 @@ static int
> phypClose(virConnectPtr conn)
> {
> ConnectionData *connection_data = conn->networkPrivateData;
> + phyp_driverPtr phyp_driver = conn->privateData;
> LIBSSH2_SESSION *session = connection_data->session;
>
> libssh2_session_disconnect(session, "Disconnecting...");
> libssh2_session_free(session);
>
> + virCapabilitiesFree(phyp_driver->caps);
> + VIR_FREE(phyp_driver);
> VIR_FREE(connection_data);
> return 0;
> }
You should free phyp_driver->uuid_table and
phyp_driver->uuid_table->lpars here too.
[...]
> @@ -1306,6 +1299,215 @@ phypDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
> VIR_WARN("%s", "Unable to determine domain's CPU.");
>
> return 0;
> +}
> +
> +static int
> +phypDomainDestroy(virDomainPtr dom)
> +{
> + ConnectionData *connection_data = dom->conn->networkPrivateData;
> + phyp_driverPtr phyp_driver = dom->conn->privateData;
> + LIBSSH2_SESSION *session = connection_data->session;
> + char *managed_system = phyp_driver->managed_system;
> + int exit_status = 0;
> + char *cmd;
> +
> + if (virAsprintf
> + (&cmd,
> + "rmsyscfg -m %s -r lpar --id %d", managed_system, dom->id) < 0) {
> + virReportOOMError(dom->conn);
> + goto err;
> + }
> +
> + char *ret = phypExec(session, cmd, &exit_status, dom->conn);
> +
> + if (exit_status < 0)
> + goto err;
> +
> + if (phypUUIDTable_RemLpar(dom->conn, dom->id) == -1)
> + goto err;
> +
> + VIR_FREE(cmd);
> + VIR_FREE(ret);
> + return 0;
> +
> + err:
> + VIR_FREE(cmd);
> + VIR_FREE(ret);
> + return 0;
> +
> +}
You're returning 0 in both cases, you should return -1 in the error case.
> +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.
Matthias
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list