Jim Fehlig reported a regression found by libvirt-TCK tests:

> ~ # perl /usr/share/libvirt-tck/tests/qemu/100-disk-encryption.t
...
> ok 4 - defined persistent domain config
> # Starting inactive domain config
> libvirt error code: 1, message: internal error: unable to execute QEMU command
> 'cont': 'drive-ide0-0-1'
> (/var/cache/libvirt-tck/300-disk-encryption/demo.qcow2) is encrypted

Commit 2279d560 converted a boolean into a pointer with the intent of
transferring that pointer out of a temporary object into the caller's
data structure.  The temporary structure meant that meta->encryption
was always NULL on entry, so we could get away with blindly allocating
the pointer when the header said so.  But later, commit 8823272d
tweaked things to do backing chain detection in-place, rather than via
a temporary object; this has the net result that meta->encryption can
be non-NULL on entry.  Not only did this turn the latent behavior into
a memory leak, it is also a behavior regression: blindly allocating a
new pointer wipes out what secrets we already knew about the chain,
making it impossible to restart the domain.

Of course, no one in their right mind should be relying on qcow2
encryption - it is fundamentally flawed.  And sadly, the TCK tests
don't get run often enough, and this shows that our virstoragetest
does not exercise encrypted images at all.  Otherwise, we could
have avoided a release containing this regression.

* src/util/virstoragefile.c (virStorageFileGetMetadataInternal):
Don't nuke an already-existing encryption.
Signed-off-by: NEric Blake <eblake@redhat.com>

Add parsers for relative and absolute backing names for local and remote
storage files.

This parser parses relative paths as relative to their parents and
absolute paths according to the protocol or local access.

For remote storage volumes, all URI based backing file names are
supported and for the qemu colon syntax the NBD protocol is supported.

Use virStorageFileReadHeader() to read headers of storage files possibly
on remote storage to retrieve the image metadata.

The backend information is now parsed by
virStorageFileGetMetadataInternal which is now exported from the util
source and virStorageFileGetMetadataFromFDInternal now doesn't need to
be exported.

My future work will modify the metadata crawler function to use the
storage driver file APIs to access the files instead of accessing them
directly so that we will be able to request the metadata for remote
files too. To avoid linking the storage driver to every helper file
using the utils code, the backing chain traversal function needs to be
moved to the storage driver source.

Additionally the virt-aa-helper and virstoragetest programs need to be
linked with the storage driver as a result of this change.

For guests backed by gluster volumes (or other network storage) we don't
fill the backing chain (see qemuDomainDetermineDiskChain). This leaves
the "relPath" field of the top image NULL. This causes a crash in
virStorageFileChainLookup() when looking up a backing element for such a
disk.

Since I'm working on adding support for network storage and one of the
steps will make the "relPath" field optional let's use STREQ_NULLABLE
instead of STREQ in virStorageFileChainLookup() to avoid the problem.

Now that virStorageFileGetMetadataFromBuf is used only for remote
filesystems, don't canonicalize the path in it.

Add argument to return backing file format of a file probed by
virStorageFileGetMetadataFromFD so that it can be used in place of
virStorageFileGetMetadataFromBuf.

Currently the protocol type with index 0 was NBD which made it hard to
distinguish whether the protocol type was actually assigned. Add a new
protocol type with index 0 to distinguish it explicitly.

The gluster volume name was previously stored as part of the source path
string. This is unfortunate when we want to do operations on the path as
the volume is used separately.

Parse and store the volume name separately for gluster storage volumes
and use the newly stored variable appropriately.

Add VIR_STORAGE_FILE_PLOOP format. This format is used
to store disk images for virtual machines in PCS and containers
in PCS, OpenVZ and also in Parallels Desktop for Mac.

This format is described on OpenVZ site -
https://openvz.org/Ploop (together with ploop devices). It
consists of XML descriptor and one or more image files: base
image and deltas. Format of the image files described here:
https://openvz.org/Ploop/format.

This patch only adds VIR_STORAGE_FILE_PLOOP constant, consequent
patches will use it in parallels driver.
Signed-off-by: NDmitry Guryanov <dguryanov@parallels.com>

All callers of virStorageFileGetMetadataFromBuf were first calling
virStorageFileProbeFormatFromBuf, to learn what format to pass in.
But this function is already wired to do the exact same probe if
the incoming format is VIR_STORAGE_FILE_AUTO, so it's simpler to
just refactor the probing into the central function.

* src/util/virstoragefile.h (virStorageFileGetMetadataFromBuf):
Drop parameter.
(virStorageFileProbeFormatFromBuf): Drop declaration.
* src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf):
Do probe here instead of in callers.
(virStorageFileProbeFormatFromBuf): Make static.
* src/libvirt_private.syms (virstoragefile.h): Drop function.
* src/storage/storage_backend_fs.c (virStorageBackendProbeTarget):
Update caller.
* src/storage/storage_backend_gluster.c
(virStorageBackendGlusterRefreshVol): Likewise.
Signed-off-by: NEric Blake <eblake@redhat.com>

Commit f22b7899 stumbled across a difference between 32-bit and
64-bit platforms when parsing "-1" as an int.  Now that we've
fixed that difference, it's time to fix the testsuite.

* src/util/virstoragefile.c (virStorageFileParseChainIndex):
Require a positive index.
Signed-off-by: NEric Blake <eblake@redhat.com>

To avoid memory leak of the "backingStoreRaw" field when reparsing
backing chains a new function is being introduced by this patch that
shall be used to clear backing store information.

The memory leak was introduced in commit 8823272d.

Commit 5c43e2e0 introduced a NULL deref if there is a failure
in virStorageFileGetMetadataInternal.

* src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf):
Fix error handling.
Signed-off-by: NEric Blake <eblake@redhat.com>

Each backing store of a given disk is associated with a unique index
(which is also formatted in domain XML) for easier addressing of any
particular backing store. With this patch, any backing store can be
addressed by its disk target and the index. For example, "vdc[4]"
addresses the backing store with index equal to 4 of the disk identified
by "vdc" target. Such shorthand can be used in any API in place for a
backing file path:

    virsh blockcommit domain vda --base vda[3] --top vda[2]
Signed-off-by: NJiri Denemark <jdenemar@redhat.com>

Returning both virStorageSourcePtr and its path member does not make a
lot of sense.
Signed-off-by: NJiri Denemark <jdenemar@redhat.com>

To avoid having the root of a backing chain present twice in the list we
need to invert the working of virStorageFileGetMetadataRecurse.

Until now the recursive worker created a new backing chain element from
the name and other information passed as arguments. This required us to
pass the data of the parent in a deconstructed way and the worker
created a new entry for the parent.

This patch converts this function so that it just fills in metadata
about the parent and creates a backing chain element from those. This
removes the duplication of the first element.

To avoid breaking the test suite, virstoragetest now calls a wrapper
that creates the parent structure explicitly and pre-fills it with the
test data with same function signature as previously used.

Don't remove detected metadata about directory based storage volumes.

To conform with the naming of the planned XML output rename the metadata
variable name.

s/backingMeta/backingStore/g

Remove the now unused pieces of the structure.

Replace the old structure with the new one. This change is a trivial
name change operation (along with change of the freeing function).

Add the required fields that are missing from the new structure that
will allow us to switch the storage file metadata code entirely to the
new structure.

Add "relPath" and "relDir" and the raw backing store name. Also allow
creating linked lists of virStorageSourcePtrs to express backing chains.

Add a free function as some parts of the code will allocate the
structure.

Remove the obsolete field replaced by data in "path".

The testsuite requires tweaking as the name of the backing file is now
stored one layer deeper in the backing chain linked list.

As a temporary step to allow killing of the "backingStore" field of
struct virStorageFileMetadata the recursive metadata retrieval function
will be converted not to use the field in the lookup process.

As for the previous patch, this change is needed to achieve
compatibility with all the existing code, where we expect a fully
qualified path of local files to be present.

To allow future change of virStorageFileMetadata to virStorageSource we
need to store a full path in the "path" variable as rest of the code
expects it to be a full path. Rename the "path" field to "relPath" to
keep tracking the info but allowing a real "path" field.

Avoid passing lot of arguments into guts of metadata retrieval to fill
the actual structure. Temporarily fill the structure before passing it
down to the actual metadata extractor.

This will later help the inversion of the steps taken to extract the
metadata so that this function can be fully converted to
virStorageSource as the data struct.

This patch also fixes regression when starting a gluster storage pool
where the volumes don't have local representation so that the
canonicalization of the volume's file name failed. Broken by commit
79f11b35

Move the code checking the presence of the backing file to the recursive
worker function instead of the metadata parser. The recursive worker
will later be changed to parse more than just local files and this
change will help the separation.

As we already pass the whole structure down the call path there's no
need to return some stuff in a separate argument. Remove the argument
and tweak callers to avoid breaking semantics.

virStorageFileGetMetadataFromBuf will be refactored later along with the
storage driver.

Don't use the backingStoreRaw as a indication of broken chains. Fill it
always and tweak the broken image chain detector to avoid changing the
semantics.

The new semantics to detect a broken chain is the presence of string in
backingStoreRaw but the lack of the backing chain metadata structure in
the chain.

Now that the raw backing store name is always filled there's no need to
pass the raw name variable separately to fill in case the backing is not
a file. Tweak the function so that it can handle a NULL in that case.

While running virstoragetest, valgrind pointed out the following
memory leak:

==8142== 2 bytes in 1 blocks are definitely lost in loss record 1 of 92
==8142==    at 0x4A069EE: malloc (vg_replace_malloc.c:270)
==8142==    by 0x4E7B53E: mdir_name (dirname-lgpl.c:78)
==8142==    by 0x4CBE2B0: virStorageFileGetMetadataInternal (virstoragefile.c:595)
==8142==    by 0x4CBE651: virStorageFileGetMetadataFromFDInternal (virstoragefile.c:1086)
==8142==    by 0x4CBEEB4: virStorageFileGetMetadataRecurse (virstoragefile.c:1175)
==8142==    by 0x4CBF1DE: virStorageFileGetMetadata (virstoragefile.c:1270)
==8142==    by 0x4028AD: testStorageChain (virstoragetest.c:275)
==8142==    by 0x407B91: virtTestRun (testutils.c:201)
==8142==    by 0x4039D7: mymain (virstoragetest.c:534)
==8142==    by 0x40830D: virtTestMain (testutils.c:789)
==8142==    by 0x3E6CE1ED1C: (below main) (libc-start.c:226)

...62 times

Domain snapshots should only permit an external snapshot into
a storage format that permits a backing chain, since the new
snapshot file necessarily must be backed by the existing file.
The C code for the qemu driver is a little bit stricter in
currently enforcing only qcow2 or qed, but at the XML parser
level, including virt-xml-validate, it is fairly easy to
enforce that a user can't request a 'raw' external snapshot.

* docs/schemas/storagecommon.rng (storageFormat): Split out...
(storageFormatBacking): ...new sublist.
* docs/schemas/domainsnapshot.rng (disksnapshotdriver): Use new
type.
* src/util/virstoragefile.h (virStorageFileFormat): Rearrange for
easier code management.
* src/util/virstoragefile.c (virStorageFileFormat, fileTypeInfo):
Likewise.
* src/conf/snapshot_conf.c (virDomainSnapshotDiskDefParseXML): Use
new marker to limit selection of formats.
Signed-off-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NJiri Denemark <jdenemar@redhat.com>

Another field no longer needed, getting us one step closer to
merging virStorageFileMetadata and virStorageSource.

* src/util/virstoragefile.h (_virStorageFileMetadata): Drop
field.
* src/util/virstoragefile.c (virStorageFileGetMetadataInternal)
(virStorageFileGetMetadataFromFDInternal): Alter signature.
(virStorageFileFreeMetadata, virStorageFileGetMetadataFromBuf)
(virStorageFileGetMetadataFromFD): Adjust clients.
* tests/virstoragetest.c (_testFileData, testStorageChain)
(mymain): Simplify test.
Signed-off-by: NEric Blake <eblake@redhat.com>

Thanks to the testsuite, I feel quite confident that this rewrite
is correct; it gives the same results for all cases except for one.
I can make the argument that _that_ case was a pre-existing bug:
when looking up relative names, the lookup is supposed to be
pegged to the directory that contains the parent qcow2 file.  Thus,
this resolves the fixme first mentioned in commit 367cd69d (even
though I accidentally removed the fixme comment early in 74430fe3).

* src/util/virstoragefile.c (virStorageFileChainLookup): Depend on
new rather than old fields.
* tests/virstoragetest.c (mymain): Adjust test to match fix.
Signed-off-by: NEric Blake <eblake@redhat.com>

The original chain lookup code had to pass in the starting name,
because it was not available in the chain.  But now that we have
added fields to the struct, this parameter is redundant.

* src/util/virstoragefile.h (virStorageFileChainLookup): Alter
signature.
* src/util/virstoragefile.c (virStorageFileChainLookup): Adjust
handling of top of chain.
* src/qemu/qemu_driver.c (qemuDomainBlockCommit): Adjust caller.
* tests/virstoragetest.c (testStorageLookup, mymain): Likewise.
Signed-off-by: NEric Blake <eblake@redhat.com>

The chain lookup function was inconsistent on whether it left
a message in the log when looking up a name that is not found
on the chain (leaving a message for OOM or if name was
relative but not part of the chain), and could litter the log
even when successful (when name was relative but deep in the
chain, use of virFindBackingFile early in the chain would complain
about a file not found).  It's easier to make the function
consistently emit a message exactly once on failure, and to let
all callers rely on the clean semantics.

* src/util/virstoragefile.c (virStorageFileChainLookup): Always
report error on failure.  Simplify relative lookups.
* src/qemu/qemu_driver.c (qemuDomainBlockCommit): Avoid
overwriting error.
Signed-off-by: NEric Blake <eblake@redhat.com>

Drop another redundant field from virStorageFileMetadata.

* src/util/virstoragefile.h (_virStorageFileMetadata): Drop
field.
* src/util/virstoragefile.c
(virStorageFileGetMetadataFromFDInternal)
(virStorageFileGetMetadataFromFD)
(virStorageFileGetMetadataRecurse): Adjust callers.
* tests/virstoragetest.c (_testFileData, testStorageChain)
(mymain): Simplify test.
Signed-off-by: NEric Blake <eblake@redhat.com>

A couple pieces of virStorageFileMetadata are used only while
collecting information about the chain, and don't need to
live permanently in the struct.  This patch refactors external
callers to collect the information separately, so that the
next patch can remove the fields.

* src/util/virstoragefile.h (virStorageFileGetMetadataFromBuf):
Alter signature.
* src/util/virstoragefile.c (virStorageFileGetMetadataInternal):
Likewise.
(virStorageFileGetMetadataFromFDInternal): Adjust callers.
* src/storage/storage_backend_fs.c (virStorageBackendProbeTarget):
Likewise.
* src/storage/storage_backend_gluster.c
(virStorageBackendGlusterRefreshVol): Likewise.
Signed-off-by: NEric Blake <eblake@redhat.com>

Finally starting to prune away some of the old fields that have
been made redundant by the new fields, on my way towards directly
reusing virStorageSource.

* src/util/virstoragefile.h (_virStorageFileMetadata): Drop
field.
* src/util/virstoragefile.c (virStorageFileGetMetadataInternal)
(virStorageFileChainLookup): Adjust callers.
* tests/virstoragetest.c (_testFileData, testStorageChain)
(mymain): Simplify test.
Signed-off-by: NEric Blake <eblake@redhat.com>