[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 5/6] util: give a specific error message when O_DIRECT doe
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work |
Date: |
Tue, 25 Aug 2020 16:23:19 +0100 |
User-agent: |
Mutt/1.14.6 (2020-07-11) |
On Tue, Aug 25, 2020 at 05:19:53PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > A common error scenario is to tell QEMU to use O_DIRECT in combination
> > with a filesystem that doesn't support it. To aid users to diagnosing
> > their mistake we want to provide a clear error message when this happens.
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > util/osdep.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/util/osdep.c b/util/osdep.c
> > index a4956fbf6b..6c24985f7a 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -345,6 +345,19 @@ qemu_open_internal(const char *name, int flags, mode_t
> > mode, Error **errp)
> >
> > if (ret == -1) {
> > const char *action = flags & O_CREAT ? "create" : "open";
> > +#ifdef O_DIRECT
> > + if (errno == EINVAL && (flags & O_DIRECT)) {
> > + ret = open(name, flags & ~O_DIRECT, mode);
> > + if (ret != -1) {
> > + close(ret);
> > + error_setg(errp, "Could not %s '%s' flags 0x%x: "
> > + "filesystem does not support O_DIRECT",
> > + action, name, flags);
> > + errno = EINVAL; /* close() clobbered earlier errno */
>
> More precise: close() may have clobbered
Either open or close in fact.
>
> Sure open() can only fail with EINVAL here?
We don't care about the errno from the open() call seen in this context,
we're restoring the EINVAL from the previous open() call above this patch
contxt, that we match on with "if (errno == EINVAL && ...)" line.
>
> > + return -1;
> > + }
> > + }
> > +#endif /* O_DIRECT */
> > error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
> > action, name, flags);
> > }
>
> There is no qemu_set_cloexec(). Intentional?
We've called close() immediately after open(). Adding qemu_set_cloexec()
doesnt make it any less racy on platforms lacking O_CLOSEXEC
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
- Re: [PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling, (continued)
[PATCH v4 4/6] util: introduce qemu_open and qemu_create with error reporting, Daniel P . Berrangé, 2020/08/21
[PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work, Daniel P . Berrangé, 2020/08/21
[PATCH v4 6/6] block/fileb: switch to use qemu_open/qemu_create for improved errors, Daniel P . Berrangé, 2020/08/21