2:07 p.m.
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(a)linux.vnet.ibm.com