[PATCH] check for KVM_GET/SET_REG ioctl failures

[PATCH] check for KVM_GET/SET_REG ioctl failures

[Muli Ben-Yehuda]

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, &regs);
+		r = ioctl(kvm->fd, KVM_GET_REGS, &regs);
+		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, &regs);
-			return r;
-		}
-	}
-
-	if (run->io.string || _in)
-		ioctl(kvm->fd, KVM_SET_REGS, &regs);
+			int savedret = r;
+			r = ioctl(kvm->fd, KVM_SET_REGS, &regs);
+			if (r == -1) {
+				perror("KVM_SET_REGS");
+				exit(1);
+			}
+
+			return savedret;
+		}
+	}
+
+	if (run->io.string || _in) {
+		r = ioctl(kvm->fd, KVM_SET_REGS, &regs);
+		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

Re: [PATCH] check for KVM_GET/SET_REG ioctl failures

[Anthony Liguori]

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, &regs);
> +		r = ioctl(kvm->fd, KVM_GET_REGS, &regs);
> +		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, &regs);
> -			return r;
> -		}
> -	}
> -
> -	if (run->io.string || _in)
> -		ioctl(kvm->fd, KVM_SET_REGS, &regs);
> +			int savedret = r;
> +			r = ioctl(kvm->fd, KVM_SET_REGS, &regs);
> +			if (r == -1) {
> +				perror("KVM_SET_REGS");
> +				exit(1);
> +			}
> +
> +			return savedret;
> +		}
> +	}
> +
> +	if (run->io.string || _in) {
> +		r = ioctl(kvm->fd, KVM_SET_REGS, &regs);
> +		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

Re: [PATCH] check for KVM_GET/SET_REG ioctl failures

[Muli Ben-Yehuda]

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

Re: [PATCH] check for KVM_GET/SET_REG ioctl failures

[Anthony Liguori]

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

Re: [PATCH] check for KVM_GET/SET_REG ioctl failures

[Muli Ben-Yehuda]

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

Re: [PATCH] check for KVM_GET/SET_REG ioctl failures

[Anthony Liguori]

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

[PATCH v2] check for KVM_GET/SET_REG ioctl failures

[Muli Ben-Yehuda]

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, &regs);
+		r = ioctl(kvm->fd, KVM_GET_REGS, &regs);
+		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, &regs);
-			return r;
-		}
-	}
-
-	if (run->io.string || _in)
-		ioctl(kvm->fd, KVM_SET_REGS, &regs);
+			int savedret = r;
+			r = ioctl(kvm->fd, KVM_SET_REGS, &regs);
+			if (r == -1)
+				return -errno;
+
+			return savedret;
+		}
+	}
+
+	if (run->io.string || _in) {
+		r = ioctl(kvm->fd, KVM_SET_REGS, &regs);
+		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

Re: [PATCH] check for KVM_GET/SET_REG ioctl failures

[James Morris]

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