On 12/09/2010 07:16 PM, Eric Blake wrote:
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).
No problem, I'll try to post earlier next time. Thanks anyway.
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.
Yeah, no way I understand this notation either. Keeping all the commit
comments on the PATCHv3 I'll send right away.
+ /* Getting the remote slot number */
+
+ char_ptr = NULL;
+ char_ptr = strchr(iface->mac, '\n');
Redundant assignments (delete the first line).
ACK, fixed.
+ 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];
ACK, fixed.
+ /* The next free slot itself: */
+ slot++;
+
+ /* Now addig the new network interface */
s/addig/adding/
ACK, fixed.
+ if (exit_status< 0 || ret == NULL)
+ goto err;
+
+ char_ptr = NULL;
+ char_ptr = strchr(ret, '\n');
Another redundant assignment.
ACK, fixed.
+ 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.
ACK, fixed.
+ 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.
ACK, fixed.
+ 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.
ACK, fixed.
+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.
ACK, fixed.
+ 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.
ACK, fixed.
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).
First option, I'll replace NetworkDriver with InterfaceDriver. I didn't
know that there were an Interface API, my deep fault on this matter.
As I said, I will keep all the commit comments for further needs.
Sending now the PATCHv3.
Thanks for the review.
Regards,
--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo@xxxxxxxxxxxxxxxxxx
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list