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@xxxxxxxxxxxxxxx wrote: > > > From: Francesc Guasch <frankie@xxxxxxxxxxxxxxx> > > > 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
