* [PATCH kvmtool 0/2] Automatically clean up ghost socket files @ 2019-05-03 17:08 Andre Przywara 2019-05-03 17:08 ` [PATCH kvmtool 1/2] list: Clean " Andre Przywara 2019-05-03 17:08 ` [PATCH kvmtool 2/2] run: Check for ghost socket file upon VM creation Andre Przywara 0 siblings, 2 replies; 5+ messages in thread From: Andre Przywara @ 2019-05-03 17:08 UTC (permalink / raw) To: Will Deacon; +Cc: kvm kvmtool is creating UNIX socket inodes to communicate between a running instance and a debug command issued by another lkvm process. When kvmtool (or the kernel) crashes, those files are not cleaned up and cause all kind of annoyances. Those two patches delete leftover socket files on calling "lkvm list", also remove an existing one (then reusing the same name) when creating a guest. This avoids random breakages when running kvmtool, and helps to run it from scripts. Cheers, Andre Andre Przywara (2): list: Clean up ghost socket files run: Check for ghost socket file upon VM creation kvm-ipc.c | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH kvmtool 1/2] list: Clean up ghost socket files 2019-05-03 17:08 [PATCH kvmtool 0/2] Automatically clean up ghost socket files Andre Przywara @ 2019-05-03 17:08 ` Andre Przywara 2019-05-24 16:27 ` Will Deacon 2019-05-03 17:08 ` [PATCH kvmtool 2/2] run: Check for ghost socket file upon VM creation Andre Przywara 1 sibling, 1 reply; 5+ messages in thread From: Andre Przywara @ 2019-05-03 17:08 UTC (permalink / raw) To: Will Deacon; +Cc: kvm 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++; continue; + } ret = callback(entry->d_name, sock); close(sock); if (ret < 0) @@ -175,6 +178,9 @@ int kvm__enumerate_instances(int (*callback)(const char *name, int fd)) closedir(dir); + if (cleaned) + pr_info("Removed %d ghost socket files", cleaned); + return ret; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH kvmtool 1/2] list: Clean up ghost socket files 2019-05-03 17:08 ` [PATCH kvmtool 1/2] list: Clean " Andre Przywara @ 2019-05-24 16:27 ` Will Deacon 0 siblings, 0 replies; 5+ messages in thread From: Will Deacon @ 2019-05-24 16:27 UTC (permalink / raw) To: Andre Przywara; +Cc: kvm 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH kvmtool 2/2] run: Check for ghost socket file upon VM creation 2019-05-03 17:08 [PATCH kvmtool 0/2] Automatically clean up ghost socket files Andre Przywara 2019-05-03 17:08 ` [PATCH kvmtool 1/2] list: Clean " Andre Przywara @ 2019-05-03 17:08 ` Andre Przywara 2019-05-24 16:27 ` Will Deacon 1 sibling, 1 reply; 5+ messages in thread From: Andre Przywara @ 2019-05-03 17:08 UTC (permalink / raw) To: Will Deacon; +Cc: kvm Kvmtool creates a (debug) UNIX socket file for each VM, using its (possibly auto-generated) name as the filename. There is a check using access(), which bails out with an error message if a socket with that name already exists. Aside from this check being unnecessary, as the bind() call later would complain as well, this is also racy. But more annoyingly the bail out is not needed most of the time: an existing socket inode is most likely just an orphaned leftover from a previous kvmtool run, which just failed to remove that file, because of a crash, for instance. Upon finding such a collision, let's first try to connect to that socket, to detect if there is still a kvmtool instance listening on the other end. If that fails, this socket will never come back to life, so we can safely clean it up and reuse the name for the new guest. However if the connect() succeeds, there is an actual live kvmtool instance using this name, so not proceeding is the only option. This should never happen with the (PID based) automatically generated names, though. This avoids an annoying (and not helpful) error message and helps automated kvmtool runs to proceed in more cases. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- kvm-ipc.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/kvm-ipc.c b/kvm-ipc.c index d9a07595..06909171 100644 --- a/kvm-ipc.c +++ b/kvm-ipc.c @@ -43,10 +43,6 @@ static int kvm__create_socket(struct kvm *kvm) snprintf(full_name, sizeof(full_name), "%s/%s%s", kvm__get_dir(), kvm->cfg.guest_name, KVM_SOCK_SUFFIX); - if (access(full_name, F_OK) == 0) { - pr_err("Socket file %s already exist", full_name); - return -EEXIST; - } s = socket(AF_UNIX, SOCK_STREAM, 0); if (s < 0) { @@ -58,6 +54,32 @@ static int kvm__create_socket(struct kvm *kvm) strlcpy(local.sun_path, full_name, sizeof(local.sun_path)); len = strlen(local.sun_path) + sizeof(local.sun_family); r = bind(s, (struct sockaddr *)&local, len); + /* Check for an existing socket file */ + if (r < 0 && errno == EADDRINUSE) { + r = connect(s, (struct sockaddr *)&local, len); + if (r == 0) { + /* + * If we could connect, there is already a guest + * using this same name. This should not happen + * for PID derived names, but could happen for user + * provided guest names. + */ + pr_err("Guest socket file %s already exists.", + full_name); + r = -EEXIST; + goto fail; + } + if (errno == ECONNREFUSED) { + /* + * This is a ghost socket file, with no-one listening + * on the other end. Since kvmtool will only bind + * above when creating a new guest, there is no + * danger in just removing the file and re-trying. + */ + unlink(full_name); + r = bind(s, (struct sockaddr *)&local, len); + } + } if (r < 0) { perror("bind"); goto fail; -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH kvmtool 2/2] run: Check for ghost socket file upon VM creation 2019-05-03 17:08 ` [PATCH kvmtool 2/2] run: Check for ghost socket file upon VM creation Andre Przywara @ 2019-05-24 16:27 ` Will Deacon 0 siblings, 0 replies; 5+ messages in thread From: Will Deacon @ 2019-05-24 16:27 UTC (permalink / raw) To: Andre Przywara; +Cc: kvm On Fri, May 03, 2019 at 06:08:21PM +0100, Andre Przywara wrote: > Kvmtool creates a (debug) UNIX socket file for each VM, using its > (possibly auto-generated) name as the filename. There is a check using > access(), which bails out with an error message if a socket with that > name already exists. > > Aside from this check being unnecessary, as the bind() call later would > complain as well, this is also racy. But more annoyingly the bail out is > not needed most of the time: an existing socket inode is most likely just > an orphaned leftover from a previous kvmtool run, which just failed to > remove that file, because of a crash, for instance. > > Upon finding such a collision, let's first try to connect to that socket, > to detect if there is still a kvmtool instance listening on the other > end. If that fails, this socket will never come back to life, so we can > safely clean it up and reuse the name for the new guest. > However if the connect() succeeds, there is an actual live kvmtool > instance using this name, so not proceeding is the only option. > This should never happen with the (PID based) automatically generated > names, though. > > This avoids an annoying (and not helpful) error message and helps > automated kvmtool runs to proceed in more cases. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > kvm-ipc.c | 30 ++++++++++++++++++++++++++---- > 1 file changed, 26 insertions(+), 4 deletions(-) > > diff --git a/kvm-ipc.c b/kvm-ipc.c > index d9a07595..06909171 100644 > --- a/kvm-ipc.c > +++ b/kvm-ipc.c > @@ -43,10 +43,6 @@ static int kvm__create_socket(struct kvm *kvm) > > snprintf(full_name, sizeof(full_name), "%s/%s%s", > kvm__get_dir(), kvm->cfg.guest_name, KVM_SOCK_SUFFIX); > - if (access(full_name, F_OK) == 0) { > - pr_err("Socket file %s already exist", full_name); > - return -EEXIST; > - } > > s = socket(AF_UNIX, SOCK_STREAM, 0); > if (s < 0) { > @@ -58,6 +54,32 @@ static int kvm__create_socket(struct kvm *kvm) > strlcpy(local.sun_path, full_name, sizeof(local.sun_path)); > len = strlen(local.sun_path) + sizeof(local.sun_family); > r = bind(s, (struct sockaddr *)&local, len); > + /* Check for an existing socket file */ > + if (r < 0 && errno == EADDRINUSE) { > + r = connect(s, (struct sockaddr *)&local, len); > + if (r == 0) { > + /* > + * If we could connect, there is already a guest > + * using this same name. This should not happen > + * for PID derived names, but could happen for user > + * provided guest names. > + */ > + pr_err("Guest socket file %s already exists.", > + full_name); > + r = -EEXIST; > + goto fail; > + } > + if (errno == ECONNREFUSED) { > + /* > + * This is a ghost socket file, with no-one listening > + * on the other end. Since kvmtool will only bind > + * above when creating a new guest, there is no > + * danger in just removing the file and re-trying. > + */ > + unlink(full_name); Can we print a diagnostic when this happens please? (hopefully the same message as whatever you end up doing in the first patch). Will ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-24 16:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-03 17:08 [PATCH kvmtool 0/2] Automatically clean up ghost socket files Andre Przywara 2019-05-03 17:08 ` [PATCH kvmtool 1/2] list: Clean " Andre Przywara 2019-05-24 16:27 ` Will Deacon 2019-05-03 17:08 ` [PATCH kvmtool 2/2] run: Check for ghost socket file upon VM creation Andre Przywara 2019-05-24 16:27 ` Will Deacon
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.