From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56029) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SBBP1-0005nI-Pa for qemu-devel@nongnu.org; Fri, 23 Mar 2012 16:48:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SBBOz-0000qt-Nt for qemu-devel@nongnu.org; Fri, 23 Mar 2012 16:48:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42007) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SBBOz-0000qe-Ev for qemu-devel@nongnu.org; Fri, 23 Mar 2012 16:48:01 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q2NKlxOw018365 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 23 Mar 2012 16:48:00 -0400 Date: Fri, 23 Mar 2012 17:48:01 -0300 From: Luiz Capitulino Message-ID: <20120323174801.405686cc@doriath.home> In-Reply-To: <20120323195303.GF23394@garlic> References: <1331765714-26209-1-git-send-email-alevy@redhat.com> <1332095353-19120-1-git-send-email-alevy@redhat.com> <1332095353-19120-2-git-send-email-alevy@redhat.com> <20120323112125.54f4d663@doriath.home> <20120323195303.GF23394@garlic> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/5] qerror: add error codes for fopen failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alon Levy Cc: qemu-devel@nongnu.org On Fri, 23 Mar 2012 21:53:03 +0200 Alon Levy wrote: > On Fri, Mar 23, 2012 at 11:21:25AM -0300, Luiz Capitulino wrote: > > On Sun, 18 Mar 2012 19:29:09 +0100 > > Alon Levy wrote: > > > > > Added: > > > > > > QERR_EINTR > > > QERR_EACCES > > > QERR_EEXIST > > > QERR_OPEN_FILE_EMFILE > > > QERR_ENOSPC > > > QERR_EPERM > > > QERR_READ_ONLY > > > QERR_ENOTDIR > > > QERR_EFBIG > > > > > > Signed-off-by: Alon Levy > > > --- > > > qerror.c | 36 ++++++++++++++++++++++++++++++++++++ > > > qerror.h | 27 +++++++++++++++++++++++++++ > > > 2 files changed, 63 insertions(+) > > > > > > diff --git a/qerror.c b/qerror.c > > > index f55d435..4915939 100644 > > > --- a/qerror.c > > > +++ b/qerror.c > > > @@ -213,6 +213,42 @@ static const QErrorStringTable qerror_table[] = { > > > .desc = "Could not open '%(filename)'", > > > }, > > > { > > > + .error_fmt = QERR_EINTR, > > > + .desc = "Interrupted open of '%(filename)'", > > > + }, > > > > There are two problems here. First, the QERR_INTR macro doesn't have the > > filename argument, so this will crash. > > Ugh, missed that. > > > > > The second problem is that, I've talked to Anthony and EINTR should not > > be returned to clients, it should be handled intead. So, please handle it > > during write(). > > > > > + { > > > + .error_fmt = QERR_EACCES, > > > + .desc = "Cannot access file'", > > > + }, > > > > We already have QERR_PERMISSION_DENIED. > > > > So you want to have both error conditions (EACCESS and EPERM) translated > to the more generic PERMISSION_DENIED? I had forgotten about our discussion about this. You could add InvalidAccess, although I think we already use QERR_PERMISSION_DENIED as EACCESS... > This means that information is > lost when looking at the error value. The main thing would be to show > the filename, which we will do later as you said, so fine I guess. > > > > + { > > > + .error_fmt = QERR_EEXIST, > > > + .desc = "File already exists'", > > > + }, > > > > Can you use the description provided by man 3 errno? This is valid for all > > errors you're adding. > > It's a bit lengthy. But sure. You can trim it. The important point is to avoid creating new descriptions. > > > > > > + { > > > + .error_fmt = QERR_OPEN_FILE_EMFILE, > > > + .desc = "Max open files when opening file'", > > > + }, > > > + { > > > + .error_fmt = QERR_ENOSPC, > > > + .desc = "No space left opening file'", > > > + }, > > > + { > > > + .error_fmt = QERR_EPERM, > > > + .desc = "Permission denied (EPERM) opening file'", > > > + }, > > > + { > > > + .error_fmt = QERR_READ_ONLY, > > > + .desc = "Read only filesystem opening file'", > > > + }, > > > + { > > > + .error_fmt = QERR_ENOTDIR, > > > + .desc = "Directory related error opening file'", > > > + }, > > > + { > > > + .error_fmt = QERR_EFBIG, > > > + .desc = "File too big opening'", > > > + }, > > > + { > > > .error_fmt = QERR_PERMISSION_DENIED, > > > .desc = "Insufficient permission to perform this operation", > > > }, > > > diff --git a/qerror.h b/qerror.h > > > index e26c635..ddc04e8 100644 > > > --- a/qerror.h > > > +++ b/qerror.h > > > @@ -181,6 +181,33 @@ QError *qobject_to_qerror(const QObject *obj); > > > #define QERR_OPEN_FILE_FAILED \ > > > "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }" > > > > > > +#define QERR_OPEN_FILE_EMFILE \ > > > + "{ 'class': 'OpenFileEMFILE', 'data': {} }" > > > + > > > +#define QERR_EINTR \ > > > + "{ 'class': 'EINTR', 'data': {} }" > > > + > > > +#define QERR_EACCES \ > > > + "{ 'class': 'EACCES', 'data': {} }" > > > + > > > +#define QERR_EEXIST \ > > > + "{ 'class': 'EEXIST', 'data': {} }" > > > > We use more descriptive names instead of using the errno name. Like AlreadyExists. > > Please fix it for errors you adding. > > Actually this was on purpose, to allow someone catching it to know it > originated in a failed systemcall. So your way the catcher would have to > go read the source (or hopefully I'll choose similar names). No, if the command has a good description, like: @AlreadyExists, if the file already exists Then it's possible to know what happened and what the client should do. > > > > > > + > > > +#define QERR_ENOSPC \ > > > + "{ 'class': 'ENOSPC', 'data': {} }" > > > + > > > +#define QERR_EPERM \ > > > + "{ 'class': 'EPERM', 'data': {} }" > > > + > > > +#define QERR_READ_ONLY \ > > > + "{ 'class': 'ReadOnly', 'data': {} }" > > > + > > > +#define QERR_ENOTDIR \ > > > + "{ 'class': 'ENOTDIR', 'data': {} }" > > > + > > > +#define QERR_EFBIG \ > > > + "{ 'class': 'EFBIG', 'data': {} }" > > > + > > > #define QERR_PERMISSION_DENIED \ > > > "{ 'class': 'PermissionDenied', 'data': {} }" > > > > > > > >