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@xxxxxxx>
> ---
> 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
