[developer] Request for review of patch that allows filtering of properties on zfs receive (Feature #2745)

[Matthew Ahrens]

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 ***@lists.illumos.org.

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

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"

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.

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
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

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

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 [phone]
------------------------------**-------------
illumos-developer
Archives: https://www.listbox.com/**member/archive/182179/=now<https://www.listbox.com/member/archive/182179/=now>
RSS Feed: https://www.listbox.com/**member/archive/rss/182179/**
21175174-cd73734d<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
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

[Steven Hartland]

Comments below:-
----- Original Message -----
From: Matthew Ahrens
To: ***@lists.illumos.org ; John Kennedy ; Christopher Siden ; ***@lists.illumos.org
Sent: Monday, February 25, 2013 9:02 PM
Subject: Re: [developer] Request for review of patch that allows filtering of properties on zfs receive (Feature #2745)


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 ***@lists.illumos.org.

Trying get an OpenIndiana (oi_151a7 32-bit) VM setup so I can do this, installed on Virtual PC but seems to hang booting @

Configuring devices.
Loading smf(5) service descriptions: 6/6

Ooo, spoke to soon, seems it doesn't like only 512Mb in the VM definition.

Anyway any hits on getting a suitable VM setup appreciated ;-)


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").
Corrected / changed
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".
Agreed, went with add_unique_option and added a param to describe it.
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.
I didn't have either of those in my main code base here, can see them in HEAD so will use them there.
+ char *sepp = strchr(propname, sepp);
I don't think this will compile; you probably want "strchr(propname, sep)". Be sure to test this out.
Fixed - nice catch, it does compile here, but I missed warning :(

zfs_receive_one(): "skip" should be a boolean_t.


Again, thanks for doing this Steven. Looking forward to having it in illumos and FreeBSD!

Changed

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 [phone]
or return the E.mail to ***@multiplay.co.uk.


-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

[Tim Cook]

Post by Steven Hartland
**
Comments below:-
----- Original Message -----
*Sent:* Monday, February 25, 2013 9:02 PM
*Subject:* Re: [developer] Request for review of patch that allows
filtering of properties on zfs receive (Feature #2745)
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
.
Trying get an OpenIndiana (oi_151a7 32-bit) VM setup so I can do this,
Configuring devices.
Loading smf(5) service descriptions: 6/6
Ooo, spoke to soon, seems it doesn't like only 512Mb in the VM definition.
Anyway any hits on getting a suitable VM setup appreciated ;-)

You want at least 4gb of memory. More if you're doing anything with dedup.



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

[Alek Pinchuk]

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

Post by Matthew Ahrens
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
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
zfs.8 line ~2383 misspells "receive"
+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).
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".
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

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" <

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.

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
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

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

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 [phone]
-------------------------------------------
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
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

[Matthew Ahrens]

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).

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 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"
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)
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.

Post by Alek Pinchuk
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.
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.
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

Post by Matthew Ahrens
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
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
zfs.8 line ~2383 misspells "receive"
+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).
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".
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

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" <

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.

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
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

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

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 [phone]
-------------------------------------------
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
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

[Matthew Ahrens]

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.

--matt

Post by Matthew Ahrens
Alek, thanks a lot for picking up these changes. I have a few comments /
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).
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?
- 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
- 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.
2043: should be "skipping receive of %s"
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)
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.

Post by Alek Pinchuk
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.
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.
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

Post by Matthew Ahrens
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
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
zfs.8 line ~2383 misspells "receive"
+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).
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".
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 <

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" <

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.

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
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

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

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 [phone]
-------------------------------------------
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-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

[Alek Pinchuk]

Hello Matt,

Post by Matthew Ahrens
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.

Post by Matthew Ahrens
--matt

Post by Matthew Ahrens
Alek, thanks a lot for picking up these changes. I have a few comments /
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".

Post by Matthew Ahrens

Post by Matthew Ahrens
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.

Post by Matthew Ahrens

Post by Matthew Ahrens
- 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
- 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.
2043: should be "skipping receive of %s"

fixed

2935: can you explain the handling of has_exprops? It seems like (a) you

Post by Matthew Ahrens

Post by Matthew Ahrens
could just inline nvlist_empty(exprops) where necessary, and (b) if
(stream_wantsnewfs), has_exprops is uninitialized (dsname is also
uninitialized)

fixed

Post by Matthew Ahrens
2567: I think that "props" is the properties from the send stream (not

Post by Matthew Ahrens
"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

Post by Matthew Ahrens

Post by Matthew Ahrens

Post by Alek Pinchuk
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.
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.
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

Post by Matthew Ahrens
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
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
zfs.8 line ~2383 misspells "receive"
+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).
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".
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 <

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" <

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.

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
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

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

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 [phone]
-------------------------------------------
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-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

[Alek Pinchuk]

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 is that now the add_unique_option() call doesn't return
anything, and will ignore duplicate props instead of failing.


-Alek

Post by Alek Pinchuk
Hello Matt,

Post by Matthew Ahrens
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.

Post by Matthew Ahrens
--matt

Post by Matthew Ahrens
Alek, thanks a lot for picking up these changes. I have a few comments
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".

Post by Matthew Ahrens

Post by Matthew Ahrens
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.

Post by Matthew Ahrens

Post by Matthew Ahrens
- 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
- 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.
2043: should be "skipping receive of %s"

fixed
2935: can you explain the handling of has_exprops? It seems like (a)

Post by Matthew Ahrens

Post by Matthew Ahrens
you could just inline nvlist_empty(exprops) where necessary, and (b) if
(stream_wantsnewfs), has_exprops is uninitialized (dsname is also
uninitialized)

fixed

Post by Matthew Ahrens
2567: I think that "props" is the properties from the send stream (not

Post by Matthew Ahrens
"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/
http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html
-Alek

Post by Matthew Ahrens

Post by Matthew Ahrens

Post by Alek Pinchuk
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.
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.
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

Post by Matthew Ahrens
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
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
zfs.8 line ~2383 misspells "receive"
+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).
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".
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 <

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" <

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.

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
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

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

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 [phone]
-------------------------------------------
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-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

[Steven Hartland]

Thanks for picking this up, I've been trying to get round to finishing it
it off but with one thing any another I've never quite managed it :(

Regards
Steve

----- Original Message -----

Post by Alek Pinchuk
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.
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.
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

Post by Matthew Ahrens
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
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
zfs.8 line ~2383 misspells "receive"
+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).
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".
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

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" <

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.

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
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

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

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 [phone]
-------------------------------------------
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-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/23734114-fd87e47b
Modify Your Subscription: https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

[Alek Pinchuk]

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

Post by Alek Pinchuk
Hello Matt,

Post by Matthew Ahrens
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.

Post by Matthew Ahrens
--matt

Post by Matthew Ahrens
Alek, thanks a lot for picking up these changes. I have a few comments
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".

Post by Matthew Ahrens

Post by Matthew Ahrens
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.

Post by Matthew Ahrens

Post by Matthew Ahrens
- 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
- 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.
2043: should be "skipping receive of %s"

fixed
2935: can you explain the handling of has_exprops? It seems like (a)

Post by Matthew Ahrens

Post by Matthew Ahrens
you could just inline nvlist_empty(exprops) where necessary, and (b) if
(stream_wantsnewfs), has_exprops is uninitialized (dsname is also
uninitialized)

fixed

Post by Matthew Ahrens
2567: I think that "props" is the properties from the send stream (not

Post by Matthew Ahrens
"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/
http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html
-Alek

Post by Matthew Ahrens

Post by Matthew Ahrens

Post by Alek Pinchuk
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.
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.
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

Post by Matthew Ahrens
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
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
zfs.8 line ~2383 misspells "receive"
+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).
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".
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 <

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" <

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.

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
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

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

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 [phone]
-------------------------------------------
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
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

[Matthew Ahrens]

Post by Matthew Ahrens
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.


On Thu, Apr 17, 2014 at 11:33 AM, Alek Pinchuk

Post by Matthew Ahrens
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?

Post by Matthew Ahrens
Again, this is Steven's FreeBSD patch with some slight modifications.
-Alek

Post by Alek Pinchuk
Hello Matt,

Post by Matthew Ahrens
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.

Post by Matthew Ahrens
--matt

Post by Matthew Ahrens
Alek, thanks a lot for picking up these changes. I have a few
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".

Post by Matthew Ahrens

Post by Matthew Ahrens
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.

Post by Matthew Ahrens

Post by Matthew Ahrens
- 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
- 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.
2043: should be "skipping receive of %s"

fixed
2935: can you explain the handling of has_exprops? It seems like (a)

Post by Matthew Ahrens

Post by Matthew Ahrens
you could just inline nvlist_empty(exprops) where necessary, and (b) if
(stream_wantsnewfs), has_exprops is uninitialized (dsname is also
uninitialized)

fixed

Post by Matthew Ahrens
2567: I think that "props" is the properties from the send stream (not

Post by Matthew Ahrens
"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/
http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html
-Alek

Post by Matthew Ahrens

Post by Matthew Ahrens

Post by Alek Pinchuk
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.
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.
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

Post by Matthew Ahrens
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
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
zfs.8 line ~2383 misspells "receive"
+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).
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".
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 <

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" <

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.

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
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

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

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 [phone]
-------------------------------------------
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-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

[Alek Pinchuk]

Hello,

Post by Matthew Ahrens

Post by Matthew Ahrens
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:

***@alek401:/volumes# zfs get sharenfs test
NAME PROPERTY VALUE SOURCE
test sharenfs off default
***@alek401:/volumes# zfs create test/ds
***@alek401:/volumes# zfs create test/ds/child
***@alek401:/volumes# zfs get sharenfs test/ds
NAME PROPERTY VALUE SOURCE
test/ds sharenfs off default
***@alek401:/volumes# zfs get sharenfs test/ds/child
NAME PROPERTY VALUE SOURCE
test/ds/child sharenfs off default
***@alek401:/volumes# zfs set sharenfs=on test/ds
***@alek401:/volumes# zfs get sharenfs test/ds/child
NAME PROPERTY VALUE SOURCE
test/ds/child sharenfs on inherited from test/ds
***@alek401:/volumes# zfs get sharenfs test/ds
NAME PROPERTY VALUE SOURCE
test/ds sharenfs on local
***@alek401:/volumes# zfs snapshot -r test/***@0

***@alek401:/volumes# zfs send -R test/***@0 | zfs recv -o sharenfs=off
test/ds2
***@alek401:/volumes# zfs get sharenfs test/ds2
NAME PROPERTY VALUE SOURCE
test/ds2 sharenfs off received
***@alek401:/volumes# zfs get sharenfs test/ds2/child
NAME PROPERTY VALUE SOURCE
test/ds2/child sharenfs off inherited from test/ds2

***@alek401:/volumes# zfs send -R test/***@0 | zfs recv -x sharenfs test/ds3
***@alek401:/volumes# zfs get sharenfs test/ds3
NAME PROPERTY VALUE SOURCE
test/ds3 sharenfs off default
***@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?

Post by Matthew Ahrens
On Thu, Apr 17, 2014 at 11:33 AM, Alek Pinchuk <

Post by Matthew Ahrens
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

Post by Matthew Ahrens

Post by Matthew Ahrens
Again, this is Steven's FreeBSD patch with some slight modifications.
-Alek

Post by Alek Pinchuk
Hello Matt,

Post by Matthew Ahrens
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.

Post by Matthew Ahrens
--matt

Post by Matthew Ahrens
Alek, thanks a lot for picking up these changes. I have a few
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".

Post by Matthew Ahrens

Post by Matthew Ahrens
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.

Post by Matthew Ahrens

Post by Matthew Ahrens
- 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
- 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.
2043: should be "skipping receive of %s"

fixed
2935: can you explain the handling of has_exprops? It seems like (a)

Post by Matthew Ahrens

Post by Matthew Ahrens
you could just inline nvlist_empty(exprops) where necessary, and (b) if
(stream_wantsnewfs), has_exprops is uninitialized (dsname is also
uninitialized)

fixed

Post by Matthew Ahrens
2567: I think that "props" is the properties from the send stream

Post by Matthew Ahrens
(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.
http://alek_p.bitbucket.org/webrevs/2745/
http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html
-Alek

Post by Matthew Ahrens

Post by Matthew Ahrens

Post by Alek Pinchuk
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.
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.
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

Post by Matthew Ahrens
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
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
zfs.8 line ~2383 misspells "receive"
+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).
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".
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 <

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" <

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.

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
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

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

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 [phone]
-------------------------------------------
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-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

[Matthew Ahrens]

Post by Alek Pinchuk
Hello,

Post by Matthew Ahrens

Post by Matthew Ahrens
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.

NAME PROPERTY VALUE SOURCE
test sharenfs off default
NAME PROPERTY VALUE SOURCE
test/ds sharenfs off default
NAME PROPERTY VALUE SOURCE
test/ds/child sharenfs off default
NAME PROPERTY VALUE SOURCE
test/ds/child sharenfs on inherited from test/ds
NAME PROPERTY VALUE SOURCE
test/ds sharenfs on local
test/ds2
NAME PROPERTY VALUE SOURCE
test/ds2 sharenfs off received
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?

Post by Alek Pinchuk
NAME PROPERTY VALUE SOURCE
test/ds3 sharenfs off default
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/***@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/***@0 sharenfs - - -
test/ds/child sharenfs off - local
test/ds/***@0 sharenfs - - -
test/recvd sharenfs rw on local
test/***@0 sharenfs - - -
test/recvd/child sharenfs rw off inherited from test/recvd
test/recvd/***@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

Post by Alek Pinchuk

Post by Matthew Ahrens
On Thu, Apr 17, 2014 at 11:33 AM, Alek Pinchuk <

Post by Matthew Ahrens
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/
http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html
-Alek

Post by Matthew Ahrens

Post by Matthew Ahrens
Again, this is Steven's FreeBSD patch with some slight modifications.
-Alek

Post by Alek Pinchuk
Hello Matt,

Post by Matthew Ahrens
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.

Post by Matthew Ahrens
--matt

Post by Matthew Ahrens
Alek, thanks a lot for picking up these changes. I have a few
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".

Post by Matthew Ahrens

Post by Matthew Ahrens
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.

Post by Matthew Ahrens

Post by Matthew Ahrens
- 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
- 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.
2043: should be "skipping receive of %s"

fixed
2935: can you explain the handling of has_exprops? It seems like

Post by Matthew Ahrens

Post by Matthew Ahrens
(a) you could just inline nvlist_empty(exprops) where necessary, and (b) if
(stream_wantsnewfs), has_exprops is uninitialized (dsname is also
uninitialized)

fixed

Post by Matthew Ahrens
2567: I think that "props" is the properties from the send stream

Post by Matthew Ahrens
(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.
http://alek_p.bitbucket.org/webrevs/2745/
http://alek_p.bitbucket.org/webrevs/2745/zfs.1m.html
-Alek

Post by Matthew Ahrens

Post by Matthew Ahrens

Post by Alek Pinchuk
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.
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.
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 <

Post by Matthew Ahrens
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
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
zfs.8 line ~2383 misspells "receive"
+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).
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".
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 <

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" <

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.

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
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

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

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 [phone]
-------------------------------------------
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-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com