Bug 32662
| Summary: | arp(8) uses "this host" with two different meanings. | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Documentation | Reporter: | Gary W. Swearingen <swear> | ||||
| Component: | Books & Articles | Assignee: | dd <dd> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | ||||||
| Priority: | Normal | ||||||
| Version: | Latest | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
|
Description
Gary W. Swearingen
2001-12-10 05:00:01 UTC
"Gary W. Swearingen" <swear@blarg.net> wrote: > 3) The -f option description doesn't say that the file may not have > comments. It doesn't say that the file may not have instructions to cook you dinner, either (well, it may, but they won't be obeyed); should we document that as well? If people trying to use comments in these files is a problem, perhaps arp should be taught to respect comments. If this can't be done for some technical reason, this should be documented, along with the reason, in a BUGS section. The rest of the patch looks okay. Responsible Changed From-To: freebsd-doc->dd I'll follow up. Dima Dorfman <dima@trit.org> writes: > "Gary W. Swearingen" <swear@blarg.net> wrote: > > 3) The -f option description doesn't say that the file may not have > > comments. > > It doesn't say that the file may not have instructions to cook you > dinner, either (well, it may, but they won't be obeyed); should we > document that as well? No, because few, if any, people will loose time because of it's omission. But the file in question is a configuration file, likely to be in /etc, and more than a few people, seeing no mention of comments, will wonder whether it's like almost all of the other config files which MAY have comments and whether the lack of mention of comments in the man page is just typical man page sketchiness, and who will then waste time experimenting to determine if comments are supported. That's what I did. I supposed that I'm not unique and that adding one line to the man page would save others some time and cost little. (But a little more than I thought, huh? :-) > If people trying to use comments in these > files is a problem, perhaps arp should be taught to respect comments. That's what I thought, but I don't do code patching and I assume that no-patch design enhancement PRs are generally unwelcome and are likely to be a waste of my and others' time. > If this can't be done for some technical reason, this should be > documented, along with the reason, in a BUGS section. It wouldn't be out-of-place there (as a design bug), even for practical reasons, but putting it with the description of the file is slightly better, IMO. And better in BUGS than nowhere. swear@blarg.net (Gary W. Swearingen) wrote: > Dima Dorfman <dima@trit.org> writes: > > If people trying to use comments in these > > files is a problem, perhaps arp should be taught to respect comments. > > That's what I thought, but I don't do code patching and I assume that > no-patch design enhancement PRs are generally unwelcome and are likely > to be a waste of my and others' time. Indeed. This does, however, seem to be the best solution; although you've made valid points, I still don't like the idea of documenting this easily-fixed shortcoming anywhere. The patch below adds support for comments to arp(8)'s file parsing routine. I've done light testing on this, but if you could try it and make sure that (a) comments work as you expect them and (b) it doesn't break any basic functionality, I would greatly appreciate it. Thanks. Index: arp.c =================================================================== RCS file: /home/ncvs/src/usr.sbin/arp/arp.c,v retrieving revision 1.34 diff -u -r1.34 arp.c --- arp.c 10 Dec 2001 06:42:56 -0000 1.34 +++ arp.c 25 Jan 2002 21:50:13 -0000 @@ -229,6 +229,8 @@ args[4] = &arg[4][0]; retval = 0; while(fgets(line, 100, fp) != NULL) { + if (line[strspn(line, " \t")] == '#') + continue; i = sscanf(line, "%49s %49s %49s %49s %49s", arg[0], arg[1], arg[2], arg[3], arg[4]); if (i < 2) { Dima Dorfman <dima@trit.org> writes: > The patch below adds support for comments to arp(8)'s file parsing > routine. I've done light testing on this, but if you could try it and > make sure that (a) comments work as you expect them and (b) it > doesn't break any basic functionality, I would greatly appreciate it. I know next to nothing about compiling and linking on FreeBSD, but I should learn, so I'll take a look at it in the next few days and let you know one way or the other. As for the comments working as I expect, I don't know what to expect. Some man pages ask for "#" in column 1; some allow them anywhere; others have other specs. It looks like the patch allows preceeding space and tabs, which is not ideal, but it's a quick and adequate kludge in the absence of a decent config file parsing library. I'll now want the man page to say what the program needs and accepts regarding comments. I'll also want it to say that only the first 99 characters of each line are read. Shall I redo my original patch, or what? Below is the patch on arp.c which I tested with a 6-line input file with
various kinds of comments or none and a few mods of that file. I tested
with the modified program instrumented with print commands to show the
-f info before and after parsing and with the error branch taken and not.
Note that I added a line to allow "trailing" comments.
Note the new additions to the the -f section of the man page. I suspect
that parts won't be appreciated, but I think they are helpful and I do
not want to further kludge config file parsing code at this time. I'll
not object if you prefer to hide the bad arp behaviors from the users.
"$FreeBSD: src/usr.sbin/arp/arp.c,v 1.22.2.5 2001/10/18 10:17:47 ru Exp $";
$FreeBSD: src/usr.sbin/arp/arp.8,v 1.8.2.5 2001/08/16 15:55:39 ru Exp $
--- /usr/src/usr.sbin/arp/arp..orig.c Wed Dec 26 14:33:44 2001
+++ /usr/src/usr.sbin/arp/arp.c Sun Jan 27 13:43:33 2002
@@ -210,6 +210,9 @@
args[4] = &arg[4][0];
retval = 0;
while(fgets(line, 100, fp) != NULL) {
+ if (line[strspn(line, " \t")] == '#')
+ continue;
+ line[strcspn(line, "#")] = '\0';
i = sscanf(line, "%49s %49s %49s %49s %49s", arg[0], arg[1],
arg[2], arg[3], arg[4]);
if (i < 2) {
--- /tmp/arp..orig.8 Sun Dec 9 20:02:06 2001
+++ /tmp/arp.8 Mon Jan 28 13:05:33 2002
@@ -81,7 +81,7 @@
.Tn ARP
entries.
.It Fl d
-A super-user may delete an entry for the host called
+A super-user may delete an entry for the host named
.Ar hostname
with the
.Fl d
@@ -96,10 +96,10 @@
Show network addresses as numbers (normally
.Nm
attempts to display addresses symbolically).
-.It Fl s Ar hostname ether_addr
+.It Fl s
Create an
.Tn ARP
-entry for the host called
+entry for the host named
.Ar hostname
with the Ethernet address
.Ar ether_addr .
@@ -122,15 +122,19 @@
.Ar ether_addr
can be given as
.Cm auto
-in which case the interfaces on this host will be examined,
+in which case the interfaces of the computer running
+.Nm
+will be examined,
and if one of them is found to occupy the same subnet, its
Ethernet address will be used.
-.It Fl S Ar hostname ether_addr
+.It Fl S
Is just like
.Fl s
except any existing
.Tn ARP
-entry for this host will be deleted first.
+entry for the host named
+.Ar hostname
+will be deleted first.
.It Fl f
Causes the file
.Ar filename
@@ -146,6 +150,10 @@
.Ed
.Pp
with argument meanings as given above.
+Comments may be placed on any line after a ``#'' character.
+Blank lines cause warnings on the error stream and an exit status of 1,
+but are otherwise be ignored.
+Lines longer than 99 characters can cause silent errors.
.El
.Sh SEE ALSO
.Xr inet 3 ,
swear@blarg.net (Gary W. Swearingen) wrote: > Below is the patch on arp.c Looks okay, except for a few minor nits: > @@ -146,6 +150,10 @@ > .Ed > .Pp > with argument meanings as given above. > +Comments may be placed on any line after a ``#'' character. Perhaps: Comments may be placed on any line after a .Dq # character. > +Blank lines cause warnings on the error stream and an exit status of 1, > +but are otherwise be ignored. I think the "otherwise be [sic] ignored" part is redundant; either they cause warnings or they're ignored, but not both. Please send this patch to -audit for review. I'll commit it if nobody objects. Thanks! --------
I wrote:
> swear@blarg.net (Gary W. Swearingen) wrote:
> > Below is the patch on arp.c
>
> Looks okay, except for a few minor nits:
>
> > @@ -146,6 +150,10 @@
> > .Ed
> > .Pp
> > with argument meanings as given above.
> > +Comments may be placed on any line after a ``#'' character.
>
> Perhaps:
>
> Comments may be placed on any line after a
> .Dq #
> character.
>
> > +Blank lines cause warnings on the error stream and an exit status of 1,
> > +but are otherwise be ignored.
>
> I think the "otherwise be [sic] ignored" part is redundant; either
> they cause warnings or they're ignored, but not both.
Oops, please ignore this. I misparsed "warnings" as "errors".
Dima Dorfman <dima@trit.org> writes: > swear@blarg.net (Gary W. Swearingen) wrote: > > Below is the patch on arp.c [snipped comments on the man page patch] > Please send this patch to -audit for review. I'll commit it if nobody > objects. This is my first encounter with -audit, so feel free to complain if I was supposed to say something about this patch that's already in the PR DB or anything else. The original file had this in it: "$FreeBSD: src/usr.sbin/arp/arp.c,v 1.22.2.5 2001/10/18 10:17:47 ru Exp $" --- /usr/src/usr.sbin/arp/arp..orig.c Wed Dec 26 14:33:44 2001 +++ /usr/src/usr.sbin/arp/arp.c Sun Jan 27 13:43:33 2002 @@ -210,6 +210,9 @@ args[4] = &arg[4][0]; retval = 0; while(fgets(line, 100, fp) != NULL) { + if (line[strspn(line, " \t")] == '#') + continue; + line[strcspn(line, "#")] = '\0'; i = sscanf(line, "%49s %49s %49s %49s %49s", arg[0], arg[1], arg[2], arg[3], arg[4]); if (i < 2) { Dima Dorfman <dima@trit.org> writes: > Perhaps: > > Comments may be placed on any line after a > .Dq # > character. (I also removed the extraneous "be" which you noticed.) --- /tmp/arp..orig.8 Sun Dec 9 20:02:06 2001 +++ /tmp/arp.8 Sun Feb 10 17:35:51 2002 @@ -81,7 +81,7 @@ .Tn ARP entries. .It Fl d -A super-user may delete an entry for the host called +A super-user may delete an entry for the host named .Ar hostname with the .Fl d @@ -96,10 +96,10 @@ Show network addresses as numbers (normally .Nm attempts to display addresses symbolically). -.It Fl s Ar hostname ether_addr +.It Fl s Create an .Tn ARP -entry for the host called +entry for the host named .Ar hostname with the Ethernet address .Ar ether_addr . @@ -122,15 +122,19 @@ .Ar ether_addr can be given as .Cm auto -in which case the interfaces on this host will be examined, +in which case the interfaces of the computer running +.Nm +will be examined, and if one of them is found to occupy the same subnet, its Ethernet address will be used. -.It Fl S Ar hostname ether_addr +.It Fl S Is just like .Fl s except any existing .Tn ARP -entry for this host will be deleted first. +entry for the host named +.Ar hostname +will be deleted first. .It Fl f Causes the file .Ar filename @@ -146,6 +150,12 @@ .Ed .Pp with argument meanings as given above. +Comments may be placed on any line after a +.Dq # +character. +Blank lines cause warnings on the error stream and an exit status of 1, +but are otherwise ignored. +Lines longer than 99 characters can cause silent errors. .El .Sh SEE ALSO .Xr inet 3 , State Changed From-To: open->closed All of the issues discussed herein regarding documentation and operation of the arp(8) utility appear to have been addressed within FreeBSD at this time. |
