[libvirt] [PATCH v4 0/3] ESX: Add routines to interface driver

[Ata E Husain]

Please see comments inline.

 

Regards,

Ata

 

-----Original Message-----
From: Matthias Bolte [mailto:matthias.bolte at googlemail.com] 
Sent: Wednesday, August 01, 2012 1:08 PM
To: Ata E Husain Bohra
Cc: libvir-list at redhat.com
Subject: Re: [libvirt] [PATCH v4 0/3] ESX: Add routines to interface driver

 

2012/7/29 Ata E Husain Bohra < <mailto:ata.husain at hotmail.com> ata.husain at hotmail.com>:

> Updated the patch against review comments from Laine and Matthias.

> 

> Ata E Husain Bohra (3):

>   ESX: Add routines to interface driver

>   ESX: Add routines to interface driver

>   ESX: Add routines to interface driver

> 

>  src/esx/esx_interface_driver.c |  499 +++++++++++++++++++++++++++++++++++++++-

>  src/esx/esx_vi.c               |  126 ++++++++++

>  src/esx/esx_vi.h               |   10 +

>  src/esx/esx_vi_generator.input |  227 ++++++++++++++++++

>  src/esx/esx_vi_generator.py    |   31 ++-

>  src/esx/esx_vi_types.c         |   18 +-

>  6 files changed, 906 insertions(+), 5 deletions(-)

 

Okay, a first quick review:

 

You should really merge your two incremental patches into the first

one and propose a single updated patch.

[AB]; Will update the review with a single patch style when needed from next time.

 

You still have your

ESX_VI__TEMPLATE__PROPERTY__DESERIALIZE_STRING_LIST workaround in

there. The problem is already fixed in git, so please remove this

part.

[AB]: I completely missed track of this commit, updated the patch.

 

There are several unchecked strdup calls. All strdup calls need to be

checked for NULL return and report an OOM error is necessary.

[AB]: Thanks for pointing this out, I was checking against NULL at few places,

but missed couple of them.

 

All esxVI_* functions already report errors. But on some calls to such

functions (for example esxVI_RemoveVirtualNic and

esxVI_LookupPhysicalNicFromPortGroup) you're reporting an extra error

overwriting the original. probably more detailed error. You should

avoid this.

[AB]: Removed.

 

I still need to review this patch on the conceptual side, I'll

probably have time for this later this week.

[AB]: I appreciate your suggestions and support. Thanks!

 

-- 

Matthias Bolte

 <http://photron.blogspot.com> http://photron.blogspot.com

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120801/4941bda2/attachment-0001.htm>