[libvirt] [PATCHv5] PHYP: Adding network interface management

[Eduardo Otubo]

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 at linux.vnet.ibm.com