| Message ID | 1380411144-9236-5-git-send-email-geyslan@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem <geyslan@gmail.com> wrote: > In case of error in the p9_client_write, the function v9fs_fid_xattr_set > should return its negative value, what was never being done. > > Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> > --- > fs/9p/xattr.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c > index 3c28cdf..0788388 100644 > --- a/fs/9p/xattr.c > +++ b/fs/9p/xattr.c > @@ -149,11 +149,10 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const > char *name, > write_count = value_len; > write_count = p9_client_write(fid, ((char *)value)+offset, > NULL, offset, write_count); > - if (write_count < 0) { > - /* error in xattr write */ > - retval = write_count; > - break; > - } > + /* error in xattr write */ > + if (write_count < 0) > + return write_count; > + > So, I'm convinced that there's a problem here, but I think the solution in the patch is incomplete. Simply returning wouldn't clunk the fid. I think the right approach is likely to keep the break, clunk and return an error if either the p9_client_write or the p9_client_clunk fails. I suppose you could make a claim that v9fs_fid_xattr_set shouldn't be clunking the fid -- but considering it's cloned the fid in its function body, it does seem like it shoudl also be cleaning up after itself. -eric -eric ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
At first, thanks for reply. 2013/10/20 Eric Van Hensbergen <ericvh@gmail.com>: > On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem <geyslan@gmail.com> wrote: >> >> In case of error in the p9_client_write, the function v9fs_fid_xattr_set >> should return its negative value, what was never being done. >> >> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> >> --- >> fs/9p/xattr.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c >> index 3c28cdf..0788388 100644 >> --- a/fs/9p/xattr.c >> +++ b/fs/9p/xattr.c >> @@ -149,11 +149,10 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const >> char *name, >> write_count = value_len; >> write_count = p9_client_write(fid, ((char *)value)+offset, >> NULL, offset, write_count); >> - if (write_count < 0) { >> - /* error in xattr write */ >> - retval = write_count; >> - break; >> - } >> + /* error in xattr write */ >> + if (write_count < 0) >> + return write_count; >> + >> > > > So, I'm convinced that there's a problem here, but I think the solution in > the patch is incomplete. Simply returning wouldn't clunk the fid. I think > the right approach is likely to keep the break, clunk and return an error if > either the p9_client_write or the p9_client_clunk fails. > > I suppose you could make a claim that v9fs_fid_xattr_set shouldn't be > clunking the fid -- but considering it's cloned the fid in its function > body, it does seem like it shoudl also be cleaning up after itself. > Right. I'll centralize the exiting assuring that fid will be clunked in case of fails. > -eric > > > -eric > ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
2013/10/21 Geyslan Gregório Bem <geyslan@gmail.com>: > At first, thanks for reply. > > 2013/10/20 Eric Van Hensbergen <ericvh@gmail.com>: >> On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem <geyslan@gmail.com> wrote: >>> >>> In case of error in the p9_client_write, the function v9fs_fid_xattr_set >>> should return its negative value, what was never being done. >>> >>> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> >>> --- >>> fs/9p/xattr.c | 9 ++++----- >>> 1 file changed, 4 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c >>> index 3c28cdf..0788388 100644 >>> --- a/fs/9p/xattr.c >>> +++ b/fs/9p/xattr.c >>> @@ -149,11 +149,10 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const >>> char *name, >>> write_count = value_len; >>> write_count = p9_client_write(fid, ((char *)value)+offset, >>> NULL, offset, write_count); >>> - if (write_count < 0) { >>> - /* error in xattr write */ >>> - retval = write_count; >>> - break; >>> - } >>> + /* error in xattr write */ >>> + if (write_count < 0) >>> + return write_count; >>> + >>> >> >> >> So, I'm convinced that there's a problem here, but I think the solution in >> the patch is incomplete. Simply returning wouldn't clunk the fid. I think >> the right approach is likely to keep the break, clunk and return an error if >> either the p9_client_write or the p9_client_clunk fails. >> >> I suppose you could make a claim that v9fs_fid_xattr_set shouldn't be >> clunking the fid -- but considering it's cloned the fid in its function >> body, it does seem like it shoudl also be cleaning up after itself. >> > > Right. I'll centralize the exiting assuring that fid will be clunked > in case of fails. > >> -eric >> >> >> -eric >> New version sent: [PATCH] 9p: fix return value in case in v9fs_fid_xattr_set() ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk
diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c index 3c28cdf..0788388 100644 --- a/fs/9p/xattr.c +++ b/fs/9p/xattr.c @@ -149,11 +149,10 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, write_count = value_len; write_count = p9_client_write(fid, ((char *)value)+offset, NULL, offset, write_count); - if (write_count < 0) { - /* error in xattr write */ - retval = write_count; - break; - } + /* error in xattr write */ + if (write_count < 0) + return write_count; + offset += write_count; value_len -= write_count; }
In case of error in the p9_client_write, the function v9fs_fid_xattr_set should return its negative value, what was never being done. Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> --- fs/9p/xattr.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)