Replace:
if (virBufferError(&buf)) {
virBufferFreeAndReset(&buf);
virReportOOMError();
...
}
with:
if (virBufferCheckError(&buf) < 0)
...
This should not be a functional change (unless some callers
misused the virBuffer APIs - a different error would be reported
then)
The parent directory doesn't necessarily need to be stored after we
don't mangle the path stored in the image. Remove it and tweak the code
to avoid using it.
Store backing chain paths as non-canonical. The canonicalization step
will be already taken. This will allow to avoid storing unnecessary
amounts of data.
Now that we store only relative names in virStorageSource's member
relPath the backingRelative member is obsolete. Remove it and adapt the
code to the removal.
Due to various refactors and compatibility with the virstoragetest the
relPath field of the virStorageSource structure was always filled either
with the relative name or the full path in case of absolutely backed
storage. Return its original purpose to store only the relative name of
the disk if it is backed relatively and tweak the tests.
This patch introduces a function that will allow us to resolve a
relative difference between two elements of a disk backing chain. This
function will be used to allow relative block commit and block pull
where we need to specify the new relative name of the image to qemu.
This patch also adds unit tests for the function to verify that it works
correctly.
Introduce a common function that will take a callback to resolve links
that will be used to canonicalize paths on various storage systems and
add extensive tests.
The image labels are stored in the virStorageSource struct. Convert the
virDomainDiskDefGetSecurityLabelDef helper not to use the full disk def
and move it appropriately.
The block commit code looks for an explicit base file relative
to the discovered top file; so for a chain of:
base <- snap1 <- snap2 <- snap3
and a command of:
virsh blockcommit $dom vda --base snap2 --top snap1
we got a sane message (here from libvirt 1.0.5):
error: invalid argument: could not find base 'snap2' below 'snap1' in chain for 'vda'
Meanwhile, recent refactoring has slightly reduced the quality of the
libvirt error messages, by losing the phrase 'below xyz':
error: invalid argument: could not find image 'snap2' in chain for 'snap3'
But we had a one-off, where we were not excluding the top file
itself in searching for the base; thankfully qemu still reports
the error, but the quality is worse:
virsh blockcommit $dom vda --base snap2 --top snap2
error: internal error unable to execute QEMU command 'block-commit': Base '/snap2' not found
Fix the one-off in blockcommit by changing the semantics of name
lookup - if a starting point is specified, then the result must
be below that point, rather than including that point. The only
other call to chain lookup was blockpull code, which was already
forcing the lookup to omit the active layer and only needs a
tweak to use the new semantics.
This also fixes the bug exposed in the testsuite, where when doing
a lookup pinned to an intermediate point in the chain, we were
unable to return the name of the parent also in the chain.
* src/util/virstoragefile.c (virStorageFileChainLookup): Change
semantics for non-NULL startFrom.
* src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Adjust caller,
to keep existing semantics.
* tests/virstoragetest.c (mymain): Adjust to expose new semantics.
Signed-off-by: Eric Blake <eblake@redhat.com>
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: Eric 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.
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: Dmitry 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: Eric 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: Eric 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 8823272d41.
Commit 5c43e2e introduced a NULL deref if there is a failure
in virStorageFileGetMetadataInternal.
* src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf):
Fix error handling.
Signed-off-by: Eric 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: Jiri 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.
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.
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: Eric Blake <eblake@redhat.com>
Signed-off-by: Jiri 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: Eric 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 367cd69 (even
though I accidentally removed the fixme comment early in 74430fe).
* src/util/virstoragefile.c (virStorageFileChainLookup): Depend on
new rather than old fields.
* tests/virstoragetest.c (mymain): Adjust test to match fix.
Signed-off-by: Eric 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: Eric 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: Eric 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: Eric 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: Eric Blake <eblake@redhat.com>
