Clay Baenziger wrote:
> 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
>>>> -------------------------------------------
>>>> 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.
It'd seem we should just remove the remove_newlines= argument
from readline() altogether then, and just not process newlines
in that function at all. Given the issue, no consumer of readline()
could/should ever call it with remove_newlines set to anything
other than False.
>
>> 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
>>>> ----------------------------------------
>>>
>>> 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?
Yes, something like this would seem cleaner...
>
>> 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).
Thanks for verifying this.
thanks,
-ethan
