Re: [PATCH 2/2] qemu_driver: remove duplicate code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jul 03, 2015 at 02:01:46PM +0200, Erik Skultety wrote:
> 
> 
> On 01/07/15 12:05, Pavel Hrdina wrote:
> > The copy of persistent definition is already done in
> > virDomainLiveConfigHelperMethod few lines above.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> >  src/qemu/qemu_driver.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 4cfae03..ca93a1a 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -10336,12 +10336,6 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
> >                                          &vmdef) < 0)
> >          goto endjob;
> >  
> > -    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> > -        /* Make a copy for updated domain. */
> > -        if (!(vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt)))
> > -            goto endjob;
> > -    }
> > -
> >      if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> >          if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
> >              virReportError(VIR_ERR_OPERATION_INVALID,
> > 
> NACK. It's not true, that a copy is made in
> virDomainLiveConfigHelperMethod, only a reference to domain definition
> is returned. The problem is that we free vmdef at the end of the API
> which might not result in a desired behavior, let's consider an inactive
> persistent domain and you try to set one of scheduler params, instead of
> modifying an freeing a copy, you manipulate and free the original
> instance, what happens then? Yep, daemon crashes in the next API (OK,
> not every time, it requires a bit of luck but after a couple of minutes
> I managed to do that as well).

True, I've missed that in the helper we don't create a copy, just take a
reference.  This makes perfect sense and in fact it's an exception, where we use
a copy to update the domain and in case of failure just drop the copy and leave
the domain definition intact.  I thought, that there is no API that have this
behavior.

I'll push only the first patch, thanks for review.

Pavel

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

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]