[prev in list] [next in list] [prev in thread] [next in thread] 

List:       illumos-developer
Subject:    Re: [developer] [REVIEW] 6649 autofs(4) should describe options set via sharectl(1M)
From:       "Robert Mustacchi" <rm () joyent ! com>
Date:       2016-02-29 23:55:36
Message-ID: 56D4DA78.7070206 () joyent ! com
[Download RAW message or body]

On 2/29/16 15:48 , Yuri Pankov wrote:
> On Mon, 29 Feb 2016 14:03:37 -0800, Robert Mustacchi wrote:
>> On 2/28/16 7:49 , Yuri Pankov wrote:
>>> On Sat, 27 Feb 2016 14:00:37 -0800, Robert Mustacchi wrote:
>>>> On 2/27/16 12:19 , Yuri Pankov wrote:
>>>>> issue: https://illumos.org/issues/6649
>>>>> webrev (mdoc): http://www.xvoid.org/illumos/webrev/il-man-autofs-mdoc/
>>>>>
>>>>> Convert to mdoc - not really looking for reviews on this, there should
>>>>> be no content changes.
>>>>
>>>> I noticed some content changes, fwiw. While some are more reasonable
>>>> (giving headers to various tables), some of it's a bit odd:
>>>>
>>>> +54, +60: The .Sy FIXME don't seem appropriate. I guess this is because
>>>> you wanted a note to yourself to fix it? I presume these two commits
>>>> will be squashed together, right?
>>>
>>> Yes, those changes are temporary to mark what looked really wrong to me.
>>> Will squash the commits once actual changes are OK.
>>>
>>>>> webrev: http://www.xvoid.org/illumos/webrev/il-man-autofs-6649/
>>>>>
>>>>> Actual content changes:
>>>>> - describe the properties set via smf(5)
>>>>> - remove the mentions of /etc/default/autofs from automount(1M) and
>>>>> automountd(1M) man pages
>>>>
>>>>   From taking a brief look at some of the code here, it appears that
>>>> we'll
>>>> actually still read and consume entries from /etc/default/autofs in
>>>> functions like set_nfsv4_ephemeral_mount_to(). What exactly is the deal
>>>> here then?
>>>
>>> Created #6690 for this, checked that no other /etc/default/autofs
>>> variables are used in the tree.
>>>
>>>> High level questions:
>>>>
>>>> o Should we explicitly be telling folks that while it's in SMF, the
>>>> only
>>>> supported interface is sharectl(1M)? By reading autofs(4), I feel like
>>>> I'd be entitled to go edit things with svccfg and svcprop, which I'm
>>>> guessing isn't the case.
>>>
>>> I've taken the wording from smb(4) as you probably already noticed.
>>> Reworded it a bit.
>>>
>>>> man/man4/autofs.4:
>>>>
>>>> +28-34: This sentence is missing some articles and doesn't quite feel
>>>> right. Perhaps something like:
>>>>
>>>> The behavior of the automount(1M) command and the automountd(1M) daemon
>>>> is controlled by property values that are stored in the Service
>>>> Management Facility (smf).
>>>
>>> Done.
>>>
>>>> +36: So, I was trying to figure out what an authorized user actually
>>>> is.
>>>> This dosn't mention it and neither does sharectl, so who's authorized,
>>>> what's required to set these properties. Seems like this needs to be
>>>> defined somewhere. I tried to follow sharectl to smb(4), but I couldn't
>>>> seem to find it there either.
>>>
>>> Was taken from smb(4) as well. Looks like those are authorizations
>>> specific to smb/smbfs (from reading sharectl(1M)), so I re-worded this
>>> paragraph.
>>>
>>>> +40-46: When I read this, it feels like it's missing an article. Maybe
>>>> something like: 'Changes made to autofs property values on an automount
>>>> or automountd command line override the values stored in SMF.'
>>>
>>> Done.
>>>
>>>> +100-102: Perhaps phrase it like: 'If an environment variable has more
>>>> than one value, those values should be separted with the escape
>>>> character XXXX'. I'm not entirely sure what the escape character is
>>>> exactly. I think it's actually two characters, '\,', is that right? If
>>>> it is those two characters, then it's probably the escape sequence
>>>> rather than the escape character.
>>>
>>> This was copy/pasted from the comment in the code that reads this
>>> property, updated with what I think should be more readable.
>>>
>>>> Could the example use some environment variables rather than the
>>>> abstract ones? I think that'd make it a little bit easier to follow. Or
>>>> at least, follow the convention that the variable itself is
>>>> capitalized,
>>>> to help explain what's going on.
>>>
>>> webrev updated.
>>
>> Thanks, a few notes:
>>
>> usr/src/man/man4/autofs.4:
>>
>> +35: We'll want that to be 'The sharectl(1M) command...'
>>
>> +91-93: The sentence is a bit garbled. How about, 'If an environment
>> variable has more than one value, those values should be separated with
>> a comma, preceded by a backslash as an escape character ("\,").'
> 
> Done.

Thanks, I'm happy with it.

Robert




-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/25758058-4e9228dc
Modify Your Subscription: https://www.listbox.com/member/?member_id=25758058&id_secret=25758058-c19b436a
Powered by Listbox: http://www.listbox.com
[prev in list] [next in list] [prev in thread] [next in thread]