Subject: Re: [sieve] Gen-ART Last Call Review of draft-ietf-sieve-include-13

From: Aaron Stone <[email-masked]>
Date: Tue, 13 Dec 2011 14:49:02 -0800

Great comments, thank you!

Cheers,
Aaron

On Tue, Dec 13, 2011 at 2:13 PM, Ben Campbell <[email-masked]> wrote:

> I am the assigned Gen-ART reviewer for this draft. For background on
> Gen-ART, please see the FAQ at <
> http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>
> Please resolve these comments along with any other Last Call comments you
> may receive.
>
> Document: draft-ietf-sieve-include-13
> Reviewer: Ben Campbell
> Review Date: 2011-12-13
> IETF LC End Date: 2011-12-15
>
> Summary: This draft is almost ready for publication as a proposed standard
>
> Major issues:
>
> None
>
> Minor issues:
>
> -- section 3.1, paragraph 4: "Implementations MUST NOT generate errors for
> recursive inclusions at upload time, as this would force an upload ordering
> requirement upon script authors / generators.  However, if an active script
> is replaced with a faulty script and would remain the active script, an
> error MUST be generated and the upload MUST fail."
>
> These two statements seem contradictory on a quick reading.  In
> particular, how can the latter assertion avoid an upload ordering
> requirement? Or do you mean faulty in some way other than being recursive?
>
> -- section 3.4.1, paragraph 5: "If a "global" command is given the name of
> a variable that has previously been defined in the immediate script with
> "set", an error MUST be generated either when the script is uploaded or at
> execution time."
>
> Does this conflict with the previous statement that it is okay for a
> global and a private variable to have the same name?
>
> -- section 3.4.2:
>
> Why do you need two ways to accomplish the same thing?
>
> Does the global namespace have the same "requires" requirement as the
> global command?
>
> -- section 4.2, paragraph 2:
>
> Can you elaborate on what permissions are proper? Is it different for an
> included script than for any other script?
>
> -- section 4.2, paragraph 3:
>
> Can you elaborate on what you mean by "safe for a storage system"?
>
> Nits/editorial comments:
>
> -- section 3.1, paragraph 4: "authors / generators"
>
> s/ "authors / generators " / "authors and generators"
>
> -- section 3.2, paragraph 9 (Top of page 6): "
> The included script MUST be a valid Sieve script.  Each script MUST
>   have its own "require" statements for all optional capabilities used
>   by that script. "
>
> Who do these normative statements apply to? As worded, it sounds like they
> apply to the user. It might be better to say that the implementation MUST
> validate that…
>
> -- section 3.4.2, paragraph 3: "Variables declared global and variables
> accessed via the global namespace MUST be one and the same."
>
> Plurality mismatch. I suggest something like "a variable declared as
> global and a variable accesses with the global namespace, otherwise having
> the same name…"
>
> -- section 5:
>
> Please explicitly mention the name or URL of the registry table to which
> this should be added.
>
>
>

---

From: Aaron Stone <[email-masked]>
Date: Mon, 19 Dec 2011 11:13:35 -0800

> requirement? Or do you mean faulty in some way other than being recursive?

If you're replacing an active script, it has to be correct all the time,
and uploads are atomic only on a per-script basis. There's a risk that if
you're uploading a set of scripts that include one another, at some
intermediate stage while some scripts are uploaded but not others, they are
in an invalid state. The managesieve spec says that scripts must be
validated at upload time. The language above is trying to say that you can
upload all of the scripts that may include one another in any order without
generating errors immediately, however, if you're replacing an active
script or a script included by the active script, then you DO have to
upload a correct script right from the get-go.

> global and a private variable to have the same name?

It doesn't conflict, because those variables live in separate namespaces.
The effect of the global command is to bind the two names. An error is
generated rather than specifying if the local overwrites the global value,
or the global overwrites the local value.

> Why do you need two ways to accomplish the same thing?

I might be making this up, but I think the original question was whether
the variables spec would have namespaces.

> Does the global namespace have the same "requires" requirement as the

Yes, but this isn't explicitly stated.

> Can you elaborate on what you mean by "safe for a storage system"?
There are both somewhat vague warnings, basically, "Don't allow 'include
"./../..//etc/passwd"' and don't allow 'include "foo$(`rm star`)"'.

> s/ "authors / generators " / "authors and generators"

Ok.

> apply to the user. It might be better to say that the implementation MUST

Ok.

> global and a variable accesses with the global namespace, otherwise having

I might insert the word "each" after MUST to account for the plurality
without rewriting the sentence.

> Please explicitly mention the name or URL of the registry table to which

Good catch, thank you.

Cheers,
Aaron

---

From: Alexey Melnikov <[email-masked]>
Date: Mon, 19 Dec 2011 20:01:35 +0000

Hi Aaron,

On 19/12/2011 19:13, Aaron Stone wrote:
> On Tue, Dec 13, 2011 at 2:13 PM, Ben Campbell <[email-masked] 
> <mailto:[email-masked]>> wrote:
>
>     I am the assigned Gen-ART reviewer for this draft. For background
>     on Gen-ART, please see the FAQ at
>     <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>
>     Please resolve these comments along with any other Last Call
>     comments you may receive.
>
>     -- section 3.1, paragraph 4: "Implementations MUST NOT generate
>     errors for recursive inclusions at upload time, as this would
>     force an upload ordering requirement upon script authors /
>     generators.  However, if an active script is replaced with a
>     faulty script and would remain the active script, an error MUST be
>     generated and the upload MUST fail."
>
> If you're replacing an active script, it has to be correct all the 
> time, and uploads are atomic only on a per-script basis. There's a 
> risk that if you're uploading a set of scripts that include one 
> another, at some intermediate stage while some scripts are uploaded 
> but not others, they are in an invalid state. The managesieve spec 
> says that scripts must be validated at upload time. The language above 
> is trying to say that you can upload all of the scripts that may 
> include one another in any order without generating errors 
> immediately, however, if you're replacing an active script or a script 
> included by the active script, then you DO have to upload a correct 
> script right from the get-go.
It looks like expanding the text to say what you explained above might 
be in order.