On 08/13/2012 12:16 PM, Eric Blake wrote:
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)?
I think what I did in v10 should suffice. I didn't update
monitor_fdset_dup_fd_add(), but I did update the calling code. If the
call fails then I set errno to EINVAL since (unless there's a bug) the
only possible error is that the fdset ID was non-existent.
It makes sense to add the asserts, but at this point I'd like to stick
with what we have in v10 if that's ok.
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).
Yes, I agree. I went through the code and at this point (with the v10
patches) we're always setting errno, or calling a library API that
should be setting it.
--
Regards,
Corey
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list