BIND 10 #2850: Define and implement ZoneTableSegmentMapped
BIND 10 Development
do-not-reply at isc.org
Thu May 16 13:59:44 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-20130528
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
-------------------------------------+-------------------------------------
Changes (by muks):
* owner: muks => jinmei
Comment:
Hi Jinmei
Replying to [comment:24 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.
`trac2850_2` was merged to `master` branch.
The following comments are addressed in the `trac2850_3` branch.
> 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.
I had not thought of the Python wrapper case, but having written such a
wrapper in #2852, I can see how this case can arise. It is a valid
point. I've also added a unittest for it now.
> > > > 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).
Nod. In the case of `MemorySegmentMapped`, it already re-gets it by
calling `resetHeader()`, so I haven't changed any code.
> > > > * 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.
This has been added now.
> > > 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.
I have reverted the seed commit (but I've left in the code cleanup
commit).
> > > - 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`.
This has been implemented.
> 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).
This has been added. I've also made the empty string ("") an invalid
name.
> - 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);
> }
> }}}
I have adopted this last method and updated the code accordingly.
> - 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.
In this case, I'll keep the call to `freeReservedMemory()` where it is.
> '''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);
> }}}
This assert has been removed now.
> - processHeader: likewise, we shouldn't need assert (and couldn't
> assert) `!grew`.
This assert has also been removed now.
> - 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_));
> ...
> }}}
This comment has been added.
> - `reset`: in the default case of the switch, I believe
> `InvalidParameter` is a better choice.
Eek! I don't know how I missed that.. it's been corrected.
> - `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.
This comment has been updated.
> '''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.
`getImplType()`'s API doc comment has been updated.
--
Ticket URL: <http://bind10.isc.org/ticket/2850#comment:27>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list