[libvirt] [PATCH v3 2/2] storage: Improve error handling on cdrom backend

[John Ferlan]

On 07/11/2018 05:44 AM, Han Han wrote:
> Implement virFileCheckCDROM in virStorageBackendVolOpen to check if
> cdrom backend is ready. Skip the error of cdrom not ready.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1596096
> 
> Signed-off-by: Han Han <hhan at redhat.com>
> ---
>  src/storage/storage_util.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> index da99043e0a..08b625d136 100644
> --- a/src/storage/storage_util.c
> +++ b/src/storage/storage_util.c
> @@ -1509,6 +1509,7 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
>      int fd, mode = 0;
>      char *base = last_component(path);
>      bool noerror = (flags & VIR_STORAGE_VOL_OPEN_NOERROR);
> +    virFileCDRomStatus cd_status;

s/;/ = VIR_FILE_CDROM_UNKNOWN;/

>  
>      if (lstat(path, sb) < 0) {
>          if (errno == ENOENT) {
> @@ -1545,6 +1546,13 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
>          return -1;
>      }
>  
> +    if (virFileCheckCDROM(path, &cd_status)) {
> +        if (cd_status != VIR_FILE_CDROM_DISC_OK) {
> +            VIR_WARN("ignoring CDROM %s is not ready", path);

And we have no idea why it's not ready ever logged.  IOW: You have a
cd_status there, but don't log what it is.

Also, what if this libvirt is compiled when __linux__ is not default and
thus uses the "#else" version of virFileCheckCDROM? The cd_status is not
filled in there, so we'd fail always.

> +            return -2;

Return of -2 is supposed to be limited to :

" If VIR_STORAGE_VOL_OPEN_NOERROR is passed, we return -2 if file mode
is unexpected or the volume is a dangling symbolic link."

Note that other places returning -2 and using VIR_WARN will use
"noerror" which is set at the top of the method.

John


> +        }
> +    }
> +
>      /* O_NONBLOCK should only matter during open() for fifos and
>       * sockets, which we already filtered; but using it prevents a
>       * TOCTTOU race.  However, later on we will want to read() the
>