On 12/29/2010 11:04 AM, Eduardo Otubo wrote:
This is the implementation of the previous patch now using virInterface* API. Ended up this patch got much more simpler, smaller and easier to review. Here is some details:
Finally getting around to reviewing this again. I'm not intentionally putting you off, it's just that I'm less familiar with phyp and it takes a good chunk of free time to review large patches in areas where I'm less familiar.
@@ -74,6 +75,11 @@
static unsigned const int HMC = 0; static unsigned const int IVM = 127; +static unsigned const int PHYP_IFACENAME_SIZE = 24; +static unsigned const int PHYP_MAC_SIZE= 12; + + +virCheckFlags(0,NULL);
This is misplaced, and causes a compile error. It should be one of the first statements (not declarations) in any function that takes a flags argument where you do not otherwise use flags, and not something done at the file scope.
static int waitsocket(int socket_fd, LIBSSH2_SESSION * session) @@ -1113,7 +1119,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth,
static virDrvOpenStatus phypOpen(virConnectPtr conn, - virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED) + virConnectAuthPtr auth, int flags) { LIBSSH2_SESSION *session = NULL; ConnectionData *connection_data = NULL;
That is, somewhere in this function. Seeing as how this causes a compile error: CC libvirt_driver_phyp_la-phyp_driver.lo phyp/phyp_driver.c:82:1: error: expected identifier or '(' before 'do' phyp/phyp_driver.c:82:290: error: expected identifier or '(' before 'while' cc1: warnings being treated as errors phyp/phyp_driver.c: In function 'phypOpen': phyp/phyp_driver.c:1122:38: error: unused parameter 'flags' [-Wunused-parameter] phyp/phyp_driver.c: In function 'phypInterfaceDestroy': how can I even be sure that you've tested your patch, to spend my time reviewing it?
+static virInterfacePtr +phypInterfaceDefineXML(virConnectPtr conn, const char *xml, + unsigned int flags) +{
+ /* Now adding the new network interface */ + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o a -s %d -a port_vlan_id=1," + "ieee_virtual_eth=0", def->name, slot);
So if this succeeds,
+ /* Need to sleep a little while to wait for the HMC to + * complete the execution of the command. + * */ + sleep(1); + + /* Getting the new interface name */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*drc_name=//'", def->name, slot);
but this fails, do you need to undo the reservation, or have you just leaked a resource?
+ + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + char_ptr = strchr(ret, '\n'); + + if (char_ptr) + *char_ptr = '\0'; + + if (memcpy(name, ret, PHYP_IFACENAME_SIZE-1) == NULL) + goto err;
Huh? memcpy() can never fail on valid input. No need for this if statement; just do the memcpy() and ignore the result (which will be name). Multiple times in this patch.
+ +static virInterfacePtr +phypInterfaceLookupByName(virConnectPtr conn, const char *name) +{
+ + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (memcpy(mac, ret, PHYP_MAC_SIZE-1) == NULL) + goto err; + + VIR_FREE(cmd); + return virGetInterface(conn, name, ret);
Memory leak; you never freed ret. You need to do something like: result = virGetInterface(conn, name, ret); VIR_FREE(ret); return result;
+static int +phypInterfaceIsActive(virInterfacePtr iface) +{
+ + if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0'; + + VIR_FREE(cmd); + VIR_FREE(ret); + return state;
The lines involving setting *char_ptr to '\0' are useless, and can safely be deleted. No need to NUL-terminate the integer portion of ret if you are just about to free it. Getting closer, but still not ready to merge in. Hopefully v5 will clinch it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org