Re: [PATCH kvmtool 1/2] list: Clean up ghost socket files

[Will Deacon <will.deacon@arm.com>]

On Fri, May 03, 2019 at 06:08:20PM +0100, Andre Przywara wrote:
> When kvmtool (or the host kernel) crashes or gets killed, we cannot
> automatically remove the socket file we created for that VM.
> A later call of "lkvm list" iterates over all those files and complains
> about those "ghost socket files", as there is no one listening on
> the other side. Also sometimes the automatic guest name generation
> happens to generate the same name again, so an unrelated "lkvm run"
> later complains and stops, which is bad for automation.
> 
> As the only code doing a listen() on this socket is kvmtool upon VM
> *creation*, such an orphaned socket file will never come back to life,
> so we can as well unlink() those sockets in the code. This spares the
> user the messages and the burden of doing it herself.
> We keep a message in the code to notify the user of this.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  kvm-ipc.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/kvm-ipc.c b/kvm-ipc.c
> index e07ad105..d9a07595 100644
> --- a/kvm-ipc.c
> +++ b/kvm-ipc.c
> @@ -101,9 +101,8 @@ int kvm__get_sock_by_instance(const char *name)
>  
>  	r = connect(s, (struct sockaddr *)&local, len);
>  	if (r < 0 && errno == ECONNREFUSED) {
> -		/* Tell the user clean ghost socket file */
> -		pr_err("\"%s\" could be a ghost socket file, please remove it",
> -				sock_file);
> +		/* Clean up the ghost socket file */
> +		unlink(local.sun_path);
>  		return r;
>  	} else if (r < 0) {
>  		return r;
> @@ -140,6 +139,7 @@ int kvm__enumerate_instances(int (*callback)(const char *name, int fd))
>  	struct dirent *entry;
>  	int ret = 0;
>  	const char *path;
> +	int cleaned = 0;
>  
>  	path = kvm__get_dir();
>  
> @@ -164,8 +164,11 @@ int kvm__enumerate_instances(int (*callback)(const char *name, int fd))
>  
>  			*p = 0;
>  			sock = kvm__get_sock_by_instance(entry->d_name);
> -			if (sock < 0)
> +			if (sock < 0) {
> +				if (errno == ECONNREFUSED)
> +					cleaned++;

This is fragile, because you're relying on errno being preserved from the
failing connect() call after kvm__get_sock_by_instance() returns. Yes,
that's true today, but it would be easy to change
kvm__get_sock_by_instance() in future and miss this detail.

Maybe just replace the existing print to that it says about each file being
removed?

Will