On 18 Nov 2009, at 02:39, Xin LI wrote:
> Here is the revised implementation for the interface description
> feature, based on feedback from src-...@.
Hi Xin Li,
Thanks for the updated patch. This looks significantly improved. Comments
inline.
> --- contrib/libpcap/inet.c (revision 199463)
> +++ contrib/libpcap/inet.c (working copy)
> @@ -403,22 +403,30 @@ add_addr_to_iflist(pcap_if_t **alldevs, const char
> pcap_addr_t *curaddr, *prevaddr, *nextaddr;
> #ifdef SIOCGIFDESCR
> struct ifreq ifrdesc;
> +#ifndef IFDESCRSIZE
> +#define _IFDESCRSIZE 64
> + char ifdescr[_IFDESCRSIZE];
> +#else
> char ifdescr[IFDESCRSIZE];
> +#endif
> int s;
> -#endif
>
> -#ifdef SIOCGIFDESCR
I'm pretty sure the intent here is that 'int s' be defined regardless of
SIOCGIFDESCR, it would be worth confirming that these patches applied to a
pre-SIOCGIFDESCR tcpdump compile correctly. We will want to upstream these
patches to the vendor so we should make sure the changes are acceptable there.
> + 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);
> + break;
> + }
> + if (errno == ENAMETOOLONG) {
> + descrlen *= 2;
> + descr = reallocf(descr, descrlen);
> + }
> + } while ((errno == ENAMETOOLONG) && (descr != NULL));
> + }
The error non-handling throughout ifconfig worries me; on the whole, your patch
seems consistent with the existing model, but here I wonder if we should be
printing a warning if reallocf() fails? Perhaps the above loop could be
restructured so that there is only a single calling point to reallocf --
perhaps while ((descr = reallocf()) != NULL) { ... }? It might make the
invariants a bit more clear.
> + DEF_CMD_ARG("description", setifdescr),
> + DEF_CMD_ARG("descr", setifdescr),
> + DEF_CMD("-description", 0, unsetifdescr),
> + DEF_CMD("-descr", 0, unsetifdescr),
Does having two undocumented short-form aliases make this more usable? We
should either document them or not have them, I guess.
> -.Dd June 18, 2004
> +.Dd November 26, 2009
Curious choice of dates. :-)
> +.It Dv SIOCGIFDESCR
> +Get the interface description, returned in the
> +.Va buffer
> +field of
> +.Va ifru_buffer
> +struct.
> +The user supplied buffer length should defined in the
> +.Va length
> +field of
> +.Va ifru_buffer
> +struct passed in as parameter.
> +.It Dv SIOCSIFDESCR
> +Set the interface description to the value of the
> +.Va buffer
> +field of
> +.Va ifru_buffer
> +struct, with
> +.Va length
> +field specifying its length.
No mention of nul's in the man page yet, but the code now seems much more
consistent about including nul's throughout.
> + case SIOCGIFDESCR:
> + error = 0;
> + if (ifr->ifr_buffer.length > ifdescr_maxlen) {
> + error = ENOMEM;
> + break;
> + }
I have three worries about this comparison:
(1) ifdescr_maxlen is signed, perhaps it should be unsigned?
(2) ifdescr_maxlen could be reduced between SIOCSIFDESCR and SIOCGIFDESCR, in
which case you can no longer query an interface description even though the
kernel is still storing it.
(3) The loop logic in the userland ifconfig tool assumes that it is OK to ask
for more bytes than the largest description, so it doubles the buffer each time
it loops. This means that it may overshoot ifdescr_maxlen leading to the call
failing even though a large enough buffer has been passed.
One of the reasons that potentially unbounded kernel buffers are so awkward is
that they lead to live-locky logic looping trying to grow a buffer using
sleeping allocation while grabbing and releasing locks trying to get a buffer
that's large enough. Most of the time the other theoretical thread you're
racing with won't exist, but this still has to be handled because someday it
will. I suggest a loop in which you create an sbuf to match the current length
of ifp->if_description, and then after the loop ends and you have a copy, see
if it fits in userspace. 99.9999% of the time, userspace will have passed a big
enough buffer, and the loop won't be exercised growing the kernel buffer.
> + else {
> + if (ifr->ifr_buffer.length <=
> sbuf_len(ifp->if_description))
> + error = ENAMETOOLONG;
This may be more clear if it's written as "< sbuf_len(ifp->if_description) +
1". However, if you make the above changes, this goes away, or is at least
refactored.
> + /*
> + * Copy 1 more byte to make sure that the copyout is NUL
Often in the BSD source code, the character is 'nul' and the pointer is 'NULL'.
> --- sys/net/if_var.h (revision 199463)
> +++ sys/net/if_var.h (working copy)
> @@ -205,7 +205,8 @@ struct ifnet {
> * be used with care where binary compatibility is required.
> */
> char if_cspare[3];
> - void *if_pspare[8];
> + void *if_pspare[7];
> + struct sbuf *if_description; /* interface description */
> int if_ispare[4];
> };
I think the conclusion here was that the ifnet spares were a mistake, but I'm
not sure how we decided to resolve that mistake. Should we first remove the
spares in one commit, and then just append new fields in a separate commit?
(Brooks?)
> Some limitations:
> * Not yet able to send announce through route socket. I need to
> figure out a proper way to do this, maybe a future feature;
> * 32-bit vs 64-bit API compatibility. Since the kernel has to copy
> in a string, is there a clean way to do this? I think we will also
> need to deal with similar issue with SIOCSIFNAME as well.
I'm not sure there's a clean way to deal with it; pointers embedded in ioctl
arguments are becoming more of a problem, so I wonder if the answer isn't to
stop introducing any new ones. The Mac OS X kernel is a bit more thorough than
us in implementing ioctls, since they more aggressively select kernel based on
hardware, and contains a lot of fairly awkward compatibility code switching on
whether the process is a 32-bit or 64-bit process and then selecting the right
data structure.
Maybe John has some thoughts on how to handle that.
Robert_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
