From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50245) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuK2S-0005sc-IE for qemu-devel@nongnu.org; Thu, 26 Jul 2012 05:07:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SuK2R-0002d3-Db for qemu-devel@nongnu.org; Thu, 26 Jul 2012 05:07:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15692) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuK2R-0002bJ-53 for qemu-devel@nongnu.org; Thu, 26 Jul 2012 05:07:19 -0400 Message-ID: <501108BB.6070204@redhat.com> Date: Thu, 26 Jul 2012 11:07:07 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1343048885-1701-1-git-send-email-coreyb@linux.vnet.ibm.com> <1343048885-1701-7-git-send-email-coreyb@linux.vnet.ibm.com> <50104C44.1050206@redhat.com> <5010C031.9040602@linux.vnet.ibm.com> In-Reply-To: <5010C031.9040602@linux.vnet.ibm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 6/6] block: Enable qemu_open/close to work with fd sets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Bryant Cc: aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, Eric Blake Am 26.07.2012 05:57, schrieb Corey Bryant: > On 07/25/2012 03:43 PM, Eric Blake wrote: >> On 07/23/2012 07:08 AM, Corey Bryant wrote: >>> +int monitor_fdset_get_fd(Monitor *mon, int64_t fdset_id, int flags) >>> +{ >>> + mon_fdset_t *mon_fdset; >>> + mon_fdset_fd_t *mon_fdset_fd; >>> + int mon_fd_flags; >>> + >>> + if (!mon) { >>> + errno = ENOENT; >>> + return -1; >>> + } >>> + >>> + QLIST_FOREACH(mon_fdset, &mon->fdsets, next) { >>> + if (mon_fdset->id != fdset_id) { >>> + continue; >>> + } >>> + QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) { >>> + if (mon_fdset_fd->removed) { >>> + continue; >>> + } >>> + >>> + mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL); >>> + if (mon_fd_flags == -1) { >>> + return -1; >> >> This says we fail on the first fcntl() failure, instead of trying other >> fds in the set. Granted, an fcntl() failure is probably the sign of a >> bigger bug (such as closing an fd at the wrong point in time), so I >> guess trying to go on doesn't make much sense once we already know we >> are hosed. >> > > I think I'll stick with it the way it is. If fcntl() fails we might > have a tainted fd set so I think we should fail. The alternative would be s/return 1/continue/, right? I think either way is acceptable. >>> + } >>> + >>> + switch (flags & O_ACCMODE) { >>> + case O_RDWR: >>> + if ((mon_fd_flags & O_ACCMODE) == O_RDWR) { >>> + return mon_fdset_fd->fd; >>> + } >>> + break; >>> + case O_RDONLY: >>> + if ((mon_fd_flags & O_ACCMODE) == O_RDONLY) { >>> + return mon_fdset_fd->fd; >>> + } >>> + break; >> >> Do we want to allow the case where the caller asked for O_RDONLY, but >> the set only has O_RDWR? After all, the caller is getting a compatible >> subset of what the set offers. > > I don't see a problem with it. I would require exact matches like you implemented, in order to prevent damage if we ever had a bug that writes to a read-only file. I believe it also makes the semantics clearer and the code simpler, while it shouldn't make much of a difference for clients. Kevin