Libguestfs - Aug 2018 - Re: [PATCH] v2v: parse_libvirt_xml: number disks from 0 (RHBZ#1615885)

On Tuesday, 14 August 2018 16:53:02 CEST Richard W.M. Jones
wrote:
> On Tue, Aug 14, 2018 at 04:04:10PM +0200, Pino Toscano wrote:
> > When parsing the libvirt XML, make sure to assign the IDs for disks
> > (s_disk_id) from 0 instead of 1, just like all the other input modes
not
> > based on libvirt XML.
> > ---
> >  v2v/parse_libvirt_xml.ml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/v2v/parse_libvirt_xml.ml b/v2v/parse_libvirt_xml.ml
> > index 78a6e71c0..dac99511c 100644
> > --- a/v2v/parse_libvirt_xml.ml
> > +++ b/v2v/parse_libvirt_xml.ml
> > @@ -246,7 +246,7 @@ let parse_libvirt_xml ?conn xml > >    (*
Non-removable disk devices. *)
> >    let disks > >      let get_disks, add_disk > > -     
let disks = ref [] and i = ref 0 in
> > +      let disks = ref [] and i = ref (-1) in
> >        let get_disks () = List.rev !disks in
> >        let add_disk qemu_uri format controller p_source > >     
incr i;
> 
> NACK.  The s_disk_id field is supposed to just be a unique ID and
> -i libvirt is giving it a unique value here so it's not wrong.
I did not say it is wrong, just that the way IDs are allocated here
is different than in other input modes.
> The problem with this bug is that the output driver used the s_disk_id
> field assuming it was a unique, monotonically increasing number
> counting from 0.  (See my other patch to fix that)
OK, this I can agree with.  Would it be possible to add this paragraph
to the commit message of the patch?

-- 
Pino Toscano

On Tue, Aug 14, 2018 at 05:32:08PM +0200, Pino Toscano
wrote:
> On Tuesday, 14 August 2018 16:53:02 CEST Richard W.M. Jones wrote:
> > On Tue, Aug 14, 2018 at 04:04:10PM +0200, Pino Toscano wrote:
> > > When parsing the libvirt XML, make sure to assign the IDs for
disks
> > > (s_disk_id) from 0 instead of 1, just like all the other input
modes not
> > > based on libvirt XML.
> > > ---
> > >  v2v/parse_libvirt_xml.ml | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/v2v/parse_libvirt_xml.ml b/v2v/parse_libvirt_xml.ml
> > > index 78a6e71c0..dac99511c 100644
> > > --- a/v2v/parse_libvirt_xml.ml
> > > +++ b/v2v/parse_libvirt_xml.ml
> > > @@ -246,7 +246,7 @@ let parse_libvirt_xml ?conn xml > >
>    (* Non-removable disk devices. *)
> > >    let disks > > >      let get_disks, add_disk >
> > -      let disks = ref [] and i = ref 0 in
> > > +      let disks = ref [] and i = ref (-1) in
> > >        let get_disks () = List.rev !disks in
> > >        let add_disk qemu_uri format controller p_source > >
>          incr i;
> > 
> > NACK.  The s_disk_id field is supposed to just be a unique ID and
> > -i libvirt is giving it a unique value here so it's not wrong.
> 
> I did not say it is wrong, just that the way IDs are allocated here
> is different than in other input modes.
Well sure you can commit this patch, but it's not a fix for the BZ
(although it may incidentally fix it).  We still need my patch to fix
the cause of the bug.

(See also the other v2v patch I posted which verifies that the IDs are
actually unique.)
> > The problem with this bug is that the output driver used the s_disk_id
> > field assuming it was a unique, monotonically increasing number
> > counting from 0.  (See my other patch to fix that)
> 
> OK, this I can agree with.  Would it be possible to add this paragraph
> to the commit message of the patch?
Yes, and remove (RHBZ#...) from the title.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Tuesday, 14 August 2018 17:44:30 CEST Richard W.M. Jones
wrote:
> On Tue, Aug 14, 2018 at 05:32:08PM +0200, Pino Toscano wrote:
> > On Tuesday, 14 August 2018 16:53:02 CEST Richard W.M. Jones wrote:
> > > On Tue, Aug 14, 2018 at 04:04:10PM +0200, Pino Toscano wrote:
> > > > When parsing the libvirt XML, make sure to assign the IDs
for disks
> > > > (s_disk_id) from 0 instead of 1, just like all the other
input modes not
> > > > based on libvirt XML.
> > > > ---
> > > >  v2v/parse_libvirt_xml.ml | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/v2v/parse_libvirt_xml.ml
b/v2v/parse_libvirt_xml.ml
> > > > index 78a6e71c0..dac99511c 100644
> > > > --- a/v2v/parse_libvirt_xml.ml
> > > > +++ b/v2v/parse_libvirt_xml.ml
> > > > @@ -246,7 +246,7 @@ let parse_libvirt_xml ?conn xml >
> > >    (* Non-removable disk devices. *)
> > > >    let disks > > > >      let get_disks,
add_disk > > > > -      let disks = ref [] and i = ref 0 in
> > > > +      let disks = ref [] and i = ref (-1) in
> > > >        let get_disks () = List.rev !disks in
> > > >        let add_disk qemu_uri format controller p_source >
> > >          incr i;
> > > 
> > > NACK.  The s_disk_id field is supposed to just be a unique ID and
> > > -i libvirt is giving it a unique value here so it's not
wrong.
> > 
> > I did not say it is wrong, just that the way IDs are allocated here
> > is different than in other input modes.
> 
> Well sure you can commit this patch, but it's not a fix for the BZ
> (although it may incidentally fix it).  We still need my patch to fix
> the cause of the bug.
Sure, at this point mine is mostly cosmetics.
> > > The problem with this bug is that the output driver used the
s_disk_id
> > > field assuming it was a unique, monotonically increasing number
> > > counting from 0.  (See my other patch to fix that)
> > 
> > OK, this I can agree with.  Would it be possible to add this paragraph
> > to the commit message of the patch?
> 
> Yes, and remove (RHBZ#...) from the title.
Sorry, I mean adding this paragraph to the commit message of your
patch, i.e. that fixes the output of rhv-upload to not rely on that
(broken) assumption.

-- 
Pino Toscano