[PATCH] pynfs minor: fixed Environment._maketree to use proper stateid during file write

[PATCH] pynfs minor: fixed Environment._maketree to use proper stateid during file write

[Volodymyr Khomenko]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0001-Minor-fixed-Environment._maketree-used-by-init-to-us.patch --]
[-- Type: text/x-patch, Size: 1451 bytes --]

From 63c0711f9cd8f8c0aaff7d0116a42b5001bddcd2 Mon Sep 17 00:00:00 2001
From: Volodymyr Khomenko <Khomenko.Volodymyr@gmail.com>
Date: Fri, 21 Jan 2022 14:52:28 +0200
Subject: [PATCH] Minor: fixed Environment._maketree (used by init) to use
 proper stateid during file write

_maketree is a part of generic init sequence for server41tests so the code should be generic.
Using zero stateid (when "other" and "seqid" are both zero, the stateid is treated
as a special anonymous stateid) is a special use-case of anonymous access
so it must not be used during generic initialization.

Signed-off-by: Volodymyr Khomenko <Khomenko.Volodymyr@gmail.com>
---
 nfs4.1/server41tests/environment.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nfs4.1/server41tests/environment.py b/nfs4.1/server41tests/environment.py
index 14b0902..0b7c976 100644
--- a/nfs4.1/server41tests/environment.py
+++ b/nfs4.1/server41tests/environment.py
@@ -198,7 +198,7 @@ class Environment(testmod.Environment):
                 log.warning("could not create /%s" % b'/'.join(path))
         # Make file-object in /tree
         fh, stateid = create_confirm(sess, b'maketree', tree + [b'file'])
-        res = write_file(sess, fh, self.filedata)
+        res = write_file(sess, fh, self.filedata, stateid=stateid)
         check(res, msg="Writing data to /%s/file" % b'/'.join(tree))
         res = close_file(sess, fh, stateid)
         check(res)
-- 
2.25.1

Re: [PATCH] pynfs minor: fixed Environment._maketree to use proper stateid during file write

[J. Bruce Fields]

On Fri, Jan 21, 2022 at 03:06:57PM +0200, Volodymyr Khomenko wrote:
> 

> From 63c0711f9cd8f8c0aaff7d0116a42b5001bddcd2 Mon Sep 17 00:00:00 2001
> From: Volodymyr Khomenko <Khomenko.Volodymyr@gmail.com>
> Date: Fri, 21 Jan 2022 14:52:28 +0200
> Subject: [PATCH] Minor: fixed Environment._maketree (used by init) to use
>  proper stateid during file write
> 
> _maketree is a part of generic init sequence for server41tests so the code should be generic.
> Using zero stateid (when "other" and "seqid" are both zero, the stateid is treated
> as a special anonymous stateid) is a special use-case of anonymous access
> so it must not be used during generic initialization.

OK, applying, but I'm a little wary.  If a server isn't accepting the
zero stateid here then I think that's a server bug.

--b.

> 
> Signed-off-by: Volodymyr Khomenko <Khomenko.Volodymyr@gmail.com>
> ---
>  nfs4.1/server41tests/environment.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/nfs4.1/server41tests/environment.py b/nfs4.1/server41tests/environment.py
> index 14b0902..0b7c976 100644
> --- a/nfs4.1/server41tests/environment.py
> +++ b/nfs4.1/server41tests/environment.py
> @@ -198,7 +198,7 @@ class Environment(testmod.Environment):
>                  log.warning("could not create /%s" % b'/'.join(path))
>          # Make file-object in /tree
>          fh, stateid = create_confirm(sess, b'maketree', tree + [b'file'])
> -        res = write_file(sess, fh, self.filedata)
> +        res = write_file(sess, fh, self.filedata, stateid=stateid)
>          check(res, msg="Writing data to /%s/file" % b'/'.join(tree))
>          res = close_file(sess, fh, stateid)
>          check(res)
> -- 
> 2.25.1
> 

RE: [PATCH] pynfs minor: fixed Environment._maketree to use proper stateid during file write

[Frank Filz]

> On Fri, Jan 21, 2022 at 03:06:57PM +0200, Volodymyr Khomenko wrote:
> >
> 
> > From 63c0711f9cd8f8c0aaff7d0116a42b5001bddcd2 Mon Sep 17 00:00:00
> 2001
> > From: Volodymyr Khomenko <Khomenko.Volodymyr@gmail.com>
> > Date: Fri, 21 Jan 2022 14:52:28 +0200
> > Subject: [PATCH] Minor: fixed Environment._maketree (used by init) to
> > use  proper stateid during file write
> >
> > _maketree is a part of generic init sequence for server41tests so the
code
> should be generic.
> > Using zero stateid (when "other" and "seqid" are both zero, the
> > stateid is treated as a special anonymous stateid) is a special
> > use-case of anonymous access so it must not be used during generic
> initialization.
> 
> OK, applying, but I'm a little wary.  If a server isn't accepting the zero
stateid
> here then I think that's a server bug.

Yea, that makes me nervous about a server bug also. Maybe we should have
explicit special stateid tests.

It's always tricky because initialization of the tree requires a bunch of
stuff to work before it's explicitly tested...

Frank

> > Signed-off-by: Volodymyr Khomenko <Khomenko.Volodymyr@gmail.com>
> > ---
> >  nfs4.1/server41tests/environment.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/nfs4.1/server41tests/environment.py
> > b/nfs4.1/server41tests/environment.py
> > index 14b0902..0b7c976 100644
> > --- a/nfs4.1/server41tests/environment.py
> > +++ b/nfs4.1/server41tests/environment.py
> > @@ -198,7 +198,7 @@ class Environment(testmod.Environment):
> >                  log.warning("could not create /%s" % b'/'.join(path))
> >          # Make file-object in /tree
> >          fh, stateid = create_confirm(sess, b'maketree', tree +
[b'file'])
> > -        res = write_file(sess, fh, self.filedata)
> > +        res = write_file(sess, fh, self.filedata, stateid=stateid)
> >          check(res, msg="Writing data to /%s/file" % b'/'.join(tree))
> >          res = close_file(sess, fh, stateid)
> >          check(res)
> > --
> > 2.25.1
> >

Re: [PATCH] pynfs minor: fixed Environment._maketree to use proper stateid during file write

[Volodymyr Khomenko]

Yes, I agree with you, accepting zero stateid is very important for the server.
However, our NFS4 server implementation is still in the early stages
of development so we intentionally have left this feature outside for
now.
So our intention is to run pynfs tests for other features we have
already implemented.

And yes, I would propose to make a dedicated test for a special
stateid feature (if it's not done yet).
From the protocol side, all-zeros and all-ones are special values for stateid.

Thanks!
Volodymyr.

On Wed, Jan 26, 2022 at 5:29 PM Frank Filz <ffilzlnx@mindspring.com> wrote:
>
> > On Fri, Jan 21, 2022 at 03:06:57PM +0200, Volodymyr Khomenko wrote:
> > >
> >
> > > From 63c0711f9cd8f8c0aaff7d0116a42b5001bddcd2 Mon Sep 17 00:00:00
> > 2001
> > > From: Volodymyr Khomenko <Khomenko.Volodymyr@gmail.com>
> > > Date: Fri, 21 Jan 2022 14:52:28 +0200
> > > Subject: [PATCH] Minor: fixed Environment._maketree (used by init) to
> > > use  proper stateid during file write
> > >
> > > _maketree is a part of generic init sequence for server41tests so the
> code
> > should be generic.
> > > Using zero stateid (when "other" and "seqid" are both zero, the
> > > stateid is treated as a special anonymous stateid) is a special
> > > use-case of anonymous access so it must not be used during generic
> > initialization.
> >
> > OK, applying, but I'm a little wary.  If a server isn't accepting the zero
> stateid
> > here then I think that's a server bug.
>
> Yea, that makes me nervous about a server bug also. Maybe we should have
> explicit special stateid tests.
>
> It's always tricky because initialization of the tree requires a bunch of
> stuff to work before it's explicitly tested...
>
> Frank
>
> > > Signed-off-by: Volodymyr Khomenko <Khomenko.Volodymyr@gmail.com>
> > > ---
> > >  nfs4.1/server41tests/environment.py | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/nfs4.1/server41tests/environment.py
> > > b/nfs4.1/server41tests/environment.py
> > > index 14b0902..0b7c976 100644
> > > --- a/nfs4.1/server41tests/environment.py
> > > +++ b/nfs4.1/server41tests/environment.py
> > > @@ -198,7 +198,7 @@ class Environment(testmod.Environment):
> > >                  log.warning("could not create /%s" % b'/'.join(path))
> > >          # Make file-object in /tree
> > >          fh, stateid = create_confirm(sess, b'maketree', tree +
> [b'file'])
> > > -        res = write_file(sess, fh, self.filedata)
> > > +        res = write_file(sess, fh, self.filedata, stateid=stateid)
> > >          check(res, msg="Writing data to /%s/file" % b'/'.join(tree))
> > >          res = close_file(sess, fh, stateid)
> > >          check(res)
> > > --
> > > 2.25.1
> > >
>