[PATCH] Interface description

[John Baldwin]

On Monday 25 January 2010 5:10:35 pm Xin LI wrote:
> Hi,
> 
> I have revised the patchset based on feedback received.  This version:
> 
>  - Unbreak the case when libpcap is being built for pre-ifdescr world.
>  - Documents the descr and -descr primitives for ifconfig(8), they are
> intended for OpenBSD compatibility.
>  - Simplify and concentrate memory allocation in ifconfig(8)
>  - Document the use of nul terminated buffer and the meaning of length
> parameter
>  - Use char* instead of sbuf and simplify the logic in kernel part.
> 
> Hopefully this version would address all problems raised by reviewers.
> Comments?

I just have two suggestions/comments:

@@ -295,6 +295,7 @@ struct      ifreq {
                struct  sockaddr ifru_addr;
                struct  sockaddr ifru_dstaddr;
                struct  sockaddr ifru_broadaddr;
+               struct { size_t length; caddr_t buffer; } ifru_buffer;
                short   ifru_flags[2];
                short   ifru_index;
                int     ifru_jid;

I prefer to not have this all on one line, but to instead be:

                struct  sockaddr ifru_addr;
                struct  sockaddr ifru_dstaddr;
                struct  sockaddr ifru_broadaddr;
                struct {
                    size_t length;
                    caddr_t buffer;
                } ifru_buffer;

Even better would be to actually define a separate type earlier
in the file I think:

	struct ifreq_buffer {
		void *buffer;
		size_t length;
	};

and then just use:

		struct	ifreq_buffer ifru_buffer;

I think caddr_t is deprecated in favor of void * for new APIs at least.

Second, it would be nice if SIOCGIFDESCR provided length feedback to userland
similar to sysctl(3).  Maybe change the code to set ifr.ifr_buffer.length to
the required length when returning ENAMETOOLONG.  Userland can then just skip
to that length directly, or instead use an idiom similar to sysctl where it
does the following:

	ifr.ifr_buffer.buffer = NULL;
	ifr.ifr_buffer.length = 0;
	for (;;) {
		if (ioctl(s, SIOCGIFDESCR, &ifr) == 0) {
			/* have descr in ifr.ifr_buffer.buffer */
		} else if (errno == ENAMETOOLONG) {
			ifr.ifr_buffer.buffer = reallocf(ifr.ifr_buffer.buffer,
			    ifr.ifr_buffer.length);
			if (ifr.ifr_buffer.buffer == NULL) {
				/* handle realloc() failure, break */
			}
			continue;
		} else {
			/* handle error, break */
		}
	}

-- 
John Baldwin