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
> ****
