Re: [PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work
On Wed, Aug 26, 2020 at 01:19:53PM +0200, Markus Armbruster wrote:
> Now back to my dislike of the #ifdeffery I voiced in reply to PATCH 2.
>
> Code now:
>
> #ifdef O_CLOEXEC
> flags |= O_CLOEXEC;
> #endif /* O_CLOEXEC */
>
> ret = open(name, flags, mode);
>
> #ifndef O_CLOEXEC
> if (ret >= 0) {
> qemu_set_cloexec(ret);
> }
> #endif /* ! O_CLOEXEC */
>
> 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);
> [O_DIRECT not supported error...]
> errno = EINVAL; /* close() clobbered earlier errno */
> return -1;
> }
> }
> #endif /* O_DIRECT */
> [generic error...]
> }
>
> Compare:
>
> #ifdef O_CLOEXEC
> flags |= O_CLOEXEC;
> ret = open(name, flags, mode);
> #else
> ret = open(name, flags, mode);
> if (ret >= 0) {
> qemu_set_cloexec(ret);
> }
> #endif /* ! O_CLOEXEC */
>
> 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);
> [O_DIRECT not supported error...]
> errno = EINVAL; /* close() clobbered earlier errno */
> return -1;
> }
> }
> #endif /* O_DIRECT */
> [generic error...]
> }
>
> I like this a bit better, but not enough to make a strong
> recommendation, let alone demand.
In v5 I've gone with neither approach, and instead spun off a helper
method qemu_open_cloexec which I think is clearer than both.
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 5/6] util: give a specific error message when O_DIRECT doesn't work
Daniel P. Berrangé writes:
> On Tue, Aug 25, 2020 at 05:19:53PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé 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
>> > Signed-off-by: Daniel P. Berrangé
>> > ---
>> > 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)) {
Suggest
/*
* Check whether open() failed due to use of O_DIRECT,
* and set a more helpful error then.
*/
>> > +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.
Ah, got it now.
I'd prefer
errno = EINVAL; /* restore first open()'s errno */
>>
>> > +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
You're right. I misread the code.
Reviewed-by: Markus Armbruster
Now back to my dislike of the #ifdeffery I voiced in reply to PATCH 2.
Code now:
#ifdef O_CLOEXEC
flags |= O_CLOEXEC;
#endif /* O_CLOEXEC */
ret = open(name, flags, mode);
#ifndef O_CLOEXEC
if (ret >= 0) {
qemu_set_cloexec(ret);
}
#endif /* ! O_CLOEXEC */
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);
[O_DIRECT not supported error...]
errno = EINVAL; /* close() clobbered earlier errno */
return -1;
}
}
#endif /* O_DIRECT */
[generic error...]
}
Compare:
#ifdef O_CLOEXEC
flags |= O_CLOEXEC;
ret = open(name, flags, mode);
#else
ret = open(name, flags, mode);
if (ret >= 0) {
qemu_set_cloexec(ret);
}
#endif /* ! O_CLOEXEC */
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);
[O_DIRECT not supported error...]
errno = EINVAL; /* close() clobbered earlier errno */
return -1;
}
}
#endif /* O_DIRECT */
[generic error...]
}
I like this a bit better, but not enough to make a strong
recommendation, let alone demand.
Re: [PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work
On Tue, Aug 25, 2020 at 05:19:53PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé 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
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > 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 5/6] util: give a specific error message when O_DIRECT doesn't work
Daniel P. Berrangé 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
> Signed-off-by: Daniel P. Berrangé
> ---
> 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
Sure open() can only fail with EINVAL here?
> +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?
[PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work
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
Signed-off-by: Daniel P. Berrangé
---
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 */
+return -1;
+}
+}
+#endif /* O_DIRECT */
error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
action, name, flags);
}
--
2.26.2
