Re: Merging ns_parse from Alpine

[Felix Janda <felix.janda@posteo.de>]

Rich Felker wrote:
> On Sun, Dec 14, 2014 at 08:38:15AM +0100, Felix Janda wrote:
> > Rich Felker wrote:
> > > I'm working on merging Timo's patch for ns_parse:
> > > 
> > > http://git.alpinelinux.org/cgit/aports/tree/main/musl/1001-add-basic-dns-record-parsing-functions.patch?id=81d50064c335467fdfd80368bac6707d70db1af7
> > > 
> > > The first issue that came up in the process is that arpa/nameser.h,
> > > which was previously not used by musl itself and really should never
> > > have been accepted in its current form, is full of junk like
> > > statement-expressions. Including it in a file that will be compiled
> > > with musl adds build dependency on these nonstandard features. I
> > > cleaned that up with no problem (just un-inlining the macros since
> > > we're adding function versions anyway), but there are a few more
> > > issues.
> > 
> > The NS_GET* macros still seem to be used a lot in the code.
> 
> Yes because they also advance the pointer, and this behavior was
> intentional in the code. I don't think it hurts to use them once
> they're fixed to be function calls.

I likely just missunderstood you. With un-inlining the macros you mean
making them call the ns_get* functions? (That seems reasonable.)


I agree with your comments on ns_initparse and ns_skiprr.

[...]
> > > int ns_parserr(ns_msg *handle, ns_sect section, int rrnum, ns_rr *rr)
> > > {
> > > 	int r;
> > > 
> > > 	if (section < 0 || section >= ns_s_max) goto bad;
> > > 	if (section != handle->_sect) {
> > > 		handle->_sect = section;
> > > 		handle->_rrnum = 0;
> > > 		handle->_msg_ptr = handle->_sections[section];
> > > 	}
> > > 	if (rrnum == -1) rrnum = handle->_rrnum;
> > > 	if (rrnum < 0 || rrnum >= handle->_counts[section]) goto bad;
> > > 	if (rrnum < handle->_rrnum) {
> > > 		handle->_rrnum = 0;
> > > 		handle->_msg_ptr = handle->_sections[section];
> > > 	}
> > > 	if (rrnum > handle->_rrnum) {
> > > 		r = ns_skiprr(handle->_msg_ptr, handle->_eom, section, rrnum - handle->_rrnum);
> > > 		if (r < 0) return -1;
> > > 		handle->_msg_ptr += r;
> > > 		handle->_rrnum = rrnum;
> > > 	}
> > > 	r = dn_expand(handle->_msg, handle->_eom, handle->_msg_ptr, rr->name, NS_MAXDNAME);
> > > 	if (r < 0) return -1;
> > 
> > dn_expand doesn't set errno.
> 
> Maybe we should just call ns_name_uncompress (below) instead here?
> 
> > [...]
> > > int ns_name_uncompress(const unsigned char *msg, const unsigned char *eom,
> > >                        const unsigned char *src, char *dst, size_t dstsiz)
> > > {
> > > 	int r;
> > > 	r = dn_expand(msg, eom, src, dst, dstsiz);
> > > 	if (r < 0) errno = EMSGSIZE;
> > > 	return r;
> > > }
> 
> Does that sound better?

That should work. I have no particular opinion on whether to call
ns_name_uncompress or to jump to "size".

Felix