[PATCH 2/6] nf: nfnl_*_str2copy_mode() should return int

[Thomas Graf]

On 08/26/14 at 12:40pm, Thomas Haller wrote:
> On Tue, 2014-08-26 at 11:21 +0100, Thomas Graf wrote:
> > On 08/26/14 at 12:03pm, Thomas Haller wrote:
> > > On Tue, 2014-08-26 at 01:09 +0200, Thomas Graf wrote:
> > > > ... to be able to return a negative error code for unknown modes.
> > > > 
> > > > Signed-off-by: Thomas Graf <tgraf at suug.ch>
> > > > ---
> > > >  include/netlink/netfilter/log.h   | 2 +-
> > > >  include/netlink/netfilter/queue.h | 2 +-
> > > >  lib/netfilter/log_obj.c           | 2 +-
> > > >  lib/netfilter/queue_obj.c         | 2 +-
> > > >  src/nf-log.c                      | 2 +-
> > > >  src/nf-queue.c                    | 2 +-
> > > >  6 files changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > 
> > > isn't this an ABI break?
> > 
> > Strictly speaking it is. We should be fine unless we compile
> > with -fshort-enums though.
> > 
> > I should have mentioned this in the commit message actually:
> > assuming that enum can translate to anything, I can't see how
> > a caller could correctly check for the -1 error condition.
> > Even if we require the caller to cast, what do we have him cast
> > it to?
> 
> shouldn't the correct way be:
> 
> if (value == (enum nfnl_queue_copy_mode) -1)

I can see that work if the same or a compatible compiler is used
for both libnl and the application. I cannot see that work if the
compiler of libnl and the compiler of the app choose a different
enum size (which would be the ABI breakage reason in the first place).

> > So either way, we are breaking the ABI in a minor way.
> > 
> > Thoughts?
> 
> given that the enum size depends on compiler/compiler options
> (-fshort-enums), it seems wrong to have them as function
> arguments/return values in first place.
> 
> for that reason, we should get rid of enums for
> that purpose.

Agreed.