Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
after the call to virSecretObjListRemove, be more explicit by calling
virObjectUnref and setting @obj to NULL for secretUndefine and in
the error path of secretDefineXML. Calling EndAPI will end up calling
Unlock on an already unlocked object which has indeteriminate results
(usually an ignored error).
The virSecretObjEndAPI will both Unref and Unlock the object; however,
the virSecretObjListRemove would have already Unlock'd the object so
calling Unlock again is incorrect. Once the virSecretObjListRemove
is called all that's left is to Unref our interest since that's the
corrollary to the virSecretObjListAdd which returned our ref interest
plus references for each hash table in which the object resides. In math
terms, after an Add there's 2 refs on the object (1 for the object and
1 for the list). After calling Remove there's just 1 ref on the object.
For the Add callers, calling EndAPI removes the ref for the object and
unlocks it, but since it's in a list the other 1 remains.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Since the virSecretObjListAdd technically consumes @def on success,
the secretDefineXML should set @def = NULL immediately and process
the remaining calls using a new @objDef variable. We can use use
VIR_STEAL_PTR since we know the Add function just stores @def in
obj->def.
Because we steal @def into @objDef, if we jump to restore_backup:
and @backup is set, then we need to ensure the @def would be
free'd properly, so we'll steal it back from @objDef. For the other
condition this fixes a double free of @def if the code had jumped to
@backup == NULL thus calling virSecretObjListRemove without setting
@def = NULL. In this case, the subsequent call to DefFree would
succeed and free @def; however, the call to EndAPI would also
call DefFree because the Unref done would be the last one for
the @obj meaning the obj->def would be used to call DefFree,
but it's already been free'd because @def wasn't managed right
within this error path.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Since we're storing a virUUIDFormat'd string in our Hash Table, let's
modify the Lookup API to receive a formatted string as well.
Signed-off-by: John Ferlan <jferlan@redhat.com>
If a remote call fails during event registration (more than likely from
a network failure or remote libvirtd restart timed just right), then when
calling the virObjectEventStateDeregisterID we don't want to call the
registered @freecb function because that breaks our contract that we
would only call it after succesfully returning. If the @freecb routine
were called, it could result in a double free from properly coded
applications that free their opaque data on failure to register, as seen
in the following details:
Program terminated with signal 6, Aborted.
#0 0x00007fc45cba15d7 in raise
#1 0x00007fc45cba2cc8 in abort
#2 0x00007fc45cbe12f7 in __libc_message
#3 0x00007fc45cbe86d3 in _int_free
#4 0x00007fc45d8d292c in PyDict_Fini
#5 0x00007fc45d94f46a in Py_Finalize
#6 0x00007fc45d960735 in Py_Main
#7 0x00007fc45cb8daf5 in __libc_start_main
#8 0x0000000000400721 in _start
The double dereference of 'pyobj_cbData' is triggered in the following way:
(1) libvirt_virConnectDomainEventRegisterAny is invoked.
(2) the event is successfully added to the event callback list
(virDomainEventStateRegisterClient in
remoteConnectDomainEventRegisterAny returns 1 which means ok).
(3) when function remoteConnectDomainEventRegisterAny is hit,
network connection disconnected coincidently (or libvirtd is
restarted) in the context of function 'call' then the connection
is lost and the function 'call' failed, the branch
virObjectEventStateDeregisterID is therefore taken.
(4) 'pyobj_conn' is dereferenced the 1st time in
libvirt_virConnectDomainEventFreeFunc.
(5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the
2nd time in libvirt_virConnectDomainEventRegisterAny.
(6) the double free error is triggered.
Resolve this by adding a @doFreeCb boolean in order to avoid calling the
freeCb in virObjectEventStateDeregisterID for any remote call failure in
a remoteConnect*EventRegister* API. For remoteConnect*EventDeregister* calls,
the passed value would be true indicating they should run the freecb if it
exists; whereas, it's false for the remote call failure path.
Patch based on the investigation and initial patch posted by
fangying <fangying1@huawei.com>.
Rather than waiting for the first save to fail, let's generate the
directory with the correct privs during initialization.
Signed-off-by: John Ferlan <jferlan@redhat.com>
When processing a virSecretPtr use 'secret' as a variable name.
When processing a virSecretObjPtr use 'obj' as a variable name.
When processing a virSecretDefPtr use 'def' as a variable name,
unless a distinction needs to be made with a 'newdef' such as
virSecretObjListAddLocked (which also used the VIR_STEAL_PTR macro
for the configFile and base64File).
Signed-off-by: John Ferlan <jferlan@redhat.com>
The public virSecret object has a single "usage_id" field
but the virSecretDef object has a different 'char *' field
for each usage type, but the code all assumes every usage
type has a corresponding single string. Get rid of the
pointless union in virSecretDef and just use "usage_id"
everywhere. This doesn't impact public XML format, only
the internal handling.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Move the enum into a new src/util/virsecret.h, rename it to be
virSecretLookupType. Add a src/util/virsecret.h in order to perform
a couple of simple operations on the secret XML and virSecretLookupTypeDef
for clearing and copying.
This includes quite a bit of collateral damage, but the goal is to remove
the "virStorage*" and replace with the virSecretLookupType so that it's
easier to to add new lookups that aren't necessarily storage pool related.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Rather than returning a "char *" indicating perhaps some sized set of
characters that is NUL terminated, alter the function to return 0 or -1
for success/failure and add two parameters to handle returning the
buffer and it's size.
The function no longer encodes the returned secret, rather it returns
the unencoded secret forcing callers to make the necessary adjustments.
Alter the callers to handle the adjusted model.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Call the internal driver callbacks rather than the public APIs to avoid
calling unnecessarily the error dispatching code and don't overwrite
the error messages provided by the APIs. They are good enough to
describe which secret is missing either by UUID or the usage (basically
name).
Introduce the final accessor's to _virSecretObject data and move the
structure from virsecretobj.h to virsecretobj.c
The virSecretObjSetValue logic will handle setting both the secret
value and the value_size. Some slight adjustments to the error path
over what was in secretSetValue were made.
Additionally, a slight logic change in secretGetValue where we'll
check for the internalFlags and error out before checking for
and erroring out for a NULL secret->value. That way, it won't be
obvious to anyone that the secret value wasn't set rather they'll
just know they cannot get the secret value since it's private.
Move and rename the secretRewriteFile, secretSaveDef, and secretSaveValue
from secret_driver to virsecretobj
Need to make some slight adjustments since the secretSave* functions
called secretEnsureDirectory, but otherwise mostly just a move of code.
Move and rename secretDeleteSaved from secret_driver into virsecretobj and
split it up into two parts since there is error path code that looks to
just delete the secret data file
Move to secret_conf.c and rename to virSecretLoadAllConfigs. Also includes
moving/renaming the supporting virSecretLoad, virSecretLoadValue, and
virSecretLoadValidateUUID.
This patch replaces most of the guts of secret_driver.c with recently
added secret_conf.c APIs in order manage secret lists and objects
using the hashed virSecretObjList* lookup API's.
Move virSecretObj from secret_driver.c to virsecretobj.h
To support being able to create a hashed secrets list, move the
virSecretObj to virsecretobj.h so that the code can at least find
the definition.
This should be a temporary situation while the virsecretobj.c code
is patched in order to support a hashed secret object while still
having the linked list support in secret_driver.c. Eventually, the
goal is to move the virSecretObj into virsecretobj.c, although it
is notable that the existing model from which virSecretObj was
derived has virDomainObj in src/conf/domain_conf.h and virNetworkObj
in src/conf/network_conf.h, so virSecretObj wouldn't be unique if
it were to remain in virsecretobj.h Still adding accessors to fetch
and store hashed object data will be the end goal.
Add definitions and infrastucture in virsecretobj.c to create and
handle a hashed virSecretObj and virSecretObjList including the class,
object, lock setup, and disposal API's. Nothing will call these yet.
This infrastructure will replace the forward linked list logic
within the secret_driver, eventually.
Commit id 'fb2bd208' essentially copied the qemuGetSecretString
creating an libxlGetSecretString. Rather than have multiple copies
of the same code, create src/secret/secret_util.{c,h} files and
place the common function in there.
Modify the the build in order to build the module as a library
which is then pulled in by both the qemu and libxl drivers for
usage from both qemu_command.c and libxl_conf.c
This new API will allocate the secret, assign the def pointer, and
insert the secret onto the passed list. Whether that's the temporary
list in loadSecrets which gets loaded into the driver list or driver
list during secretDefineXML.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Add a temporary helper to search for a specific secret by address
on the list and remove it if it's found. The following patch will
introduce a common allocation and listInsert helper. That means
error paths of the routines calling would need a way to remove the
secret off the list.
Signed-off-by: John Ferlan <jferlan@redhat.com>
This patch removes need for secretBase64Path and secretComputePath. Similar
to the configFile, create an entry for base64File, which will be generated
as the driver->configDir, the UUID value, plus the ".base" suffix. Rather
than generating on the fly, store this in the virSecretObj.
The buildup of the pathname done in loadSecrets where the failure to build
is ignored which is no different than the failure to generate the name
in secretLoadValue which would have been ignored in the failure path
after secretLoad.
This also removes the need for secretComputPath and secretBase64Path.
Signed-off-by: John Ferlan <jferlan@redhat.com>
This patch removes the need for secretXMLPath. Instead save 'path' during
loadSecret as 'configFile'. The secretXMLPath is nothing more than an
open coded virFileBuildPath. All that code did was concantenate the
driver->configDir, the UUID of the secret, and the ".xml" suffix to form
the configFile name which we now will generate and save instead.
Signed-off-by: John Ferlan <jferlan@redhat.com>
The 'secretLoad' was essentially open coding virFileBuildPath.
Adjust the logic to have the caller build the path and pass it. The net
sum of ignoring the virFileBuildPath failure is the same as before where
the failure to virAsprintf the path would have been ignored anyway in
the secretLoad error path.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Remove the need for the local 'secret' in secretConnectListAllSecrets.
A subsequent patch will rename the ObjPtr entry to secret.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Rather than having it interspersed with other changes, do it once.
Remove a couple ^L, 1 argument per line for functions, less than 80 chars
per line, use of spacing between logical groups of code, use of one line
if statements when doing fetch followed by comparison, use direct return
when no cleanup to be done.
Signed-off-by: John Ferlan <jferlan@redhat.com>
For stateless, client side drivers, it is never correct to
probe for secondary drivers. It is only ever appropriate to
use the secondary driver that is associated with the
hypervisor in question. As a result the ESX & HyperV drivers
have both been forced to do hacks where they register no-op
drivers for the ones they don't implement.
For stateful, server side drivers, we always just want to
use the same built-in shared driver. The exception is
virtualbox which is really a stateless driver and so wants
to use its own server side secondary drivers. To deal with
this virtualbox has to be built as 3 separate loadable
modules to allow registration to work in the right order.
This can all be simplified by introducing a new struct
recording the precise set of secondary drivers each
hypervisor driver wants
struct _virConnectDriver {
virHypervisorDriverPtr hypervisorDriver;
virInterfaceDriverPtr interfaceDriver;
virNetworkDriverPtr networkDriver;
virNodeDeviceDriverPtr nodeDeviceDriver;
virNWFilterDriverPtr nwfilterDriver;
virSecretDriverPtr secretDriver;
virStorageDriverPtr storageDriver;
};
Instead of registering the hypervisor driver, we now
just register a virConnectDriver instead. This allows
us to remove all probing of secondary drivers. Once we
have chosen the primary driver, we immediately know the
correct secondary drivers to use.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Since virSecretFree will call virObjectUnref anyway, let's just use that
directly so as to avoid the possibility that we inadvertently clear out
a pending error message when using the public API.
Convert all remaining clients of readdir to use the new
interface, so that we can ensure (unlikely) errors while
reading a directory are reported.
* src/openvz/openvz_conf.c (openvzAssignUUIDs): Use new
interface.
* src/parallels/parallels_storage.c (parallelsFindVolumes)
(parallelsFindVmVolumes): Report readdir failures.
* src/qemu/qemu_driver.c (qemuDomainSnapshotLoad): Ignore readdir
failures.
* src/secret/secret_driver.c (loadSecrets): Likewise.
* src/qemu/qemu_hostdev.c
(qemuHostdevHostSupportsPassthroughVFIO): Report readdir failures.
* src/xen/xen_inotify.c (xenInotifyOpen): Likewise.
* src/xen/xm_internal.c (xenXMConfigCacheRefresh): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
Any source file which calls the logging APIs now needs
to have a VIR_LOG_INIT("source.name") declaration at
the start of the file. This provides a static variable
of the virLogSource type.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Coverity found an issue in lxc_driver and uml_driver that we don't
check the return value of register functions.
I've also updated all other places and unify the way we check the
return value.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Convert the type of loop iterators named 'i', 'j', k',
'ii', 'jj', 'kk', to be 'size_t' instead of 'int' or
'unsigned int', also santizing 'ii', 'jj', 'kk' to use
the normal 'i', 'j', 'k' naming
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
