Re: [Qemu-devel] [PATCH v2 1/5] error: add error_set_file_open_failed

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 15 Mar 2012 17:21:01 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Thu, Mar 15, 2012 at 11:52:32AM -0300, Luiz Capitulino wrote:
> > On Thu, 15 Mar 2012 00:55:10 +0200
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > Signed-off-by: Alon Levy <alevy@redhat.com>
> > > ---
> > >  cpus.c               |    4 ++--
> > >  error.c              |   44 ++++++++++++++++++++++++++++++++++++++++++++
> > >  error.h              |    4 ++++
> > >  qerror.c             |   36 ++++++++++++++++++++++++++++++++++++
> > >  qerror.h             |   27 +++++++++++++++++++++++++++
> > >  qga/commands-posix.c |    2 +-
> > >  6 files changed, 114 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/cpus.c b/cpus.c
> > > index 17b055f..c87035f 100644
> > > --- a/cpus.c
> > > +++ b/cpus.c
> > > @@ -1178,7 +1178,7 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
> > >  
> > >      f = fopen(filename, "wb");
> > >      if (!f) {
> > > -        error_set(errp, QERR_OPEN_FILE_FAILED, filename);
> > > +        error_set_file_open_failed(errp, filename, errno);
> > >          return;
> > >      }
> > >  
> > > @@ -1208,7 +1208,7 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
> > >  
> > >      f = fopen(filename, "wb");
> > >      if (!f) {
> > > -        error_set(errp, QERR_OPEN_FILE_FAILED, filename);
> > > +        error_set_file_open_failed(errp, filename, errno);
> > >          return;
> > >      }
> > >  
> > > diff --git a/error.c b/error.c
> > > index 990050f..b32d7f5 100644
> > > --- a/error.c
> > > +++ b/error.c
> > > @@ -144,3 +144,47 @@ void error_set_qobject(Error **errp, QObject *obj)
> > >  
> > >      *errp = err;
> > >  }
> > > +
> > > +void error_set_file_open_failed(Error **errp, const char *file_name, int ret)
> > > +{
> > 
> > I prefer having:
> > 
> >  void qemu_fopen_err(const char *filename, const char *mode, Error **errp);
> 
> OK.
> 
> > 
> > It could live in osdep.c I think.
> 
> OK.
> 
> > 
> > Also, please split this into two patches:
> > 
> >  1. Add all the errors
> >  2. Add qemu_fopen_err()
> 
> OK.
> 
> > 
> > And don't change unrelated code. Other commands that will benefit from the
> > new function should be converted in a different series.
> > 
> 
> OK.
> 
> > CC'ing Anthony in case he has a different suggestion for the fopen()
> > wrapper (of course we can an open() version too).
> > 
> > > +    const char *fmt = NULL;
> > > +
> > > +    switch (ret) {
> > > +    case EACCES:
> > > +        fmt = QERR_OPEN_FILE_EACCES;
> > > +        break;
> > > +    case EINTR:
> > > +        fmt = QERR_OPEN_FILE_EINTR;
> > > +        break;
> > > +    case EEXIST:
> > > +        fmt = QERR_OPEN_FILE_EEXIST;
> > > +        break;
> > > +    case EMFILE:
> > > +        fmt = QERR_OPEN_FILE_EMFILE;
> > > +        break;
> > > +    case ENOSPC:
> > > +        fmt = QERR_OPEN_FILE_ENOSPC;
> > > +        break;
> > > +    case EPERM:
> > > +        fmt = QERR_OPEN_FILE_EPERM;
> > > +        break;
> > > +    case EROFS:
> > > +        fmt = QERR_OPEN_FILE_EROFS;
> > > +        break;
> > > +    case ENOTDIR:
> > > +        fmt = QERR_OPEN_FILE_ENOTDIR;
> > > +        break;
> > > +    case EFBIG:
> > > +        fmt = QERR_OPEN_FILE_EFBIG;
> > > +        break;
> > > +    default:
> > > +        /*
> > > +         * EINVAL and ENOTSUP will result in the default
> > > +         *
> > > +         * ENOENT too, it's used by (for instance) bdrv_create_file for
> > > +         * a different purpose then open (2) so just give a generic error.
> > > +         */
> > > +        fmt = QERR_OPEN_FILE_FAILED;
> > > +    }
> > > +    error_set(errp, fmt, file_name);
> > > +}
> > > diff --git a/error.h b/error.h
> > > index 6361f40..f3a80f3 100644
> > > --- a/error.h
> > > +++ b/error.h
> > > @@ -67,4 +67,8 @@ void error_free(Error *err);
> > >   */
> > >  bool error_is_type(Error *err, const char *fmt);
> > >  
> > > +/**
> > > + * Helper to set error for a open (2) style return code.
> > > + */
> > > +void error_set_file_open_failed(Error **errp, const char *file_name, int ret);
> > >  #endif
> > > diff --git a/qerror.c b/qerror.c
> > > index f55d435..23c260b 100644
> > > --- a/qerror.c
> > > +++ b/qerror.c
> > > @@ -213,6 +213,42 @@ static const QErrorStringTable qerror_table[] = {
> > >          .desc      = "Could not open '%(filename)'",
> > >      },
> > >      {
> > > +        .error_fmt = QERR_OPEN_FILE_EINTR,
> > > +        .desc      = "Interrupted open of '%(filename)'",
> > > +    },
> > 
> > I see that you're adding _OPEN_FILE_ flavors to be able to pass the filename
> > in the error. But this just workarounds a limitation of qerror: we should be
> > able to set the error message at run-time.
> > 
> > I think the best thing to do is to have QERR_EINTR instead, with a generic
> > message like "Operation has been interrupted" and we can improve QError to
> > extend it to contain the file name.
> > 
> 
> Sounds like we would not have the filename then, which would be a shame.

We can have it in the future.

> But I don't mind doing that.
> 
> > This is valid for all errors below.
> > 
> > > +    {
> > > +        .error_fmt = QERR_OPEN_FILE_EACCES,
> > > +        .desc      = "Cannot access '%(filename)'",
> > > +    },
> > 
> > We already have PermissionDenied.
> > 
> 
> Not the same use case - QERR_PERMISSION_DENIED is used for internal
> errors, not for an open / file related system permission error.

No problem.

> 
> > > +    {
> > > +        .error_fmt = QERR_OPEN_FILE_EEXIST,
> > > +        .desc      = "File already exists '%(filename)'",
> > > +    },
> > 
> > I wouldn't mind not having this one because most of the time qemu doesn't
> > care if the file already exists.
> 
> I was really just going over everything open (2) manpage and copying
> what looked like it made sense. I can drop it.

We should only report possible errors, something like ENXIO is not possible
in this case.

> 
> > 
> > > +    {
> > > +        .error_fmt = QERR_OPEN_FILE_EMFILE,
> > > +        .desc      = "Max open files when opening '%(filename)'",
> > > +    },
> > 
> > This is an exception for what I said about _OPEN_FILE_ flavors above, this
> > is fine because it's really a file error.
> 
> They are all open (2) errors, see previous comment :)
> 
> > 
> > > +    {
> > > +        .error_fmt = QERR_OPEN_FILE_ENOSPC,
> > > +        .desc      = "No space left opening '%(filename)'",
> > > +    },
> > > +    {
> > > +        .error_fmt = QERR_OPEN_FILE_EPERM,
> > > +        .desc      = "Permission denied (EPERM) for '%(filename)'",
> > > +    },
> > 
> > I wouldn't mind reporting EACCESS and EPERM the same.
> 
> But why do that? they are different at the system level, no reason to
> merge them. Except for EPERM being a bit cryptic (to me) it doesn't seem
> to be the same as EACCESS.

I'm avoiding having a huge amount of errors w/o strong justification, but
this is minor to me, do what you think is best.

> 
> > 
> > > +    {
> > > +        .error_fmt = QERR_OPEN_FILE_EROFS,
> > > +        .desc      = "Read only filesystem opening '%(filename)'",
> > > +    },
> > 
> > Let's have a QERR_READ_ONLY.
> 
> OK.
> 
> > 
> > > +    {
> > > +        .error_fmt = QERR_OPEN_FILE_ENOTDIR,
> > > +        .desc      = "Directory related error opening '%(filename)'",
> > > +    },
> > > +    {
> > > +        .error_fmt = QERR_OPEN_FILE_EFBIG,
> > > +        .desc      = "File too big opening '%(filename)'",
> > > +    },
> > > +    {
> > >          .error_fmt = QERR_PERMISSION_DENIED,
> > >          .desc      = "Insufficient permission to perform this operation",
> > >      },
> > > diff --git a/qerror.h b/qerror.h
> > > index e26c635..6ab9b8d 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_EINTR \
> > > +    "{ 'class': 'OpenFileEINTR', 'data': { 'filename': %s } }"
> > > +
> > > +#define QERR_OPEN_FILE_EACCES \
> > > +    "{ 'class': 'OpenFileEACCES', 'data': { 'filename': %s } }"
> > > +
> > > +#define QERR_OPEN_FILE_EEXIST \
> > > +    "{ 'class': 'OpenFileEEXIST', 'data': { 'filename': %s } }"
> > > +
> > > +#define QERR_OPEN_FILE_EMFILE \
> > > +    "{ 'class': 'OpenFileEMFILE', 'data': { 'filename': %s } }"
> > > +
> > > +#define QERR_OPEN_FILE_ENOSPC \
> > > +    "{ 'class': 'OpenFileENOSPC', 'data': { 'filename': %s } }"
> > > +
> > > +#define QERR_OPEN_FILE_EPERM \
> > > +    "{ 'class': 'OpenFileEPERM', 'data': { 'filename': %s } }"
> > > +
> > > +#define QERR_OPEN_FILE_EROFS \
> > > +    "{ 'class': 'OpenFileEROFS', 'data': { 'filename': %s } }"
> > > +
> > > +#define QERR_OPEN_FILE_ENOTDIR \
> > > +    "{ 'class': 'OpenFileENOTDIR', 'data': { 'filename': %s } }"
> > > +
> > > +#define QERR_OPEN_FILE_EFBIG \
> > > +    "{ 'class': 'OpenFileEFBIG', 'data': { 'filename': %s } }"
> > > +
> > >  #define QERR_PERMISSION_DENIED \
> > >      "{ 'class': 'PermissionDenied', 'data': {} }"
> > >  
> > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > index 7b2be2f..4e5ef7f 100644
> > > --- a/qga/commands-posix.c
> > > +++ b/qga/commands-posix.c
> > > @@ -134,7 +134,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> > >      slog("guest-file-open called, filepath: %s, mode: %s", path, mode);
> > >      fh = fopen(path, mode);
> > >      if (!fh) {
> > > -        error_set(err, QERR_OPEN_FILE_FAILED, path);
> > > +        error_set_file_open_failed(err, path, errno);
> > >          return -1;
> > >      }
> > >  
> > 
>