Hi Ethan,
Thank you for your comments. My responses in-line.
Thank you,
Clay
On Mon, 28 Sep 2009, Ethan Quach wrote:
>>> usr/src/cmd/installadm/installadm_common.py
>>> -------------------------------------------
>>> 36 - What's the significance of using the name "Unix" here?
>>
>> None much. It was suggested in the code walk through by someone. However,
>> it has no meaning and perhaps should just be File_ as it just trivially
>> extends the file class.
>
> Okay, can you please go ahead and make this change then. My
> concern was that if our goal for this code is for it to approach
> something portable, then naming something that wasn't necessarily
> Unix specific with that name could cause confusion down the line.
This has been changed. The class is now known as File_()
>>> 65 - In this method, if newlines = False, and blanklines = True,
>>> what will get returned at 107 when processing a blankline in
>>> from a file? An empty string '' ??
>>
>> An empty string would be returned but from line 104 not 107. Why, are you
>> thinking a particular use should be warned against in the docstring?
>
> Sorry, that's what I meant to ask about, the earlier return at 105ish.
>
> Let me restate the comment then...
>
> At 107, it returns the empty string to denote to the caller that we're
> out of lines, but if we're returning the empty string at 105 for the
> case above, then won't the caller get confused?
Yes, this is a very good realization. Thank you! I've modified readlines
to perform the newline removal for now so that this problem is not hit.
> i.e. if we're processing a file with an empty line in the middle of
> the file somewhere, and its being processed with newlines = False and
> blanklines = True, won't this wad of code (readlines() + readline() )
> stop processing in the middle of the file?
>
>>>
>>> usr/src/cmd/installadm/delete-service.py
>>> ----------------------------------------
>>> 413 - I dislike how you have to visit an additional time just
>>> to check existence. I thought there existed some NO_IDX
>>> exception when searching a list. If so, you can then just
>>> wrap 415 in a try
>>
>> Hi Ethan, there is a ValueError thrown. I think we're getting into pretty
>> micro-style/optimization here. However, I can change the code from:
>>
>> To find out this is a value error, try:
>>>>> a=[1,2]
>>>>> a[3]
>> Traceback (most recent call last):
>> File "<stdin>", line 1, in ?
>> IndexError: list index out of range
>>
>> (This is what vfstabObj.fields.MOUNT_POINT[idx] could in all fairness
>> raise)
>>
>>>>> a.index(3)
>> Traceback (most recent call last):
>> File "<stdin>", line 1, in ?
>> ValueError: list.index(x): x not in list
>>
>> (This is what vfstabObj.fields.MOUNT_POINT.index(boot_archive) would raise)
>>
>> Thus the code now reads:
>>
>> # look for filesystem in /etc/vfstab
>> try:
>> # calculate index for mount point
>> idx = vfstabObj.fields.MOUNT_POINT.index(boot_archive)
>> try:
>> # remove line containing micro root
>> del(vfstabObj.fields.MOUNT_POINT[idx])
>> except IOError, e:
>> sys.stderr.write(str(e) + "\n")
>> # microroot was not found in /etc/vfstab
>> except (ValueError, IndexError):
>> sys.stderr.write (_("Microroot for service %s " +
>> "not in vfstab.\n") % service.serviceName)
>> return
>>
>>> try:
>>> idx = vfstabObj.fields.MOUNT_POINT.index(boot_archive)
>>>
>>> try:
>>> # remove line containing micro root
>>> del(vfstabObj.fields.MOUNT_POINT[idx])
>>> except IOError, e:
>>> sys.stderr.write(str(e) + "\n")
>>>
>>> except NO_INDEX_OR_WHATEVER_THE_EX_IS, e:
>>> sys.stderr.write (_("Microroot for service %s " +
>>> "not in vfstab.\n") % service.serviceName)
>>>
>>>
>>> (Ideally, I would have like to have seen the caller not
>>> need to handle the idx at all, but I won't push for that
>>> at this point.)
>>
>> Dude, I can't make it magically delete things, one has to tell it what you
>> want to delete. How else would one specify what to delete? This is a list
>> after all, not an associative array (aka dictionary) as otherwise the
>> DBFile would have to have knowledge of what columns contain unique data and
>> which can have repeated data.
>>
>
> That's part of the point. Why should the caller need to know if the
> vfstabObj is a list or an array? Secondly, your current implementation
> has the caller extracting something from the object, just to pass it
> back into the object. That something is the idx in this case, and
> there doesn't seem to be any value or meaning for the caller to
> ever have to handle that. So my hinted suggestion was to build a
> member function for the object to hide that detail. For example,
> the caller code could end up looking like this:
>
> try:
> vfstabObj.delentry_by_mntpnt(boot_archive)
>
> # or if we want to parameterize the field out ...
> # (this is probably not correct python syntax, but I know
> # its possible somehow.)
> # vfstabObj.delentry(com.VFSTAB.fields.MOUNT_POINT, boot_archive)
>
> except (ValueError, IndexError):
>
> except IOError, e:
>
> except Whatever Else:
>
>
> Is this over java-fying the code?
Yes, I think this is why Python is different than Java :)
I think this why Python implements the type() function. This is how the
class is implemented modeling a list.
Certainly, we could at the cost of losing a lot of generalization and
adding more code implement something which is very vfstab specific.
But, that's how I see it...
> Lastly, as I noted in the original comment, this is not something
> I am not pushing for this in this putback, but since you're asking
> I'm just clarifying what my comment was.
Cool, thank you for clarifying. I was very confused how, in Python, we
could implement something cleaner. I think as you well put it, the answer
isn't a very Pythonic approach likely -- or at least not very general.
Perhaps a vfstab_obj.del.<field>("key to delete") would be a possible
approach which would be extensible?
> Based on Jan's comment and your filing of 11537, the vfstab
> looks like it may be handled differently altogether in our AI
> server tools, correct?
>
>
>>
>>> 453 - This doesn't look correct. The manpage for in.tftpd
>>> does not say that 'homedir' is the argument to the -s
>>> option. In fact, based on the synopsis, it looks like
>>> 'homedir' can float.
>>>
>>> SYNOPSIS
>>> in.tftpd [-d] [-T rexmtval] [-s] [homedir]
>>>
>>>
>>> So it seems you have to parse fields.SERVER_ARGUMENTS,
>>> looking for the argument that's not an option, and not
>>> an option argument.
>>
>> Thank you for catching this! I've fixed it to take the last argument --
>> much easier to parse out too.
>
> Did you check whether or not the [homedir] argument can float?
> Based on the synopsis, [homedir] looks like it can be put anywhere
> in the command line, not necessarily always at the end. Can you
> verify whether or not this is the case?
I did and I can't do, for example, '/usr/sbin/in.tftpd /tftpboot -d'.
Further, I check that what I get is an absolute path (not to be confused
with rexmtval (which is a numeric value).
>>
>>> 474-478 - I really think we should leave it up to the caller
>>> to check this, so that the interface of findTFTProot()
>>> is defined as always to return something, with the default
>>> being /tftpboot
>>>
>>> (Looking ahead at your code, the callers already do this
>>> checking)
>>
>> Actually this is broken both ways. The callers try to build a path off an
>> unset baseDir variable. I added a meaningful error for the user that we
>> can't do much deleting without a TFTProot (this egregious enough I'm
>> tempted to do a SystemExit, but right now I've added the following checks
>> after line 501:
>> else:
>> sys.stderr.write (_("Unable to remove the grub executable, boot "+
>> "archive, or menu.lst file\nwithout a valid "+
>> "tftp root directory.\n"))
>
>
> Okay, that's fine.
>
> Comment on new delete_service.py file:
> 471-473 - Can you update this error message to be more clear?
> I couldn't tell from the message that something doesn't exist.
> Here's a suggested rewrite of the comment
>
> "The tftp root directory (%s) found from the configuration
> file (%s) does not exist. Using default: %s\n"
Done
> 482-483 - Same comment.
>
> "The tftp root directory (%s) does not exist.\n"
Done
>>
>>
>>> 500-501, 688-689 - Umm, can you please make findTFTProot()
>>> cache its answer then. The chances of someone changing
>>> /etc/inetd.conf in between time is negligible afaic.
>>>
>>> And actually, if you do my previously suggested comment,
>>> you can remove the if()'s at 500 and 688.
>>
>> As the inetd class will be caching the data this is trivial in code
>> runtime. And not easily coded (as findTFTProot is a function not a class)
>> further 500-501, 688-689 are in different scopes (well one's a nested
>> function of the other but that would be very ugly to understand where those
>> vars are coming from to get it to persist). If you think this will save
>> more than 500ms off the runtime of this program I'll profile it to see but
>> this is trivial and very over-optimizing to concern ourselves with.
>
> Since you're not taking my original comment for 474-478 above,
> nevermind this comment.
>
>>
>>>
>>> usr/src/cmd/installadm/delete_client.py
>>> ---------------------------------------
>>> 41 - this comment isn't needed anymore.
>>
>> I've changed from:
>> "Parse and validate options when called as delete-client"
>> to:
>> "Parse and validate options"
>>
>> and for further concern I've changed from parse_client_options to
>> parse_options.
>
> Okay thanks.
>
>>
>> Please I'd like to stop working on nits and ensure the program works this
>> has been out for code review for greater than nearly a month.
>
> You've got a large amount of delta here for the differential though.
> You can't expect to make this much change again and not get an
> initial-like review for the parts that are freshly written.
That is true, just want to be done; we're getting close!
>>
>>> 43 - is this comment still accurate?
>>
>> It is, but perhaps changing service to client would cause less confusion as
>> it returns a client_Data class
>
> That would be a good start.
Done
> thanks,
> -ethan
