On Wed, Aug 26, 2020 at 01:03:19PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berra...@redhat.com> writes: > > > 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. > > 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?
Once we push the qemu_dup call into monitor_fdset_dup_fd_add, we might as well go the whole way and merge monitor_fdset_get_fd into it too. So I've done that, turning 3 calls into 1. 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 :|