From: Markus Armbruster <armbru@redhat.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, "Max Reitz" <mreitz@redhat.com>,
"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
qemu-block@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v4 3/6] util: add Error object for qemu_open_internal error reporting
Date: Wed, 26 Aug 2020 13:03:19 +0200 [thread overview]
Message-ID: <87k0xl7jpk.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20200825153627.GA107278@redhat.com> ("Daniel P. =?utf-8?Q?Be?= =?utf-8?Q?rrang=C3=A9=22's?= message of "Tue, 25 Aug 2020 16:36:27 +0100")
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Tue, Aug 25, 2020 at 05:14:21PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > Instead of relying on the limited information from errno, we can now
>> > also provide detailed error messages.
>>
>> The more detailed error messages are currently always ignored, but the
>> next patches will fix that.
>>
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> > util/osdep.c | 21 +++++++++++++++++++--
>> > 1 file changed, 19 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/util/osdep.c b/util/osdep.c
>> > index 9ff92551e7..9c7118d3cb 100644
>> > --- a/util/osdep.c
>> > +++ b/util/osdep.c
>> > @@ -284,7 +284,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
>> > * Opens a file with FD_CLOEXEC set
>> > */
>> > static int
>> > -qemu_open_internal(const char *name, int flags, mode_t mode)
>> > +qemu_open_internal(const char *name, int flags, mode_t mode, Error **errp)
>> > {
>> > int ret;
>> >
>> > @@ -298,24 +298,31 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
>> >
>> > fdset_id = qemu_parse_fdset(fdset_id_str);
>> > if (fdset_id == -1) {
>> > + error_setg(errp, "Could not parse fdset %s", name);
>> > errno = EINVAL;
>> > return -1;
>> > }
>> >
>> > fd = monitor_fdset_get_fd(fdset_id, flags);
>> > if (fd < 0) {
>> > + error_setg_errno(errp, -fd, "Could not acquire FD for %s flags %x",
>> > + name, flags);
>> > errno = -fd;
>> > return -1;
>> > }
>> >
>> > dupfd = qemu_dup_flags(fd, flags);
>> > if (dupfd == -1) {
>> > + error_setg_errno(errp, errno, "Could not dup FD for %s flags %x",
>> > + name, flags);
>> > return -1;
>> > }
>> >
>> > ret = monitor_fdset_dup_fd_add(fdset_id, dupfd);
>> > if (ret == -1) {
>> > close(dupfd);
>> > + error_setg(errp, "Could not save FD for %s flags %x",
>> > + name, flags);
>>
>> Can this happen?
>
> Well there's code in monitor_fdset_dup_fd_add that can return -1.
It fails when
* @fdset_id contains @dupfd
@dupfd is a fresh file descriptor. If @fdset_id already contains it,
it's stale there. That would be a programming error. Recommend to
assert.
* @fdset_id is not in @mon_fdsets
monitor_fdset_get_fd() fails the same way. monitor_fdset_dup_fd_add()
can fail that way after monitor_fdset_get_fd() succeed only if the fd
set went away between the two. Could that happen? Would it be safe?
This is the only user of monitor_fdset_dup_fd_add(). Why not remove
the awkward failure mode by making monitor_fdset_dup_fd_add() dup the
fd and add?
>> > errno = EINVAL;
>> > return -1;
>> > }
>> > @@ -336,6 +343,16 @@ qemu_open_internal(const char *name, int flags, mode_t mode)
>> > }
>> > #endif /* ! O_CLOEXEC */
>> >
>> > + if (ret == -1) {
>> > + const char *action = "open";
>> > + if (flags & O_CREAT) {
>> > + action = "create";
>> > + }
>> > + error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
>> > + action, name, flags);
>>
>> Not a good user experience:
>>
>> Could not open '/etc/shadow' flags 0x0: Permission denied
>>
>> Better:
>>
>> Could not open '/etc/shadow' for reading: Permission denied
>>
>> Are you sure flags other than the access mode (O_RDONLY, O_WRONLY,
>> O_RDWR) must be included in the error message?
>
> It was the flags other than access mode that I was thinking were
> more important to log. I'm ambivalent htough, so can drop the
> flags if it is thought to be overkill.
Hexadecimal flags are borderline useless even for developers: to make
sense of them, you have to grep -R /usr/include/. For mere mortals,
they are confusing in addition to useless.
>> If you must report flags in hexadecimal, then please reporting them more
>> consistently. Right now you have
>>
>> for %s flags 0x%x
>> '%s' flags %x
>>
>> Perhaps '%s' with flags 0x%x
>>
>> > + }
>> > +
>> > +
>> > return ret;
>> > }
>> >
>> > @@ -352,7 +369,7 @@ int qemu_open_old(const char *name, int flags, ...)
>> > }
>> > va_end(ap);
>> >
>> > - ret = qemu_open_internal(name, flags, mode);
>> > + ret = qemu_open_internal(name, flags, mode, NULL);
>> >
>> > #ifdef O_DIRECT
>> > if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
>>
>>
>
> Regards,
> Daniel
next prev parent reply other threads:[~2020-08-26 11:06 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-21 17:20 [PATCH v4 0/6] block: improve error reporting for unsupported O_DIRECT Daniel P. Berrangé
2020-08-21 17:21 ` [PATCH v4 1/6] util: rename qemu_open() to qemu_open_old() Daniel P. Berrangé
2020-08-21 17:21 ` [PATCH v4 2/6] util: refactor qemu_open_old to split off variadic args handling Daniel P. Berrangé
2020-08-25 14:56 ` Markus Armbruster
2020-08-25 15:03 ` Daniel P. Berrangé
2020-08-26 11:19 ` Markus Armbruster
2020-08-21 17:21 ` [PATCH v4 3/6] util: add Error object for qemu_open_internal error reporting Daniel P. Berrangé
2020-08-25 15:14 ` Markus Armbruster
2020-08-25 15:36 ` Daniel P. Berrangé
2020-08-26 11:03 ` Markus Armbruster [this message]
2020-08-27 13:27 ` Daniel P. Berrangé
2020-08-21 17:21 ` [PATCH v4 4/6] util: introduce qemu_open and qemu_create with " Daniel P. Berrangé
2020-08-25 15:16 ` Markus Armbruster
2020-08-21 17:21 ` [PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work Daniel P. Berrangé
2020-08-25 15:19 ` Markus Armbruster
2020-08-25 15:23 ` Daniel P. Berrangé
2020-08-26 11:19 ` Markus Armbruster
2020-09-02 17:10 ` Daniel P. Berrangé
2020-08-21 17:21 ` [PATCH v4 6/6] block/fileb: switch to use qemu_open/qemu_create for improved errors Daniel P. Berrangé
2020-08-25 15:28 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k0xl7jpk.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=f4bug@amsat.org \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).