Bug 524147 - Review Request: libnetdevname -Development environment for libnetdevname
Summary: Review Request: libnetdevname -Development environment for libnetdevname
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Praveen K Paladugu
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-18 06:25 UTC by srinivas
Modified: 2011-05-31 16:21 UTC (History)
10 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-05-31 16:21:14 UTC
praveen_paladugu: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
libnetdevname - patch incorporating review comments (2.61 KB, application/force-download)
2009-09-25 16:45 UTC, Narendra K
no flags Details

Description srinivas 2009-09-18 06:25:20 UTC
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-1.fc11.src.rpm

Description:
The libnetdevname package contains development files required to build
libnetdevname-based software.

Comment 1 srinivas 2009-09-18 06:31:25 UTC
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.

Comment 2 srinivas 2009-09-18 07:09:45 UTC
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

Comment 3 Bill Nottingham 2009-09-18 16:50:43 UTC
Doesn't this require not-yet-upstream kernel code?

Comment 4 Bill Nottingham 2009-09-18 18:39:13 UTC
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.

Comment 5 Andy Gospodarek 2009-09-18 20:10:09 UTC
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?

Comment 6 Narendra K 2009-09-18 20:28:28 UTC



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'

Comment 7 Narendra K 2009-09-18 21:01:30 UTC



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'

Comment 8 Andy Gospodarek 2009-09-18 21:05:30 UTC
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?

Comment 9 Sandeep K. Shandilya 2009-09-23 18:14:43 UTC
(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.

Comment 10 Sandeep K. Shandilya 2009-09-23 18:16:55 UTC
(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/

Comment 11 Narendra K 2009-09-25 16:45:09 UTC
Created attachment 362690 [details]
libnetdevname - patch incorporating review comments

Comment 12 Narendra K 2009-09-25 16:46:09 UTC
I have attached a patch incorporating review comments.

Comment 13 Bill Nottingham 2009-09-25 16:53:03 UTC
That still requires the caller allocate the string; having the API return a newly-allocated string seems simpler to me. Andy, Matt - other ideas?

Comment 14 Shyam kumar Iyer 2009-09-25 18:41:22 UTC
>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 ??

Comment 15 Narendra K 2009-09-29 11:25:06 UTC
(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 ?

Comment 16 Praveen K Paladugu 2009-10-05 14:05:28 UTC
Is there a reason to explicitly mention 
Provides: %{release_name} = %{version}-%{release} 

Is this checked in any other package?

Comment 17 Praveen K Paladugu 2009-10-05 15:49:33 UTC
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@GLIBC_2.0
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

Comment 18 srinivas 2009-10-06 11:46:06 UTC
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/

Comment 19 Praveen K Paladugu 2009-10-07 19:49:31 UTC
Everything looks good. Please go ahead and request CVS.

Thanks 
Praveen

Comment 20 srinivas 2009-10-08 05:36:27 UTC
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

Comment 21 Kevin Fenzi 2009-10-09 02:46:36 UTC
Is the short description here correct? 
This package isn't just development files is it?

Comment 22 srinivas 2009-10-09 10:00:36 UTC
(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

Comment 23 Kevin Fenzi 2009-10-10 22:09:37 UTC
Thanks. 

cvs done.

Comment 24 Praveen K Paladugu 2011-05-31 16:21:14 UTC
This package is currently replaced by biosdevname package.


Note You need to log in before you can comment on or make changes to this bug.