BIND 10 #2850: Define and implement ZoneTableSegmentMapped
BIND 10 Development
do-not-reply at isc.org
Tue May 14 06:59:00 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 think we can merge the branch to master at this point. There are
some open issues, but those won't affect other tickets that use the
result of the branch. So it'd be more helpful if we share the current
version of the branch in master.
Replying to [comment:22 muks]:
> > > > - we need to handle the "default" case above. enums cannot
completely
> > > > eliminate this case.
> > >
> > > I think of this everytime when writing `switch`, but I believe the
> > > current approach is superior.
[...]
> > Not all "modern" compilers can detect the logical violation of the
> > enum range (in fact, no compilers that I know of complain about it).
>
> I don't understand why we would want to support something like this. A
> lot of code in the tree will break when forcibly made like this.
For example? While not adopted as an official guideline, I (both as a
developer or reviewer) generally try to catch these "impossible" enum
cases if it can be called by arbitrary applications. In this
particular case, it's particular risky IMO, because it would have a
Python wrapper, which would convert int to `MemorySegmentOpenMode`
using `static_cast`. I admit it might look awkward, and I wish
compilers are really smart enough to reject such cases. But that's
not the reality.
> > Replying to [comment:16 muks]:
> > It would be independent from whether/what to do for checksum.
>
> Separately, are you considering removing the checksum functionality?
I've not fully thought over it. But I suspect we'll need some kind of
framework for integrity check in any case.
> > > I missed talking about two more issues:
> > >
> > > * The `setNamedAddress()` interface could be problematic in practice
as
> > > it may relocate addresses in the segment.
[...]
> > I agree it's error prone. One possibility of improving it would be to
> > have setNamedAddress() return the latest real address (that may be
> > re-addressed after setting the name):
[...]
> > or, we might even pass `void**`:
> >
> > The latter is safer, while a bit inconvenient when we want to pass
> > NULL.
>
> I have implemented your latest suggestion for this, which is to reserve
> an `offset_ptr` during `MemorySegmentMapped` construction (please see
the
> previous comment).
Okay, but note that this suggested update doesn't solve the main
problem here: passed address may still be reallocated in
`setNamedAddress()`, so the caller needs to make sure to re-get
the address by `getNamedAddress()` (unless set... returns false).
> > > * If there is a missing checksum or header (by corruption or
programming
> > > mistake), `READ_WRITE` mode will not warn (it will think the file
was
> > > freshly created). Is there a reason why we'd want to add `CREATE`
> > > functionality to this mode too?
> >
> > Yes. This way the memmgr can always specify READ_WRITE, whether the
> > file already exists or not (the latter is the case of the very first
> > time). CREATE is normally unnecessary; it would be used only when
> > READ_WRITE fails due to possible data corruption (memmgr could remove
> > the file by itself and create it with READ_WRITE, though).
>
> Hmm. Shall I check in `READ_WRITE` mode if the file exists before
> proceeding to create a fresh checksum, header, etc.?
Ah, to be very accurate, maybe. But I think it's okay to do some
lightweight check, e.g., checking if `allMemoryDeallocated()` is true.
> > Specific comments on the revised branch follow:
>
> > - createData and verifyData: they rely on the fact that
> > `UniformRandomIntegerGenerator` produces the same sequence of random
> > numbers from the same seed.
>
> This has been updated so that the data for the test is created in one
> place. I have not removed my modifications to
> `UniformRandomIntegerGenerator` as it may be useful to have a seed
> argument in the RNG (and there were also some cleanups made).
I'd apply YAGNI here, especially because we'll full-rewrite the
resolver. Once merged master, any code establishes inertia against
cleanup, so if we want to apply YAGNI, this is the best timing.
But if you see strong need for it in foreseeable future, I won't argue
about it any more.
> > - what about processHeader() failure in case of READ_WRITE?
>
> In case of `READ_WRITE`, if the header is missing, there's no way in the
> current implementation for us to know if it's because the file was
> created freshly, or because there was corruption.
>
> Shall I add code to check if the mapped file exists in `reset()` before
> a `MemorySegmentMapped` is constructed? This way we can know for sure if
> a header must exist beforehand.
It's probably too much, but we may be able to do the check with
`allMemoryDeallocated`.
Comments on the latest branch (including answers to some of the
questions):
'''memory_segment(_mapped, base)'''
- I'd like to reserve names beginning with '_' for internal use and
shouldn't be used with `xxxNamedAddress`, at least in documentation,
and preferably also updating the implementation so it explicitly
checks the condition and rejects reserved names (but we should
probably defer it to a separate task; this is quite beyond scope of
the original subject of the task and we've already spent too much time
for
unrelated changes).
- I'd like to avoid making allMemoryDeallocated() non const. Also, it
could now even throw an exception in theory. I have a couple of
suggestions of alternative:
- cheating + no-throw: compiler shouldn't complain even if we insist
it's const because the non-const operation is done through a
pointer member variable. and, in some sense, it could be
justified as we actually preserve externally visible state of the
class. to make this point clearer and make it 100% exception free,
we could insist reserveMemory() should succeed without growing the
segment (although this may not be 100% guaranteed in the boost API,
this should be the case at least in practice).
- count the allocated/deallocated size in `MemorySegmentMapped`
itself (just like `MemorySegmentLocal`), and implement
`allMemoryDeallocated` as follows:
{{{#!cpp
bool
MemorySegmentMapped::allMemoryDeallocated() {
const size_t num_named_obj =
impl_->base_sgmt_->get_num_named_objects();
const size_t expected_num_named_obj = impl_->read_only_ ? 0 : 1;
return (allocated_size_ == 0 && num_named_obj ==
expected_num_named_obj);
}
}}}
- as for 3c98ade56f07c44740f1bde2e37f8fdc9842bd18: this is because if
`allocate()` fails due to `bad_alloc`, it resets `base_sgmt_`. but
while looking into problems for #2836, I realized this behavior was
bad and proposed a change
(3a6ae66c2a737475ed55fb28cd3cd49b49e7295e). With this change that
particular problem should be resolved, but in any case I believe
we should call `freeReservedMemory()` before `flush()`; otherwise
the effect of `freeReservedMemory()` might not be flushed to the
underlying file before other process starts using it.
'''zone_table_segment_mapped.cc'''
- processChecksum(): with the latest changes, setNamedAddress() can
return true, so in theory we cannot assert it's false (although very
unlikely in this context). but in this case it shouldn't matter
anyway because the rest of the code including its callers doesn't
rely on the absolute addresses. so I'd simply remove the assert.
{{{#!cpp
const bool grew =
segment.setNamedAddress(ZONE_TABLE_CHECKSUM_NAME,
checksum);
assert(!grew);
}}}
- processHeader: likewise, we shouldn't need assert (and couldn't
assert) `!grew`.
- processHeader: as it should be unlikely to fail I think we should
move forward for now, but as we know through #2836, there are
several potential pitfalls here. I'd at least leave comments, and
create a ticket so we can hopefully make it cleaner and safer.
{{{#!cpp
// FIXME: in theory this code is not safe:
// - ZoneTable::create could throw MemorySegmentGrown, leaking
// ptr
// - even on successful return from ZoneTable::create(), ptr
// could be reallocated due to its internal implementation
detail
// So, to make it 100% safe we should protect both ptr and
// zone table in something similar to SegmentObjectHolder, get
// their addresses via the holder's get() method, and expect
// MemorySegmentGrown and handle it. However, in this
specific
// context the segment should have sufficient capacity in
practice
// and the above cases are extremely unlikely to happen. So
// we go for simpler code for now.
ZoneTableHeader* new_header = new(ptr)
ZoneTableHeader(ZoneTable::create(segment, rrclass_));
...
}}}
- `reset`: in the default case of the switch, I believe
`InvalidParameter` is a better choice.
- `isWritable`: this comment seems to need revise:
{{{#!cpp
// If reset() was never performed for this segment, or if the
// most recent reset() had failed, then the segment is not
// writable.
}}}
It should include the case where it's cleared.
'''zone_table_segment.h'''
- `getImplType`: I'd specify the returned name should be identical to
the corresponding type for the derived class passed to create().
The implementation already (maybe by coincidence) meets this, so
it's only the documentation matter. This string comes from the
configuration and also return value of the method implemented in
#2943. memmgr should be able to know the relationship between
these.
--
Ticket URL: <http://bind10.isc.org/ticket/2850#comment:24>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list