[PATCH] v2v: parse_libvirt_xml: number disks from 0 (RHBZ#1615885)
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; -- 2.17.1
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. 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) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
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
On Tue, Aug 14, 2018 at 05:48:53PM +0200, Pino Toscano wrote:
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.
Sure, I will do. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
participants (2)
-
Pino Toscano -
Richard W.M. Jones