Hello Andrzej, There is still some work to be done for this. See last email from Matt. Feel free to pick this one up as I'm not going to get to it anytime soon.
-Alek On Tue, Nov 4, 2014 at 1:56 AM, Andrzej Szeszo <[email protected]> wrote: > Hi All > > What is the status of this integration? I have started looking at it > myself and only then found this thread, webrev, etc. > > Cheers, > > Andrzej > > > On 19 April 2014 02:16, Matthew Ahrens <[email protected]> > wrote: > >> >> >> >> On Fri, Apr 18, 2014 at 2:29 PM, Alek Pinchuk <[email protected]> >> wrote: >> >>> Hello, >>> >>> Matt, thanks for taking a look again. See response inline: >>> >>> On Fri, Apr 18, 2014 at 10:26 AM, Matthew Ahrens <[email protected]> >>> wrote: >>> >>>> On Sat, Apr 5, 2014 at 4:22 PM, Matthew Ahrens <[email protected]> >>>> wrote: >>>> >>>>> I just read the Oracle documentation. It sounds like they implemented >>>>> it so that "-o prop=val" implements the option C that I described: set >>>>> the property on the topmost filesystem, and "force inherit" it onto all >>>>> descendants (doing the equivalent of a "zfs inherit" on each of the >>>>> descendants). And I am guessing that they made "-x" do the equivalent of >>>>> "zfs inherit" after receiving the property. These semantics seems >>>>> reasonable, and I think *we should implement the same behavior*. >>>>> >>>>> >>>> Do you have a response to the above comment? I would like to see a >>>> strong argument for why we should implement a feature that on the surface >>>> appears to be the same as an Oracle ZFS feature, but is actually subtly >>>> different. >>>> >>> >>> If I understand you correctly the patch already does this: >>> >>> root@alek401:/volumes# zfs get sharenfs test >>> NAME PROPERTY VALUE SOURCE >>> test sharenfs off default >>> root@alek401:/volumes# zfs create test/ds >>> root@alek401:/volumes# zfs create test/ds/child >>> root@alek401:/volumes# zfs get sharenfs test/ds >>> NAME PROPERTY VALUE SOURCE >>> test/ds sharenfs off default >>> root@alek401:/volumes# zfs get sharenfs test/ds/child >>> NAME PROPERTY VALUE SOURCE >>> test/ds/child sharenfs off default >>> root@alek401:/volumes# zfs set sharenfs=on test/ds >>> root@alek401:/volumes# zfs get sharenfs test/ds/child >>> NAME PROPERTY VALUE SOURCE >>> test/ds/child sharenfs on inherited from test/ds >>> root@alek401:/volumes# zfs get sharenfs test/ds >>> NAME PROPERTY VALUE SOURCE >>> test/ds sharenfs on local >>> root@alek401:/volumes# zfs snapshot -r test/ds@0 >>> >>> root@alek401:/volumes# zfs send -R test/ds@0 | zfs recv -o sharenfs=off >>> test/ds2 >>> root@alek401:/volumes# zfs get sharenfs test/ds2 >>> NAME PROPERTY VALUE SOURCE >>> test/ds2 sharenfs off received >>> root@alek401:/volumes# zfs get sharenfs test/ds2/child >>> NAME PROPERTY VALUE SOURCE >>> test/ds2/child sharenfs off inherited from test/ds2 >>> >> >> I don't see where you are treating the topmost filesystem differently >> from the filesystems beneath it, though in practice it seems to be doing >> that in the example above. What would happen if the property was also set >> on test/ds/child? >> >> >> >>> root@alek401:/volumes# zfs send -R test/ds@0 | zfs recv -x sharenfs >>> test/ds3 >>> root@alek401:/volumes# zfs get sharenfs test/ds3 >>> NAME PROPERTY VALUE SOURCE >>> test/ds3 sharenfs off default >>> root@alek401:/volumes# zfs get sharenfs test/ds3/child >>> NAME PROPERTY VALUE SOURCE >>> test/ds3/child sharenfs off default >>> >> >>> Perhaps we can have a quick chat to establish what you would like to see >>> done here? >>> >> >> Really what I would like to see is a comparison of the behavior of these >> changes and Oracle ZFS. It should cover things like: >> >> Incremental receive, where the property is set on child filesystems; does >> -o prop=value set the property on all the filesystems? Does it set the >> property on the top filesystem that is received and the children inherit >> that prop? >> >> For example, if you do "zfs send -R test/ds@a | zfs recv -o sharenfs=rw >> test/recvd", I think that Oracle ZFS would result in the following: >> >> # zfs get -o all -r sharenfs test >> NAME PROPERTY VALUE RECEIVED SOURCE >> test sharenfs off - default >> test/ds sharenfs on - local >> test/ds@0 sharenfs - - - >> test/ds/child sharenfs off - local >> test/ds/child@0 sharenfs - - - >> test/recvd sharenfs rw on local >> test/recvd@0 sharenfs - - - >> test/recvd/child sharenfs rw off inherited from >> test/recvd >> test/recvd/child@0 sharenfs - - - >> >> I.e. the result of doing "zfs set sharenfs=rw test/recvd; zfs inherit >> sharenfs test/recvd/child". I don't see how the code under review would >> accomplish this. It seems like it would do "zfs set sharenfs=rw >> test/recvd; zfs set sharenfs=rw test/recvd/child". >> >> --matt >> >> >>> >>> >>>> >>>> On Thu, Apr 17, 2014 at 11:33 AM, Alek Pinchuk < >>>> [email protected]> wrote: >>>> >>>>> Hello, >>>>> >>>>> I've updated the webrev a bit based on some review comments. >>>>> >>>>> Please see the updated webrev: http:// >>>>> <http://alek_p.bitbucket.org/webrevs/2745/>alek_p.bitbucket.org >>>>> <http://alek_p.bitbucket.org/webrevs/2745/>/ >>>>> <http://alek_p.bitbucket.org/webrevs/2745/>webrevs >>>>> <http://alek_p.bitbucket.org/webrevs/2745/>/2745/ >>>>> <http://alek_p.bitbucket.org/webrevs/2745/> >>>>> >>>>> HTML version of the modified manpage: http:// >>>>> <http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html> >>>>> alek_p.bitbucket.org >>>>> <http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html>/ >>>>> <http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html>webrevs >>>>> <http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html>/2745/ >>>>> <http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html>zfs >>>>> <http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html>.1m.html >>>>> <http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html> >>>>> >>>>> >>>>> The biggest change from the earlier webrevs is that now the >>>>> add_unique_option() call doesn't return anything, and will ignore >>>>> duplicate >>>>> props instead of failing. >>>>> >>>>> Since the behavior is pretty obvious (the identical options are >>>> useless but harmless, just like if I specify any other option multiple >>>> times, e.g. -ee), how about not printing anything in this case? >>>> >>> >>> Sure, I've update the webrev. >>> >>> Please see the updated webrev: http://alek_p.bitbucket.org/webrevs/2745/ >>> HTML version of the modified manpage: >>> http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html >>> >>> -Alek >>> >>> >>>> >>>> >>>>> Again, this is Steven's FreeBSD patch with some slight modifications. >>>>> >>>>> >>>>> -Alek >>>>> >>>>>> On Apr 8, 2014 2:03 PM, "Alek Pinchuk" <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> Hello Matt, >>>>>>> >>>>>>> Thanks for taking a look. See response inline: >>>>>>> >>>>>>> >>>>>>> On Sat, Apr 5, 2014 at 4:22 PM, Matthew Ahrens <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> I just read the Oracle documentation. It sounds like they >>>>>>>> implemented it so that "-o prop=val" implements the option C that I >>>>>>>> described: set the property on the topmost filesystem, and "force >>>>>>>> inherit" it onto all descendants (doing the equivalent of a "zfs >>>>>>>> inherit" >>>>>>>> on each of the descendants). And I am guessing that they made "-x" do >>>>>>>> the >>>>>>>> equivalent of "zfs inherit" after receiving the property. These >>>>>>>> semantics >>>>>>>> seems reasonable, and I think *we should implement the same >>>>>>>> behavior*. I'm also fine with dropping the "dataset#prop" stuff. >>>>>>>> >>>>>>>> >>>>>>> In the interest of time I've dropped the "dataset#prop" stuff from >>>>>>> the patch. If someone needs this features in illumos please post a >>>>>>> webrev >>>>>>> that adds it. >>>>>>> >>>>>>> >>>>>>> >>>>>>>> --matt >>>>>>>> >>>>>>>> >>>>>>>> On Sat, Apr 5, 2014 at 4:14 PM, Matthew Ahrens <[email protected] >>>>>>>> > wrote: >>>>>>>> >>>>>>>>> Alek, thanks a lot for picking up these changes. I have a few >>>>>>>>> comments / questions: >>>>>>>>> >>>>>>>>> ndmpd_zfs.c and be_create.c have Nexenta copyrights added to >>>>>>>>> them. I just wanted to confirm that these changes were contributed by >>>>>>>>> Nexenta. It's fine if so, it just stood out compared to the other >>>>>>>>> files >>>>>>>>> which have no copyright added (which is also fine). >>>>>>>>> >>>>>>>> >>>>>>> Yes the changes in those two files are Nexenta sponsored work. I've >>>>>>> made them while "on the clock". >>>>>>> >>>>>>> >>>>>>>> >>>>>>>>> zfs.1m: >>>>>>>>> It looks like you've copied the documentation from the Oracle >>>>>>>>> manpages. Are you sure that we are allowed to do that? I.e. what >>>>>>>>> license >>>>>>>>> grants us the right to reuse Oracle's documentation? >>>>>>>>> >>>>>>>>> >>>>>>> I just ported what was in the FreeBSD patch. The manpage part was >>>>>>> changing the FreeBSD markup to use the illumos manpage markup. Did not >>>>>>> know >>>>>>> that the manpage was lifted from Oracle. >>>>>>> In light of this I "rewrote" the manpage for this patch. >>>>>>> >>>>>>> It is now probably too minimalistic but is not copied from Oracle. >>>>>>> It would be great to get some feedback on what people would like to see >>>>>>> in >>>>>>> there. >>>>>>> >>>>>>> >>>>>>>> I don't quite follow the documentation around the "-o" and "-x" >>>>>>>>> flags: >>>>>>>>> - if I specify "-o prop=value", it says it's as though I did "zfs >>>>>>>>> set prop=value". So the property is marked as a "locally set" >>>>>>>>> property, as >>>>>>>>> opposed to a "received" property, right? >>>>>>>>> - If I specify "-o prop=value", and it is an inheritable property >>>>>>>>> and I am receiving a "-R" stream, is the behavior: >>>>>>>>> - A. set the property on every filesystem (this seems bad) >>>>>>>>> - B. set the property on the topmost filesystem that's part >>>>>>>>> of the stream (descendants will inherit it or not depending on what >>>>>>>>> their >>>>>>>>> local settings are) >>>>>>>>> - C. set the property on the topmost filesystem, and "force >>>>>>>>> inherit" it onto all descendants (doing the equivalent of a "zfs >>>>>>>>> inherit" >>>>>>>>> on each of the descendants) >>>>>>>>> - B or C seem OK, but you should document the behavior. >>>>>>>>> Unfortunately, it looks like A is what was implemented. >>>>>>>>> - if I specify "-x prop", it says it's as though the property was >>>>>>>>> not present in the send stream. Given that, I don't understand the >>>>>>>>> following sentences "If a received property needs to be overridden, >>>>>>>>> the >>>>>>>>> effective value will be set or inherited, depending on if the >>>>>>>>> property is >>>>>>>>> inheritable or not. In the case of an incremental update, -x leaves >>>>>>>>> any >>>>>>>>> existing local setting or explicit inheritance unchanged (since the >>>>>>>>> received property is already overridden)." Why would a received >>>>>>>>> property >>>>>>>>> need to be overridden? The latter sentence implies that you're >>>>>>>>> actually >>>>>>>>> setting the property as a received property, and then overriding it >>>>>>>>> with a >>>>>>>>> "local inherit". Which is different from what would happen if the >>>>>>>>> property >>>>>>>>> was not present in the send stream. It sounds like it might be that >>>>>>>>> "-x >>>>>>>>> prop" for inheritable properties actually does the equivalent of "zfs >>>>>>>>> inherit prop <every fs that's received>", unless there is already a >>>>>>>>> local >>>>>>>>> setting for that fs in which case it has no effect. It looks like >>>>>>>>> what's >>>>>>>>> implemented is that -x causes us to ignore that prop if it is in the >>>>>>>>> stream. Please change the documentation to match. >>>>>>>>> >>>>>>>>> libzfs_sendrecv.c: >>>>>>>>> 2043: should be "skipping receive of %s" >>>>>>>>> >>>>>>>> >>>>>>> fixed >>>>>>> >>>>>>> 2935: can you explain the handling of has_exprops? It seems like >>>>>>>>> (a) you could just inline nvlist_empty(exprops) where necessary, and >>>>>>>>> (b) if >>>>>>>>> (stream_wantsnewfs), has_exprops is uninitialized (dsname is also >>>>>>>>> uninitialized) >>>>>>>>> >>>>>>>> >>>>>>> fixed >>>>>>> >>>>>>> >>>>>>>> 2567: I think that "props" is the properties from the send stream >>>>>>>>> (not "current properties"), and "exprops" is the properties set on the >>>>>>>>> command line (via -x and -o). I don't know what "external properties" >>>>>>>>> means. >>>>>>>>> >>>>>>>> >>>>>>> changed the comments, but the variable name is still exprops. I can >>>>>>> change that (to cli_props?) if needed. >>>>>>> >>>>>>> Please see the updated webrev: >>>>>>> http://alek_p.bitbucket.org/webrevs/2745/ >>>>>>> HTML version of the modified manpage: >>>>>>> http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html >>>>>>> >>>>>>> -Alek >>>>>>> >>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Fri, Apr 4, 2014 at 9:11 PM, Alek Pinchuk < >>>>>>>>> [email protected]> wrote: >>>>>>>>> >>>>>>>>>> Hello, >>>>>>>>>> >>>>>>>>>> I know members of the OpenZFS community (including my employer) >>>>>>>>>> would really like to see this cross the goalline and be integrated. >>>>>>>>>> >>>>>>>>>> And since it looks like this effort has stalled (and I couldn't >>>>>>>>>> get a hold of Steven) I've decided to port his FreeBSD patch to >>>>>>>>>> illumos and >>>>>>>>>> the webrev is available at >>>>>>>>>> http://alek_p.bitbucket.org/webrevs/2745 >>>>>>>>>> >>>>>>>>>> I had to touch two files (ndmpd_zfs.c & be_create.c) that were >>>>>>>>>> not part of the FreeBSD patch. >>>>>>>>>> >>>>>>>>>> The rest of the code is basically unchanged from Steven's. I >>>>>>>>>> think I've addressed all of Matt's review comments from the previous >>>>>>>>>> email. >>>>>>>>>> Please take a look at the webrev and let me know if there is >>>>>>>>>> anything else >>>>>>>>>> that needs to be changed. >>>>>>>>>> >>>>>>>>>> This is what the illumos ZFS manpage looks like after the patch: >>>>>>>>>> http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html >>>>>>>>>> >>>>>>>>>> I'll make sure Steven Hartland is the author of the patch >>>>>>>>>> submitted for RTI. >>>>>>>>>> >>>>>>>>>> -Alek >>>>>>>>>> >>>>>>>>>> P.S. >>>>>>>>>> git pbchk will complain since I didn't add any copyright to the >>>>>>>>>> files changed by the FreeBSD patch since it didn't have any copyright >>>>>>>>>> additions in it. >>>>>>>>>> Copyrights: >>>>>>>>>> usr/src/lib/libzfs/common/libzfs_impl.h: no copyright claim for >>>>>>>>>> current year found >>>>>>>>>> usr/src/lib/libzfs/common/libzfs_dataset.c: no copyright claim >>>>>>>>>> for current year found >>>>>>>>>> usr/src/cmd/zfs/zfs_main.c: no copyright claim for current year >>>>>>>>>> found >>>>>>>>>> usr/src/lib/libzfs/common/libzfs.h: no copyright claim for >>>>>>>>>> current year found >>>>>>>>>> usr/src/lib/libzfs/common/libzfs_sendrecv.c: no copyright claim >>>>>>>>>> for current year found >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Mon, Feb 25, 2013 at 1:02 PM, Matthew Ahrens < >>>>>>>>>> [email protected]> wrote: >>>>>>>>>> >>>>>>>>>>> Steven, thanks for following up on this. >>>>>>>>>>> >>>>>>>>>>> It would be really great if you could integrate this into >>>>>>>>>>> illumos. I think all you would need to do is convert the manpage >>>>>>>>>>> changes >>>>>>>>>>> (illumos uses a different markup language for manpages), apply your >>>>>>>>>>> diffs, >>>>>>>>>>> build and test on illumos, and post a webrev for review on >>>>>>>>>>> [email protected]. >>>>>>>>>>> >>>>>>>>>>> It would also be great if you could work with John Kennedy to >>>>>>>>>>> integrate test for the new features into the zfs test suite. >>>>>>>>>>> >>>>>>>>>>> I took another look at your diffs and noticed just a few small >>>>>>>>>>> things that could be improved: >>>>>>>>>>> >>>>>>>>>>> zfs.8 line ~2383 misspells "receive" >>>>>>>>>>> line ~2420: >>>>>>>>>>> +If a received property needs to be overridden, the effective >>>>>>>>>>> value can be >>>>>>>>>>> +set or inherited, depending on the property. >>>>>>>>>>> I think you mean "will be set or inherited" (the user doesn't >>>>>>>>>>> choose). You could also mention what it depends on ("depending on >>>>>>>>>>> if the >>>>>>>>>>> property is inheritable or not"). >>>>>>>>>>> >>>>>>>>>>> We shoud clarify that the filesystem/volume names must be >>>>>>>>>>> specified as they existed on the sending system (as opposed to the >>>>>>>>>>> names >>>>>>>>>>> that they will be received into). >>>>>>>>>>> >>>>>>>>>>> zfs_main.c: >>>>>>>>>>> since parsepropname() is used not just for properties (e.g. for >>>>>>>>>>> -l), you might call it something like "addonce" and not have its >>>>>>>>>>> error >>>>>>>>>>> message use the term "property". >>>>>>>>>>> >>>>>>>>>>> props_override(): >>>>>>>>>>> >>>>>>>>>>> Can your calls to nvlist_add_*() ever fail? (e.g. due to the >>>>>>>>>>> name already existing)? I don't think so, in which case you should >>>>>>>>>>> VERIFY0(nvlist_add_*()), or better yet use the fnvlist_* routines. >>>>>>>>>>> >>>>>>>>>>> + char *sepp = strchr(propname, sepp); >>>>>>>>>>> I don't think this will compile; you probably want >>>>>>>>>>> "strchr(propname, sep)". Be sure to test this out. >>>>>>>>>>> >>>>>>>>>>> zfs_receive_one(): "skip" should be a boolean_t. >>>>>>>>>>> >>>>>>>>>>> Again, thanks for doing this Steven. Looking forward to having >>>>>>>>>>> it in illumos and FreeBSD! >>>>>>>>>>> >>>>>>>>>>> --matt >>>>>>>>>>> >>>>>>>>>>> On Sat, Feb 23, 2013 at 6:02 PM, Steven Hartland < >>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>> >>>>>>>>>>>> Sorry its taken so long to get round to this, just had a really >>>>>>>>>>>> busy >>>>>>>>>>>> number of months. Anyway all the changes have been made, see >>>>>>>>>>>> below >>>>>>>>>>>> for individual details, patches are attached. >>>>>>>>>>>> >>>>>>>>>>>> ----- Original Message ----- From: "Steven Hartland" >>>>>>>>>>>> >>>>>>>>>>>>> ----- Original Message ----- From: "Matthew Ahrens" < >>>>>>>>>>>>> [email protected]> >>>>>>>>>>>>> >>>>>>>>>>>>>> Here are some code-specific notes: >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> parsepropname(): I see that you're following the bad example >>>>>>>>>>>>>> set by >>>>>>>>>>>>>> parseprop() and parse_depth() of operating on optarg. It >>>>>>>>>>>>>> would be >>>>>>>>>>>>>> better to explicitly pass the name of the property to parse, >>>>>>>>>>>>>> rather >>>>>>>>>>>>>> than operating on the global optarg. To isolate the uglyness >>>>>>>>>>>>>> of this >>>>>>>>>>>>>> global variable, optarg should only be used within the >>>>>>>>>>>>>> function that >>>>>>>>>>>>>> calls getopt(). It would be great if you wanted to fix all >>>>>>>>>>>>>> of these, >>>>>>>>>>>>>> but if not that's OK. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> I'll look into this thanks, as you said following the example >>>>>>>>>>>>> ;-) >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Fixed this, and created a seperate patch for fixing the >>>>>>>>>>>> original code too >>>>>>>>>>>> see attached >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> zfs_receive() can't deal with an empty list of properties or >>>>>>>>>>>>>> datasets? >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Was just forward loading that logic at the time, will review >>>>>>>>>>>>> and update >>>>>>>>>>>>> if its doesn't make sense any more. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Minor changes needed to faciliated this which have been done. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> props_overrride(): >>>>>>>>>>>>>> >>>>>>>>>>>>>> this function could use some comments explaining what is >>>>>>>>>>>>>> doing. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Will do. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Comment expanded to give more insite into what its doing >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> however I think that the following idiom for iterating over a >>>>>>>>>>>>>> nvlist >>>>>>>>>>>>>> is more easily understood, because it keeps all the iteration >>>>>>>>>>>>>> logic in >>>>>>>>>>>>>> one place, and handles "continue" statements: >>>>>>>>>>>>>> >>>>>>>>>>>>>> for (nvpair_t *pair = nvlist_next_nvpair(nvl, NULL); >>>>>>>>>>>>>> pair != NULL; >>>>>>>>>>>>>> pair = nvlist_next_nvpair(nvl, pair)) { >>>>>>>>>>>>>> ... >>>>>>>>>>>>>> } >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Not a problem will change. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Changed >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> the variable you call "tods" would usually be named "atp" (the >>>>>>>>>>>>>> pointer >>>>>>>>>>>>>> to the "at" character). This is where you will have a bug >>>>>>>>>>>>>> when >>>>>>>>>>>>>> someone does "-o userquota@username@pool/fsname=1g". >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> I wasn't aware that style of permissions existed, not sure >>>>>>>>>>>>> they do >>>>>>>>>>>>> in the version of FreeBSD I based this work on, but definitely >>>>>>>>>>>>> something to get fixed. >>>>>>>>>>>>> >>>>>>>>>>>>> Any suggestions on an alternative syntax? >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Separator changed to # and code changes to sepp (the pointer to >>>>>>>>>>>> the >>>>>>>>>>>> separator character) >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> It seems like the only way you could get a non-string, >>>>>>>>>>>>>> non-boolean >>>>>>>>>>>>>> nvpair would be a programming error (i.e. bad user input >>>>>>>>>>>>>> could not >>>>>>>>>>>>>> cause it). Therefore you should assert this is the case, >>>>>>>>>>>>>> rather than >>>>>>>>>>>>>> print an error message. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Yes I agree, will do. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> These are now asserts >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> It seems like you could omit the "if >>>>>>>>>>>>>> (!nvlist_exists([gd]xprops)" >>>>>>>>>>>>>> check, because the -x props will be removed after this anyway. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> It would but didn't want it going through all the effort of >>>>>>>>>>>>> adding it >>>>>>>>>>>>> only to remove it later for performance was my thinking. Do >>>>>>>>>>>>> you believe >>>>>>>>>>>>> this wouldn't result in any performance loss if removed? >>>>>>>>>>>>> >>>>>>>>>>>>> Likewise the "!nvlist_exists(doprops" check, because the dx >>>>>>>>>>>>>> props will >>>>>>>>>>>>>> be added after this anyway. So you can simplify the logic to: >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> Given there will be little performance difference either way >>>>>>>>>>>> I've decided >>>>>>>>>>>> to keep it as it was so that the output in verbose case is what >>>>>>>>>>>> the user >>>>>>>>>>>> would expect. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Regards >>>>>>>>>>>> Steve >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> ================================================ >>>>>>>>>>>> This e.mail is private and confidential between Multiplay (UK) >>>>>>>>>>>> Ltd. and the person or entity to whom it is addressed. In the >>>>>>>>>>>> event of >>>>>>>>>>>> misdirection, the recipient is prohibited from using, copying, >>>>>>>>>>>> printing or >>>>>>>>>>>> otherwise disseminating it or any information contained in it. >>>>>>>>>>>> In the event of misdirection, illegible or incomplete >>>>>>>>>>>> transmission please telephone +44 845 868 1337 >>>>>>>>>>>> or return the E.mail to [email protected]. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> ------------------------------------------- >>>>>>>>>>>> illumos-developer >>>>>>>>>>>> Archives: https://www.listbox.com/member/archive/182179/=now >>>>>>>>>>>> RSS Feed: https://www.listbox.com/member/archive/rss/182179/ >>>>>>>>>>>> 21175174-cd73734d >>>>>>>>>>>> Modify Your Subscription: https://www.listbox.com/member/?&id_ >>>>>>>>>>>> secret=21175174-792643f6 <https://www.listbox.com/member/?&;> >>>>>>>>>>>> >>>>>>>>>>>> Powered by Listbox: http://www.listbox.com >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> *illumos-zfs* | Archives >>>>>>>>>>> <https://www.listbox.com/member/archive/182191/=now> >>>>>>>>>>> <https://www.listbox.com/member/archive/rss/182191/23976944-8a53fa63> >>>>>>>>>>> | Modify <https://www.listbox.com/member/?&;> Your Subscription >>>>>>>>>>> <http://www.listbox.com> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> *illumos-zfs* | Archives >>>>>>>>>> <https://www.listbox.com/member/archive/182191/=now> >>>>>>>>>> <https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460> >>>>>>>>>> | Modify <https://www.listbox.com/member/?&;> Your Subscription >>>>>>>>>> <http://www.listbox.com> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> *illumos-zfs* | Archives >>>>>>>> <https://www.listbox.com/member/archive/182191/=now> >>>>>>>> <https://www.listbox.com/member/archive/rss/182191/23976944-8a53fa63> >>>>>>>> | Modify <https://www.listbox.com/member/?&;> Your Subscription >>>>>>>> <http://www.listbox.com> >>>>>>>> >>>>>>> >>>>>>> >>>>> *illumos-developer* | Archives >>>>> <https://www.listbox.com/member/archive/182179/=now> >>>>> <https://www.listbox.com/member/archive/rss/182179/21175174-cd73734d> >>>>> | Modify <https://www.listbox.com/member/?&;> Your Subscription >>>>> <http://www.listbox.com> >>>>> >>>> >>>> >>> >> *illumos-developer* | Archives >> <https://www.listbox.com/member/archive/182179/=now> >> <https://www.listbox.com/member/archive/rss/182179/21174983-884d0c04> | >> Modify >> <https://www.listbox.com/member/?member_id=21174983&id_secret=21174983-e58407f8> >> Your Subscription <http://www.listbox.com> >> > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
