mbox series

[v4,0/6] block: improve error reporting for unsupported O_DIRECT

Message ID 20200821172105.608752-1-berrange@redhat.com
Headers show
Series block: improve error reporting for unsupported O_DIRECT | expand

Message

Daniel P. Berrangé Aug. 21, 2020, 5:20 p.m. UTC 
v1: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00269.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00589.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg07098.html

See patch commit messages for rationale

Ideally we would convert other callers of qemu_open to use
qemu_open_err, and eventually remove qemu_open, renaming
qemu_open_err back to qemu_open.  Given soft freeze is just
days away though, I'm hoping this series is simple enough
to get into this release, leaving bigger cleanup for later.

Improved in v4:

 - Use assert() for programmer mistakes
 - Split second patch into three distinct parts
 - Misc typos
 - Improve commit message

Improved in v3:

 - Re-arrange the patches series, so that the conversion to Error
   takes place first, then the improve O_DIRECT reporting
 - Rename existing method to qemu_open_old
 - Use a pair of new methods qemu_open + qemu_create to improve
   arg checking

Improved in v2:

 - Mention that qemu_open_err is preferred over qemu_open
 - Get rid of obsolete error_report call
 - Simplify O_DIRECT handling
 - Fixup iotests for changed error message text

Daniel P. Berrangé (6):
  util: rename qemu_open() to qemu_open_old()
  util: refactor qemu_open_old to split off variadic args handling
  util: add Error object for qemu_open_internal error reporting
  util: introduce qemu_open and qemu_create with error reporting
  util: give a specific error message when O_DIRECT doesn't work
  block/fileb: switch to use qemu_open/qemu_create for improved errors

 accel/kvm/kvm-all.c            |  2 +-
 backends/rng-random.c          |  2 +-
 backends/tpm/tpm_passthrough.c |  8 ++--
 block/file-posix.c             | 16 +++----
 block/file-win32.c             |  5 +-
 block/vvfat.c                  |  5 +-
 chardev/char-fd.c              |  2 +-
 chardev/char-pipe.c            |  6 +--
 chardev/char.c                 |  2 +-
 dump/dump.c                    |  2 +-
 hw/s390x/s390-skeys.c          |  2 +-
 hw/usb/host-libusb.c           |  2 +-
 hw/vfio/common.c               |  4 +-
 include/qemu/osdep.h           |  8 +++-
 io/channel-file.c              |  2 +-
 net/vhost-vdpa.c               |  2 +-
 os-posix.c                     |  2 +-
 qga/channel-posix.c            |  4 +-
 qga/commands-posix.c           |  6 +--
 target/arm/kvm.c               |  2 +-
 tests/qemu-iotests/051.out     |  4 +-
 tests/qemu-iotests/051.pc.out  |  4 +-
 tests/qemu-iotests/061.out     |  2 +-
 tests/qemu-iotests/069.out     |  2 +-
 tests/qemu-iotests/082.out     |  4 +-
 tests/qemu-iotests/111.out     |  2 +-
 tests/qemu-iotests/226.out     |  6 +--
 tests/qemu-iotests/232.out     | 12 ++---
 tests/qemu-iotests/244.out     |  6 +--
 ui/console.c                   |  2 +-
 util/osdep.c                   | 83 ++++++++++++++++++++++++++++------
 util/oslib-posix.c             |  2 +-
 32 files changed, 136 insertions(+), 77 deletions(-)

-- 
2.26.2

Comments

Daniel P. Berrangé Sept. 2, 2020, 5:10 p.m. UTC | #1 
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