On Fri, 2015-10-16 at 09:27 +0200, Johannes Thumshirn wrote:
> On Thu, 2015-10-15 at 10:54 -0700, Vasu Dev wrote:
> > On Fri, 2015-10-09 at 13:12 +0200, Johannes Thumshirn wrote:
> > > This patch series replaces all usage of libHBAAPIv2 and
> > > libhbalinux2 from
> > > fcoe-utils and replaces them with an internal version operating
> > > directly on the
> > > respective sysfs files.
> > > 
> > > This removes the two dependencies but pulls in libpciacces (which
> > > got pulled in
> > > by libhbalinux2 in the current version of fcoe-utils). Nevertheless
> > > this way it
> > > is possible to get rid of a lot of code.
> > > 
> > > Changes to v2:
> > > o Do not hardcode /sys/class/net/<device>/ctlr_<num>/ to ctlr_0 but
> > > read out of
> > >   sysfs
> > > 
> > > Changes to v3:
> > > o Checkpatch clean the patches.
> > > o Copy over comments from libhbalinux2/pci.c into sysfs_hba.c pci
> > > code.
> > > o Move defines to header files.
> > > 
> > > Changes to v4:
> > > o Checkpatch clean the patches (again).
> > > o Add Intel Copyright (for pci code) to sysfs_hba.c
> > > o Add check for NULL pointer dereference in
> > > get_pci_dev_from_netdev()
> > > o Remove trailing '\n' from pci domain, bus, device, function
> > > string in
> > >   get_pci_dev_from_netdev()
> > > 
> > > Johannes Thumshirn (9):
> > >   fcoe-utils: Add sysfs_hba to fcoemon_utils
> > >   fcoeadm: Use internal sysfs based hba lib for information
> > >   fcoeadm: Use internal sysfs lib to display target and LUN info
> > >   fcoeadm: Use internal sysfs lib to display port statistics
> > >   fcoeadm: Get rid of some includes
> > >   fcoemon: Use internal sysfs_hba library
> > >   libutil: remove definition of sa_hex_format()
> > >   fcping: Convert fcping to internal sysfs based implementation
> > >   configure.ac/Makefile.am: Remove libHBAAPIv2 and libhbalinux2
> > > 
> > >  Makefile.am             |   13 +-
> > >  configure.ac            |    8 +-
> > >  fcoe-utils.spec.in      |    2 +-
> > >  fcoeadm_display.c       | 1602 ++++++++++++++---------------------
> > > ------------
> > >  fcoeadm_display.h       |    4 +-
> > >  fcoemon.c               |    2 +-
> > >  fcping.c                |  183 ++----
> > >  include/fcoemon_utils.h |    3 -
> > >  include/sysfs_hba.h     |  118 ++++
> > >  lib/sysfs_hba.c         |  699 +++++++++++++++++++++
> > >  10 files changed, 1369 insertions(+), 1265 deletions(-)
> > >  create mode 100644 include/sysfs_hba.h
> > >  create mode 100644 lib/sysfs_hba.c
> > > 
> > 
> > Johannes, 
> > 
> > The series is so far looking good except that I found a new memory
> > corruption issue in fcping due to double mem free, I'll be sending
> > the
> > fix for that soon and with that  series is good to apply.
> 
> Thanks for crafting the patch.
> 

Not really, as you must have seen that the code change is just one line
and rest detailed description with reference to actual error log text. I
think pasting error logs or complete crashes logs is good practice as
opposed to very terse patch descriptions.

> > 
> > So far only I've been reviewing actively and looking into these
> > patches
> > for any regression and not sure how exhaustive that coverage is for
> > this
> > series doing substantial amount of code changes " 10 files changed,
> > 1369
> > insertions(+), 1265 deletions(-". 
> > 
> > For all these reasons I think it must be tested more thoroughly and
> > to
> > allow that I'm going to stage these patches in a branch (-next) and
> > then
> > have -next branch run though BAT for next kernel cycle 4.3 before
> > merging them into mainline.
> 
> Sounds reasonable. We have it included in SLE12-SP1 and so far I've onl
> y had one complaint which triggered v3 of the patchset. I'll be adding
> your patch to our package as well.
> 

I didn't expect pull into SP1 so soon but anyway outside the scope of
upstream.

> > 
> > Thanks for doing this big overhauling and getting rid off two
> > libraries
> > dependency completely.
> 
> It's less maintenance for us (and others as well of cause) on the
> distro side, so I'm looking forward to getting it merged.
> 
> 

Pushed to next branch, so all set.

Thanks again,
Vasu


_______________________________________________
fcoe-devel mailing list
[email protected]
http://lists.open-fcoe.org/mailman/listinfo/fcoe-devel

Reply via email to