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

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@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.

It could look something like this:

  let compressed     match xpath_string expr with
    | None -> None
    | Some "identity" -> Some false
    | Some "gzip" -> Some true
    | Some s -> error (f_"unsupported compression in OVF: %s") s in

  let compressed     match compressed with
    | None -> (* do detection *)
    | Some c -> c in

Thanks,
-- 
Pino Toscano

On Fri, 09 Dec 2016 14:01:40 +0100
Pino Toscano <ptoscano@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@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.


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. This seems like a lot of hassle to just handle some
potentially broken OVAs. Or do we know this really happened in the past?


    Tomas

-- 
Tomáš Golembiovský <tgolembi@redhat.com>

On Saturday, 10 December 2016 13:50:08 CET Tomas Golembiovsky
wrote:
> On Fri, 09 Dec 2016 14:01:40 +0100
> Pino Toscano <ptoscano@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@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