Sebastien Roy wrote: > Max and Cecilia, > > This case looks good overall; I'd like a few clarifications and have a > few questions/comments and nit-picks in-line: > > On Tue, 2008-08-12 at 15:40 +0800, Cecilia Hu wrote: > >> 4.2. Python 'dlpi' module >> >> Classification of interfaces provided in this module is 'Committed'. >> The on-line help clearly shows all interfaces and their counterparts >> of dlpi functions in this module as below: >> > > In documenting all of the functions, the definition of error semantics > isn't brought up at all. For a Committed interface, I would expect that > to be made explicit. I've asked some specific questions regarding > return values in-line, but this is more of a general comment that I > think needs to be addressed. > In general, error handling in Python code is done by catching the exception raised by the function it calls. It is required to do so when writing C extension for Python. So, in this dlpi module for Python, I followed the requirement by calling PyErr_SetString(PyExc_OSError, err_code_from_libdlpi_interface) to raise the exception whenever dlpi interface returns an error.
So, in C, we check return value for error, but, in Python, we raise exceptions, instead. And PyErr_setString() is the standard way to raise an exception in C code for other Python code to catch and handle. > >> CLASSES >> __builtin__.object >> link >> >> class link(__builtin__.object) >> | link(linkname, modes) -> link object >> | >> | Open linkname in specified mode. >> | Three modes are supported: PASSIVE, RAW, NATIVE. >> | See dlpi_open(3DLPI). >> > > in dlpi_open(3DLPI), the "modes" are actually "flags" that can be > bitwise-OR'ed together to achieve a combination of behaviors > (DLPI_PASSIVE, DLPI_RAW, and DLPI_NATIVE). I assume that this is also > what's intended with "modes", although it isn't made explicit. > Yes, you can OR them. I'll make it more clear in the on-line help text. > On a related note, I'm no python expert, but aren't symbol names like > "PASSIVE", "RAW", and "NATIVE" likely to clash with symbols defined by > other classes potentially imported by an application? > As pointed out by Danek, these symbols is only visible in dlpi class. > >> | >> | bind(...) >> | bind(sap) -> unsigned int >> | >> | Attempts to bind the link to specified sap, or ANY_SAP. >> | Returns the real sap gets bound. >> | See dlpi_bind(3DLPI). >> > > Suggested rewording: "It returns the SAP that the function actually > bound to, which could be different from the SAP requested." OK. > Also what > happens on error? How does the application differentiate between errors > and SAP values? > An exception will be raised if there is an error. > >> | >> | disabmulti(...) >> | disabmulti(address) -> None >> | >> | Disable a specified multicast address on this link. >> | See dlpi_disabmulti(3DLPI). >> > > dlpi_disabmulti() can fail... > An exception will be raised if dlpi_disabmulti() fails. > >> | >> | enabmulti(...) >> | enabmulti(address) -> None >> | >> | Enable a specified multicast address on this link. >> | See dlpi_enabmulti(3DLPI). >> > > dlpi_enabmulti() can fail, and I think any sane application would want > to know whether or not enabling multicast reception worked. Similar > comment for other functions that don't have error semantics. > An exception will be raised if there is an error. So, application written in Python can catch the exception and handle the error. > >> | >> | enabnotify(...) >> | enabnotify(events, function[, arg]) -> unsigned long >> | >> | Enables a notification callback for the set of specified >> events, >> | which must be one or more (by a logical OR) events listed >> as below: >> | NOTE_LINK_DOWN Notify when link has gone down >> | NOTE_LINK_UP Notify when link has come up >> | NOTE_PHYS_ADDR Notify when address changes >> | NOTE_SDU_SIZE Notify when MTU changes >> | NOTE_SPEED Notify when speed changes >> | NOTE_PROMISC_ON_PHYS Notify when DL_PROMISC_PHYS is set >> | NOTE_PROMISC_OFF_PHYS Notify when DL_PROMISC_PHYS is cleared >> | Returns a handle for this registration. >> | See dlpi_enabnotify(3DLPI). >> > > What happens if this fails? > An exception will be raised if there is an error, just like others. > >> | >> | get_bcastaddr(...) >> | get_bcastaddr() -> string >> | >> | Returns the broadcast address of the link. >> | See dlpi_info(3DLPI). >> > > What is the link has no broadcast address? > If dlpi_info() fails (I get broadcast address by calling dlpi_info()), an exception will be raised accordingly. If dlpi_info() returns no error, but there is no broadcast address, a string Python object, whose length is zero, will be returned. > >> | recv(...) >> | recv(trunksize[, timeout]) -> (string, string) >> | >> | Attempts to receive message over this link. >> | You need to specify the trunk size for the received message. >> | And you can specify timeout value in seconds. >> | Returns (source address, message data). >> | See dlpi_recv(3DLPI). >> > > If timeout is not specified, does recv() behave as dlpi_recv() would if > "msec" were 0 or -1 (i.e., does it not block, or does it block forever > or until a message is received?). > Ah...I did not make it clear. The default value is -1, which means it'll block forever. I'll fix the on-line help here. > >> | >> | send(...) >> | send(destaddr, message[, sap, minpri, maxpri]) -> None >> | >> | Attempts to send message over this link to destaddr. >> | You can specify new sap, and priority range (minpri, maxpri). >> | See dlpi_send(3DLPI). >> > > IMO, "you can specify new sap" is ambiguous. The idea is that a message > is sent to the specified destination address and SAP. If the SAP isn't > specified as the "sap" argument, then the bound SAP is used instead. At > least that's what dlpi_send() does, and I think you want the same > semantics. > Yes, I'll change the on-line help text to make it clear. > >> DATA >> ANY_SAP = 4294967295 >> CURR_PHYS_ADDR = 2 >> DEF_TIMEOUT = 5 >> FACT_PHYS_ADDR = 1 >> IDLE = 3 >> NATIVE = 32 >> NOTE_LINK_DOWN = 8 >> NOTE_LINK_UP = 16 >> NOTE_PHYS_ADDR = 1 >> NOTE_PROMISC_OFF_PHYS = 4 >> NOTE_PROMISC_ON_PHYS = 2 >> NOTE_SDU_SIZE = 128 >> NOTE_SPEED = 256 >> PASSIVE = 2 >> PROMISC_MULTI = 3 >> PROMISC_PHYS = 1 >> PROMISC_SAP = 2 >> RAW = 4 >> UNBOUND = 0 >> UNKNOWN = -1 >> > > Are the values part of the committed interface? If not, then I would > either remove the values from the case materials or explicitly state > that only the symbol names are committed, and that their values are not > an interface. > Only the symbol names are committed. Yes, I need to make it more clear here. Thanks, Seb! Max > -Seb > > >
