Libguestfs - Jul 2017 - Re: [PATCH v2 1/2] v2v: bootloaders: Handle no Bootloader::Tools default section (RHBZ#1472208).

On Tuesday, 18 July 2017 19:59:20 CEST Richard W.M. Jones
wrote:
> In SUSE guests, handle the case where
> Bootloader::Tools::GetDefaultSection () returns undef.
> 
> Previously this would return an empty string and cause a bogus error
> in subsequent code:
> 
> virt-v2v: error: libguestfs error: statns: statns_stub: path must start
> with a / character
> ---
I saw this was pushed, even though v1 was NACKed by me, and there was
no further information on actual guests (i.e. real world, not the one
that comes out of the virt-builder templates) affected by this.

The pushed patch is basically dead code, and I don't see why the urge
to have it regardless.

Furthermore, I don't see the logic in have me re-NACK every new version
of a patch series that I NACKed, while the new series is merely a
repost of the old one, or in general the reasons of the NACK did not
change at all.

-- 
Pino Toscano

On Fri, Jul 28, 2017 at 10:02:34AM +0200, Pino Toscano
wrote:
> On Tuesday, 18 July 2017 19:59:20 CEST Richard W.M. Jones wrote:
> > In SUSE guests, handle the case where
> > Bootloader::Tools::GetDefaultSection () returns undef.
> > 
> > Previously this would return an empty string and cause a bogus error
> > in subsequent code:
> > 
> > virt-v2v: error: libguestfs error: statns: statns_stub: path must
start
> > with a / character
> > ---
> 
> I saw this was pushed, even though v1 was NACKed by me, and there was
> no further information on actual guests (i.e. real world, not the one
> that comes out of the virt-builder templates) affected by this.
> 
> The pushed patch is basically dead code, and I don't see why the urge
> to have it regardless.
> 
> Furthermore, I don't see the logic in have me re-NACK every new version
> of a patch series that I NACKed, while the new series is merely a
> repost of the old one, or in general the reasons of the NACK did not
> change at all.
I disagree that this is dead code.  Two commits add a regression test
of v2v on the opensuse guests (as we already have for Fedora and other
guests), and one of those commits is this patch which is needed for
that regression test to work.  Regression tests are a good thing -
they run on every release (as indeed I am running them right now) and
ensure that other aspects of the code don't regress because of some
unintentional change.

Also I don't think the patch is incorrect, even if it is only
applicable for certain types of "just installed / never booted" guests
that we probably wouldn't see outside of the regression testing.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Friday, 28 July 2017 10:16:33 CEST Richard W.M. Jones
wrote:
> On Fri, Jul 28, 2017 at 10:02:34AM +0200, Pino Toscano wrote:
> > On Tuesday, 18 July 2017 19:59:20 CEST Richard W.M. Jones wrote:
> > > In SUSE guests, handle the case where
> > > Bootloader::Tools::GetDefaultSection () returns undef.
> > > 
> > > Previously this would return an empty string and cause a bogus
error
> > > in subsequent code:
> > > 
> > > virt-v2v: error: libguestfs error: statns: statns_stub: path must
start
> > > with a / character
> > > ---
> > 
> > I saw this was pushed, even though v1 was NACKed by me, and there was
> > no further information on actual guests (i.e. real world, not the one
> > that comes out of the virt-builder templates) affected by this.
> > 
> > The pushed patch is basically dead code, and I don't see why the
urge
> > to have it regardless.
> > 
> > Furthermore, I don't see the logic in have me re-NACK every new
version
> > of a patch series that I NACKed, while the new series is merely a
> > repost of the old one, or in general the reasons of the NACK did not
> > change at all.
> 
> I disagree that this is dead code.  Two commits add a regression test
> of v2v on the opensuse guests (as we already have for Fedora and other
> guests), and one of those commits is this patch which is needed for
> that regression test to work.  Regression tests are a good thing -
> they run on every release (as indeed I am running them right now) and
> ensure that other aspects of the code don't regress because of some
> unintentional change.
Regression test is good when it tests actual stuff, not unrealistic
scenarios that (at least so far) haven't happened in real world yet.
Also, as mentioned in my previous replies, before applying any kind of
workaround, we need to check what is the actual problem at hand.

IOW, the v2v regression testing with those openSUSE templates is
partially flawed, because surely the content is not 100% what is going
to be in a real-world guest.
> Also I don't think the patch is incorrect, even if it is only
> applicable for certain types of "just installed / never booted"
guests
> that we probably wouldn't see outside of the regression testing.
I didn't say the patch is incorrect, but that is dead code. This adds
checks for a situation that so far will not ever happen in real guests,
but only in that very specific guest that virt-builder produces from
the openSUSE templates.  Just booting the guest once fixes the grub2
configuration into something standard, and even installing openSUSE
from the install media produces a working grub2 configuration.

-- 
Pino Toscano