Bug 1202 - packed_rrset_data is not always 'packed'
packed_rrset_data is not always 'packed'
Status: RESOLVED FIXED
Product: unbound
Classification: Unclassified
Component: server
unspecified
Other All
: P5 enhancement
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-06 17:06 CET by JINMEI Tatuya
Modified: 2017-01-10 10:03 CET (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description JINMEI Tatuya 2017-01-06 17:06:51 CET
util/data/packed_rrset.h has the following code comment about struct
packed_rrset_data:

 * The data is packed, stored contiguously in memory.
 * memory layout:
...

But this is not always the case.  At least packed_rrset_data created
or adjusted in insert_rr() and find_tag_datas() of
services/local.zone.c does not necessarily meet this assumption, since
they allocate memory for rr_len, rr_ttl, rr_data and wired data
separately.

This means, for example, we cannot subsequently use utilities for
these '(not-really-)packed data' that rely on the memory layout
assumption, such as packed_rrset_copy_alloc/region or
packed_rrset_sizeof().

Such an exception is probably intentional, and there may be some
implicit usage assumption such as this layout can only be assumed when
the data is retrieved from cache.  If so, I wouldn't be opposed to
that intent per se, but it would be safer to note that in code
comments.  I would note:
- the memory layout assumption is not always met except for some known
  cases (describing in which case we can assume that)
- add note to public functions that rely on the assumption as such

I'm referring to svn trunk rev 3982, but it applies to many older
revisions, too.
Comment 1 Wouter Wijngaards 2017-01-09 10:46:03 CET
Hi Jinmei,

This comment added to packed_rrset.h:
 * It is not always stored contiguously, in that case, an unpacked-packed
 * rrset has the arrays separate.  A bunch of routines work on that, but
 * the packed rrset that is contiguous is for the rrset-cache and the
 * cache-response routines in daemon/worker.c.

I think that may be sufficient?

Best regards, Wouter
Comment 2 JINMEI Tatuya 2017-01-09 18:13:44 CET
Works for me (as long as the behavior is intentional, it's just a matter of documentation, for which I don't have a strong opinion).
Comment 3 Wouter Wijngaards 2017-01-10 10:03:57 CET
Hi Jinmei,

Yes, documentation is a good idea if the design gets this awkward.

Best regards, Wouter