6:07 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org