In some places where virDomainObjListForEach() is called the
passed callback calls virDomainObjListRemoveLocked(). Well, this
is unsafe, because the former only grabs a read lock but the
latter modifies the list.
I've identified the following unsafe calls:

- qemuProcessReconnectAll()
- libxlReconnectDomains()

The rest seem to be safe.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Wire up the use of a checkpoint list into each domain, similar to the
existing snapshot list.  This includes adding a function for checking
that a redefine operation fits in with the existing list, as well as
various filtering capabilities over the list contents.
Signed-off-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NDaniel P. Berrangé <berrange@redhat.com>

It's a premature optimization. It's perfectly acceptable for
'error' label to deal with @vm == NULL case.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

If there are two concurrent threads, one of which is removing a
domain from the list and the other is trying to add it back they
may serialize in the following order:

1) vm->removing is set and @vm is unlocked.
2) The tread that's trying to add the domain onto the list locks
   the list and tries to find, if the domain already exists.
3) Our lookup functions say it doesn't, so the thread proceeds to
   virHashAddEntry() which fails with 'Duplicate key' error.

This is obviously not helpful error message at all.

The problem lies in our lookup functions
(virDomainObjListFindByUUIDLocked() and
virDomainObjListFindByNameLocked()) which return NULL even if the
object is still on the list. They do this so that the object is
not mistakenly looked up by some driver. The fix consists of
moving 'removing' check one level up and thus allowing
virDomainObjListAddLocked() to produce meaningful error message.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NCole Robinson <crobinso@redhat.com>

snapshot_conf.h was mixing three separate types: the snapshot
definition, the snapshot object, and the snapshot object list.
Separate out the snapshot object list code into its own file, and
update includes for affected clients.

This is just code motion, but done in preparation of sharing a lot of
the object list code with checkpoints.
Signed-off-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Despite its name, this is really just a general-purpose string
manipulation function, so it should be moved to the virstring
module and renamed accordingly.

A few trivial whitespace changes are squashed in.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
ACKed-by: NPeter Krempa <pkrempa@redhat.com>

Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>). VIR_ONCE_GLOBAL_INIT is almost
exclusively called without an ending semicolon, but let's
standardize on using one like the other macros.

Add a dummy struct definition at the end of the macro, so
the compiler will require callers to add a semicolon.
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>
Signed-off-by: NCole Robinson <crobinso@redhat.com>

In many files there are header comments that contain an Author:
statement, supposedly reflecting who originally wrote the code.
In a large collaborative project like libvirt, any non-trivial
file will have been modified by a large number of different
contributors. IOW, the Author: comments are quickly out of date,
omitting people who have made significant contribitions.

In some places Author: lines have been added despite the person
merely being responsible for creating the file by moving existing
code out of another file. IOW, the Author: lines give an incorrect
record of authorship.

With this all in mind, the comments are useless as a means to identify
who to talk to about code in a particular file. Contributors will always
be better off using 'git log' and 'git blame' if they need to  find the
author of a particular bit of code.

This commit thus deletes all Author: comments from the source and adds
a rule to prevent them reappearing.

The Copyright headers are similarly misleading and inaccurate, however,
we cannot delete these as they have legal meaning, despite being largely
inaccurate. In addition only the copyright holder is permitted to change
their respective copyright statement.
Reviewed-by: NErik Skultety <eskultet@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

If @vm has flagged as "to be removed" virDomainObjListFindByNameLocked
returns NULL (although the definition actually exists). Therefore, the
possibility exits that "virHashAddEntry" will raise the error
"Duplicate key" => virDomainObjListAddObjLocked fails =>
virDomainObjEndAPI(&vm) is called and this leads to a freeing of @def
since @def is already assigned to vm->def. But actually this leads to
a double free since the common usage pattern is that the caller of
virDomainObjListAdd(Locked) is responsible for freeing @def in case of
an error.

Let's fix this by setting vm->def to NULL in case of an error.

Backtrace:

   ➤  bt
   #0  virFree (ptrptr=0x7575757575757575)
   #1  0x000003ffb5b25b3e in virDomainResourceDefFree
   #2  0x000003ffb5b37c34 in virDomainDefFree
   #3  0x000003ff9123f734 in qemuDomainDefineXMLFlags
   #4  0x000003ff9123f7f4 in qemuDomainDefineXML
   #5  0x000003ffb5cd2c84 in virDomainDefineXML
   #6  0x000000011745aa82 in remoteDispatchDomainDefineXML
   ...
Reviewed-by: NBjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: NMarc Hartmayer <mhartmay@linux.ibm.com>

SKIP_OSTYPE_CHECKS only hides some error reporting at this point,
so it can be foled into SKIP_VALIDATE
Acked-by: NMichal Privoznik <mprivozn@redhat.com>
Signed-off-by: NCole Robinson <crobinso@redhat.com>

When adding a new object to the domain object list, there should
have been 2 virObjectRef calls made one for each list into which
the object was placed to match the 2 virObjectUnref calls that
would occur during Remove as part of virHashRemoveEntry when
virObjectFreeHashData is called when the element is removed from
the hash table as set up in virDomainObjListNew.

Some drivers (libxl, lxc, qemu, and vz) handled this inconsistency
by calling virObjectRef upon successful return from virDomainObjListAdd
in order to use virDomainObjEndAPI when done with the returned @vm.
While others (bhyve, openvz, test, and vmware) handled this via only
calling virObjectUnlock upon successful return from virDomainObjListAdd.

This patch will "unify" the approach to use virDomainObjEndAPI
for any @vm successfully returned from virDomainObjListAdd.

Because list removal is so tightly coupled with list addition,
this patch fixes the list removal algorithm to return the object
as entered - "locked and reffed".  This way, the callers can then
decide how to uniformly handle add/remove success and failure.
This removes the onus on the caller to "specially handle" the
@vm during removal processing.

The Add/Remove logic allows for some logic simplification such
as in libxl where we can Remove the @vm directly rather than
needing to set a @remove_dom boolean and removing after the
libxlDomainObjEndJob completes as the @vm is locked/reffed.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Rather than open code within virDomainObjListRemove, just call
the *Locked function.

Additionally, add comments to virDomainObjListRemove to describe
the usage model.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Use the FindBy{UUID|Name}Locked helpers which will return a locked
and ref counted object rather than the direct virHashLookup and
virObjectLock of the returned object. We'll need to temporarily
virObjectUnref when we assign a new domain @def, but that will
change shortly when virDomainObjListAddObjLocked returns the
correct reference counted object.

Use the virDomainObjEndAPI in the error path to Unref/Unlock for
the corresponding Unref/Unlock of either the FindBy* return or
the virDomainObjNew since both return a reffed/locked object.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Create helpers virDomainObjListFindByUUIDLocked and
virDomainObjListFindByNameLocked to avoid the need
to lock the domain object list leaving that task
for the caller.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Rework the code such that virDomainObjListFindByID will always
return a locked/ref counted object so that the callers can
always do the same cleanup logic to call virDomainObjEndAPI.
Makes accessing the objects much more consistent.

NB:
There were 2 callers (lxcDomainLookupByID and qemuDomainLookupByID)
that were already using the ByID name, but not virDomainObjEndAPI -
these were changed as well in this update/patch.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NJim Fehlig <jfehlig@suse.com>

Now that every caller is using virDomainObjListFindByUUIDRef,
let's just remove it and keep the name as virDomainObjListFindByUUID.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NJim Fehlig <jfehlig@suse.com>

So far we are repeating the following lines over and over:

  if (!(virSomeObjectClass = virClassNew(virClassForObject(),
                             "virSomeObject",
                             sizeof(virSomeObject),
                             virSomeObjectDispose)))
      return -1;

While this works, it is impossible to do some checking. Firstly,
the class name (the 2nd argument) doesn't match the name in the
code in all cases (the 3rd argument). Secondly, the current style
is needlessly verbose. This commit turns example into following:

  if (!(VIR_CLASS_NEW(virSomeObject,
                      virClassForObject)))
      return -1;
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NDaniel P. Berrangé <berrange@redhat.com>

Create a common helper to add an object to the locked domain
objlist hash tables and use it.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

If the virHashAddEntry fails, then we need to "careful" about
how we free the @obj. When virDomainObjParseFile returns there
is one reference and the object is locked, so use virDomainObjEndAPI
when done.

Add a virObjectRef in the error path for the second virHashAddEntry
call since it doesn't call virObjectRef, but virHashRemoveEntry
will call virObjectUnref because virObjectFreeHashData is called
when the element is removed from the hash table as set up in
virDomainObjListNew.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

If the virHashAddEntry fails, then we need to "careful" about
how we free the @vm. When virDomainObjNew returns there is one
reference and the object is locked, so use virDomainObjEndAPI
when done.

Add a virObjectRef in the error path for the second virHashAddEntry
call since it doesn't call virObjectRef, but virHashRemoveEntry
will call virObjectUnref because virObjectFreeHashData is called
when the element is removed from the hash table as set up in
virDomainObjListNew.

Eventually these paths should goto error and error should be changed
to use EndAPI as well, but that requires more adjustments to other
paths in the code to have a locked and ref counted @vm.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Use bool instead of an int.

Some failures of the post parse callback can be tolerated. This is
specifically desired when loading the configs of existing VMs. In such
case the post parse callback should not really be modifying anything
in the definition.

This patch adds a parse flag VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL
which will allow the callbacks to report non-fatal failures by returning
a positive return value. In such case the field 'postParseFailed' in the
domain definition is set to true, to notify the drivers that the
callback failed and possibly needs to be re-run.

When dropping a domain report which one was dropped so that it's not
necessary to rummage through the logs.

Rather than overload virObjectUnlock as commit id '77f4593b' has
done, create a separate virObjectRWUnlock API that will force the
consumers to make the proper decision regarding unlocking the
RWLock's. Similar to the RWLockRead and RWLockWrite, use the
virObjectGetRWLockableObj helper. This restores the virObjectUnlock
code to using the virObjectGetLockableObj.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Instead of making virObjectLock be the entry point for two
different types of locks, let's create a virObjectRWLockWrite API
which will only handle the virObjectRWLockableClass objects.

Use the new virObjectRWLockWrite for the virdomainobjlist code
in order to handle the Add, Remove, Rename, and Load operations
that need to be very synchronous.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Since the class it represents is based on virObjectRWLockableClass
and in order to make sure we differentiate just in case anyone somehow
believes they could use virObjectLockRead for a virObjectLockableClass,
let's rename the API to use the RW in the name. Besides the RW locks
refer to pthread_rwlock_{init|rdlock|wrlock|unlock|destroy} while the
other locks refer to pthread_mutex_{init|lock|unlock|destroy}.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

There is no reason why two threads trying to look up two domains
should mutually exclude each other. Utilize new
virObjectRWLockable that was just introduced.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NPavel Hrdina <phrdina@redhat.com>

While searching for an element using a function it may be
desirable to know the element key for future operation.
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>
Reviewed-by: NJiri Denemark <jdenemar@redhat.com>

So far our code is full of the following pattern:

  dom = virGetDomain(conn, name, uuid)
  if (dom)
      dom->id = 42;

There is no reasong why it couldn't be just:

  dom = virGetDomain(conn, name, uuid, id);

After all, client domain representation consists of tuple (name,
uuid, id).
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

We want to pass the proper opaque pointer instead of NULL to
virDomainDefParse and subsequently virDomainDefParseNode too.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Use it instead of opendir everywhere we need to check for ENOENT.

This fixes the disappearance of domains and networks starting with a
dot.

https://bugzilla.redhat.com/show_bug.cgi?id=1333248

Introduce a helper that only calls closedir if DIR* is non-NULL
and sets it to NULL afterwards.

In libxl driver we do virObjectRef in libxlDomainObjBeginJob,
If virCondWaitUntil failed, it goes to error, do virObjectUnref,
There's a chance that someone undefine the vm at the same time,
and refs unref to zero, vm is freed in libxlDomainObjBeginJob.
But the vm outside function is not Null, we do virObjectUnlock(vm).
That's how we overwrite the vm memory after it's freed. I fix it.
Signed-off-by: NWang Yufei <james.wangyufei@huawei.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Until now we weren't able to add checks that would reject configuration
once accepted by the parser. This patch adds a new callback and
infrastructure to add such checks. In this patch all the places where
rejecting a now-invalid configuration wouldn't be a good idea are marked
with a new parser flag.

Our existing virHashForEach method iterates through all items disregarding the
fact, that some of the iterators might have actually failed. Errors are usually
dispatched through an error element in opaque data which then causes the
original caller of virHashForEach to return -1. In that case, virHashForEach
could return as soon as one of the iterators fail. This patch changes the
iterator return type and adjusts all of its instances accordingly, so the
actual refactor of virHashForEach method can be dealt with later.
Signed-off-by: NErik Skultety <eskultet@redhat.com>

A pretty nasty deadlock occurs while trying to rename a VM in parallel
with virDomainObjListNumOfDomains.
The short description of the problem is as follows:

Thread #1:

qemuDomainRename:
    ------> aquires domain lock by qemuDomObjFromDomain
       ---------> waits for domain list lock in any of the listed functions:
          - virDomainObjListFindByName
          - virDomainObjListRenameAddNew
          - virDomainObjListRenameRemove

Thread #2:

virDomainObjListNumOfDomains:
    ------> aquires domain list lock
       ---------> waits for domain lock in virDomainObjListCount

Introduce generic virDomainObjListRename function for renaming domains.
It aquires list lock in right order to avoid deadlock. Callback is used
to make driver specific domain updates.
Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Our domain_conf.* files are big enough. Not only they contain XML
parsing code, but they served as a storage of all functions whose
name is virDomain prefixed. This is just wrong as it gathers not
related functions (and modules) into one big file which is then
harder to maintain. Split virDomainObjList module into a separate
file called virdomainobjlist.[ch].
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>