BIND 10 #2850: Define and implement ZoneTableSegmentMapped
BIND 10 Development
do-not-reply at isc.org
Thu May 2 01:04:18 UTC 2013
#2850: Define and implement ZoneTableSegmentMapped
-------------------------------------+-------------------------------------
Reporter: jinmei | Owner:
Type: task | jinmei
Priority: medium | Status:
Component: data source | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-20130514
Sub-Project: DNS | Resolution:
Estimated Difficulty: 5 | CVSS Scoring:
Total Hours: 0 | Defect Severity: N/A
| Feature Depending on Ticket:
| shared memory data source
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Comment (by jinmei):
I'm in the middle of the review, but I'm dumping the comments so far.
I guess these are most substantial in the whole review and would be
quite independent, so you probably want to check them sooner.
'''general'''
- On a closer look, we really don't even need `getZoneWriter()` method
at all, as `ZoneWriter` is now a concrete class and can be
instantiated directly. That will make the user side simpler. I
suggest:
- removing this method
- checking the "writable" condition in the `ZoneWriter` constructor
- adjusting the user side accordingly (which should be trivial). I
committed this change for client_list.cc (85873bf)
'''Makefile.am (multiple)'''
- We need to make sure "mapped" related sources are compiled only if
"USE_SHARED_MEMORY" is set. I also suggest building and testing the
whole tree `--without-shared-memory`.
'''zone_table_segment.cc'''
- Like Makefile.am, we need to make sure we won't include or
instantiate mapped-related things when unavailable.
- getZoneWriter: I suggest using `isc::InvalidOperation` instead of
`Unexpected`. I'd use the latter only when it's really
"unexpected", such as a very rare system error the case where it's
most likely an internal bug but we cannot completely eliminate the
possibility of a bad user input. (but per the above general
suggestion, this suggestion would go to the `ZoneWriter` constructor).
'''zone_table_segment.h'''
- getZoneWriter: it needs to describe possible exception.
(but per the above general suggestion, this suggestion would go to
the `ZoneWriter` constructor).
- isWritable doc: in general, I'd like to keep the base class
documentation agnostic about derived class details. So I suggest
changing it to:
{{{#!cpp
/// The user of the zone table segment will load or update zones
/// into the segment only for writable ones. The precise definition
/// of "writability" differs in different derived classes (see derived
/// class documentation). In general, however, the user should only
rely
/// on this interface rather than assume a specific definition for a
/// specific type of segment.
}}}
And update derived classes documentation about their definition of
writability.
- isWritable doc: it's probably safe to say an implementation of this
method must be exception free (simply because it should be able to
be so).
- reset() must be defined as virtual (consider, e.g., how we implement
#2852. no one other than the base `ZoneTableSegment` class (and
memmgr, though still indirectly in that case) has to be aware of the
(non)existence of specific derived classes).
'''zone_table_segment_mapped.h'''
- As noted above, `reset` must be defined at the base class, and so be
`MemorySegmentOpenMode`. The documentation should be abstracted
(CREATE, READ_WRITE, READ_ONLY should be explained at some
conceptual level, not directly referring to a "file", etc).
'''zone_table_segment_mapped.cc'''
- you don't have to define the destructor (in this case)
- I'd avoid hardcoding magic keywords like "zone_table_checksum".
- isWritable(): I'd simply return false if `mem_sgmt_` is NULL.
In fact, this is completely valid because it's indeed "non writable"
without the underlying memory segment. If we throw in that case,
the caller (possibly) needs to do behave differently depending on
whether it results in an exception, which is convenient. And, I
actually expect we'll encounter this situation in client_list.
- getHeader, getMemorySegment: in my sense I wouldn't use `Unexpected`
in these cases (see above). Also, if we allow getHeader to throw,
this should be documented in the base class.
'''ZoneTableSegmentMapped::reset'''
- I'd perform validation checks at the beginning of the method.
- this method is too big to understand. in particular, the
huge switch block severely damages the readability (for example in
my screen I cannot even be sure how many cases it has "at a
glance"). I suggest delegating each case to helper private methods:
{{{#!cpp
void
ZoneTableSegmentMapped::reset(MemorySegmentOpenMode mode,
isc::data::ConstElementPtr params)
{
// validation
switch (mode) {
case CREATE:
resetCreate();
break;
case READ_WRITE:
resetReadWrite();
break;
case READ_ONLY:
resetReadOnly();
break;
default:
// hmm, what should we do? (see below)
}
}
}}}
If the method is concise like this, it's easier to notice that there
could be a missing case as commented above (and see the next point).
- we need to handle the "default" case above. enums cannot completely
eliminate this case.
- this method repeats same/similar patterns for handling checksum and
table header. I'd consider unify them, especially because some
cases would be tricky (see below).
- I'd like to clarify the level of exception guarantee as much as
possible. The current implementation only provides weak guarantee
(strictly speaking it doesn't even provide the weak level because
some allocated resource could remain in the mapped file). I noticed
it's probably impossible to provide the strong guarantee (either
success or fall back to original state) for all cases due to the
restriction of mapped memory segment, but I can still see it
possible in many other cases (and these are actually our intended
use cases). For example, if the files are different, we can delay
releasing the original segment (we need to remember the original
file name). So, I would:
- clarify in which case we can provide the strong guarantee
- if it's not too complicated, provide the strong guarantee when
it's possible
- also if possible, make it possible for the caller to tell which
level of exception guarantee is provided in case of exception
(e.g., by using different types of exception).
- document these, including what the caller should do in each case.
note also that this method must be virtual, and the documentation
should be generic (not relying on the mapped version specifics).
- maybe we should also make it possible to clear (just close any
existing segment) the segment?
- in general, we need to cover the case where allocate() results in
`MemorySegmentGrown` exception. same for `ZoneTable::create()`, and
setNamedAddress (if it returns true). for that matter, I noticed
`ZoneTable::create()` itself would need some tricker consideration
regarding this point. One solution would be to handle these things
correctly, of course. But, considering it should be quite unlikely
at the initialization time (with a sufficiently large size of
initial segment size), maybe some optimistic simplification is
acceptable:
{{{#!cpp
// allocate the space for the named address first
segment->setNamedAddress("zone_table_header", NULL);
try {
void* ptr = segment->allocate(sizeof(ZoneTableHeader));
ZoneTableHeader* new_header = new(ptr)
ZoneTableHeader(ZoneTable::create(*segment, rrclass_));
const bool grown =
segment->setNamedAddress("zone_table_header", new_header);
assert(!grown); // note that we've already allocated the space
above.
header_ = new_header;
} catch (const MemorySegmentGrown&) {
// Unlikely case. Don't bother to try to recover.
isc_throw(MaybeSomeCustomException);
}
}}}
- I'd assert checksum is not NULL here:
{{{#!cpp
uint32_t* checksum = static_cast<uint32_t*>
(segment->getNamedAddress("zone_table_checksum"));
}}}
- not directly related, but I just realized the return value of
`getNamedAddress()` is ambiguous: it cannot distinguish the case
where the given name doesn't exist from the case where the named
address is NULL. If not too heavy, I'd also revise the interface
so it returns `std::pair<bool, void*>`, whose first member indicates
whether the name exists.
- In my sense I wouldn't use `Unexpected` in this case (see above)
{{{#!cpp
if (saved_checksum != new_checksum) {
isc_throw(isc::Unexpected,
"Saved checksum doesn't match mapped segment
data");
}
}}}
The most likely cause of this would be that the segment data is
broken, which is completely possible. I'd probably use a dedicated
exception type for cases of broken segments in general; the caller
may want to handle that case specifically (e.g., remove it and
create a new one from the scratch). I'd also include the file name
in the what() message (maybe also for some other cases too).
- this breaks strong exception guarantee:
{{{#!cpp
header_ = static_cast<ZoneTableHeader*>
(segment->getNamedAddress("zone_table_header"));
if (!header_) {
isc_throw(isc::Unexpected,
"There is no previously saved ZoneTableHeader in a "
"mapped segment opened in read-only mode.");
}
}}}
- this comment should be able to be removed:
{{{#!cpp
// The segment was already shrunk when it was last closed. Check
// that its checksum is consistent.
// FIXME: We can't really do this as we can't set the checksum
// to 0 for checksum calculation in a read-only segment.
}}}
--
Ticket URL: <http://bind10.isc.org/ticket/2850#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list