10:51 a.m.
On 01/14/2011 10:07 PM, Eric Blake wrote:
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.
No problem, I understand your point on not having a way to test it. In,
future work, I'll try to send smaller patches so it gets much easier for
you all to review.
And sorry for the delay on replying this email, I was on a vacation, so
catching up all now.
> @@ -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?
Works for me.
otubo@vader ~ $ uname -a
Linux vader 2.6.35-25-generic #44-Ubuntu SMP Fri Jan 21 17:40:48 UTC
2011 i686 GNU/Linux
otubo@vader ~/develop/libvirt master $ gcc -v
Using built-in specs.
Target: i686-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro
4.4.4-14ubuntu5' --with-bugurl=file:///usr/share/doc/gcc-4.4/README.Bugs
--enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr
--program-suffix=-4.4 --enable-shared --enable-multiarch
--enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib
--without-included-gettext --enable-threads=posix
--with-gxx-include-dir=/usr/include/c++/4.4 --libdir=/usr/lib
--enable-nls --with-sysroot=/ --enable-clocale=gnu
--enable-libstdcxx-debug --enable-objc-gc --enable-targets=all
--disable-werror --with-arch-32=i686 --with-tune=generic
--enable-checking=release --build=i686-linux-gnu --host=i686-linux-gnu
--target=i686-linux-gnu
Thread model: posix
gcc version 4.4.5 (Ubuntu/Linaro 4.4.4-14ubuntu5)
Don't know why or how, but it did compiled.
I'll fix again and send 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?
I put now a roll-back on this part in case it couldn't get the correct
interface name.
> +
> + 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.
Fixed.
> +
> +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;
Fixed
> +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.
Also fixed.
Getting closer, but still not ready to merge in. Hopefully v5 will
clinch it.
Will send a patch right a way. I'll review and test a lot better before
sending it. Hope this time we can push it. :-)
Thank you very much.
--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo(a)linux.vnet.ibm.com