On Wed, 11 Nov 2009, Xin LI wrote:
Author: delphij
Date: Wed Nov 11 21:30:58 2009
New Revision: 199201
URL: http://svn.freebsd.org/changeset/base/199201
Log:
Add interface description capability as inspired by OpenBSD.
Patches like this keep being proposed and rejected, so I was a bit surprised
to see it hit the tree (but also see that I missed yet another thread on it on
net@ in August, apparently). Most of my thoughts on it are here:
http://www.freebsd.org/cgi/query-pr.cgi?pr=83622
This commit seems to have quite a few problems, and would likely have
benefited from further review before being committed.
Modified: head/contrib/libpcap/inet.c
==============================================================================
--- head/contrib/libpcap/inet.c Wed Nov 11 21:18:27 2009 (r199200)
+++ head/contrib/libpcap/inet.c Wed Nov 11 21:30:58 2009 (r199201)
@@ -403,22 +403,30 @@ add_addr_to_iflist(pcap_if_t **alldevs,
pcap_addr_t *curaddr, *prevaddr, *nextaddr;
#ifdef SIOCGIFDESCR
struct ifreq ifrdesc;
+#ifdef __FreeBSD__
+#define _IFDESCRSIZE 64
Seems like this should be #ifdef IFDESCRSIZE rather than ifdef FreeBSD? Also,
have you checked with upstream to see if autoconf foo wouldn't be preferred?
+ char ifdescr[_IFDESCRSIZE];
+#else
char ifdescr[IFDESCRSIZE];
- int s;
#endif
+ int s;
-#ifdef SIOCGIFDESCR
/*
* Get the description for the interface.
*/
memset(&ifrdesc, 0, sizeof ifrdesc);
strlcpy(ifrdesc.ifr_name, name, sizeof ifrdesc.ifr_name);
+#ifdef __FreeBSD__
+ ifrdesc.ifr_buffer.buffer = ifdescr;
+ ifrdesc.ifr_buffer.length = _IFDESCRSIZE;
+#else
ifrdesc.ifr_data = (caddr_t)&ifdescr;
sizeof(ifdescr) would be more robust in the presence of future code changes.
@@ -258,6 +258,12 @@ Disable permanently promiscuous mode.
Another name for the
.Fl alias
parameter.
+.It Cm description Ar value
+Specify a description of the interface.
+This can be used to label interfaces in situations where they may
+otherwise be difficult to distinguish.
+.It Cm -description
+Clear the interface description.
Possibly a comment on length limits would be appropriate?
Modified: head/sbin/ifconfig/ifconfig.c
==============================================================================
--- head/sbin/ifconfig/ifconfig.c Wed Nov 11 21:18:27 2009
(r199200)
+++ head/sbin/ifconfig/ifconfig.c Wed Nov 11 21:30:58 2009
(r199201)
@@ -83,6 +83,8 @@ static const char rcsid[] =
struct ifreq ifr;
char name[IFNAMSIZ];
+char *descr = NULL;
+size_t descrlen = 64;
int setaddr;
int setmask;
int doalias;
@@ -822,6 +824,36 @@ setifname(const char *val, int dummy __u
free(newname);
}
+/* ARGSUSED */
+static void
+setifdescr(const char *val, int dummy __unused, int s,
+ const struct afswtch *afp)
+{
+ char *newdescr;
+
+ newdescr = strdup(val);
+ if (newdescr == NULL) {
+ warn("no memory to set ifdescr");
+ return;
+ }
+ ifr.ifr_buffer.buffer = newdescr;
+ ifr.ifr_buffer.length = strlen(newdescr);
+ if (ioctl(s, SIOCSIFDESCR, (caddr_t)&ifr) < 0) {
+ warn("ioctl (set descr)");
+ free(newdescr);
+ return;
I'm confused about the semantics here: is ifr_buffer.buffer guaranteed to be
nul-terminated or not? In some cases the code seems to imply nul-termination
(especially on the string coming back to userspace), but in other places (such
as here), the nul termination is not included in the buffer. It would be nice
to consistently do one or the other, and given the way the code is written, I
suggest always having the nul termination.
@@ -866,6 +898,23 @@ status(const struct afswtch *afp, const
printf(" mtu %d", ifr.ifr_mtu);
putchar('\n');
+ descr = reallocf(descr, descrlen);
+ if (descr != NULL) {
+ do {
+ ifr.ifr_buffer.buffer = descr;
+ ifr.ifr_buffer.length = descrlen;
+ if (ioctl(s, SIOCGIFDESCR, &ifr) == 0) {
+ if (strlen(descr) > 0)
+ printf("\tdescription: %s\n", descr);
Here, we seem to assume that the buffer is nul-terminated.
+ break;
+ }
+ if (errno == ENAMETOOLONG) {
+ descrlen *= 2;
+ descr = reallocf(descr, descrlen);
+ }
+ } while (errno == ENAMETOOLONG);
+ }
Shouldn't we be erroring out if reallocf() fails? That seems to be normal
practice elsewhere in ifconfig.
Modified: head/sys/net/if.c
==============================================================================
--- head/sys/net/if.c Wed Nov 11 21:18:27 2009 (r199200)
+++ head/sys/net/if.c Wed Nov 11 21:30:58 2009 (r199201)
@@ -463,6 +463,8 @@ if_free_internal(struct ifnet *ifp)
#ifdef MAC
mac_ifnet_destroy(ifp);
#endif /* MAC */
+ if (ifp->if_description != NULL)
+ sbuf_delete(ifp->if_description);
IF_AFDATA_DESTROY(ifp);
IF_ADDR_LOCK_DESTROY(ifp);
ifq_delete(&ifp->if_snd);
@@ -2090,6 +2092,45 @@ ifhwioctl(u_long cmd, struct ifnet *ifp,
ifr->ifr_phys = ifp->if_physical;
break;
+ case SIOCGIFDESCR:
+ IF_AFDATA_RLOCK(ifp);
+ if (ifp->if_description == NULL)
+ error = ENOMSG;
+ else
+ error = copystr(sbuf_data(ifp->if_description),
+ ifr->ifr_buffer.buffer,
+ ifr->ifr_buffer.length, NULL);
+ IF_AFDATA_RUNLOCK(ifp);
Isn't copystr() for use only on kernel addresses, and isn't ifr_buffer a user
address? And if this is a user address, you can't access it while the
IF_AFDATA lock is held.
+ case SIOCSIFDESCR:
+ error = priv_check(td, PRIV_NET_SETIFDESCR);
+ if (error)
+ return (error);
+
+ IF_AFDATA_WLOCK(ifp);
This is not really what this lock is for, perhaps it's time to introduce an
actual per-if lock.
+ if (ifp->if_description == NULL) {
+ ifp->if_description = sbuf_new_auto();
Can't do potentially sleeping memory allocations with the afdata lock held.
+ if (ifp->if_description == NULL) {
+ error = ENOMEM;
+ IF_AFDATA_WUNLOCK(ifp);
+ break;
+ }
+ } else
+ sbuf_clear(ifp->if_description);
+
+ if (sbuf_copyin(ifp->if_description, ifr->ifr_buffer.buffer,
+ ifr->ifr_buffer.length) == -1)
Access to user addresses while holding the afdata lock isn't allowed.
Here, ifr_buffer.buffer is treated as non-nul-terminated.
Some administrative limit on string length here would be appropriate; perhaps
128 bytes or 1024 bytes -- panicking because someone passes in a 1GB string by
mistake in some C code is not what we want to do.
Modified: head/sys/net/if.h
==============================================================================
--- head/sys/net/if.h Wed Nov 11 21:18:27 2009 (r199200)
+++ head/sys/net/if.h Wed Nov 11 21:30:58 2009 (r199201)
@@ -294,6 +294,7 @@ struct ifreq {
struct sockaddr ifru_addr;
struct sockaddr ifru_dstaddr;
struct sockaddr ifru_broadaddr;
+ struct { size_t length; caddr_t buffer; } ifru_buffer;
While ifreq already contains a pointer, this adds two more fields that vary in
size across 32/64-bit, making it more difficult to support in 32-bit processes
on a 64-bit kernel.
Modified: head/sys/net/if_var.h
==============================================================================
--- head/sys/net/if_var.h Wed Nov 11 21:18:27 2009 (r199200)
+++ head/sys/net/if_var.h Wed Nov 11 21:30:58 2009 (r199201)
@@ -198,6 +198,7 @@ struct ifnet {
void *if_pf_kif;
void *if_lagg; /* lagg glue */
u_char if_alloctype; /* if_type at time of allocation */
+ struct sbuf *if_description; /* interface description */
/*
* Spare fields are added so that we can modify sensitive data
@@ -205,7 +206,7 @@ struct ifnet {
* be used with care where binary compatibility is required.
*/
char if_cspare[3];
- void *if_pspare[8];
+ void *if_pspare[7];
int if_ispare[4];
};
Could you confirm that ifnet hasn't changed size on arm, i386, and amd64?
This looks like fairly dubious use of a spare field -- normally I'd expect to
see if_cspare, which is 3 bytes long, remain after if_alloctype, and that
pspare[0] be replaced in-place to prevent alignment decisions in the compiler
from spacing out the structure further (i.e., adding another byte of padding
on i386 and another 5 bytes of padding on amd64 after if_cspare and before
if_pspare).
Robert N M Watson
Computer Laboratory
University of Cambridge
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
