BIND 10 #363: "H" and "I" are probably harmful for PyArg_ParseTuple()

[BIND 10 Development]

#363: "H" and "I" are probably harmful for PyArg_ParseTuple()
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:  defect   |               Status:  reviewing
                 Priority:  minor    |            Milestone:  A-Team-
                Component:           |  Sprint-20110316
  DNSPacket API                      |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  0.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by zzchen_pku):

 * owner:  zzchen_pku => jinmei


Comment:

 Replying to [comment:13 jinmei]:
 > The "Actually..." part hasn't been addressed (but to address that
 > we'll also need to update the C++ Message/MessageRenderer
 > implementation.  So this set of changes would probably go to a
 > separate ticket).
 Message_init, Message_clear, MessageRenderer_setCompressMode already have
 value check, for example:
 {{{
 if (i == Message::PARSE) {
     self->message = new Message(Message::PARSE);
     return (0);
 } else if (i == Message::RENDER) {
     self->message = new Message(Message::RENDER);
     return (0);
 } else {
     PyErr_SetString(PyExc_TypeError, "Message mode must be Message.PARSE
 or Message.RENDER");
     return (-1);
 }
 }}}
 All other values will throw PyExc_TypeError exception.
 > Did you clarify the 'type error' vs 'overflow' for negative values
 > throughout the interfaces (not only for Name_init)?  The error policy
 > should be consistent.
 > These don't seem to be addressed in that it still checks 0xffff (and
 > it still returns 'overflow' for a negative 'pos', see above).
 After checking python doc again, I think ValueError and IndexError are
 more suitable for clarifying the exceptions, how do you fell?
 > > > - RRTTL_init: this doesn't seem to be correct:
 >
 > > I think ignore overflow is fine for this specific case, so I'll keep
 "k", is this ok?
 Okay, I use long long type instead.

 > Besides, simply responding to the RRTTL case doesn't really address my
 > main concern.  My question was more general, not only specific to
 > RRTTL: whether you confirmed all of your new code was really covered
 > by tests (preferably in the way of TDD).  As I said, this was probably
 > not the case.  Please make sure everything was really covered.
 Obviously, I missed something, should be more careful later.
 I runned coverage tool again, all of my new code has been covered by
 unittest.
 > And one more thing regarding tests: now we tightened the range check
 > at the level of python wrapper, I'd also confirm the check isn't too
 > tight with the possible min/max values.
 Most of them just check the possible range for corresponding type, which
 should be ok, did you find something with too tight checking?

-- 
Ticket URL: <http://bind10.isc.org/ticket/363#comment:15>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development