2:59 p.m.
I understand that sending a big giant patch is not the best way the get
things done. I split the patch into two parts: The first part I'll send
it right away, regarding ONLY the addition of the interface management
stuff. The second, that I'll send it later on, regards the regressions,
leaks and other non-related issues. Below I comment all the items I
fixed on this interface-management-only patch.
Regards,
> +
> + virBufferAddLit(&buf, "lshwres ");
> + if (system_type == HMC)
> + virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> + virBufferVSprintf(&buf,
> + " -r virtualio --rsubtype eth --level lpar "
> + " -F mac_addr,slot_num|"
> + " sed -n '/%s/ s/^.*,//p'", iface->mac);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + virReportOOMError();
> + goto err;
> + }
> + cmd = virBufferContentAndReset(&buf);
> +
> + ret = phypExec(session, cmd,&exit_status, iface->conn);
> +
> + if (exit_status< 0 || ret == NULL)
> + goto err;
> +
> + if (virStrToLong_i(ret,&char_ptr, 10,&slot_num) == -1)
...before overwriting it here.
As an idea for a separate cleanup patch, you reuse a particular idiom of
constructing a command in a virBuffer, then checking if that buffer
worked, then calling phypExec on the string conversion. Would it be
worth a refactoring that changes phypExec from taking a char* to instead
taking a virBuffer, to put the error checking in just one place and make
all other callers use less code?
Just a comment on this refactoring: I was already planning a great
refactoring on all phypExec execution flow. I'll send a patch only with
this topic.
...
> + virBufferVSprintf(&buf,
> + " -r virtualio --rsubtype eth"
> + " --id %d -o r -s %d", lpar_id, slot_num);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + virReportOOMError();
> + return -1;
> + }
> + cmd = virBufferContentAndReset(&buf);
> +
> + ret = phypExec(session, cmd,&exit_status, iface->conn);
> +
> + if (exit_status< 0 || ret != NULL)
> + goto err;
Normally, your idiom has been to declare error if ret == NULL, but here
you reversed the logic. Is this a command where you really expect a
NULL return, or does phypExec return "" when the command otherwise had
no output in which case your logic is backwards?
Yes, this one goes against the rules. This command, when successful,
always returns an output. I'll Add a comment pointing this exception for
further notice.
> +static virInterfacePtr
> +phypInterfaceDefineXML(virConnectPtr conn, const char *xml,
> + unsigned int flags)
> +{
> + ret = phypExec(session, cmd,&exit_status, conn);
> +
> + if (exit_status< 0 || ret == NULL)
> + goto err;
> +
> + if (virStrToLong_i(ret,&char_ptr, 10,&slot) == -1)
> + goto err;
> +
> + /* The next free slot itself: */
> + slot++;
> +
> + /* 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);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + virReportOOMError();
> + goto err;
> + }
> + cmd = virBufferContentAndReset(&buf);
> +
> + ret = phypExec(session, cmd,&exit_status, conn);
Mem leak. You assigned to ret twice without an intermediate VIR_FREE().
Fixed all the occurrences of this issue. My bad, I messed up some git
merges and I think I skip this one.
> +
> + if (exit_status< 0 || ret != NULL)
> + goto err;
> +
> + /* Need to sleep a little while to wait for the HMC to
> + * complete the execution of the command.
> + * */
> + sleep(1);
Is a usleep for a few milliseconds sufficient, or does it have to be for
the entire second? Is this racy, where a system under high load will
need longer than a second?
I can't estimate exactly how much time the HMC takes to finish this
process. But according to my tests, is does not takes more than one
second perform the operation, no matter the system load.
> +
> + if (exit_status< 0 || ret == NULL){
Formatting nit: s/){/) {/
> + /* roll back and excluding interface if error*/
> + VIR_FREE(cmd);
> + VIR_FREE(ret);
> +
> + virBufferAddLit(&buf, "chhwres ");
> + if (system_type == HMC)
> + virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> + virBufferVSprintf(&buf,
> + " -r virtualio --rsubtype eth"
> + " -p %s -o r -s %d", def->name, slot);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + virReportOOMError();
> + goto err;
> + }
> + goto err;
> + }
> +
> + char_ptr = strchr(ret, '\n');
> +
> + memcpy(name, ret, PHYP_IFACENAME_SIZE-1);
This doesn't guarantee that name is NUL-terminated (and it's wasteful if
name is much shorter than PHYP_IFACENAME_SIZE). Are you sure that's okay...
The name of the interface is generated automatically and has exactly
PHYP_IFACENAME_SIZE characters.
--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo(a)linux.vnet.ibm.com