All comments save one accepted and changed.
For detailed explanation read inline.
On Dec 18, 2009, at 10:30 AM, Sarah Jelinek wrote:
>>
>> 483 disk = Tgt_NEW(TgtDisk);
>> 484 if (disk == NULL) {
>> 485 PyErr_NoMemory();
>> 486 goto TgtDisk_Create_CLEANUP;
>> 487 }
>> 488
>
>
> disk.c:
>
> Seems like you could just return the PyErr_NoMemory() here. No need to do the
> 'goto'. I don't think you need to call the Py_XDECREF() on this.
Starting with this as its the only one I didn't accept.
Technically I can return PyErr_NoMemory() at this instance.
But all others I do need to decrement the reference.
I prefer to handle all the cases the same, and have one paradigm.
I am flexible on this but here's my take:
yes Py_XDECREF() does a check for NULL and Py_DECREF() doesn't
we are in a failure case, I don't think saving a cmp instruction is worth a
special case
Other comments accepted and fixed as suggested, or clarified inline.
> Hi Keith and all,
>
> My code review comments for the library code. I reviewed all files in:
>
> libtarget_pymod
> libtd
> libzoneinfo
>
> If a file is not listed I had no comments for it. I will send my review of
> the profile code later today.
>
> thanks,
> sarah
> ****
>
> libtarget_pymod/discover.c:
>> 69 //PyThreadState *_save;
>> 70 //_save = PyEval_SaveThread();
>
> These need to be removed or used.
Removed. The Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS take care of this.
We refactored code to have one entry/exit. Which allows us to use the standard
macros.
I should not have left those in.
>
>
>> 319 if (attrl != NULL)
>> 320 td_attribute_list_free(attrl);
>
> Not sure you need to check for attrl != NULL here. Looks like the function
> will do the right thing, either way. Ditto for other places you are checking
> for this.
Agreed all cases in discover.c, all removed.
>
>
>> 97 PyObject *blocks = NULL; /* Python2.4 missing uint64_t fmt spec
>> */
>
> This reference to an issue in python 2.4 should no longer be an issue, right?
>
>> 136 /* Python 2.4 has no scan code for a potentially 64-bit value */
>
> So, does python 2.6 have scan code for a potential 64-bit value?
Yes, but we are not
>
> instantiate.c:
[... snip ... already addressed by Jean ]
>
> partition.c:
>> 33 /*
>> 34 * NOTE: libdiskmgmt has labelled partition data incorrectly. We correct
>> it here
>> 35 * in the Python names. TgtPartition data structure still use the
>> 36 * libdiskmgmt naming scheme. Here is the conversion:
>> 37 *
>> 38 * libdiskmgmt | Python | example
>> 39 * --------------+--------+----------------
>> 40 * id | number | 1
>> 41 * type | id | 0xBF (Solaris2)
>> 42 * typetypething | type | ??? (Primary)
>> 43 *
>> 44 * libdiskmgt doesn't yet recognize anything but Primary partitions but
>> will
>> 45 * soon change.
>> 46 */
>
> Two things.. if there are bugs in libdiskmgt, please file a bug report. The
> last comment is no longer valid, so please remove.
Last comment removed. Jean clued me in on the id/type thing. I think we will
need to be absolutely sure before filing a bug as I suspect the nomenclature in
libdiskmgmt is ARCed.
>
>> 57 * XXX until extended partitions project goes back 1-4 are only partition
>> 58 * "id" libdiskmgmt understands.
>> 59 */
>> 60 #define MAXID 4
>> 61 #define STR_MAXID STRINGIZE(MAXID)
>
> This needs to be changed. And, we should use the fdisk defines for this, not
> our own. Extended partition support has been integrated into libdiskmgt.
>
>> 502 assert(result != NULL);
>
> We are asserting here that there are always slice objects as children of a
> partition. Is this always true?
Code has changed a lot, but I believe you are referring to:
static PyObject *
TgtPartition_get_children(TgtPartition *self, void *closure)
{
PyObject *result = self->children;
assert(result != NULL);
Py_INCREF(result);
return (result);
}
Yes, self->children *must* exist. It is guaranteed by TgtPartition_New() to be
an empty tuple if never set.
>
> General question:
> -Is it possible to have more than one 'label' in python for goto's? It seems
> like the answer is no, but I see that we have errors handled sometimes inline
> and sometimes with a 'goto'. I actually find this confusing. Especially,
> when we have an error label but we don't always use it. For example:
>> f (newblk % geo->cylsz != 0) {
>> 603 PyErr_SetString(PyExc_ValueError,
>> 604 "\"blocks\" must be a multiple of "
>> 605 "tgt.Geometry.cylsz");
>> 606 return (-1);
>> 607 }
>
> Is there anyway to combine this with the other label and use a goto?
This particular case is removed as we discovered that cylinder boundaries don't
need to be used for partitions.
But the larger question's answer is that we can't always use the same label if
we want to be "Pythonic".
That is the 1st arg to PyErr_SetString() is not always the same. In this case
we had wanted a ValueError exception to be raised.
Sometimes we want a TypeError, etc. And we also were trying to be more helpful
with the strings.
Although in practice we have found that PyErr_Format() is more helpful that
PyErr_SetString().
We would probably work more on converting those except that I think we would
recommend future modules use "ctypes" and not
implement C extension modules.
>
> Another general comment: Sometimes in the code we comment the reference
> counting manipulation that is done, and sometimes we don't. Perhaps we should
> be more liberal in the areas where it might be confusing. For example:
>
>> 663 static PyObject *
>> 664 TgtPartition_get_id(TgtPartition *self, void *closure)
>> 665 {
>> 666 PyObject *result = PyInt_FromLong((long)self->id);
>> 667 if (result == NULL)
>> 668 return (PyErr_NoMemory());
>> 669 return (result);
>> 670 }
>
> The call to PyIn_FromLong returns a new reference, but this may not be
> obvious to other folks. Not a deal breaker, just a suggestion.
>
>> 40 #define MAXNUM 15
>
> Is there a define somewhere we can use that is more 'official'. Kind of like
> what is provided in fdisk.h?
Yes and changed.
>
> slice.c:
>
> It looks like the definition for
>> 200 #define SET_BOOL(nm) self->nm = (nm == Py_True) ? 1 : 0
>
> SET_BOOL is the same wherever it is used. I think it would be cleaner to use
> #defines in the header files as required and only if the macro is really
> different to the #define and #undef inline.
>
>> 214 * type: type object, do *NOT* assume it is TgtSliceType!
>
> This comment implies you are going to do some checking with regard to the
> 'type' value passed in,but in the code it is simply
> assumed to be a slice object:
> 225 self = (TgtSlice *)type->tp_alloc(type, 0);
Actually that is intentional. Perhaps comment is misleading.
What I meant was, don't use `TgtSlice.tp_alloc(...)`.
This method, call the alloc of whatever the heck was passed in, is the norm.
This way if somebody subclassed tgt.Slice in Python we would call correct alloc
routine.
>
> But, it's certainly possible I misunderstand how tp_alloc() works.
>> 439 if (self->user != NULL)
>
> This check isn't required.
Well actually we don't know if self->user is a character string or not.
As we convert NULL into "unknown" on the fly in TgtSlice_get_user().
>> 518 return (PyLong_FromLongLong(self->blocks));
>
>
> Sometimes you return the value from these calls, and sometimes you check for
> NULL and return PyErr_NoMemory():
>
>> 550 if (result == NULL)
>> 551 return (PyErr_NoMemory());
>
> It would be good if this was consistent, unless there is a specific reason
> that this is different in some cases.
Nope, I got no reason.
PyLong_FromLongLong() and similar will set PyErr_NoMemory() on its own.
So 550/551 has been removed.
