Re: [libvirt] [PATCH v9 6/7] block: Enable qemu_open/close to work with fd sets

On 08/13/2012 07:44 AM, Corey Bryant wrote: > I'll send a new version shortly with these updates also. > >>> + >>> + ret = monitor_fdset_dup_fd_add(fdset_id, dupfd); >>> + if (ret == -1) { >>> + close(dupfd); >>> + return -1; >> >> This function appears to promise a reasonable errno on failure. Actually, looking at that function again, +int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd) +{ + MonFdset *mon_fdset; + MonFdsetFd *mon_fdset_fd_dup; + + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { + if (mon_fdset->id != fdset_id) { + continue; + } + QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) { + if (mon_fdset_fd_dup->fd == dup_fd) { + return -1; + } + } + mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup)); + mon_fdset_fd_dup->fd = dup_fd; + QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next); + return 0; + } + return -1; +} The only way it could fail is if we are trying to add an fd that is already in the set, or if we don't find mon_fdset; both of which would indicate logic bugs earlier in our program. Would it be worth asserting that these conditions are impossible, and making this function return void (the addition is always successful if it returns, since g_malloc0 aborts rather than failing with ENOMEM)?

And the more I think about it, the more I think that qemu_open MUST provide a sane errno value on exit, so you need to make sure that all exit paths out of qemu_open have a sensible errno (whether or not the helper functions also have to leave errno sane is a matter of taste).