Re: [PATCH 0/9] Grab modify job for changing domain XML

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 6/4/19 12:53 PM, Nikolay Shirokovskiy wrote:
> On 30.05.2019 12:33, Michal Privoznik wrote:
> > This is an alternative proposal to:
> >
> > https://www.redhat.com/archives/libvir-list/2019-May/msg00830.html
> >
> > The problem I'm trying to fix is described here:
> >
> > https://www.redhat.com/archives/libvir-list/2019-May/msg00810.html
> >
> > Michal Prívozník (9):
> >    virDomainObjListAddLocked: Drop useless @cleanup label
> >    virDomainObjListAddObjLocked: Don't expect vm->def to be set
> >    virDomainObjListAddLocked: Set vm->def only in success path
> >    virDomainObjIsActive: Allow vm->def to be NULL
> >    virDomainObjListAdd: Leave def assigning as an exercise for caller
> >    qemu: Allow vm->def == NULL in job control APIs
> >    qemu: Grab modify job for changing domain XML
> >    lxc: Grab modify job for changing domain XML
> >    libxl: Grab modify job for changing domain XML
> >
> >   src/bhyve/bhyve_driver.c    | 10 +++++---
> >   src/conf/domain_conf.h      |  2 +-
> >   src/conf/virdomainobjlist.c | 48 ++++++++++++++----------------------
> >   src/conf/virdomainobjlist.h |  3 +--
> >   src/libxl/libxl_domain.c    |  3 ++-
> >   src/libxl/libxl_driver.c    | 48 ++++++++++++++++++++++++------------
> >   src/libxl/libxl_migration.c | 14 +++++------
> >   src/lxc/lxc_domain.c        |  3 ++-
> >   src/lxc/lxc_driver.c        | 23 +++++++++++------
> >   src/openvz/openvz_conf.c    | 12 ++++-----
> >   src/openvz/openvz_driver.c  | 17 +++++++------
> >   src/qemu/qemu_domain.c      | 30 ++++++++++++++---------
> >   src/qemu/qemu_driver.c      | 49 ++++++++++++++++++++++++++-----------
> >   src/qemu/qemu_migration.c   | 13 +++++++---
> >   src/test/test_driver.c      | 21 +++++++++-------
> >   src/vmware/vmware_conf.c    |  4 +--
> >   src/vmware/vmware_driver.c  |  9 +++----
> >   src/vz/vz_sdk.c             |  4 ++-
> >   18 files changed, 183 insertions(+), 130 deletions(-)
>
> In patches 8 and 9 there are same issues as in qemu part patches.
>
> In order to overcome NULL names in acquire job and remove inactive domain functions
> I suggest using next approach:
>
>      if (!(vm = virDomainObjListAdd(driver->domains, def,
>                                     driver->xmlopt, 0)))
>          goto cleanup;
>                                                                                                                         
>      if (!vm->def) {
>          virDomainObjAssignDef(vm, def, false, NULL);
>          def = NULL;
>      }
>                                                                                                                         
>      if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
>          qemuDomainRemoveInactive(driver, vm);
>          goto cleanup;
>      }
>                                                                                                                         
>      if (def) {
>          virDomainObjAssignDef(vm, def, false, &oldDef);
>          def = NULL;
>      }
>
> Then we don't need other patches that support case when vm->def == NULL.
>
> First assign can be moved to virDomainObjListAddLocked to the branch
> where new object is created although virDomainObjListAdd will have
> a bit weird semantics in this case.

And that's why I did not want to do that. Assigning definition only in 
some cases is bad design IMO.

But what you suggest may actually work. Let me write the patches and 
test it.

Michal

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