[virt-tools-list] [PATCH 2/2] virtinst: Use isoinfo to extract files from ISOs

[Cole Robinson]

On 11/08/2017 01:23 AM, Andrew Wong wrote:
> ---
>  virtinst/urlfetcher.py | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 

Nice work! Seems to work in my minimal testing. Some comments below.

> diff --git a/virtinst/urlfetcher.py b/virtinst/urlfetcher.py
> index 1288668a..3d197afd 100644
> --- a/virtinst/urlfetcher.py
> +++ b/virtinst/urlfetcher.py
> @@ -305,11 +305,7 @@ class _MountedURLFetcher(_LocalURLFetcher):
>          if self.location.startswith("nfs:"):
>              cmd = [mountcmd, "-o", "ro", self.location[4:], self._srcdir]
>          else:
> -            if stat.S_ISBLK(os.stat(self.location)[stat.ST_MODE]):
> -                mountopt = "ro"
> -            else:
> -                mountopt = "ro,loop"
> -            cmd = [mountcmd, "-o", mountopt, self.location, self._srcdir]
> +            cmd = [mountcmd, "-o", "ro", self.location, self._srcdir]
>

I think the block device mounting was only intended to support the
/dev/cdrom or /dev/sr0 case, which will also work fine with your isoinfo
commans, so you can drop this whole chunk and the S_ISBLK handling moved
below

>          logging.debug("mount cmd: %s", cmd)
>          if not self._in_test_suite:
> @@ -338,6 +334,33 @@ class _MountedURLFetcher(_LocalURLFetcher):
>              self._mounted = False
>  
>  
> +class _ISOURLFetcher(_URLFetcher):
> +    _cache_file_list = None
> +
> +    def _make_full_url(self, filename):
> +        return "/" + filename
> +
> +    def _grabber(self, url):
> +        """
> +        Use isoinfo to grab the file
> +        """
> +        print "_grabber: filename", url

This print should be removed, we already log this info elsewhere, see
virt-install --debug output

> +        output = subprocess.check_output(
> +                ["isoinfo", "-J", "-i", self.location, "-x", url])

But I would like to log the commands we are running.
logging.debug("Running command", cmd)

> +        return io.BytesIO(output), len(output)
> +
> +    def _hasFile(self, filename):
> +        """
> +        Use isoinfo to list and search for the file
> +        """
> +        if not self._cache_file_list:
> +            output = subprocess.check_output(
> +                    ["isoinfo", "-J", "-i", self.location, "-f"])
> +            self._cache_file_list = output.splitlines(False)
> +
> +        return filename in self._cache_file_list

Log here too

Thanks,
Cole

> +
> +
>  def fetcherForURI(uri, *args, **kwargs):
>      if uri.startswith("http://") or uri.startswith("https://"):
>          fclass = _HTTPURLFetcher
> @@ -348,9 +371,12 @@ def fetcherForURI(uri, *args, **kwargs):
>      elif os.path.isdir(uri):
>          # Pointing to a local tree
>          fclass = _LocalURLFetcher
> +    elif stat.S_ISBLK(os.stat(uri)[stat.ST_MODE]):
> +        # Pointing to a block device
> +        fclass = _MountedURLFetcher
>      else:
>          # Pointing to a path, like an .iso to mount
> -        fclass = _MountedURLFetcher
> +        fclass = _ISOURLFetcher
>      return fclass(uri, *args, **kwargs)
>  
>  
>