[sheepdog] Unreliable error code handling in sheep/plainstore.c

[Roy Yang]

Thanks Hitoshi,

It is a GNU C extension, and implicitly add  strerror (errno) .

Since errno is used for system errors, should we modify it or
we can solve the issue by just adjusting the order of sd_err and err_to_stderr?
Just some thoughts, 

Roy 

> Date: Wed, 4 Mar 2015 23:06:25 +0900
> From: mitake.hitoshi at gmail.com
> To: bsdnet at outlook.com
> CC: namei.unix at gmail.com; skypexu at gmail.com; sheepdog at lists.wpkg.org
> Subject: Re: [sheepdog] Unreliable error code handling in sheep/plainstore.c
> 
> At Wed, 4 Mar 2015 01:14:29 -0800,
> Roy Yang wrote:
> > 
> > [1  <multipart/alternative (7bit)>]
> > [1.1  <text/plain; iso-8859-1 (quoted-printable)>]
> > 
> > 
> > 
> > Maybe I miss something here.
> > sd_err("failed to write object %"PRIx64", path=%s, 
> > offset=%" PRId32 ", size=%"PRId32", result=%zd, %m", oid, path,
> >                         iocb->offset, iocb->length, size);
> > 
> > 6 format positions with %, but only 5 values.
> > This looks a bug, and maybe the root cause why sd_err set errno?
> > 
> > Reset errno inside log_write will hide such errors if this is a 
> > real bug.
> 
> The last %m doesn't require its value. It just used for print string
> representation of current errno.
> http://www.gnu.org/software/libc/manual/html_node/Other-Output-Conversions.html
> 
> Thanks,
> Hitoshi
> 
> > Roy
> > 
> > > Date: Wed, 4 Mar 2015 16:31:52 +0800
> > > From: namei.unix at gmail.com
> > > To: skypexu at gmail.com
> > > CC: sheepdog at lists.wpkg.org
> > > Subject: Re: [sheepdog] Unreliable error code handling in sheep/plainstore.c
> > > 
> > > On Wed, Mar 04, 2015 at 04:21:46PM +0800, Xu Yifeng wrote:
> > > > I found some unreliable error handling in plainstore.c, reason is
> > > > combination of
> > > > sd_err() and err_to_sderr(), because it is not guaranteed that
> > > > sd_err() does not
> > > > muck errno, it is possible passing an mucked errno to
> > > > err_to_sderr(), see following
> > > > code in plain_store.c, line 758:
> > > >    if (unlikely(size != len)) {
> > > >                 sd_err("failed to write object %"PRIx64", path=%s,
> > > > offset=%"
> > > >                        PRId32", size=%"PRId32", result=%zd, %m", oid, path,
> > > >                        iocb->offset, iocb->length, size);
> > > >                 ret = err_to_sderr(path, oid, errno);
> > > >                 goto out;
> > > >         }
> > > > 
> > > > it is possible that sd_err() may muck the errno, causing an
> > > > irrelevant errno to be passed to
> > > > err_to_sderr!
> > > 
> > > Good catch!
> > > 
> > > It is really a hidden bug for a long time. This might explain why I sometimes saw
> > > strange logs in the past!
> > > 
> > > Could you please cook a decent patch for this problem? The fix looks good to me.
> > > 
> > > Thanks,
> > > Yuan
> > > -- 
> > > sheepdog mailing list
> > > sheepdog at lists.wpkg.org
> > > https://lists.wpkg.org/mailman/listinfo/sheepdog
> > 
> >  		 	   		  
> > [1.2  <text/html; iso-8859-1 (quoted-printable)>]
> > 
> > [2  <text/plain; us-ascii (7bit)>]
> > -- 
> > sheepdog mailing list
> > sheepdog at lists.wpkg.org
> > https://lists.wpkg.org/mailman/listinfo/sheepdog
 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wpkg.org/pipermail/sheepdog/attachments/20150304/cb76a136/attachment-0004.html>