From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57542) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W87PC-000097-2w for qemu-devel@nongnu.org; Tue, 28 Jan 2014 07:04:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W87P7-0005nF-35 for qemu-devel@nongnu.org; Tue, 28 Jan 2014 07:04:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:63629) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W87P6-0005nB-QP for qemu-devel@nongnu.org; Tue, 28 Jan 2014 07:04:33 -0500 Date: Tue, 28 Jan 2014 14:09:17 +0200 From: "Michael S. Tsirkin" Message-ID: <20140128120917.GA18395@redhat.com> References: <1390906551-4845-1-git-send-email-kirill.shutemov@linux.intel.com> <20140128112650.GA18171@redhat.com> <20140128114059.80CDFE0090@blue.fi.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140128114059.80CDFE0090@blue.fi.intel.com> Subject: Re: [Qemu-devel] [PATCH] [RESEND-try-3] hw/9pfs: fix P9_STATS_GEN handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Kirill A. Shutemov" Cc: armbru@redhat.com, qemu-devel@nongnu.org, aliguori@amazon.com, aneesh.kumar@linux.vnet.ibm.com 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 > > > Reviewed-by: Daniel P. Berrange > > > Reviewed-by: Aneesh Kumar K.V > > > > 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