[libvirt] [PATCH 01/12] virsh: Force boot emulation is only required for virDomainCreateWithFlags

[Marc Hartmayer]

On Wed, Jun 13, 2018 at 12:53 AM +0200, John Ferlan <jferlan at redhat.com> wrote:
> On 05/09/2018 10:56 AM, Marc Hartmayer wrote:
>> The force boot emulation is only required for virDomainCreateWithFlags
>> as the flag VIR_DOMAIN_START_FORCE_BOOT was introduced with the commit
>> 27c85260532f879be5674a4eed0811c21fd34f94 (2011). But
>> virDomainCreateXMLWithFiles is newer and therefore already had support
>> for VIR_DOMAIN_START_FORCE_BOOT. This means there is now no second
>> call with VIR_DOMAIN_START_FORCE_BOOT removed from flags for
>> virDomainCreateXMLWithFiles in case the first call
>> fails. virDomainCreateXMLWithFiles was introduced with commit
>> d76227bea35cc49374a94414f6d46e3493ac2a52 (2013).
>>
>> This patch takes this into account and simplifies the function. In
>> addition, it's now easier to extend the function.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
>> ---
>>  tools/virsh-domain.c | 52 ++++++++++++++++++++++++++++------------------------
>>  1 file changed, 28 insertions(+), 24 deletions(-)
>>
>
> OK so I took the bait ;-)...  I agree 110% what you've changed to is
> easier to read; however, I think there's a subtle difference with your
> changes...  I'm not sure this new flow will work quite right "if" for
> some reason someone passes the --force-boot flag to/for a startup that
> uses CreateWithFiles, such as lxc.
>
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index 598d2fa4a4bd..7cf8373f05bc 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -4038,40 +4038,44 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>>      if (vshCommandOptBool(cmd, "force-boot"))
>>          flags |= VIR_DOMAIN_START_FORCE_BOOT;>
>> -    /* We can emulate force boot, even for older servers that reject it.  */
>
> FWIW: I see that this hunk was added by commit id 691ec08b shortly after
> adding support for force_boot via commit id 27c85260.
>
>> -    if (flags & VIR_DOMAIN_START_FORCE_BOOT) {
>> -        if ((nfds ?
>> -             virDomainCreateWithFiles(dom, nfds, fds, flags) :
>> -             virDomainCreateWithFlags(dom, flags)) == 0)
>> -            goto started;
>> -        if (last_error->code != VIR_ERR_NO_SUPPORT &&
>> -            last_error->code != VIR_ERR_INVALID_ARG) {
>> -            vshReportError(ctl);
>> -            goto cleanup;
>> -        }
>
> Previously if flags & VIR_DOMAIN_START_FORCE_BOOT, we'd try w/
> CreateWithFiles without changing @flags.
>
> For lxc, eventually we'd end up in lxcDomainCreateWithFiles which would
> do a virCheckFlags and "fail" with VIR_ERR_INVALID_ARG because the flag
> VIR_DOMAIN_START_FORCE_BOOT is not supported.

Okay. I haven’t thought that a newer API wouldn’t support this flag.

>
> That would fall through this removed code and remove the FORCE_BOOT
> flag. Then we'd again call virDomainCreateWithFiles w/ @flags adjusted
> to remove VIR_DOMAIN_START_FORCE_BOOT
>
>> -        vshResetLibvirtError();
>> -        rc = virDomainHasManagedSaveImage(dom, 0);
>> -        if (rc < 0) {
>> -            /* No managed save image to remove */
>> -            vshResetLibvirtError();
>> -        } else if (rc > 0) {
>> -            if (virDomainManagedSaveRemove(dom, 0) < 0) {
>> +    /* Prefer older API unless we have to pass extra parameters */
>> +    if (nfds) {
>> +        rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
>
> With this new code if @flags has VIR_DOMAIN_START_FORCE_BOOT, then
> there's no fallback and we fail.
>
>> +    } else if (flags) {
>> +        rc = virDomainCreateWithFlags(dom, flags);
>> +        /* We can emulate force boot, even for older servers that
>> +         * reject it. */
>> +        if (rc < 0 && flags & VIR_DOMAIN_START_FORCE_BOOT) {
>> +            if (last_error->code != VIR_ERR_NO_SUPPORT &&
>> +                last_error->code != VIR_ERR_INVALID_ARG) {
>
> The CreateWithFlags code could "fail" w/ INVALID_ARG if some hypervisor
> didn't support some flag, but even calling it again without the
> FORCE_BOOT may not change the result.
>
> This (existing, I know) code assumes the failure is from the lack of a
> FORCE_BOOT flag as added via commit 691ec08b.  Having commit id afb50d79
> further complicate the logic especially w/r/t FORCE_BOOT (which I think
> makes no sense for a container hypervisor, but then again I'm not the
> expert there ;-))

:)

> .  Still since it's in the API and documented, it's not
> like we could remove it (whether or not it was an unintentional
> cut-copy-paste, change API name).

Anyway, thanks for the review!

[…snip]

--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294