[Libguestfs] [PATCH v3 2/6] v2v: ova: don't detect compressed disks, read the OVF instead

[Pino Toscano]

On Saturday, 10 December 2016 13:50:08 CET Tomas Golembiovsky wrote:
> On Fri, 09 Dec 2016 14:01:40 +0100
> Pino Toscano <ptoscano at redhat.com> wrote:
> 
> > On Wednesday, 7 December 2016 17:13:06 CET Tomáš Golembiovský wrote:
> > > The information whether the disk is gzip compressed or not is stored
> > > in the OVF. There is no reason to do the detection.
> > >
> > > Signed-off-by: Tomáš Golembiovský <tgolembi at redhat.com>
> > > ---
> > >  v2v/input_ova.ml          | 28 +++++++++++++++++-----------
> > >  v2v/test-v2v-i-ova-gz.ovf |  2 +-
> > >  2 files changed, 18 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
> > > index b283629..61930f0 100644
> > > --- a/v2v/input_ova.ml
> > > +++ b/v2v/input_ova.ml
> > > @@ -275,6 +275,13 @@ object
> > >              | None -> error (f_"no href in ovf:File (id=%s)") file_ref
> > >              | Some s -> s in
> > >
> > > +          let expr = sprintf "/ovf:Envelope/ovf:References/ovf:File[@ovf:id='%s']/@ovf:compression" file_ref in
> > > +          let compressed =
> > > +            match xpath_string expr with
> > > +            | None | Some "identity" -> false
> > > +            | Some "gzip" -> true
> > > +            | Some s -> error (f_"unsupported compression in OVF: %s") s in
> >
> > I'd still do the detection when no ovf:compression attribute is found
> > (that is, the None case above).
> > After all, right now we do the detection for any ova already, so a
> > "broken ova" (with no compression attribute in the ovf but with a
> > compressed image) is already handled by the current code.
> 
> I understand what you are trying to achieve here. Unfortunately
> that's not possible, missing attribute means "no compression". As the
> specification says:
> 
>     When a File element is compressed using gzip, the ovf:compression
>     attribute shall be set to “gzip”. Otherwise, the ovf:compression
>     attribute shall be set to “identity” or the entire attribute
>     omitted.

Yep, I know this -- OTOH, we make extra efforts to support OVAs actually
produced, even with non-standard features (example of this: all the
extra formats -- tar.gz. tar.xz, zip -- instead of the uncompressed tar
container).

> Also the main reason for doing this check was to be able to skip the
> detection. The detection would be problematic because we would have to
> extract the header of each file, perform the check and clean the
> leftovers.

Consdering also your work with patch #5 (which applies only with
uncompressed tar OVAs, basically) to read the offset and size of the
image file within the tar, we could just read the first 512 bytes of it
(like done now), and decide whether to untar+uncompress it or go ahead
reading it from the tar as your implementation does.

Yes, I agree it adds a bit of complexity.

> This seems like a lot of hassle to just handle some
> potentially broken OVAs. Or do we know this really happened in the past?

The problem in this case is that, since we always extracted the tar and
determined the file type of the image without looking at the compression
information in the ovf, we have no way to know whether there are such
OVAs around.

Anyway, I guess it should be better to wait for Rich's opinion on this
too, since he has more experience with this.

Thanks,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170104/2092179b/attachment.sig>