Bug 524147

Initial rpmlint output:
 rpmlint rpmbuild/SPECS/libnetdevname.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[root@XXX ~]# rpmlint rpmbuild/SRPMS/libnetdevname-0.0.2-1.fc11.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[root@XXXX~]# rpmlint rpmbuild/RPMS/x86_64/libnetdevname-*
libnetdevname.x86_64: W: incoherent-version-in-changelog 0.0.2.fc11 ['0.0.2-1.fc11', '0.0.2-1']
libnetdevname.x86_64: E: useless-provides libnetdevname
libnetdevname.x86_64: W: self-obsoletion libnetdevname <= 0.0.2-1.fc11 obsoletes libnetdevname = 0.0.2-1.fc11
libnetdevname.x86_64: W: self-obsoletion libnetdevname <= 0.0.2-1.fc11 obsoletes libnetdevname = 0.0.2-1.fc11
libnetdevname-devel.x86_64: E: useless-provides libnetdevname-devel
libnetdevname-devel.x86_64: W: self-obsoletion libnetdevname-devel <= 0.0.2-1.fc11 obsoletes libnetdevname-devel = 0.0.2-1.fc11
libnetdevname-devel.x86_64: W: self-obsoletion libnetdevname-devel <= 0.0.2-1.fc11 obsoletes libnetdevname-devel = 0.0.2-1.fc11
3 packages and 0 specfiles checked; 2 errors, 5 warnings.

Would be working on fixing on these errors and warnings.
Will have an update soon on this.

Took care of all the errors and warnings.
Here's the rpmlint output after the changes done:
rpmlint ../RPMS/x86_64/libnetdevname-*
6 packages and 0 specfiles checked; 0 errors, 0 warnings.
[root@XXXX SPECS]# rpmlint ../SRPMS/libnetdevname-0.0.2-2.fc11.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[root@XXXX SPECS]# rpmlint libnetdevname.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

Spec URL: http://linux.dell.com/files/fedora/libnetdevname/libnetdevname.spec
SRPM URL: http://linux.dell.com/files/fedora/libnetdevname/
SRPM: libnetdevname-0.0.2-2.fc11.src.rpm

Doesn't this require not-yet-upstream kernel code?

This is not a package review. This is more of a code review.

I'm not really a fan of the API:

extern int netdev_pathname_to_name (char *);

This modifies the data you pass to it, which is ugly. Something like:

extern int netdev_pathname_to_name (const char *name, char **ret);

would be better. (Or, just directly return the name, and NULL on failure, setting errno appropriately.)

In the absence of kernel support, all /dev/net names throw errors. Passing it the existing /dev/net/tun also errors.

It only checks for /dev/net as a prefix. You pass it /dev/foo, or " bar", or other invalid device names... it returns them.

So if I understand what's being done by this code (the git-tree at http://linux.dell.com/git/development/libnetdevname.git is empty, so I haven't looked at it much), it is really here to help user-space apps that would like to use an argument other than 'ethX' to identify the card, do so.  Is that correct?

This doesn't cover any of the work that creates the symlinks between /dev/ethX and /dev/net/by-chassis-label/Embedded_NIC_1.  I also presume that 'Embedded_NIC_1' is a name coming from the BIOS.  Is that also correct?

http://linux.dell.com/git/?p=development/libnetdevname.git;a=summary(In reply to comment #5)
> So if I understand what's being done by this code (the git-tree at
> http://linux.dell.com/git/development/libnetdevname.git is empty, so I haven't
> looked at it much), it is really here to help user-space apps that would like
> to use an argument other than 'ethX' to identify the card, do so.  Is that
> correct?
> 

Yes. That is correct. I would look into why the link is not working.

> This doesn't cover any of the work that creates the symlinks between /dev/ethX
> and /dev/net/by-chassis-label/Embedded_NIC_1.  I also presume that
> 'Embedded_NIC_1' is a name coming from the BIOS.  Is that also correct?  

Yes. 'Embedded_NIC_1' is coming from smbios table type 41. This link is created with the help of "biosdevname" tool( http://linux.dell.com/projects.shtml#biosdevname) which would take the os given name of the interface and return the bios given name. The udev rule would look like -

KERNEL=="eth*", NAME="net/%k", MODE="0666" - would create /dev/net/ethN node and 
SUBSYSTEM=="net", PROGRAM="/sbin/biosdevname -i %k --policy=smbios_names", 
SYMLINK += "net/by-chassis-id/%c"

would return 'Embedded_NIC_1'

http://linux.dell.com/git/?p=development/libnetdevname.git;a=summary(In reply to comment #5)
> So if I understand what's being done by this code (the git-tree at
> http://linux.dell.com/git/development/libnetdevname.git is empty, so I haven't
> looked at it much), it is really here to help user-space apps that would like
> to use an argument other than 'ethX' to identify the card, do so.  Is that
> correct?
> 

Yes. That is correct. I would look into why the link is not working.

> This doesn't cover any of the work that creates the symlinks between /dev/ethX
> and /dev/net/by-chassis-label/Embedded_NIC_1.  I also presume that
> 'Embedded_NIC_1' is a name coming from the BIOS.  Is that also correct?  

Yes. 'Embedded_NIC_1' is coming from smbios table type 41. This link is created with the help of "biosdevname" tool( http://linux.dell.com/projects.shtml#biosdevname) which would take the os given name of the interface and return the bios given name. The udev rule would look like -

KERNEL=="eth*", NAME="net/%k", MODE="0666" - would create /dev/net/ethN node and 
SUBSYSTEM=="net", PROGRAM="/sbin/biosdevname -i %k --policy=smbios_names", 
SYMLINK += "net/by-chassis-id/%c"

would return 'Embedded_NIC_1'

Once I look at the code I'll be able to comment (and maybe wouldn't need to ask this question), but what happens if there is no symlink created.

I wonder if the udev rule should call another script that can call biosdevname (and/or another application) so that this solution is not x86[_64]-specific (which I understand it to be right now based on the fact that biosdevname wouldn't run on ppc or arm).

So when taking about the working case, is the next step that our configuration files will be based on the '/dev/net/by-chassis-label/Embedded_NIC_1' names rather than ethN since those devices might change names depending on when the modules are loaded?

(In reply to comment #4)
> I'm not really a fan of the API:
> 
> extern int netdev_pathname_to_name (char *);
> 
> This modifies the data you pass to it, which is ugly. Something like:
This should be fixed.
> 
> In the absence of kernel support, all /dev/net names throw errors. Passing it
> the existing /dev/net/tun also errors.
This should be fixed.
> 
> It only checks for /dev/net as a prefix. You pass it /dev/foo, or " bar", or
> other invalid device names... it returns them. 
if you pass /dev/foo as a param to the caller the lib will return the same device (/dev/foo/*) to the ioctl call the call will fail. This is the same behaviour without libnetdevname. I dont see why this is a problem.

(In reply to comment #5)
> So if I understand what's being done by this code (the git-tree at
> http://linux.dell.com/git/development/libnetdevname.git is empty, so I haven't
> looked at it much)

The new git tree should be up now on
http://linux.dell.com/git/libnetdevname.git/

Created attachment 362690 [details]
libnetdevname - patch incorporating review comments

I have attached a patch incorporating review comments.

That still requires the caller allocate the string; having the API return a newly-allocated string seems simpler to me. Andy, Matt - other ideas?

>That still requires the caller allocate the string; having the API return a
>newly-allocated string seems simpler to me. Andy, Matt - other ideas?

The apps would then have to take care of freeing the newly allocated string ...

Would that be acceptable ??

(In reply to comment #13)
> That still requires the caller allocate the string; having the API return a
> newly-allocated string seems simpler to me. Andy, Matt - other ideas?  

The caller(the apps) allocates the memory and caller(apps) frees the memory is easier in terms of tracking and freeing the memory allocated than the library allocating the memory. Also some applications like "ip" have a scenario where an "input device" and an "output device" have to be specified in the commandline. I thought such scenarios are easier to handle with this model. 

Any thoughts ?

Is there a reason to explicitly mention 
Provides: %{release_name} = %{version}-%{release} 

Is this checked in any other package?

Review: 
#user1  rpmlint rpmbuild/SRPMS/libnetdevname-0.0.3-1.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

#user1 rpmlint rpmbuild/RPMS/i386/libnetdevname-0.0.3-1.i386.rpm
libnetdevname.i386: W: incoherent-version-in-changelog 0.0.3 ['0.0.3-1', '0.0.3-1']
libnetdevname.i386: E: useless-provides libnetdevname
libnetdevname.i386: W: self-obsoletion libnetdevname <= 0.0.3-1 obsoletes libnetdevname = 0.0.3-1
libnetdevname.i386: W: self-obsoletion libnetdevname <= 0.0.3-1 obsoletes libnetdevname = 0.0.3-1
libnetdevname.i386: W: shared-lib-calls-exit /usr/lib/libnetdevname.so.0.0.3 exit
1 packages and 0 specfiles checked; 1 errors, 4 warnings.

#user1 rpmlint rpmbuild/RPMS/i386/libnetdevname-devel-0.0.3-1.i386.rpm
libnetdevname-devel.i386: E: useless-provides libnetdevname-devel
libnetdevname-devel.i386: W: self-obsoletion libnetdevname-devel <= 0.0.3-1 obsoletes libnetdevname-devel = 0.0.3-1
libnetdevname-devel.i386: W: self-obsoletion libnetdevname-devel <= 0.0.3-1 obsoletes libnetdevname-devel = 0.0.3-1
libnetdevname-devel.i386: W: no-documentation
1 packages and 0 specfiles checked; 1 errors, 3 warnings.

#user1 rpmlint rpmbuild/RPMS/i386/libnetdevname-debuginfo-0.0.3-1.i386.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.



Few more:
1) "INSTALL -p" in the spec file is not required, since the Makefile.in already adds the "-p" flag
2) No need to explicitly mention gcc in BuildRequires.
http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2
3) Since gcc package requires libgcc, there is no need to mention libgcc as well. (can be verified with rpm -qi --requires gcc)


To fix the rpmlint errors:
1) Do not use <= in obsoletes, use < only.
2) No need to explicitly mention %Provides: %{name}= %{version}-%{release}. This is automatically captured. can be verified with rpm -qpi --provides rpmbuild/RPMS/i386/libnetdevname-0.0.3-1.i386.rpm
3) the above two comments apply for devel package as well.
4) Make sure to add release number also in the changelog.
5) rpmlint -I shared-lib-calls-exit
shared-lib-calls-exit:
This library package calls exit() or _exit(), probably in a non-fork()
context. Doing so from a library is strongly discouraged - when a library
function calls exit(), it prevents the calling program from handling the
error, reporting it to the user, closing files properly, and cleaning up any
state that the program has. It is preferred for the library to return an
actual error code and let the calling program decide how to handle the
situation.


Thank you 
Praveen

Fixed all the rpmlint errors/warnings.
Here's the current rpmlint o/p:

rpmlint libnetdevname.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
rpmlint ../SRPMS/libnetdevname-0.0.3-3.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
rpmlint ../RPMS/x86_64/libnetdevname-*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

The updated spec and srpm can be found at:

http://linux.dell.com/files/fedora/libnetdevname/

Everything looks good. Please go ahead and request CVS.

Thanks 
Praveen

New Package CVS Request
=======================
Package Name: libnetdevname
Short Description: development files required to build libnetdevname-based software.  
Owners: srini praveenp
Branches: F-10 F-11 F-12 EL-4 EL-5
InitialCC: mdomsch

Is the short description here correct? 
This package isn't just development files is it?

(In reply to comment #20)
> New Package CVS Request
> =======================
> Package Name: libnetdevname
> Short Description: development files required to build libnetdevname-based
> software.  
> Owners: srini praveenp
> Branches: F-10 F-11 F-12 EL-4 EL-5
> InitialCC: mdomsch  

That's incorrect. Here's the correct description:
This library package provides interface for mapping network path names
to kernel names

Thanks. 

cvs done.

This package is currently replaced by biosdevname package.