Page MenuHomeFreeBSD

Rename libifc back to libifconfig
ClosedPublic

Authored by marieheleneka_gmail.com on Sep 1 2016, 1:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 2, 4:49 PM
Unknown Object (File)
Thu, Apr 11, 4:44 AM
Unknown Object (File)
Mar 22 2024, 6:59 PM
Unknown Object (File)
Mar 22 2024, 6:59 PM
Unknown Object (File)
Mar 8 2024, 7:48 AM
Unknown Object (File)
Feb 24 2024, 5:11 AM
Unknown Object (File)
Feb 23 2024, 9:12 AM
Unknown Object (File)
Jan 25 2024, 7:15 PM

Details

Summary

Renaming libifc back to libifconfig in response to feedback on initial commit of this library. Sticking to 'libifconfig' (and 'ifconfig_' as function prefix) should reduce chances of namespace collisions, make it more clear what the library does, and be more in line with what existing libraries do.

Submitted by: Marie Helene Kvello-Aune <marieheleneka@gmail.com>
Differential Review: https://reviews.freebsd.org/D7742

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marieheleneka_gmail.com retitled this revision from to Rename libifc back to libifconfig.
marieheleneka_gmail.com updated this object.
marieheleneka_gmail.com edited the test plan for this revision. (Show Details)
marieheleneka_gmail.com added a reviewer: kp.
marieheleneka_gmail.com set the repository for this revision to rS FreeBSD src repository - subversion.

This appears (in Phabricator at least) to be done as deletion of the old names and addition of the new; wouldn't it be better to do move+edit instead, to preserve the Subversion history?

This appears (in Phabricator at least) to be done as deletion of the old names and addition of the new; wouldn't it be better to do move+edit instead, to preserve the Subversion history?

Yes, it should definitely be done via svn mv for commit.

Didn't follow the code closely, but style errors should probably be fixed. I'm not our seasoned style(9) expert though, feel free to argue.

lib/libifconfig/libifconfig.c
60 ↗(On Diff #19929)

Identical license blocks; consider merging them into one.

77 ↗(On Diff #19929)

Superfluous blank line.

83 ↗(On Diff #19929)

Why not sizeof(*h) instead of explicit type?

85 ↗(On Diff #19929)

I'll mention it just once, but you might consider avoiding C99 style and declare int i as normal local variable. It's a moot point though as we do indeed use C99 constructs in the tree.

92 ↗(On Diff #19929)

Superfluous blank line.

104 ↗(On Diff #19929)

Superfluous blank line.

111 ↗(On Diff #19929)

Superfluous blank line.

115 ↗(On Diff #19929)

According to style(9), in functions with no local variables opening brace should be followed by a newline (there're several places like this below).

118 ↗(On Diff #19929)

Superfluous blank line.

130 ↗(On Diff #19929)

Isn't descr guaranteed to be properly initialized in the code below?

133 ↗(On Diff #19929)

sizeof(ifr) perhaps?

167 ↗(On Diff #19929)

Superfluous blank line.

175 ↗(On Diff #19929)

Ditto. sizeof(variable) vs. sizeof(type) thing.

204 ↗(On Diff #19929)

Superfluous blank line.

205 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

209 ↗(On Diff #19929)

OK, I won't mention this any more...

220 ↗(On Diff #19929)

Superfluous blank line.

221 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

245 ↗(On Diff #19929)

Superfluous blank line.

246 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

259 ↗(On Diff #19929)

Superfluous blank line.

260 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

273 ↗(On Diff #19929)

Superfluous blank line.

274 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

287 ↗(On Diff #19929)

Superfluous blank line.

288 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

301 ↗(On Diff #19929)

Superfluous blank line.

302 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

338 ↗(On Diff #19929)

Superfluous blank line.

339 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

355 ↗(On Diff #19929)

Superfluous blank line.

356 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

369 ↗(On Diff #19929)

Superfluous blank line.

370 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

lib/libifconfig/libifconfig.h
1 ↗(On Diff #19929)

Consider starting license comment block with /*- (also below).

33 ↗(On Diff #19929)

While #pragma once might work, I believe our usual #ifndef _LIBIFCONFIG_H_ guard is still in order (both can be used together, with pragma first).

53 ↗(On Diff #19929)

Superfluous blank line.

lib/libifconfig/libifconfig_internal.c
33 ↗(On Diff #19929)

Superfluous blank line.

40 ↗(On Diff #19929)

Missorted #includes?

42 ↗(On Diff #19929)

Superfluous blank line.

57 ↗(On Diff #19929)

Superfluous blank line.

62 ↗(On Diff #19929)

Consider int rcode, s; instead. Also, s makes a bad variable name, even for a local one.

68 ↗(On Diff #19929)

... and rcode = ... here.

72 ↗(On Diff #19929)

Superfluous blank line.

77 ↗(On Diff #19929)

Return type and function name should be separated not with a space, but a newline.

90 ↗(On Diff #19929)

Avoid using contractions (yes, even in comments).

lib/libifconfig/libifconfig_internal.h
33 ↗(On Diff #19929)

Consider using standard #ifndef ... guard in addition to the pragma.

37 ↗(On Diff #19929)

Superfluous blank line.

share/examples/libifconfig/ifcreate.c
38 ↗(On Diff #19929)

Missorted #includes?

40 ↗(On Diff #19929)

Superfluous blank line.

49 ↗(On Diff #19929)

Consider moving it up.

51 ↗(On Diff #19929)

Banal and unclear comment at the same time. Consider rewording it so it makes more sense and justification of its own presence.

60 ↗(On Diff #19929)

lifh is a local variable, what's the point of nullifying it?

67 ↗(On Diff #19929)

Contractions again, yuck...

86 ↗(On Diff #19929)

Ditto (see above).

share/examples/libifconfig/ifdestroy.c
40 ↗(On Diff #19929)

Superfluous blank line.

56 ↗(On Diff #19929)

Consider declaring both ifname and lifh at the top of the function.

81 ↗(On Diff #19929)

Ditto.

share/examples/libifconfig/setdescription.c
47 ↗(On Diff #19929)

Same advice.

48 ↗(On Diff #19929)

Same observation about the comment.

85 ↗(On Diff #19929)

Do not see a reason to nullify these three local pointers.

share/examples/libifconfig/setmtu.c
40 ↗(On Diff #19929)

Superfluous blank line.

52 ↗(On Diff #19929)

Ditto.

88 ↗(On Diff #19929)

Ditto.

danfe: Thans for the review. Those will be addressed, but not in this commit. This one will be strictly the rename. The next one will fix the style remarks (and subsequent commits will fix bugs and add new features).

Ah, sorry, I didn't realize this is just about svn mv. I'm fine with it as long as I don't need to repeat this review in a new DR. :-)

marieheleneka_gmail.com updated this object.
marieheleneka_gmail.com edited edge metadata.

Patch now applies against lib/libifc and share/examples/libifc instead of attempting to move the files.
Before commit, please use 'svn mv' as follows to rename things properly:

svn mv lib/libifc lib/libifconfig
svn mv lib/libifconfig/libifc.c lib/libifconfig/libifconfig.c
svn mv lib/libifconfig/libifc.h lib/libifconfig/libifconfig.h
svn mv lib/libifconfig/libifc_internal.c lib/libifconfig/libifconfig_internal.c
svn mv lib/libifconfig/libifc_internal.h lib/libifconfig/libifconfig_internal.h
svn mv share/examples/libifc share/examples/libifconfig

danfe: Thanks for the feedback on style(9). I will address it in the upcoming style(9) fix, no need to submit the feedback again. :)

cem accepted this revision.EditedSep 1 2016, 4:00 PM
cem added a reviewer: cem.
cem added a subscriber: cem.

LGTM (with svn mv as described above). Style cleanups can come later.

This revision is now accepted and ready to land.Sep 1 2016, 4:00 PM
This revision was automatically updated to reflect the committed changes.