[libvirt] [PATCH] Fixed documentation for destroy storage pool
eskultet at redhat.com
Fri Jan 5 09:46:26 UTC 2018
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 at telecos.upc.edu wrote:
> > > From: Francesc Guasch <frankie at 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
> > 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.
More information about the libvir-list