Re: [libvirt] [PATCHv2] PHYP: Adding network interface management
On 11/22/2010 06:03 PM, Eduardo Otubo wrote:
Apologies for my review backlog (if you haven't guessed, I sometimes
table big patches for later, then forget to come back rapidly).
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:
* MAC size and interface name are fixed due to specifications on HMC,
both are created automatically and CAN'T be specified from user. They
have the following format:
* MAC: 122980003002
HMC represents MAC as a decimal number? How hideous, but not your
fault; and this comment in the commit message will be useful for anyone
auditing the code in the future.
+ /* Getting the remote slot number */
+
+ char_ptr = NULL;
+ char_ptr = strchr(iface->mac, '\n');
Redundant assignments (delete the first line).
+ if (VIR_ALLOC_N(name, PHYP_IFACENAME_SIZE) < 0) {
+ virReportOOMError();
+ goto err;
+ }
+
+ if (VIR_ALLOC_N(mac, PHYP_MAC_SIZE) < 0) {
+ virReportOOMError();
+ goto err;
+ }
Since these arrays are so small, it's faster to just stack-allocate them:
char name[PHYP_IFACENAME_SIZE];
+ /* The next free slot itself: */
+ slot++;
+
+ /* Now addig the new network interface */
s/addig/adding/
+ if (exit_status < 0 || ret == NULL)
+ goto err;
+
+ char_ptr = NULL;
+ char_ptr = strchr(ret, '\n');
Another redundant assignment.
+ if (memcpy(mac, ret, PHYP_IFACENAME_SIZE) == NULL)
+ goto err;
+
+ VIR_FREE(cmd);
+ VIR_FREE(ret);
+ return virGetInterface(conn, name, mac);
+
+ err:
+ VIR_FREE(name);
+ VIR_FREE(cmd);
+ VIR_FREE(ret);
+ return NULL;
This leaks name if you end up calling virGetInterface. And both paths
leak mac. But if you change name and mac to be stack-allocated, then
you don't have to worry about freeing them.
+ ret = phypExec(session, cmd, &exit_status, conn);
+
+ if (exit_status < 0 || ret == NULL)
+ goto err;
+
+ VIR_FREE(cmd);
+ return virGetInterface(conn, name, ret);
+
+ err:
+ VIR_FREE(cmd);
+ VIR_FREE(ret);
+ return NULL;
This leaks ret if virGetInterface is called.
+ ret = phypExec(session, cmd, &exit_status, iface->conn);
+
+ if (exit_status < 0 || ret == NULL)
+ goto err;
+
+ if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1)
+ goto err;
+
+ if (char_ptr)
+ *char_ptr = '\0';
+
+ VIR_FREE(cmd);
+ return state;
Another leak of ret.
+static int
+phypListInterfaces(virConnectPtr conn, char **const names, int nnames)
s/int/size_t/
+ ret = phypExec(session, cmd, &exit_status, conn);
+
+ /* I need to parse the textual return in order to get the network interfaces */
+ if (exit_status < 0 || ret == NULL)
+ goto err;
+ else {
Rather than indenting the rest of the function inside an else{} block,
you can leave the code at the top level indentation since the if() block
was an unconditional goto.
+ ret = phypExec(session, cmd, &exit_status, conn);
+
+ if (exit_status < 0 || ret == NULL)
+ goto err;
+
+ if (virStrToLong_i(ret, &char_ptr, 10, &nnets) == -1)
+ goto err;
+
+ if (char_ptr)
+ *char_ptr = '\0';
+
+ VIR_FREE(cmd);
+ return nnets;
Another leak of ret.
int
@@ -4117,7 +4651,7 @@ phypRegister(void)
return -1;
if (virRegisterStorageDriver(&phypStorageDriver) < 0)
return -1;
- if (virRegisterNetworkDriver(&phypNetworkDriver) < 0)
+ if (virRegisterInterfaceDriver(&phypInterfaceDriver) < 0)
Are you intending to replace NetworkDriver with InterfaceDriver, or
should you be supporting both drivers simultaneously (although this may
be more an indication of how unfamiliar I am with the difference between
what the two drivers are supposed to provide).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)