| Message ID | 20190503170821.260705-3-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Automatically clean up ghost socket files | expand |
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
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;
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(-)