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. 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/?member_id=23976944&id_secret=23976944-d1dde2de>Your >>> Subscription >>> <http://www.listbox.com> >>> >> >>
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
