From: Alexander Bluhm <alexander.bluhm@gmx.net>
Subject: Re: improve vmd error message when out of tap(4)'s
To: Dave Voutila <dv@sisu.io>
Cc: Tech <tech@openbsd.org>
Date: Wed, 3 Jan 2024 16:11:26 +0100

On Wed, Jan 03, 2024 at 09:51:14AM -0500, Dave Voutila wrote:
> The below diff improves the error messages related to opening the "next
> available" tap(4) device when using vmd(8). I feel this pops up every
> once in awhile and just did again [1] on misc@, so about time we improve
> it a bit.
> 
> We really need either custom error codes or something sent back to
> vmctl and not just piggy backing on errnos as they often don't make
> sense. That's a diff for another day as it's going to be pretty
> invasive. This one still relies on ENOENT being communicated...not the
> best but far better than "Unknown error". The bigger imrovement is in
> the vmd logging pointing out the tap we couldn't open.
> 
> ok?

OK bluhm@

> before the diff:
> 
>  # for i in `jot 5`; do vmctl start -L -b /bsd.rd "test-${i}"; done
>  vmctl: started vm 5 successfully, tty /dev/ttyp4
>  vmctl: started vm 6 successfully, tty /dev/ttyp5
>  vmctl: started vm 7 successfully, tty /dev/ttyp6
>  vmctl: started vm 8 successfully, tty /dev/ttyp7
>  vmctl: start vm command failed: Unknown error: -1
> 
>  # $(which vmd) -dv
>  vmd: startup
>  vmd: started test-1 (vm 5) successfully, tty /dev/ttyp4
>  vmd: started test-2 (vm 6) successfully, tty /dev/ttyp5
>  vmd: started test-3 (vm 7) successfully, tty /dev/ttyp6
>  vmd: started test-4 (vm 8) successfully, tty /dev/ttyp7
>  vmd: config_setvm: can't open tap tap
>  vmd: failed to start vm test-5
> 
> w/ the diff:
> 
>  # for i in `jot 5`; do vmctl start -L -b /bsd.rd "test-${i}"; done
>  vmctl: started vm 5 successfully, tty /dev/ttyp4
>  vmctl: started vm 6 successfully, tty /dev/ttyp5
>  vmctl: started vm 7 successfully, tty /dev/ttyp6
>  vmctl: started vm 8 successfully, tty /dev/ttyp7
>  vmctl: start vm command failed: No such file or directory
> 
>  # $(which vmd) -dv
>  vmd: startup
>  vmd: started test-1 (vm 5) successfully, tty /dev/ttyp4
>  vmd: started test-2 (vm 6) successfully, tty /dev/ttyp5
>  vmd: started test-3 (vm 7) successfully, tty /dev/ttyp6
>  vmd: started test-4 (vm 8) successfully, tty /dev/ttyp7
>  vmd: config_setvm: can't open /dev/tap4
>  vmd: failed to start vm test-5
> 
> [1] https://marc.info/?l=openbsd-misc&m=170421999315682&w=2
> 
> -dv@
> 
> diffstat refs/heads/master refs/heads/vmd-tap
>  M  usr.sbin/vmd/config.c  |   2+  1-
>  M  usr.sbin/vmd/vmm.c     |  23+  8-
> 
> 2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff refs/heads/master refs/heads/vmd-tap
> commit - b98228b267823b1070a646dd33a2951b3fcc0082
> commit + eae7a7dce824b151f8307a72f0be9673441175e7
> blob - 3bb246a0092cbd4318721794c81ee11964528820
> blob + 23afa2ddf617bbfefcc66a4ef46ec34a990fd27d
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -417,7 +417,8 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
>  			s = ifname;
>  		}
>  		if (tapfds[i] == -1) {
> -			log_warnx("%s: can't open tap %s", __func__, s);
> +			ret = errno;
> +			log_warnx("%s: can't open /dev/%s", __func__, s);
>  			goto fail;
>  		}
>  		if ((vif->vif_name = strdup(s)) == NULL) {
> blob - b6b138a6e3f1cc8660e95fb2b6668be256a36308
> blob + 7bc6adf721df80d51b3a0775a3e633d00eb7a2fc
> --- usr.sbin/vmd/vmm.c
> +++ usr.sbin/vmd/vmm.c
> @@ -586,26 +586,41 @@ terminate_vm(struct vm_terminate_params *vtp)
>   * Parameters
>   *  ifname: a buffer of at least IF_NAMESIZE bytes.
>   *
> - * Returns a file descriptor to the tap node opened, or -1 if no tap
> - * devices were available.
> + * Returns a file descriptor to the tap node opened or -1 if no tap devices were
> + * available, setting errno to the open(2) error.
>   */
>  int
>  opentap(char *ifname)
>  {
> -	int i, fd;
> +	int err = 0, i, fd;
>  	char path[PATH_MAX];
> 
>  	for (i = 0; i < MAX_TAP; i++) {
>  		snprintf(path, PATH_MAX, "/dev/tap%d", i);
> +
> +		errno = 0;
>  		fd = open(path, O_RDWR | O_NONBLOCK);
> -		if (fd != -1) {
> -			snprintf(ifname, IF_NAMESIZE, "tap%d", i);
> -			return (fd);
> +		if (fd != -1)
> +			break;
> +		err = errno;
> +		if (err == EBUSY) {
> +			/* Busy...try next tap. */
> +			continue;
> +		} else if (err == ENOENT) {
> +			/* Ran out of /dev/tap* special files. */
> +			break;
> +		} else {
> +			log_warn("%s: unexpected error", __func__);
> +			break;
>  		}
>  	}
> -	strlcpy(ifname, "tap", IF_NAMESIZE);
> 
> -	return (-1);
> +	/* Record the last opened tap device. */
> +	snprintf(ifname, IF_NAMESIZE, "tap%d", i);
> +
> +	if (err)
> +		errno = err;
> +	return (fd);
>  }
> 
>  /*