[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] [RESEND-try-3] hw/9pfs: fix P9_STATS_GEN handli
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] [RESEND-try-3] hw/9pfs: fix P9_STATS_GEN handling |
Date: |
Tue, 28 Jan 2014 14:09:17 +0200 |
On Tue, Jan 28, 2014 at 01:40:59PM +0200, Kirill A. Shutemov wrote:
> Michael S. Tsirkin wrote:
> > On Tue, Jan 28, 2014 at 12:55:51PM +0200, Kirill A. Shutemov wrote:
> > > Currently we have few issues with P9_STATS_GEN:
> > >
> > > - We don't try to read st_gen anything except files or directories, but
> > > still set P9_STATS_GEN bit in st_result_mask. It may mislead client:
> > > we present garbage as valid st_gen.
> > >
> > > - If we failed to get valid st_gen with ENOTTY, we ignore error, but
> > > still set P9_STATS_GEN bit in st_result_mask.
> > >
> > > - If we failed to get valid st_gen with any other errno, we fail
> > > getattr altogether. It's excessive: we block valid client use-cases,
> > > like chdir(2) to non-readable directory with execution bit set.
> > >
> > > The patch fixes these issues and cleanup code a bit.
> > >
> > > Signed-off-by: Kirill A. Shutemov <address@hidden>
> > > Reviewed-by: Daniel P. Berrange <address@hidden>
> > > Reviewed-by: Aneesh Kumar K.V <address@hidden>
> >
> > Would be better to split unrelated issues out to separate patches.
>
> They are not totally unrelated: they all unbreak P9_STATS_GEN.
>
> But yes, I can split if it needed.
Probably a good idea.
If you can append explanation on how to reproduce the bug
that's fixed for each patch, even better.
> > > diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> > > index 8cbb8ae32a03..3e51fcd152f8 100644
> > > --- a/hw/9pfs/virtio-9p.c
> > > +++ b/hw/9pfs/virtio-9p.c
> > > @@ -1080,10 +1080,18 @@ static void v9fs_getattr(void *opaque)
> > > /* fill st_gen if requested and supported by underlying fs */
> > > if (request_mask & P9_STATS_GEN) {
> > > retval = v9fs_co_st_gen(pdu, &fidp->path, stbuf.st_mode,
> > > &v9stat_dotl);
> > > - if (retval < 0) {
> > > + switch (retval) {
> > > + case 0:
> > > + /* we have valid st_gen: update result mask */
> > > + v9stat_dotl.st_result_mask |= P9_STATS_GEN;
> > > + break;
> > > + case -EINTR:
> > > + /* request cancelled */
> > > goto out;
> >
> > Shouldn't EINTR be retried?
>
> No. It could be canceled by client (with Tflush) on purpose and client can
> retry if needed.
>
> --
> Kirill A. Shutemov