Fixed documentation for destroy storage pool

[libvirt] [PATCH] Fixed documentation for destroy storage pool

Posted by frankie@telecos.upc.edu 6 years, 3 months ago

From: Francesc Guasch <frankie@telecos.upc.edu>

---
 lib/Sys/Virt/StoragePool.pm | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/Sys/Virt/StoragePool.pm b/lib/Sys/Virt/StoragePool.pm
index 0bc1d50..2ba5101 100644
--- a/lib/Sys/Virt/StoragePool.pm
+++ b/lib/Sys/Virt/StoragePool.pm
@@ -115,14 +115,11 @@ C<define_storage_pool> method in L<Sys::Virt>.
 
 Remove the configuration associated with a storage pool previously defined
 with the C<define_storage pool> method in L<Sys::Virt>. If the storage pool is
-running, you probably want to use the C<shutdown> or C<destroy>
-methods instead.
+running, you probably want to use the C<destroy> method instead.
 
 =item $pool->destroy()
 
-Immediately terminate the machine, and remove it from the virtual
-machine monitor. The C<$pool> handle is invalid after this call
-completes and should not be used again.
+Immediately stop the storage pool.
 
 =item $flag = $pool->get_autostart();
 
-- 
2.14.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fixed documentation for destroy storage pool

Posted by Erik Skultety 6 years, 3 months ago

On Sat, Dec 30, 2017 at 09:15:34AM +0100, frankie@telecos.upc.edu wrote:
> From: Francesc Guasch <frankie@telecos.upc.edu>
>
> ---
>  lib/Sys/Virt/StoragePool.pm | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/lib/Sys/Virt/StoragePool.pm b/lib/Sys/Virt/StoragePool.pm
> index 0bc1d50..2ba5101 100644
> --- a/lib/Sys/Virt/StoragePool.pm
> +++ b/lib/Sys/Virt/StoragePool.pm
> @@ -115,14 +115,11 @@ C<define_storage_pool> method in L<Sys::Virt>.
>
>  Remove the configuration associated with a storage pool previously defined
>  with the C<define_storage pool> method in L<Sys::Virt>. If the storage pool is
> -running, you probably want to use the C<shutdown> or C<destroy>
> -methods instead.
> +running, you probably want to use the C<destroy> method instead.

If you want to make the pool unmanaged by libvirt, destroy doesn't help at
all since it would only stop a running pool, but wouldn't undefine it.
Therefore, we should either omit the sentence completely or use something like
this: 'Calling C<undefine> on a running pool makes it transient, thus leaving
the underlying object intact, so if object discard is desired, C<destroy> should
be called first.'
However, truth to be told, even my suggested sentence isn't correct, since
undefine on running pools results in an error - we need to fix that since it
should behave the same way as domains and make them transient. Maybe we can
drop the additional sentence now and update it later when things work the
expected way.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fixed documentation for destroy storage pool

Posted by Francesc Guasch 6 years, 3 months ago

On 03/01/18 10:47, Erik Skultety wrote:
> On Sat, Dec 30, 2017 at 09:15:34AM +0100, frankie@telecos.upc.edu wrote:
>> From: Francesc Guasch <frankie@telecos.upc.edu>
>> diff --git a/lib/Sys/Virt/StoragePool.pm b/lib/Sys/Virt/StoragePool.pm
>> index 0bc1d50..2ba5101 100644
>> --- a/lib/Sys/Virt/StoragePool.pm
>> +++ b/lib/Sys/Virt/StoragePool.pm
>> @@ -115,14 +115,11 @@ C<define_storage_pool> method in L<Sys::Virt>.
>>
>>   Remove the configuration associated with a storage pool previously defined
>>   with the C<define_storage pool> method in L<Sys::Virt>. If the storage pool is
>> -running, you probably want to use the C<shutdown> or C<destroy>
>> -methods instead.
>> +running, you probably want to use the C<destroy> method instead.
> 
> If you want to make the pool unmanaged by libvirt, destroy doesn't help at
> all since it would only stop a running pool, but wouldn't undefine it.
> Therefore, we should either omit the sentence completely or use something like
> this: 'Calling C<undefine> on a running pool makes it transient, thus leaving
> the underlying object intact, so if object discard is desired, C<destroy> should
> be called first.'

Good point. But I use destroy to set the storage pool inactive, it works 
for me.

> However, truth to be told, even my suggested sentence isn't correct, since
> undefine on running pools results in an error - we need to fix that since it
> should behave the same way as domains and make them transient. Maybe we can
> drop the additional sentence now and update it later when things work the
> expected way.
> 

My initial concern was that shutdown is not in the Storage Pool API. 
Only destroy can be used. I guess it got pasted from the Domain module.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fixed documentation for destroy storage pool

Posted by Erik Skultety 6 years, 3 months ago

On Fri, Jan 05, 2018 at 09:47:51AM +0100, Francesc Guasch wrote:
> On 03/01/18 10:47, Erik Skultety wrote:
> > On Sat, Dec 30, 2017 at 09:15:34AM +0100, frankie@telecos.upc.edu wrote:
> > > From: Francesc Guasch <frankie@telecos.upc.edu>
> > > diff --git a/lib/Sys/Virt/StoragePool.pm b/lib/Sys/Virt/StoragePool.pm
> > > index 0bc1d50..2ba5101 100644
> > > --- a/lib/Sys/Virt/StoragePool.pm
> > > +++ b/lib/Sys/Virt/StoragePool.pm
> > > @@ -115,14 +115,11 @@ C<define_storage_pool> method in L<Sys::Virt>.
> > >
> > >   Remove the configuration associated with a storage pool previously defined
> > >   with the C<define_storage pool> method in L<Sys::Virt>. If the storage pool is
> > > -running, you probably want to use the C<shutdown> or C<destroy>
> > > -methods instead.
> > > +running, you probably want to use the C<destroy> method instead.
> >
> > If you want to make the pool unmanaged by libvirt, destroy doesn't help at
> > all since it would only stop a running pool, but wouldn't undefine it.
> > Therefore, we should either omit the sentence completely or use something like
> > this: 'Calling C<undefine> on a running pool makes it transient, thus leaving
> > the underlying object intact, so if object discard is desired, C<destroy> should
> > be called first.'
>
> Good point. But I use destroy to set the storage pool inactive, it works for
> me.
>
> > However, truth to be told, even my suggested sentence isn't correct, since
> > undefine on running pools results in an error - we need to fix that since it
> > should behave the same way as domains and make them transient. Maybe we can
> > drop the additional sentence now and update it later when things work the
> > expected way.
> >
>
> My initial concern was that shutdown is not in the Storage Pool API. Only
> destroy can be used. I guess it got pasted from the Domain module.

Yeah, that's some historical artifact we can't get rid of. Sadly, the naming is
very unfortunate. Anyhow, I went ahead and pushed your patch (once we fix the
storage pool's behavior we might want to adjust <undefine> again, but for the
time being, it's fine).

Just a small advice, when sending patches against libvirt derivatives, i.e.
perl, python, etc. we use a prefix in the subject, e.g.
"[libvirt-<derivative>]...". Also, it's always nice to provide a commit message
to explain the reason behind the change. I took care of the commit message and
pushed the patch, congratulations on your first contribution.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list