BIND 10 #1451: Create DDNS process module
BIND 10 Development
do-not-reply at isc.org
Mon Dec 19 11:08:02 UTC 2011
#1451: Create DDNS process module
-------------------------------------+-------------------------------------
Reporter: jelte | Owner: jinmei
Type: task | Status: reviewing
Priority: major | Milestone:
Component: DDNS | Sprint-20111220
Keywords: | Resolution:
Defect Severity: N/A | Sensitive: 0
Feature Depending on Ticket: DDNS | Sub-Project: DNS
Add Hours to Ticket: 0 | Estimated Difficulty: 6
Internal?: 0 | Total Hours: 9
-------------------------------------+-------------------------------------
Changes (by jelte):
* owner: jelte => jinmei
Comment:
Replying to [comment:13 jinmei]:
> It looks almost okay. I have two last comments.
>
> - I'm afraid it could be dangerous to do all shutdown jobs in the
> middle of a signal handler. For example, check_command() could
> still be called after shutdown(), but the latter may destruct some
> fundamental attribute of the class at that point. In general, I
> think it's safer to minimize the things within a signal handler
> (e.g., just setting a flag) and do other complicated jobs in the
> usual control flow. I have a proposed patch to implement this idea.
> (dddns.diff, attached)
>
Ah, now I get it; I was only looking at the code and didn't see the
problem (as the proposed patch currently doesn't alter the behaviour). The
comment 'and cleanup should go here' was incorrect, and probably the name
too (these caused the confusion I think).
Applied your patch, but with a few amendments; I think the logging of the
event really should go in the trigger function (as the biggest reason to
have two log messages is to be able to see if it didn't shut down
immediately and why). And I renamed the former shutdown() to
shutdown_cleanup(), for added clarity. Also added a line in docstring that
trigger_shutdown() is the function one needs when the module needs to be
stopped.
This function is now a noop, which contradicts the no-unused-code
principle, but as this whole dummy module is kind of a violation on that,
I think it is ok.
> - TestMain repeats the same pattern and I think it's better to combine
> them in a separate helper method. See attached diff
> (exception.diff)
>
> If you are fine with these patched, please just apply them and merge.
> If you simply don't agree, I'm okay with it for now; please feel free
> to merge the current code. If you don't agree right now but want to
> discuss it more, please continue.
Applied as-is.
I assume the changes are trivial enough, but for completeness, I am
assigning this back to you for one last time ;)
--
Ticket URL: <http://bind10.isc.org/ticket/1451#comment:15>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list