Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On 05/23/2016 12:37 PM, Michal Privoznik wrote:
> On 23.05.2016 11:16, Maxim Nestratov wrote:
>> 24.02.2015 13:12, Daniel P. Berrange пишет:
>>
>>> The undefine operation should always be allowed to succeed
>>> regardless of whether any NVRAM file exists. ie we should
>>> not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
>>> flag. It is valid for the app to decide it wants the NVRAM
>>> file left on disk, in the same way that disk images are left
>>> on disk at undefine.
>>> ---
>>> src/qemu/qemu_driver.c | 20 +++-
>>> 1 file changed, 7 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index bec05d4..302bf48 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
>>> if (!virDomainObjIsActive(vm) &&
>>> vm->def->os.loader && vm->def->os.loader->nvram &&
>>> -virFileExists(vm->def->os.loader->nvram)) {
>>> -if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
>>> -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>>> - _("cannot delete inactive domain with
>>> nvram"));
>>> -goto cleanup;
>>> -}
>>> -
>>> -if (unlink(vm->def->os.loader->nvram) < 0) {
>>> -virReportSystemError(errno,
>>> - _("failed to remove nvram: %s"),
>>> - vm->def->os.loader->nvram);
>>> -goto cleanup;
>>> -}
>>> +virFileExists(vm->def->os.loader->nvram) &&
>>> +(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
>>> +(unlink(vm->def->os.loader->nvram) < 0)) {
>>> +virReportSystemError(errno,
>>> + _("failed to remove nvram: %s"),
>>> + vm->def->os.loader->nvram);
>>> +goto cleanup;
>>> }
>>> if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir,
>>> vm) < 0)
>>
>> As I found out the discussion followed this patch didn't come to a
>> conclusion and this or any other patches on the matter weren't commited.
>> We hit the problem with inability to undefine a domain leaving nvram
>> untouched recently and this patch would solve it perfectly.
>> I think it's worth commiting IMHO and maybe the documentation should
>> reflect this slight change in behavior.
>> Any new thoughts?
>
> Does this Cole's suggestion sounds reasonable?
>
> https://www.redhat.com/archives/libvir-list/2015-February/msg00974.html
>
That UNDEFINE_REMOVE_STATE or whatever flag would definitely be useful, but
it's kind of separate. Dan's points elsewhere in the thread are valid, that
the API doesn't provide any way to undefine a VM but _not_ remove autocreated
nvram file. That seems potentially problematic. However before Dan's patch
could be applied, someone needs to verify how libvirt handles the case when
redefining a new VM with UEFI but there's already an old nvram vars file in
place... does libvirt error, or silently use the old one, or silently
overwrite the old one? It may need extra patches, or more documentation
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
23.05.2016 19:37, Michal Privoznik пишет:
On 23.05.2016 11:16, Maxim Nestratov wrote:
24.02.2015 13:12, Daniel P. Berrange пишет:
The undefine operation should always be allowed to succeed
regardless of whether any NVRAM file exists. ie we should
not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
flag. It is valid for the app to decide it wants the NVRAM
file left on disk, in the same way that disk images are left
on disk at undefine.
---
src/qemu/qemu_driver.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bec05d4..302bf48 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) &&
vm->def->os.loader && vm->def->os.loader->nvram &&
-virFileExists(vm->def->os.loader->nvram)) {
-if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot delete inactive domain with
nvram"));
-goto cleanup;
-}
-
-if (unlink(vm->def->os.loader->nvram) < 0) {
-virReportSystemError(errno,
- _("failed to remove nvram: %s"),
- vm->def->os.loader->nvram);
-goto cleanup;
-}
+virFileExists(vm->def->os.loader->nvram) &&
+(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
+(unlink(vm->def->os.loader->nvram) < 0)) {
+virReportSystemError(errno,
+ _("failed to remove nvram: %s"),
+ vm->def->os.loader->nvram);
+goto cleanup;
}
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir,
vm) < 0)
As I found out the discussion followed this patch didn't come to a
conclusion and this or any other patches on the matter weren't commited.
We hit the problem with inability to undefine a domain leaving nvram
untouched recently and this patch would solve it perfectly.
I think it's worth commiting IMHO and maybe the documentation should
reflect this slight change in behavior.
Any new thoughts?
Does this Cole's suggestion sounds reasonable?
https://www.redhat.com/archives/libvir-list/2015-February/msg00974.html
Michal
As far as I understand the proposed flag would eliminate the necessity
to specify all flags (SNAPSHOT, SAVE_STATE, NVRAM and some future ones)
to force undefining.
But this won't allow us to undefine a domain and left a nvram file
untouched.
Maxim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On 23.05.2016 11:16, Maxim Nestratov wrote:
> 24.02.2015 13:12, Daniel P. Berrange пишет:
>
>> The undefine operation should always be allowed to succeed
>> regardless of whether any NVRAM file exists. ie we should
>> not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
>> flag. It is valid for the app to decide it wants the NVRAM
>> file left on disk, in the same way that disk images are left
>> on disk at undefine.
>> ---
>> src/qemu/qemu_driver.c | 20 +++-
>> 1 file changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index bec05d4..302bf48 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
>> if (!virDomainObjIsActive(vm) &&
>> vm->def->os.loader && vm->def->os.loader->nvram &&
>> -virFileExists(vm->def->os.loader->nvram)) {
>> -if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
>> -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> - _("cannot delete inactive domain with
>> nvram"));
>> -goto cleanup;
>> -}
>> -
>> -if (unlink(vm->def->os.loader->nvram) < 0) {
>> -virReportSystemError(errno,
>> - _("failed to remove nvram: %s"),
>> - vm->def->os.loader->nvram);
>> -goto cleanup;
>> -}
>> +virFileExists(vm->def->os.loader->nvram) &&
>> +(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
>> +(unlink(vm->def->os.loader->nvram) < 0)) {
>> +virReportSystemError(errno,
>> + _("failed to remove nvram: %s"),
>> + vm->def->os.loader->nvram);
>> +goto cleanup;
>> }
>> if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir,
>> vm) < 0)
>
> As I found out the discussion followed this patch didn't come to a
> conclusion and this or any other patches on the matter weren't commited.
> We hit the problem with inability to undefine a domain leaving nvram
> untouched recently and this patch would solve it perfectly.
> I think it's worth commiting IMHO and maybe the documentation should
> reflect this slight change in behavior.
> Any new thoughts?
Does this Cole's suggestion sounds reasonable?
https://www.redhat.com/archives/libvir-list/2015-February/msg00974.html
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
24.02.2015 13:12, Daniel P. Berrange пишет:
The undefine operation should always be allowed to succeed
regardless of whether any NVRAM file exists. ie we should
not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
flag. It is valid for the app to decide it wants the NVRAM
file left on disk, in the same way that disk images are left
on disk at undefine.
---
src/qemu/qemu_driver.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bec05d4..302bf48 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) &&
vm->def->os.loader && vm->def->os.loader->nvram &&
-virFileExists(vm->def->os.loader->nvram)) {
-if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot delete inactive domain with nvram"));
-goto cleanup;
-}
-
-if (unlink(vm->def->os.loader->nvram) < 0) {
-virReportSystemError(errno,
- _("failed to remove nvram: %s"),
- vm->def->os.loader->nvram);
-goto cleanup;
-}
+virFileExists(vm->def->os.loader->nvram) &&
+(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
+(unlink(vm->def->os.loader->nvram) < 0)) {
+virReportSystemError(errno,
+ _("failed to remove nvram: %s"),
+ vm->def->os.loader->nvram);
+goto cleanup;
}
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
As I found out the discussion followed this patch didn't come to a
conclusion and this or any other patches on the matter weren't commited.
We hit the problem with inability to undefine a domain leaving nvram
untouched recently and this patch would solve it perfectly.
I think it's worth commiting IMHO and maybe the documentation should
reflect this slight change in behavior.
Any new thoughts?
Maxim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Wed, Feb 25, 2015 at 11:11:37AM +0100, Peter Krempa wrote:
> On Tue, Feb 24, 2015 at 17:46:12 +0100, Kashyap Chamarthy wrote:
> > On Tue, Feb 24, 2015 at 10:12:20AM +, Daniel P. Berrange wrote:
> > > The undefine operation should always be allowed to succeed
> > > regardless of whether any NVRAM file exists. ie we should
> > > not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
> > > flag. It is valid for the app to decide it wants the NVRAM
> > > file left on disk, in the same way that disk images are left
> > > on disk at undefine.
> > > ---
> > > src/qemu/qemu_driver.c | 20 +++-
> > > 1 file changed, 7 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > > index bec05d4..302bf48 100644
> > > --- a/src/qemu/qemu_driver.c
> > > +++ b/src/qemu/qemu_driver.c
> > > @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
> > >
> > > if (!virDomainObjIsActive(vm) &&
> > > vm->def->os.loader && vm->def->os.loader->nvram &&
> > > -virFileExists(vm->def->os.loader->nvram)) {
> > > -if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
> > > -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > > - _("cannot delete inactive domain with
> > > nvram"));
> > > -goto cleanup;
> > > -}
> > > -
> > > -if (unlink(vm->def->os.loader->nvram) < 0) {
> > > -virReportSystemError(errno,
> > > - _("failed to remove nvram: %s"),
> > > - vm->def->os.loader->nvram);
> > > -goto cleanup;
> > > -}
> > > +virFileExists(vm->def->os.loader->nvram) &&
> > > +(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
> > > +(unlink(vm->def->os.loader->nvram) < 0)) {
> > > +virReportSystemError(errno,
> > > + _("failed to remove nvram: %s"),
> > > + vm->def->os.loader->nvram);
> > > +goto cleanup;
> > > }
> > >
> > > if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
> >
> > With the above patch applied, I built libvirt RPMs, installed and
> > restarted libvirtd then I tried the below test (which failed) an AArch64
> > Fedora21 guest.
> >
> > Edit the Fedora 21 libvirt guest. Guest XML here[*]:
> >
> > $ virsh edit devstack
> > [. . .]
> >
> > Try to add the below fragment under 'os' element:
> >
> > > type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd
> > /var/lib/libvirt/nvram/fedora-21-aarch64-nvram
> >
> > It fails to validate the XML:
> >
> > error: XML document failed to validate against schema: Unable to
> > validate doc against /usr/share/libvirt/schemas/domain.rng
> > Extra element devices in interleave
> > Element domain failed to validate content
> >
> > Failed. Try again? [y,n,i,f,?]:
> >
>
> The and need to be put after the
> element due to the ordering in libvirt's schema.
Actually I tested that too, and forgot to mention -- validation still
fails the same way, desipte the above ordering.
This patch from Martin Kletzander fixes it:
https://www.redhat.com/archives/libvir-list/2015-February/msg01022.html
> I'll look into the
> schema whether we can make the interleave span the element too.
>
> Until then either put it in the right place or use the 'i' key to ignore
> the schema check.
Yes, 'i' is what I did to test it.
Thanks.
--
/kashyap
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Tue, Feb 24, 2015 at 05:46:12PM +0100, Kashyap Chamarthy wrote: > On Tue, Feb 24, 2015 at 10:12:20AM +, Daniel P. Berrange wrote: [. . .] > Edit the Fedora 21 libvirt guest. Guest XML here[*]: > > $ virsh edit devstack > [. . .] > > Try to add the below fragment under 'os' element: > > type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd > /var/lib/libvirt/nvram/fedora-21-aarch64-nvram > > It fails to validate the XML: > > error: XML document failed to validate against schema: Unable to validate > doc against /usr/share/libvirt/schemas/domain.rng > Extra element devices in interleave > Element domain failed to validate content > > Failed. Try again? [y,n,i,f,?]: I ignored the validation here 'i' (thanks Michal Privoznik for the suggestion on IRC -- this looks like something to do with some XML validation work from Martin Kletzander), and went ahead to test the actual bug this patch fixes: It works: A guest w/ 'nvmram' XML element as below: $ virsh dumpxml node2 | grep -i nvram -A2 -B3 hvm /usr/share/AAVMF/AAVMF_CODE.fd /var/lib/libvirt/nvram/fedora-21-aarch64-nvram Can be undefined successfully. -- /kashyap -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Tue, Feb 24, 2015 at 17:46:12 +0100, Kashyap Chamarthy wrote:
> On Tue, Feb 24, 2015 at 10:12:20AM +, Daniel P. Berrange wrote:
> > The undefine operation should always be allowed to succeed
> > regardless of whether any NVRAM file exists. ie we should
> > not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
> > flag. It is valid for the app to decide it wants the NVRAM
> > file left on disk, in the same way that disk images are left
> > on disk at undefine.
> > ---
> > src/qemu/qemu_driver.c | 20 +++-
> > 1 file changed, 7 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index bec05d4..302bf48 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
> >
> > if (!virDomainObjIsActive(vm) &&
> > vm->def->os.loader && vm->def->os.loader->nvram &&
> > -virFileExists(vm->def->os.loader->nvram)) {
> > -if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
> > -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > - _("cannot delete inactive domain with nvram"));
> > -goto cleanup;
> > -}
> > -
> > -if (unlink(vm->def->os.loader->nvram) < 0) {
> > -virReportSystemError(errno,
> > - _("failed to remove nvram: %s"),
> > - vm->def->os.loader->nvram);
> > -goto cleanup;
> > -}
> > +virFileExists(vm->def->os.loader->nvram) &&
> > +(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
> > +(unlink(vm->def->os.loader->nvram) < 0)) {
> > +virReportSystemError(errno,
> > + _("failed to remove nvram: %s"),
> > + vm->def->os.loader->nvram);
> > +goto cleanup;
> > }
> >
> > if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
>
> With the above patch applied, I built libvirt RPMs, installed and
> restarted libvirtd then I tried the below test (which failed) an AArch64
> Fedora21 guest.
>
> Edit the Fedora 21 libvirt guest. Guest XML here[*]:
>
> $ virsh edit devstack
> [. . .]
>
> Try to add the below fragment under 'os' element:
>
> type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd
> /var/lib/libvirt/nvram/fedora-21-aarch64-nvram
>
> It fails to validate the XML:
>
> error: XML document failed to validate against schema: Unable to validate
> doc against /usr/share/libvirt/schemas/domain.rng
> Extra element devices in interleave
> Element domain failed to validate content
>
> Failed. Try again? [y,n,i,f,?]:
>
The and need to be put after the
element due to the ordering in libvirt's schema. I'll look into the
schema whether we can make the interleave span the element too.
Until then either put it in the right place or use the 'i' key to ignore
the schema check.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On 02/24/2015 10:23 AM, Peter Krempa wrote:
> On Tue, Feb 24, 2015 at 15:11:28 +, Daniel Berrange wrote:
>> On Tue, Feb 24, 2015 at 04:07:02PM +0100, Peter Krempa wrote:
>>> On Tue, Feb 24, 2015 at 10:12:20 +, Daniel Berrange wrote:
The undefine operation should always be allowed to succeed
regardless of whether any NVRAM file exists. ie we should
not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
flag. It is valid for the app to decide it wants the NVRAM
file left on disk, in the same way that disk images are left
on disk at undefine.
---
src/qemu/qemu_driver.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bec05d4..302bf48 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) &&
vm->def->os.loader && vm->def->os.loader->nvram &&
-virFileExists(vm->def->os.loader->nvram)) {
-if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot delete inactive domain with nvram"));
-goto cleanup;
-}
-
-if (unlink(vm->def->os.loader->nvram) < 0) {
-virReportSystemError(errno,
- _("failed to remove nvram: %s"),
- vm->def->os.loader->nvram);
-goto cleanup;
-}
+virFileExists(vm->def->os.loader->nvram) &&
+(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
+(unlink(vm->def->os.loader->nvram) < 0)) {
+virReportSystemError(errno,
+ _("failed to remove nvram: %s"),
+ vm->def->os.loader->nvram);
+goto cleanup;
}
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
>>>
>>>
>>> We have prior art in denying to undefine a domain that has information
>>> stored in libvirt-internal locations such as the managed save image and
>>> snapshot metadata.
>>>
>>> While it makes sense to allow removing the VM without deleting the NVRAM
>>> file when the user specified an external path, we should avoid doing so
>>> if the NVRAM is in the libvirt managed path. (Same with externaly
>>> managed snapshots or save images).
>>
>> I'm not a real fan of refusal in the managed save case either. The issue
>> is that we're forcing a policy onto the application and not giving it any
>> chance to define a different policy. ie currently apps have a choice of
>> getting an error, or always deleting the nvram, not giving them any chance
>> to delete guest without deleting nvram which is a valid choice they should
>> be permitted to have. By removing this check we allow apps to make their
>> own policy decision with their more complete view of the world. I don't
>> think the quesiton of managed vs unmanaged storage should even come into
>> it either, because that presupposes the application will only ever use
>> managed storage. OpenStack does not currently use libvirt storage pools
>> at all but does wish to use NVRAM. We should not deny the option to undefine
>> the guest in this case.
>
> In that case we probably should have negated the logic here and delete
> all the stuff by default and give the user option to leave the data
> behind.
>
> The original motivation is apparently that we should not allow anything
> that would represent state of the deleted VM to be transferred
> accidentaly to a new VM with same name. For the save image or snapshots
> the risk of persisting any data is low as a save image would not
> function without it's disk and still be somewhat secure as it would
> contain the whole memory image including security. For the NVRAM though
> it might uncover data stored there or even make the VM unbootable.
>
> I agree that the current state is not ideal as we basically force the
> user to specify all the necessary flags. I think we can safely avoid
> displaying the message in cases when it's not stored in the
> libvirt-internal path but for the internal path I'm not convinced that
> it would be a great idea to change the default.
>
> We can perhaps add a flag that woult either enable all the "UNDEFINE*"
> flags or perhaps invert the logic of them so the user could leave them
> behind.
>
FWIW a flag for UNDEFINE_REMOVE_ALL_STATE or something would definitely be
useful for virt-manager. The managed save, snapshot, and nvram undefine errors
all hit us and required patches, it would be useful to have one flag that
future proofs us.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Tue, Feb 24, 2015 at 10:12:20AM +, Daniel P. Berrange wrote:
> The undefine operation should always be allowed to succeed
> regardless of whether any NVRAM file exists. ie we should
> not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
> flag. It is valid for the app to decide it wants the NVRAM
> file left on disk, in the same way that disk images are left
> on disk at undefine.
> ---
> src/qemu/qemu_driver.c | 20 +++-
> 1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index bec05d4..302bf48 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
>
> if (!virDomainObjIsActive(vm) &&
> vm->def->os.loader && vm->def->os.loader->nvram &&
> -virFileExists(vm->def->os.loader->nvram)) {
> -if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
> -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("cannot delete inactive domain with nvram"));
> -goto cleanup;
> -}
> -
> -if (unlink(vm->def->os.loader->nvram) < 0) {
> -virReportSystemError(errno,
> - _("failed to remove nvram: %s"),
> - vm->def->os.loader->nvram);
> -goto cleanup;
> -}
> +virFileExists(vm->def->os.loader->nvram) &&
> +(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
> +(unlink(vm->def->os.loader->nvram) < 0)) {
> +virReportSystemError(errno,
> + _("failed to remove nvram: %s"),
> + vm->def->os.loader->nvram);
> +goto cleanup;
> }
>
> if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
With the above patch applied, I built libvirt RPMs, installed and
restarted libvirtd then I tried the below test (which failed) an AArch64
Fedora21 guest.
Edit the Fedora 21 libvirt guest. Guest XML here[*]:
$ virsh edit devstack
[. . .]
Try to add the below fragment under 'os' element:
/usr/share/AAVMF/AAVMF_CODE.fd
/var/lib/libvirt/nvram/fedora-21-aarch64-nvram
It fails to validate the XML:
error: XML document failed to validate against schema: Unable to validate
doc against /usr/share/libvirt/schemas/domain.rng
Extra element devices in interleave
Element domain failed to validate content
Failed. Try again? [y,n,i,f,?]:
However, when I downgrade the libvirt RPMs to version
1.2.11-1.fc22.aarch64 and attempted to add the same XML fragment as
above, it works just fine.
Tested with:
$ uname -r; rpm -q libvirt-daemon-kvm qemu-system-aarch64
3.18.6-200.fc21.aarch64
libvirt-daemon-kvm-1.2.13-1.fc21.aarch64
qemu-system-aarch64-2.2.0-5.fc22.aarch64
libvirt git after I applied your patch:
$ git describe
v1.2.13-rc1-1-gb63296f
[*] The XML I'm tring to edit:
---
devstack
d02624c5-460e-436b-baaa-973cfee554c8
8388608
8388608
1
hvm
host
destroy
restart
restart
/usr/bin/qemu-system-aarch64
---
--
/kashyap
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
[/me still reading the complete discussion.] On Tue, Feb 24, 2015 at 04:07:02PM +0100, Peter Krempa wrote: [. . .] > We have prior art in denying to undefine a domain that has information > stored in libvirt-internal locations such as the managed save image and > snapshot metadata. > > While it makes sense to allow removing the VM without deleting the NVRAM > file when the user specified an external path, we should avoid doing so > if the NVRAM is in the libvirt managed path. (Same with externaly > managed snapshots or save images). I see, that explains, as I noticed it[*] when I placed the 'nvram' file in /var/lib/libvirt/ $ virsh dumpxml node2 | grep -i nvram -A2 -B3 hvm /usr/share/AAVMF/AAVMF_CODE.fd /var/lib/libvirt/nvram/fedora-21-aarch64-nvram [*] https://bugzilla.redhat.com/show_bug.cgi?id=1195667 -- libvirt fails to undefine a guest with `nvram` XML element -- /kashyap -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Tue, Feb 24, 2015 at 04:41:44PM +0100, Peter Krempa wrote: > On Tue, Feb 24, 2015 at 15:31:14 +, Daniel Berrange wrote: > > On Tue, Feb 24, 2015 at 04:23:41PM +0100, Peter Krempa wrote: > > > The original motivation is apparently that we should not allow anything > > > that would represent state of the deleted VM to be transferred > > > accidentaly to a new VM with same name. For the save image or snapshots > > > the risk of persisting any data is low as a save image would not > > > function without it's disk and still be somewhat secure as it would > > > contain the whole memory image including security. For the NVRAM though > > > it might uncover data stored there or even make the VM unbootable. > > > > > > I agree that the current state is not ideal as we basically force the > > > user to specify all the necessary flags. I think we can safely avoid > > > displaying the message in cases when it's not stored in the > > > libvirt-internal path but for the internal path I'm not convinced that > > > it would be a great idea to change the default. > > > > This is the problem with trying to put this kind of policy into libvirt > > though. It is targetting one use case, but has forgotten other valid > > use cases. For example, consider if the NVRAM file or the managed save > > image were stored in a filesystem that was NFS. The application wishes > > to undefine the config on one host and define it on another host. Any > > checks of this kind will always be wrong for some portion of use cases. > > The mgmt app has the option to use either non-managed save or store the > NVRAM in a non-default location for example ... I remember that in the virDomainCreateFlags we have START_FORCE_BOOT which is defined to discard any existing managed save file. Should we either extend that to also discard the NVRAM file, or alternatively add a START_RESET_NVRAM flag as a way to boot with clean BIOS ? Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Tue, Feb 24, 2015 at 15:54:24 +, Daniel Berrange wrote: > On Tue, Feb 24, 2015 at 04:41:44PM +0100, Peter Krempa wrote: > > On Tue, Feb 24, 2015 at 15:31:14 +, Daniel Berrange wrote: > > > On Tue, Feb 24, 2015 at 04:23:41PM +0100, Peter Krempa wrote: > > > > The original motivation is apparently that we should not allow anything > > > > that would represent state of the deleted VM to be transferred > > > > accidentaly to a new VM with same name. For the save image or snapshots > > > > the risk of persisting any data is low as a save image would not > > > > function without it's disk and still be somewhat secure as it would > > > > contain the whole memory image including security. For the NVRAM though > > > > it might uncover data stored there or even make the VM unbootable. > > > > > > > > I agree that the current state is not ideal as we basically force the > > > > user to specify all the necessary flags. I think we can safely avoid > > > > displaying the message in cases when it's not stored in the > > > > libvirt-internal path but for the internal path I'm not convinced that > > > > it would be a great idea to change the default. > > > > > > This is the problem with trying to put this kind of policy into libvirt > > > though. It is targetting one use case, but has forgotten other valid > > > use cases. For example, consider if the NVRAM file or the managed save > > > image were stored in a filesystem that was NFS. The application wishes > > > to undefine the config on one host and define it on another host. Any > > > checks of this kind will always be wrong for some portion of use cases. > > > > The mgmt app has the option to use either non-managed save or store the > > NVRAM in a non-default location for example ... > > I remember that in the virDomainCreateFlags we have START_FORCE_BOOT > which is defined to discard any existing managed save file. > > Should we either extend that to also discard the NVRAM file, or alternatively > add a START_RESET_NVRAM flag as a way to boot with clean BIOS ? I'd vote for a new one. You might want to discard the save file and keep nvram settings. The other combination will be invalid at least for qemu as the NVRAM will be contained in the save image. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Tue, Feb 24, 2015 at 15:31:14 +, Daniel Berrange wrote: > On Tue, Feb 24, 2015 at 04:23:41PM +0100, Peter Krempa wrote: > > In that case we probably should have negated the logic here and delete > > all the stuff by default and give the user option to leave the data > > behind. > > I think that would have been even more unsatisfactory - the semantics > of undefine were always just that they just remove the configuration, > leaving any other aspect of the VM untouched. Making it delete actual > data by default would be a significant semantic change IMHO. My wording wasn't clear enough. I was thinking that the initial design should be like this. Changing this is not an option now. > > > The original motivation is apparently that we should not allow anything > > that would represent state of the deleted VM to be transferred > > accidentaly to a new VM with same name. For the save image or snapshots > > the risk of persisting any data is low as a save image would not > > function without it's disk and still be somewhat secure as it would > > contain the whole memory image including security. For the NVRAM though > > it might uncover data stored there or even make the VM unbootable. > > > > I agree that the current state is not ideal as we basically force the > > user to specify all the necessary flags. I think we can safely avoid > > displaying the message in cases when it's not stored in the > > libvirt-internal path but for the internal path I'm not convinced that > > it would be a great idea to change the default. > > This is the problem with trying to put this kind of policy into libvirt > though. It is targetting one use case, but has forgotten other valid > use cases. For example, consider if the NVRAM file or the managed save > image were stored in a filesystem that was NFS. The application wishes > to undefine the config on one host and define it on another host. Any > checks of this kind will always be wrong for some portion of use cases. The mgmt app has the option to use either non-managed save or store the NVRAM in a non-default location for example ... > > > We can perhaps add a flag that woult either enable all the "UNDEFINE*" > > flags or perhaps invert the logic of them so the user could leave them > > behind. > > IMHO we should just remove this check, and the one for managed save, from > the API entirely and leave such error scenario checking to the higher level > applications which understand the context of usage in a way that libvirt > itself never can. Well, okay, but then you should note in the docs that by doing the undefine without the flags some data in libvirt internal paths may be left behind and persist into a new VM with the same name. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Tue, Feb 24, 2015 at 04:23:41PM +0100, Peter Krempa wrote: > In that case we probably should have negated the logic here and delete > all the stuff by default and give the user option to leave the data > behind. I think that would have been even more unsatisfactory - the semantics of undefine were always just that they just remove the configuration, leaving any other aspect of the VM untouched. Making it delete actual data by default would be a significant semantic change IMHO. > The original motivation is apparently that we should not allow anything > that would represent state of the deleted VM to be transferred > accidentaly to a new VM with same name. For the save image or snapshots > the risk of persisting any data is low as a save image would not > function without it's disk and still be somewhat secure as it would > contain the whole memory image including security. For the NVRAM though > it might uncover data stored there or even make the VM unbootable. > > I agree that the current state is not ideal as we basically force the > user to specify all the necessary flags. I think we can safely avoid > displaying the message in cases when it's not stored in the > libvirt-internal path but for the internal path I'm not convinced that > it would be a great idea to change the default. This is the problem with trying to put this kind of policy into libvirt though. It is targetting one use case, but has forgotten other valid use cases. For example, consider if the NVRAM file or the managed save image were stored in a filesystem that was NFS. The application wishes to undefine the config on one host and define it on another host. Any checks of this kind will always be wrong for some portion of use cases. > We can perhaps add a flag that woult either enable all the "UNDEFINE*" > flags or perhaps invert the logic of them so the user could leave them > behind. IMHO we should just remove this check, and the one for managed save, from the API entirely and leave such error scenario checking to the higher level applications which understand the context of usage in a way that libvirt itself never can. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Tue, Feb 24, 2015 at 15:11:28 +, Daniel Berrange wrote:
> On Tue, Feb 24, 2015 at 04:07:02PM +0100, Peter Krempa wrote:
> > On Tue, Feb 24, 2015 at 10:12:20 +, Daniel Berrange wrote:
> > > The undefine operation should always be allowed to succeed
> > > regardless of whether any NVRAM file exists. ie we should
> > > not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
> > > flag. It is valid for the app to decide it wants the NVRAM
> > > file left on disk, in the same way that disk images are left
> > > on disk at undefine.
> > > ---
> > > src/qemu/qemu_driver.c | 20 +++-
> > > 1 file changed, 7 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > > index bec05d4..302bf48 100644
> > > --- a/src/qemu/qemu_driver.c
> > > +++ b/src/qemu/qemu_driver.c
> > > @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
> > >
> > > if (!virDomainObjIsActive(vm) &&
> > > vm->def->os.loader && vm->def->os.loader->nvram &&
> > > -virFileExists(vm->def->os.loader->nvram)) {
> > > -if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
> > > -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > > - _("cannot delete inactive domain with
> > > nvram"));
> > > -goto cleanup;
> > > -}
> > > -
> > > -if (unlink(vm->def->os.loader->nvram) < 0) {
> > > -virReportSystemError(errno,
> > > - _("failed to remove nvram: %s"),
> > > - vm->def->os.loader->nvram);
> > > -goto cleanup;
> > > -}
> > > +virFileExists(vm->def->os.loader->nvram) &&
> > > +(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
> > > +(unlink(vm->def->os.loader->nvram) < 0)) {
> > > +virReportSystemError(errno,
> > > + _("failed to remove nvram: %s"),
> > > + vm->def->os.loader->nvram);
> > > +goto cleanup;
> > > }
> > >
> > > if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
> >
> >
> > We have prior art in denying to undefine a domain that has information
> > stored in libvirt-internal locations such as the managed save image and
> > snapshot metadata.
> >
> > While it makes sense to allow removing the VM without deleting the NVRAM
> > file when the user specified an external path, we should avoid doing so
> > if the NVRAM is in the libvirt managed path. (Same with externaly
> > managed snapshots or save images).
>
> I'm not a real fan of refusal in the managed save case either. The issue
> is that we're forcing a policy onto the application and not giving it any
> chance to define a different policy. ie currently apps have a choice of
> getting an error, or always deleting the nvram, not giving them any chance
> to delete guest without deleting nvram which is a valid choice they should
> be permitted to have. By removing this check we allow apps to make their
> own policy decision with their more complete view of the world. I don't
> think the quesiton of managed vs unmanaged storage should even come into
> it either, because that presupposes the application will only ever use
> managed storage. OpenStack does not currently use libvirt storage pools
> at all but does wish to use NVRAM. We should not deny the option to undefine
> the guest in this case.
In that case we probably should have negated the logic here and delete
all the stuff by default and give the user option to leave the data
behind.
The original motivation is apparently that we should not allow anything
that would represent state of the deleted VM to be transferred
accidentaly to a new VM with same name. For the save image or snapshots
the risk of persisting any data is low as a save image would not
function without it's disk and still be somewhat secure as it would
contain the whole memory image including security. For the NVRAM though
it might uncover data stored there or even make the VM unbootable.
I agree that the current state is not ideal as we basically force the
user to specify all the necessary flags. I think we can safely avoid
displaying the message in cases when it's not stored in the
libvirt-internal path but for the internal path I'm not convinced that
it would be a great idea to change the default.
We can perhaps add a flag that woult either enable all the "UNDEFINE*"
flags or perhaps invert the logic of them so the user could leave them
behind.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Tue, Feb 24, 2015 at 04:07:02PM +0100, Peter Krempa wrote:
> On Tue, Feb 24, 2015 at 10:12:20 +, Daniel Berrange wrote:
> > The undefine operation should always be allowed to succeed
> > regardless of whether any NVRAM file exists. ie we should
> > not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
> > flag. It is valid for the app to decide it wants the NVRAM
> > file left on disk, in the same way that disk images are left
> > on disk at undefine.
> > ---
> > src/qemu/qemu_driver.c | 20 +++-
> > 1 file changed, 7 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index bec05d4..302bf48 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
> >
> > if (!virDomainObjIsActive(vm) &&
> > vm->def->os.loader && vm->def->os.loader->nvram &&
> > -virFileExists(vm->def->os.loader->nvram)) {
> > -if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
> > -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > - _("cannot delete inactive domain with nvram"));
> > -goto cleanup;
> > -}
> > -
> > -if (unlink(vm->def->os.loader->nvram) < 0) {
> > -virReportSystemError(errno,
> > - _("failed to remove nvram: %s"),
> > - vm->def->os.loader->nvram);
> > -goto cleanup;
> > -}
> > +virFileExists(vm->def->os.loader->nvram) &&
> > +(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
> > +(unlink(vm->def->os.loader->nvram) < 0)) {
> > +virReportSystemError(errno,
> > + _("failed to remove nvram: %s"),
> > + vm->def->os.loader->nvram);
> > +goto cleanup;
> > }
> >
> > if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
>
>
> We have prior art in denying to undefine a domain that has information
> stored in libvirt-internal locations such as the managed save image and
> snapshot metadata.
>
> While it makes sense to allow removing the VM without deleting the NVRAM
> file when the user specified an external path, we should avoid doing so
> if the NVRAM is in the libvirt managed path. (Same with externaly
> managed snapshots or save images).
I'm not a real fan of refusal in the managed save case either. The issue
is that we're forcing a policy onto the application and not giving it any
chance to define a different policy. ie currently apps have a choice of
getting an error, or always deleting the nvram, not giving them any chance
to delete guest without deleting nvram which is a valid choice they should
be permitted to have. By removing this check we allow apps to make their
own policy decision with their more complete view of the world. I don't
think the quesiton of managed vs unmanaged storage should even come into
it either, because that presupposes the application will only ever use
managed storage. OpenStack does not currently use libvirt storage pools
at all but does wish to use NVRAM. We should not deny the option to undefine
the guest in this case.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file
On Tue, Feb 24, 2015 at 10:12:20 +, Daniel Berrange wrote:
> The undefine operation should always be allowed to succeed
> regardless of whether any NVRAM file exists. ie we should
> not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
> flag. It is valid for the app to decide it wants the NVRAM
> file left on disk, in the same way that disk images are left
> on disk at undefine.
> ---
> src/qemu/qemu_driver.c | 20 +++-
> 1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index bec05d4..302bf48 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
>
> if (!virDomainObjIsActive(vm) &&
> vm->def->os.loader && vm->def->os.loader->nvram &&
> -virFileExists(vm->def->os.loader->nvram)) {
> -if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
> -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("cannot delete inactive domain with nvram"));
> -goto cleanup;
> -}
> -
> -if (unlink(vm->def->os.loader->nvram) < 0) {
> -virReportSystemError(errno,
> - _("failed to remove nvram: %s"),
> - vm->def->os.loader->nvram);
> -goto cleanup;
> -}
> +virFileExists(vm->def->os.loader->nvram) &&
> +(flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
> +(unlink(vm->def->os.loader->nvram) < 0)) {
> +virReportSystemError(errno,
> + _("failed to remove nvram: %s"),
> + vm->def->os.loader->nvram);
> +goto cleanup;
> }
>
> if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
We have prior art in denying to undefine a domain that has information
stored in libvirt-internal locations such as the managed save image and
snapshot metadata.
While it makes sense to allow removing the VM without deleting the NVRAM
file when the user specified an external path, we should avoid doing so
if the NVRAM is in the libvirt managed path. (Same with externaly
managed snapshots or save images).
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
