Re: [PATCH] getroot: Correctly handle missing btrfs device identifiers.

[Andrei Borzenkov <arvidjaar@gmail.com>]

17.03.2016 18:41, James Johnston пишет:
> From 97bcb9eb8c34329a8ca1ec88fd89296e273ceb24 Mon Sep 17 00:00:00 2001
> From: James Johnston <johnstonj.public@codenest.com>
> Date: Thu, 17 Mar 2016 15:10:49 +0000
> Subject: [PATCH] getroot: Correctly handle missing btrfs device identifiers.
> 
> The btrfs driver's BTRFS_IOC_FS_INFO ioctl only tells you the maximum
> device ID and the number of devices.  It does not tell you which
> device IDs may no longer be allocated - for example, if a device is
> later deleted from the file system, its device ID won't be allocated
> any more.
> 
> You must then probe each device ID with BTRFS_IOC_DEV_INFO to see if
> it is a valid device or not.  It is not an error condition for this
> ioctl to return ENODEV: it seems to mean that the device was deleted
> by the user.
> 

Kernel returns ENODEV if device was not found; we do not know why it was
not found. We need some other way to verify that all devices are
actually present to preserve current behavior.

Actually we probably need to handle redundant layout where non-essential
devices are missing but that is another topic.

> Signed-off-by: James Johnston <johnstonj.public@codenest.com>
> ---
>  grub-core/osdep/linux/getroot.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/grub-core/osdep/linux/getroot.c b/grub-core/osdep/linux/getroot.c
> index 10480b6..852f3a2 100644
> --- a/grub-core/osdep/linux/getroot.c
> +++ b/grub-core/osdep/linux/getroot.c
> @@ -242,15 +242,29 @@ grub_find_root_devices_from_btrfs (const char *dir)
>        struct btrfs_ioctl_dev_info_args devi;
>        memset (&devi, 0, sizeof (devi));
>        devi.devid = i;
> +      /* We have to probe all possible device IDs, since btrfs doesn't
> +	 give us a list of all the valid ones.  */
>        if (ioctl (fd, BTRFS_IOC_DEV_INFO, &devi) < 0)
>  	{
> -	  close (fd);
> -	  free (ret);
> -	  return NULL;
> +	  /* ENODEV is normal, e.g. if the first device is deleted from
> +	     an array. */
> +	  int last_error = errno;
> +	  if (last_error != ENODEV)

Why this temporary variable? Why you cannot check errno directly?

> +	    {
> +	      grub_util_warn(_("btrfs device info could not be read "
> +			       "for %s, device %d: errno %d"), dir, i,
> +			       last_error);
> +	      close (fd);
> +	      free (ret);
> +	      return NULL;
> +	    }
> +	}
> +      else
> +	{
> +	  ret[j++] = xstrdup ((char *) devi.path);
> +	  if (j >= fsi.num_devices)
> +	    break;
>  	}
> -      ret[j++] = xstrdup ((char *) devi.path);
> -      if (j >= fsi.num_devices)
> -	break;
>      }
>    close (fd);
>    ret[j] = 0;
>