[PATCH v1] tools/hv: terminate fcopy daemon if read from uio fails

[PATCH v1] tools/hv: terminate fcopy daemon if read from uio fails

[Olaf Hering]

Terminate endless loop in reading fails, to avoid flooding syslog.

This happens if the "Guest services" integration service is
disabled at runtime in the VM settings.

Also handle an interrupted system call, and continue in this case.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---

A more complete fix is to handle this properly in the kernel,
by making the file descriptor unavailable for further operations.

 tools/hv/hv_fcopy_uio_daemon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/hv_fcopy_uio_daemon.c
index 7a00f3066a98..281fd95dc0d8 100644
--- a/tools/hv/hv_fcopy_uio_daemon.c
+++ b/tools/hv/hv_fcopy_uio_daemon.c
@@ -468,8 +468,10 @@ int main(int argc, char *argv[])
 		 */
 		ret = pread(fcopy_fd, &tmp, sizeof(int), 0);
 		if (ret < 0) {
+			if (errno == EINTR || errno == EAGAIN)
+				continue;
 			syslog(LOG_ERR, "pread failed: %s", strerror(errno));
-			continue;
+			goto close;
 		}
 
 		len = HV_RING_SIZE;

Re: [PATCH v1] tools/hv: terminate fcopy daemon if read from uio fails

[Saurabh Singh Sengar]

On Fri, Oct 25, 2024 at 04:28:27PM +0200, Olaf Hering wrote:
> Terminate endless loop in reading fails, to avoid flooding syslog.
> 
> This happens if the "Guest services" integration service is
> disabled at runtime in the VM settings.
> 
> Also handle an interrupted system call, and continue in this case.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
> 
> A more complete fix is to handle this properly in the kernel,
> by making the file descriptor unavailable for further operations.
> 
>  tools/hv/hv_fcopy_uio_daemon.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/hv_fcopy_uio_daemon.c
> index 7a00f3066a98..281fd95dc0d8 100644
> --- a/tools/hv/hv_fcopy_uio_daemon.c
> +++ b/tools/hv/hv_fcopy_uio_daemon.c
> @@ -468,8 +468,10 @@ int main(int argc, char *argv[])
>  		 */
>  		ret = pread(fcopy_fd, &tmp, sizeof(int), 0);
>  		if (ret < 0) {
> +			if (errno == EINTR || errno == EAGAIN)
> +				continue;
>  			syslog(LOG_ERR, "pread failed: %s", strerror(errno));

Thanks for the patch. Changes look good to me, I will suggest to improve this
log message incase the error type is EIO by suggesting that users verify if
'Guest Services' is enabled.

- Saurabh

Re: [PATCH v1] tools/hv: terminate fcopy daemon if read from uio fails

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

Mon, 28 Oct 2024 02:06:20 -0700 Saurabh Singh Sengar <ssengar@linux.microsoft.com>:

> Thanks for the patch. Changes look good to me, I will suggest to improve this
> log message incase the error type is EIO by suggesting that users verify if
> 'Guest Services' is enabled.

No error happens if the state of "Guest services" remains unchanged during
the life time of the VM (either "enabled" or "disabled"). Also no error happens
if "Guest services" changes from "disabled" to "enabled". But the error happens
if "Guest services" changes from "enabled" to "disabled". I think the actual
error "EIO" is not relevant for the commit message. Maybe you mean the second
paragraph should read like this?

This happens if the state of "Guest services" integration service is changed
from "enabled" to "disabled" at runtime in the VM settings.



I'm probably not telling news if I say that this behavior is a regression compared
to the hv_utils based variant of fcopy. In the past one could flip the state of
"Guest services", it would always come back if state changed to "enabled" again,
because the "hv_fcopy" device node came back. With UIO the device node does not
come back AFAICS. Not a big deal to reboot the VM in this case.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] tools/hv: terminate fcopy daemon if read from uio fails

[Saurabh Singh Sengar]

On Mon, Oct 28, 2024 at 04:01:56PM +0100, Olaf Hering wrote:
> Mon, 28 Oct 2024 02:06:20 -0700 Saurabh Singh Sengar <ssengar@linux.microsoft.com>:
> 
> > Thanks for the patch. Changes look good to me, I will suggest to improve this
> > log message incase the error type is EIO by suggesting that users verify if
> > 'Guest Services' is enabled.
> 
> No error happens if the state of "Guest services" remains unchanged during
> the life time of the VM (either "enabled" or "disabled"). Also no error happens
> if "Guest services" changes from "disabled" to "enabled". But the error happens
> if "Guest services" changes from "enabled" to "disabled".

All this is correct.

> I think the actual
> error "EIO" is not relevant for the commit message. Maybe you mean the second
> paragraph should read like this?
> 
> This happens if the state of "Guest services" integration service is changed
> from "enabled" to "disabled" at runtime in the VM settings.

Right, we can inform user by putting this info in syslog error.

- Saurabh