On 02/16/2011 10:38 AM, Eduardo Otubo wrote:
> 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:
>
> src/phyp/phyp_driver.c | 648 +++++++++++++++++++++++++++++++++++++++++-------
> 1 files changed, 562 insertions(+), 86 deletions(-)
>
All right - this version compiles, so we're already off to a better
start than v4, but not ready to apply yet.
> @@ -1113,8 +1116,10 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth,
>
> static virDrvOpenStatus
> phypOpen(virConnectPtr conn,
> - virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED)
> + virConnectAuthPtr auth, int flags)
> {
> + virCheckFlags(0, VIR_DRV_OPEN_DECLINED);
> +
> LIBSSH2_SESSION *session = NULL;
This introduces a declaration-after-statement. We require C99 for other
reasons, therefore this will still compile, but we still like to stick
with C89 decl-before-statement where it makes sense; that is,
virCheckFlags is usually delayed until after all the declarations.
> ConnectionData *connection_data = NULL;
> char *string = NULL;
> @@ -1125,6 +1130,7 @@ phypOpen(virConnectPtr conn,
> char *char_ptr;
> char *managed_system = NULL;
>
> +
> if (!conn || !conn->uri)
Why the spurious whitespace change?
> return VIR_DRV_OPEN_DECLINED;
>
> @@ -1176,9 +1182,6 @@ phypOpen(virConnectPtr conn,
> * */
> char_ptr = strchr(managed_system, '/');
>
> - if (char_ptr)
> - *char_ptr = '\0';
> -
This hunk is a regression. Later on, you assign managed_system to
phyp_driver->managed_system; without this hunk, that assignment will not
contain any /, but after this patch, you've changed what gets assigned.
> @@ -1354,11 +1357,6 @@ phypGetLparNAME(LIBSSH2_SESSION * session, const char *managed_system,
> if (exit_status < 0 || ret == NULL)
> goto err;
>
> - char *char_ptr = strchr(ret, '\n');
> -
> - if (char_ptr)
> - *char_ptr = '\0';
> -
> VIR_FREE(cmd);
> return ret;
This hunk is unrelated to network interface management; and it also
looks like a regression (you're now returning a newline and subsequent
text, where before-hand you were not). Could you please separate this
into multiple patches, each touching one smaller issue, rather than
sending one giant patch that mixes both cleanups of existing code and
addition of new code? Hint: 'git reset HEAD^; git add -p'.
> @@ -1650,9 +1639,6 @@ phypGetBackingDevice(virConnectPtr conn, const char *managed_system,
>
> char_ptr = strchr(backing_device, '\n');
>
> - if (char_ptr)
> - *char_ptr = '\0';
> -
> VIR_FREE(cmd);
> VIR_FREE(ret);
> return backing_device;
Another regression. I'll quit pointing them out, and instead focus on
the new code for the rest of my review.
> @@ -3273,6 +3223,542 @@ phypGetStoragePoolXMLDesc(virStoragePoolPtr pool, unsigned int flags)
> }
>
> static int
> +phypInterfaceDestroy(virInterfacePtr iface,
> + unsigned int flags)
> +{
> + virCheckFlags(0, -1);
Again, this line belongs...
> +
> + ConnectionData *connection_data = iface->conn->networkPrivateData;
> + phyp_driverPtr phyp_driver = iface->conn->privateData;
> + LIBSSH2_SESSION *session = connection_data->session;
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + char *managed_system = phyp_driver->managed_system;
> + int system_type = phyp_driver->system_type;
> + int exit_status = 0;
> + int slot_num = 0;
> + int lpar_id = 0;
> + char *char_ptr;
> + char *cmd = NULL;
> + char *ret = NULL;
...here, after declarations.
> +
> + /* Getting the remote slot number */
> +
> + char_ptr = strchr(iface->mac, '\n');
Dead assignment - you don't use this value of char_ptr for anything...
> +
> + 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?
...
> + 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?
> +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().
> +
> + 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?
> +
> + /* Getting the new interface name */
> + virBufferAddLit(&buf, "lshwres ");
> + if (system_type == HMC)
> + virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> + virBufferVSprintf(&buf,
> + " -r virtualio --rsubtype slot --level slot"
> + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; "
> + "s/^.*drc_name=//'", def->name, slot);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + virReportOOMError();
> + goto err;
> + }
> + cmd = virBufferContentAndReset(&buf);
> +
> + ret = phypExec(session, cmd, &exit_status, conn);
Another leak of ret.
> +
> + 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...
> +
> + /* Getting the new interface mac addr */
> + virBufferAddLit(&buf, "lshwres ");
> + if (system_type == HMC)
> + virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> + virBufferVSprintf(&buf,
> + "-r virtualio --rsubtype eth --level lpar "
> + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; "
> + "s/^.*mac_addr=//'", def->name, slot);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + virReportOOMError();
> + goto err;
> + }
> + cmd = virBufferContentAndReset(&buf);
> +
> + ret = phypExec(session, cmd, &exit_status, conn);
> +
> + if (exit_status < 0 || ret == NULL)
> + goto err;
> +
> + memcpy(mac, ret, PHYP_MAC_SIZE-1);
> +
> + VIR_FREE(cmd);
> + VIR_FREE(ret);
> + return virGetInterface(conn, name, mac);
...if you get here with a non-NUL-terminated name?
> +static virInterfacePtr
> +phypInterfaceLookupByName(virConnectPtr conn, const char *name)
> +{
> + 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;
> +
> + /*Getting the lpar_id for the interface */
> + virBufferAddLit(&buf, "lshwres ");
> + if (system_type == HMC)
> + virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> + virBufferVSprintf(&buf,
> + " -r virtualio --rsubtype slot --level slot "
> + " -F drc_name,lpar_id |"
> + " sed -n '/%s/ s/^.*,//p'", name);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + virReportOOMError();
> + return NULL;
> + }
> + cmd = virBufferContentAndReset(&buf);
> +
> + ret = phypExec(session, cmd, &exit_status, conn);
Another leak of ret. I'm starting to feel a bit frustrated that I've
pointed this out in previous rounds of reviews, and you still haven't
fixed the issues.
> +
> + if (exit_status < 0 || ret == NULL)
> + goto err;
> +
> + if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1)
> + goto err;
> +
> + /*Getting the interface mac */
> + virBufferAddLit(&buf, "lshwres ");
> + if (system_type == HMC)
> + virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> + virBufferVSprintf(&buf,
> + " -r virtualio --rsubtype eth --level lpar "
> + " -F lpar_id,slot_num,mac_addr|"
> + " sed -n '/%d,%d/ s/^.*,//p'", lpar_id, slot);
> +
> + if (virBufferError(&buf)) {
> + virBufferFreeAndReset(&buf);
> + virReportOOMError();
> + return NULL;
> + }
> + cmd = virBufferContentAndReset(&buf);
> +
> + ret = phypExec(session, cmd, &exit_status, conn);
Another leak of ret. More of the same through the rest of the patch.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list