* [PATCH] check for KVM_GET/SET_REG ioctl failures @ 2007-02-06 18:46 Muli Ben-Yehuda [not found] ` <20070206184639.GA4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Muli Ben-Yehuda @ 2007-02-06 18:46 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm-devel check for KVM_GET/SET_REGS ioctl failures Signed-off-by: Muli Ben-Yehuda <muli-7z/5BgaJwgfQT0dZR+AlfA@public.gmane.org> diff -r ee6f88782b5f -r 9a3e101daf18 user/kvmctl.c --- a/user/kvmctl.c Tue Feb 06 10:55:17 2007 +0000 +++ b/user/kvmctl.c Tue Feb 06 17:05:56 2007 +0200 @@ -235,19 +235,23 @@ static int handle_io(kvm_context_t kvm, int delta; struct translation_cache tr; int _in = (run->io.direction == KVM_EXIT_IO_IN); + int r; translation_cache_init(&tr); if (run->io.string || _in) { regs.vcpu = run->vcpu; - ioctl(kvm->fd, KVM_GET_REGS, ®s); + r = ioctl(kvm->fd, KVM_GET_REGS, ®s); + if (r == -1) { + perror("KVM_GET_REGS"); + exit(1); + } } delta = run->io.string_down ? -run->io.size : run->io.size; while (more_io(run, first_time)) { void *value_addr; - int r; if (!run->io.string) { if (_in) @@ -327,13 +331,25 @@ static int handle_io(kvm_context_t kvm, } first_time = 0; if (r) { - ioctl(kvm->fd, KVM_SET_REGS, ®s); - return r; - } - } - - if (run->io.string || _in) - ioctl(kvm->fd, KVM_SET_REGS, ®s); + int savedret = r; + r = ioctl(kvm->fd, KVM_SET_REGS, ®s); + if (r == -1) { + perror("KVM_SET_REGS"); + exit(1); + } + + return savedret; + } + } + + if (run->io.string || _in) { + r = ioctl(kvm->fd, KVM_SET_REGS, ®s); + if (r == -1) { + perror("KVM_SET_REGS"); + exit(1); + } + } + run->emulated = 1; return 0; } ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier. Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20070206184639.GA4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org>]
* Re: [PATCH] check for KVM_GET/SET_REG ioctl failures [not found] ` <20070206184639.GA4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> @ 2007-02-06 19:26 ` Anthony Liguori [not found] ` <45C8D680.3020704-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Anthony Liguori @ 2007-02-06 19:26 UTC (permalink / raw) To: Muli Ben-Yehuda; +Cc: kvm-devel Muli Ben-Yehuda wrote: > check for KVM_GET/SET_REGS ioctl failures > > Signed-off-by: Muli Ben-Yehuda <muli-7z/5BgaJwgfQT0dZR+AlfA@public.gmane.org> > > diff -r ee6f88782b5f -r 9a3e101daf18 user/kvmctl.c > --- a/user/kvmctl.c Tue Feb 06 10:55:17 2007 +0000 > +++ b/user/kvmctl.c Tue Feb 06 17:05:56 2007 +0200 > @@ -235,19 +235,23 @@ static int handle_io(kvm_context_t kvm, > int delta; > struct translation_cache tr; > int _in = (run->io.direction == KVM_EXIT_IO_IN); > + int r; > > translation_cache_init(&tr); > > if (run->io.string || _in) { > regs.vcpu = run->vcpu; > - ioctl(kvm->fd, KVM_GET_REGS, ®s); > + r = ioctl(kvm->fd, KVM_GET_REGS, ®s); > + if (r == -1) { > + perror("KVM_GET_REGS"); > + exit(1); > Please don't add exit()'s to a library, these are the sort of things we should be trying to remove. In the very least, you should probably expect to see EINTRs. Regards, Anthony Liguori > + } > } > > delta = run->io.string_down ? -run->io.size : run->io.size; > > while (more_io(run, first_time)) { > void *value_addr; > - int r; > > if (!run->io.string) { > if (_in) > @@ -327,13 +331,25 @@ static int handle_io(kvm_context_t kvm, > } > first_time = 0; > if (r) { > - ioctl(kvm->fd, KVM_SET_REGS, ®s); > - return r; > - } > - } > - > - if (run->io.string || _in) > - ioctl(kvm->fd, KVM_SET_REGS, ®s); > + int savedret = r; > + r = ioctl(kvm->fd, KVM_SET_REGS, ®s); > + if (r == -1) { > + perror("KVM_SET_REGS"); > + exit(1); > + } > + > + return savedret; > + } > + } > + > + if (run->io.string || _in) { > + r = ioctl(kvm->fd, KVM_SET_REGS, ®s); > + if (r == -1) { > + perror("KVM_SET_REGS"); > + exit(1); > + } > + } > + > run->emulated = 1; > return 0; > } > > ------------------------------------------------------------------------- > Using Tomcat but need to do more? Need to support web services, security? > Get stuff done quickly with pre-integrated technology to make your job easier. > Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo > http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 > _______________________________________________ > kvm-devel mailing list > kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org > https://lists.sourceforge.net/lists/listinfo/kvm-devel > > ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier. Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <45C8D680.3020704-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] check for KVM_GET/SET_REG ioctl failures [not found] ` <45C8D680.3020704-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> @ 2007-02-06 19:39 ` Muli Ben-Yehuda [not found] ` <20070206193939.GE4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Muli Ben-Yehuda @ 2007-02-06 19:39 UTC (permalink / raw) To: Anthony Liguori; +Cc: kvm-devel On Tue, Feb 06, 2007 at 01:26:56PM -0600, Anthony Liguori wrote: > Please don't add exit()'s to a library, these are the sort of things > we should be trying to remove. I agree on general principles, but do we envision libkvm.a being used in anything outside qemu? in any case, if not exit(), what would you suggest? from a quick reading of the code, it doesn't appear feasible to go on when get or set regs fail. > In the very least, you should probably expect to see EINTRs. Hmm, which code path do you have in mind that can return EINTR here? Cheers, Muli ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier. Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20070206193939.GE4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org>]
* Re: [PATCH] check for KVM_GET/SET_REG ioctl failures [not found] ` <20070206193939.GE4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> @ 2007-02-06 20:59 ` Anthony Liguori [not found] ` <45C8EC44.509-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> 2007-02-06 22:36 ` [PATCH] " James Morris 1 sibling, 1 reply; 8+ messages in thread From: Anthony Liguori @ 2007-02-06 20:59 UTC (permalink / raw) To: Muli Ben-Yehuda; +Cc: kvm-devel Muli Ben-Yehuda wrote: > On Tue, Feb 06, 2007 at 01:26:56PM -0600, Anthony Liguori wrote: > > >> Please don't add exit()'s to a library, these are the sort of things >> we should be trying to remove. >> > > I agree on general principles, but do we envision libkvm.a being used > in anything outside qemu? in any case, if not exit(), what would you > suggest? from a quick reading of the code, it doesn't appear feasible > to go on when get or set regs fail. > Propagate it up and let QEMU decide what to do. There is at least one other user of libkvmctl that I know of. >> In the very least, you should probably expect to see EINTRs. >> > > Hmm, which code path do you have in mind that can return EINTR here? > QEMU uses signals quite a bit so I think being defensive on syscalls is best. It's stung me a few times. I don't think that the {GET,SEG}_REGS ioctls would actually return EINTR ATM. Regards, Anthony Liguori > Cheers, > Muli > ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier. Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <45C8EC44.509-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] check for KVM_GET/SET_REG ioctl failures [not found] ` <45C8EC44.509-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> @ 2007-02-06 21:09 ` Muli Ben-Yehuda [not found] ` <20070206210926.GI4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Muli Ben-Yehuda @ 2007-02-06 21:09 UTC (permalink / raw) To: Anthony Liguori; +Cc: kvm-devel On Tue, Feb 06, 2007 at 02:59:48PM -0600, Anthony Liguori wrote: > Propagate it up and let QEMU decide what to do. > > There is at least one other user of libkvmctl that I know of. Ok, I'll rework it this way (and fix the rest of exit()'s in libkvm while I'm at it). Do we agree the right thing for qemu to do on ioctl failures at this time is exit()? > QEMU uses signals quite a bit so I think being defensive on syscalls > is best. It's stung me a few times. Syscalls don't return EINTR unless they're written specifically to check for sigpending() and return EINTR. I don't think there's any point to check for EINTR here when we know that the ioctl() will never return it. If that changes, we'll have to audit all of the callers anyway and change them. Cheers, Muli ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier. Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20070206210926.GI4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org>]
* Re: [PATCH] check for KVM_GET/SET_REG ioctl failures [not found] ` <20070206210926.GI4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> @ 2007-02-06 21:20 ` Anthony Liguori 2007-02-06 21:35 ` [PATCH v2] " Muli Ben-Yehuda 1 sibling, 0 replies; 8+ messages in thread From: Anthony Liguori @ 2007-02-06 21:20 UTC (permalink / raw) To: Muli Ben-Yehuda; +Cc: kvm-devel Muli Ben-Yehuda wrote: > On Tue, Feb 06, 2007 at 02:59:48PM -0600, Anthony Liguori wrote: > > >> Propagate it up and let QEMU decide what to do. >> >> There is at least one other user of libkvmctl that I know of. >> > > Ok, I'll rework it this way (and fix the rest of exit()'s in libkvm > while I'm at it). Do we agree the right thing for qemu to do on ioctl > failures at this time is exit()? > I don't think it's the best thing, but it's acceptable IMHO. Regards, Anthony Liguori >> QEMU uses signals quite a bit so I think being defensive on syscalls >> is best. It's stung me a few times. >> > > Syscalls don't return EINTR unless they're written specifically to > check for sigpending() and return EINTR. I don't think there's any > point to check for EINTR here when we know that the ioctl() will never > return it. If that changes, we'll have to audit all of the callers > anyway and change them. > > Cheers, > Muli > ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier. Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] check for KVM_GET/SET_REG ioctl failures [not found] ` <20070206210926.GI4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> 2007-02-06 21:20 ` Anthony Liguori @ 2007-02-06 21:35 ` Muli Ben-Yehuda 1 sibling, 0 replies; 8+ messages in thread From: Muli Ben-Yehuda @ 2007-02-06 21:35 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm-devel Check for KVM_GET/SET_REGS ioctl failures (and return -errno which the caller of kvm_run will promptely ignore). Signed-off-by: Muli Ben-Yehuda <muli-7z/5BgaJwgfQT0dZR+AlfA@public.gmane.org> diff -r d116fd1b3260 -r 74c55612f99d user/kvmctl.c --- a/user/kvmctl.c Tue Feb 06 21:26:46 2007 +0200 +++ b/user/kvmctl.c Tue Feb 06 23:29:49 2007 +0200 @@ -235,19 +235,21 @@ static int handle_io(kvm_context_t kvm, int delta; struct translation_cache tr; int _in = (run->io.direction == KVM_EXIT_IO_IN); + int r; translation_cache_init(&tr); if (run->io.string || _in) { regs.vcpu = run->vcpu; - ioctl(kvm->fd, KVM_GET_REGS, ®s); + r = ioctl(kvm->fd, KVM_GET_REGS, ®s); + if (r == -1) + return -errno; } delta = run->io.string_down ? -run->io.size : run->io.size; while (more_io(run, first_time)) { void *value_addr; - int r; if (!run->io.string) { if (_in) @@ -327,13 +329,22 @@ static int handle_io(kvm_context_t kvm, } first_time = 0; if (r) { - ioctl(kvm->fd, KVM_SET_REGS, ®s); - return r; - } - } - - if (run->io.string || _in) - ioctl(kvm->fd, KVM_SET_REGS, ®s); + int savedret = r; + r = ioctl(kvm->fd, KVM_SET_REGS, ®s); + if (r == -1) + return -errno; + + return savedret; + } + } + + if (run->io.string || _in) { + r = ioctl(kvm->fd, KVM_SET_REGS, ®s); + if (r == -1) + return -errno; + + } + run->emulated = 1; return 0; } ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier. Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] check for KVM_GET/SET_REG ioctl failures [not found] ` <20070206193939.GE4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> 2007-02-06 20:59 ` Anthony Liguori @ 2007-02-06 22:36 ` James Morris 1 sibling, 0 replies; 8+ messages in thread From: James Morris @ 2007-02-06 22:36 UTC (permalink / raw) To: Muli Ben-Yehuda; +Cc: kvm-devel On Tue, 6 Feb 2007, Muli Ben-Yehuda wrote: > On Tue, Feb 06, 2007 at 01:26:56PM -0600, Anthony Liguori wrote: > > > Please don't add exit()'s to a library, these are the sort of things > > we should be trying to remove. > > I agree on general principles, but do we envision libkvm.a being used > in anything outside qemu? Definitely. - James -- James Morris <jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org> ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier. Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2007-02-06 22:36 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-02-06 18:46 [PATCH] check for KVM_GET/SET_REG ioctl failures Muli Ben-Yehuda [not found] ` <20070206184639.GA4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> 2007-02-06 19:26 ` Anthony Liguori [not found] ` <45C8D680.3020704-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> 2007-02-06 19:39 ` Muli Ben-Yehuda [not found] ` <20070206193939.GE4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> 2007-02-06 20:59 ` Anthony Liguori [not found] ` <45C8EC44.509-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> 2007-02-06 21:09 ` Muli Ben-Yehuda [not found] ` <20070206210926.GI4292-k73YwwB0fHlWk0Htik3J/w@public.gmane.org> 2007-02-06 21:20 ` Anthony Liguori 2007-02-06 21:35 ` [PATCH v2] " Muli Ben-Yehuda 2007-02-06 22:36 ` [PATCH] " James Morris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.