On 12/29/2010 11:04 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:
Finally getting around to reviewing this again. I'm not intentionally
putting you off, it's just that I'm less familiar with phyp and it takes
a good chunk of free time to review large patches in areas where I'm
less familiar.
> @@ -74,6 +75,11 @@
>
> static unsigned const int HMC = 0;
> static unsigned const int IVM = 127;
> +static unsigned const int PHYP_IFACENAME_SIZE = 24;
> +static unsigned const int PHYP_MAC_SIZE= 12;
> +
> +
> +virCheckFlags(0,NULL);
This is misplaced, and causes a compile error. It should be one of the
first statements (not declarations) in any function that takes a flags
argument where you do not otherwise use flags, and not something done at
the file scope.
>
> static int
> waitsocket(int socket_fd, LIBSSH2_SESSION * session)
> @@ -1113,7 +1119,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth,
>
> static virDrvOpenStatus
> phypOpen(virConnectPtr conn,
> - virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED)
> + virConnectAuthPtr auth, int flags)
> {
> LIBSSH2_SESSION *session = NULL;
> ConnectionData *connection_data = NULL;
That is, somewhere in this function.
Seeing as how this causes a compile error:
CC libvirt_driver_phyp_la-phyp_driver.lo
phyp/phyp_driver.c:82:1: error: expected identifier or '(' before 'do'
phyp/phyp_driver.c:82:290: error: expected identifier or '(' before 'while'
cc1: warnings being treated as errors
phyp/phyp_driver.c: In function 'phypOpen':
phyp/phyp_driver.c:1122:38: error: unused parameter 'flags'
[-Wunused-parameter]
phyp/phyp_driver.c: In function 'phypInterfaceDestroy':
how can I even be sure that you've tested your patch, to spend my time
reviewing it?
> +static virInterfacePtr
> +phypInterfaceDefineXML(virConnectPtr conn, const char *xml,
> + unsigned int flags)
> +{
> + /* 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);
So if this succeeds,
> + /* Need to sleep a little while to wait for the HMC to
> + * complete the execution of the command.
> + * */
> + sleep(1);
> +
> + /* 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);
but this fails, do you need to undo the reservation, or have you just
leaked a resource?
> +
> + 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;
> +
> + char_ptr = strchr(ret, '\n');
> +
> + if (char_ptr)
> + *char_ptr = '\0';
> +
> + if (memcpy(name, ret, PHYP_IFACENAME_SIZE-1) == NULL)
> + goto err;
Huh? memcpy() can never fail on valid input. No need for this if
statement; just do the memcpy() and ignore the result (which will be
name). Multiple times in this patch.
> +
> +static virInterfacePtr
> +phypInterfaceLookupByName(virConnectPtr conn, const char *name)
> +{
> +
> + ret = phypExec(session, cmd, &exit_status, conn);
> +
> + if (exit_status < 0 || ret == NULL)
> + goto err;
> +
> + if (memcpy(mac, ret, PHYP_MAC_SIZE-1) == NULL)
> + goto err;
> +
> + VIR_FREE(cmd);
> + return virGetInterface(conn, name, ret);
Memory leak; you never freed ret. You need to do something like:
result = virGetInterface(conn, name, ret);
VIR_FREE(ret);
return result;
> +static int
> +phypInterfaceIsActive(virInterfacePtr iface)
> +{
> +
> + if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1)
> + goto err;
> +
> + if (char_ptr)
> + *char_ptr = '\0';
> +
> + VIR_FREE(cmd);
> + VIR_FREE(ret);
> + return state;
The lines involving setting *char_ptr to '\0' are useless, and can
safely be deleted. No need to NUL-terminate the integer portion of ret
if you are just about to free it.
Getting closer, but still not ready to merge in. Hopefully v5 will
clinch it.
--
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