On Wed, Jun 13, 2018 at 12:53 AM +0200, John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
