On Tue, Aug 25, 2020 at 05:14:21PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berra...@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é <berra...@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. > > > 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. > > 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 -- |: 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 :|