Hi Sue,
Thanks for the comments. The status on the issues you addressed seem ok
to me.
thanks,
sarah
***
> Hi Sarah,
>
> I'll address the comments towards the bottom.
>
> On 12/18/09 09:30, Sarah Jelinek wrote:
>> 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.
>>
>>
>>> 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.
>>
>>> 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.
>>
>>> 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?
>>
>> instantiate.c:
>>
>> -General: I am a bit confused here. Why do we have libti code in the
>> libtarget_pymod library? I know we need to translate the text
>> installer objects to libti type of attributes, but in looking at the
>> libti_pymod, it seems that there are already functions in there to do
>> what we need to do. Specifically, py_ti_create_target(). You would
>> have to translate the text installer object tuples to the TI
>> attributes and create a new tuple. Is there a reason you felt that
>> setting up the nvlist and calling ti_create_target() here was
>> required, instead of setting up the tuple and calling the libti_pymod
>> functions? If there are gaps in the libti_pymod code, or you plan to
>> merge this in, can you please file a bug to move/merge this code with
>> that as we move forward?
>>
>>
>> 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.
>>
>>> 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?
>>
>> 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?
>>
>> 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?
>>
>> 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);
>>
>> But, it's certainly possible I misunderstand how tp_alloc() works.
>>> 439 if (self->user != NULL)
>>
>> This check isn't required.
>>> 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.
>>
>> zpool.c:
>> -The printfs are for debugging I presume? It would probably be better
>> ro have an explicit debug value that can be set and print accordingly.
>>> 114 if (self->mountpoint == NULL) {
>>> 115 PyErr_NoMemory();
>>> 116 return (-1);
>>> 117 }
>>
>> This type of error in this file seems like a good candidate for a goto.
>>
>> Nit:
>>> 219 * type: type object, do *NOT* assume it is TgtDiskType!
>>
>> This comment is wrong, it is a TgtZFSDataset type.
>>
>> libtd/td_dd.c:
>>> if (errn != 0 || slice_stats == NULL) {
>>> 1965 DDM_DEBUG(DDM_DBGLVL_ERROR,
>>> 1966 "ddm_get_slice_stats(): Can't get slice inuse
>>> data, "
>>> 1967 "for %s, err=%d\n", name, errn);
>>> 1968 return (errn);
>>> 1969 }
>>
>> Shouldn't this be a && here? If the err == 0, then returning and error
>> seems incorrect, even if the slice_stats are NULL. Not sure if this
>> can happen.
>
> Per our chat, will change this to just check errn != 0 here.
>
>> libzoneinfo_pymod/libzoneinfo.c:
>>
>> -Kind of an unfortunate name for this, at first I thought that this
>> code had to do with Solaris Zones. maybe consider renaming. Not a huge
>> deal, just a thought. I suppose you modeled the name after the system
>> libzoneinfo?
>
> Yes. Although I am also not enamored with the name libzoneinfo for the
> reason you mentioned, I felt the consistency in naming with the system
> library was important.
>
> Thanks for the comments,
> Sue
>
>> sarah
>> ****
