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.
If you trigger bug 1033369, we get the error message:
error from service: Invalid argument
Which is a bit too generic to pinpoint what is actually failing. This
changes it to:
error from service: CreateMachine: Invalid argument
Acked-by: Eric Blake <eblake@redhat.com>
This is the only callsite.
We drop use of localerror.name here, because it's not actually useful
to us: rather than the parameter name which received an invalid value
(which was assumed), it's actually the the dbus errno equivalent.
Just use the error string.
Acked-by: Eric Blake <eblake@redhat.com>
Inspired by a simpler patch from "Wangrui (K) <moon.wangrui@huawei.com>".
A submitted patch pointed out that virNetlinkCommand() was doing an
improper typecast of the return value from nl_recv() (int to
unsigned), causing it to miss error returns, and that even after
remedying that problem, virNetlinkCommand() was calling VIR_FREE() on
the pointer returned from nl_recv() (*resp) even if nl_recv() had
returned an error, and that in this case the pointer was verifiably
invalid, as it was pointing to memory that had been allocated by
libnl, but then freed prior to returning the error.
While reviewing this patch, I noticed several other problems with this
seemingly simple function (at least one of them as serious as the
problem being reported/fixed by the aforementioned patch), and decided
they all deserved to be fixed. Here is the list:
1) The return value from nl_recv() must be assigned to an int (rather
than unsigned int) in order to detect failure.
2) When nl_recv() returns an error or 0, the contents of *resp is
invalid, and should be simply set to 0, *not* VIR_FREE()'d.
3) When nl_recv() returns 0, errno is not set, so the logged error
message should not reference errno (it *is* an error though).
4) The first error return from virNetlinkCommand returns -EINVAL,
incorrectly implying that the caller can expect the return value to
be of the "-errno" variety, which is not true in any other case.
5) The 2nd error return returns directly with garbage in *resp. While
the caller should never use *resp in this case, it's still good
practice to set it to NULL.
6) For the next 5 (!!) error conditions, *resp will contain garbage,
and virNetlinkCommand() will goto it's cleanup code which will
VIR_FREE(*resp), almost surely leading to a segfault.
In addition to fixing these 6 problems, this patch also makes the
following two changes to make the function conform more closely to the
style of other libvirt code:
1) Change the handling of return code from "named rc and defaulted to
0, but changed to -1 on error" to the more common "named ret and
defaulted to -1, but changed to 0 on success".
2) Rename the "error" label to "cleanup", since the code that follows
is executed in success cases as well as failure.
This partially reverts commits b279e52f7 and ea18f8b2.
It turns out our code base is full of:
if ((struct.member = virBlahFromString(str)) < 0)
goto error;
Meanwhile, the C standard says it is up to the compiler whether
an enum is signed or unsigned when all of its declared values
happen to be positive. In my testing (Fedora 20, gcc 4.8.2),
the compiler picked signed, and nothing changed. But others
testing with gcc 4.7 got compiler warnings, because it picked
the enum to be unsigned, but no unsigned value is less than 0.
Even worse:
if ((struct.member = virBlahFromString(str)) <= 0)
goto error;
is silently compiled without warning, but incorrectly treats -1
from a bad parse as a large positive number with no warning; and
without the compiler's help to find these instances, it is a
nightmare to maintain correctly. We could force signed enums
with a dummy negative declaration in each enum, or cast the
result of virBlahFromString back to int after assigning to an
enum value, or use a temporary int for collecting results from
virBlahFromString, but those actions are all uglier than what we
were trying to cure by directly using enum types for struct
values in the first place. It's better off to just live with int
members, and use 'switch ((virFoo) struct.member)' where we want
the compiler to help, than to track down all the conversions from
string to enum and ensure they don't suffer from type problems.
* src/util/virstorageencryption.h: Revert back to int declarations
with comment about enum usage.
* src/util/virstoragefile.h: Likewise.
* src/conf/domain_conf.c: Restore back to casts in switches.
* src/qemu/qemu_driver.c: Likewise.
* src/qemu/qemu_command.c: Add cast rather than revert.
Signed-off-by: Eric Blake <eblake@redhat.com>
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>
For internal structs, we might as well be type-safe and let the
compiler help us with less typing required on our part (getting
rid of casts is always nice). In trying to use enums directly,
I noticed two problems in virstoragefile.h that can't be fixed
without more invasive refactoring: virStorageSource.format is
used as more of a union of multiple enums in storage volume
code (so it has to remain an int), and virStorageSourcePoolDef
refers to pooltype whose enum is declared in src/conf, but where
src/util can't pull in headers from src/conf.
* src/util/virstoragefile.h (virStorageNetHostDef)
(virStorageSourcePoolDef, virStorageSource): Use enums instead of
int for fields of internal types.
* src/qemu/qemu_command.c (qemuParseCommandLine): Cover all values.
* src/conf/domain_conf.c (virDomainDiskSourceParse)
(virDomainDiskSourceFormat): Simplify clients.
* src/qemu/qemu_driver.c
(qemuDomainSnapshotCreateSingleDiskActive)
(qemuDomainSnapshotPrepareDiskExternalBackingInactive)
(qemuDomainSnapshotPrepareDiskExternalOverlayActive)
(qemuDomainSnapshotPrepareDiskInternal): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
The VIR_ENUM_DECL/VIR_ENUM_IMPL helper macros already append 'Type'
to the enum name being converted; it looks silly to have functions
with 'TypeType' in their name. Even though some of our enums have
to have a 'Type' suffix, the corresponding string conversion
functions do not.
* src/conf/secret_conf.h (VIR_ENUM_DECL): Rename virSecretUsageType.
* src/conf/storage_conf.h (VIR_ENUM_DECL): Rename
virStoragePoolAuthType, virStoragePoolSourceAdapterType,
virStoragePartedFsType.
* src/conf/domain_conf.c (virDomainDiskDefParseXML)
(virDomainFSDefParseXML, virDomainFSDefFormat): Update callers.
* src/conf/secret_conf.c (virSecretDefParseUsage)
(virSecretDefFormatUsage): Likewise.
* src/conf/storage_conf.c (virStoragePoolDefParseAuth)
(virStoragePoolDefParseSource, virStoragePoolSourceFormat):
Likewise.
* src/lxc/lxc_controller.c (virLXCControllerSetupLoopDevices):
Likewise.
* src/storage/storage_backend_disk.c
(virStorageBackendDiskPartFormat): Likewise.
* src/util/virstorageencryption.c (virStorageEncryptionSecretParse)
(virStorageEncryptionSecretFormat): Likewise.
* tools/virsh-secret.c (cmdSecretList): Likewise.
* src/libvirt_private.syms (secret_conf.h, storage_conf.h): Export
corrected names.
Signed-off-by: Eric Blake <eblake@redhat.com>
Continuing the work of consistent enum cleanups; this time in
virstorageencryption.h.
* src/util/virstorageencryption.h (virStorageEncryptionFormat):
Convert to typedef, renaming to avoid collision with function.
(virStorageEncryptionSecret, virStorageEncryption): Directly use
enums.
Signed-off-by: Eric Blake <eblake@redhat.com>
In "src/conf/" there are many enumeration (enum) declarations.
Similar to the recent cleanup to "src/util" directory, it's
better to use a typedef for variable types, function types and
other usages. Other enumeration and folders will be changed to
typedef's in the future. Most of the files changed in this
commit are related to storage (storage_conf) enums.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
If the XML_PARSE_NOENT flag is passed to libxml2, then any
entities in the input document will be fully expanded. This
allows the user to read arbitrary files on the host machine
by creating an entity pointing to a local file. Removing
the XML_PARSE_NOENT flag means that any entities are left
unchanged by the parser, or expanded to "" by the XPath
APIs.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Commit 1b14c44 broke the build on FreeBSD by changing
the signature of a few functions without updating the
corresponding stubs that are used when WITH_MACVTAP
or WITH_VIRTUALPORT is not defined.
In "src/util/" there are many enumeration (enum) declarations.
Sometimes, it's better using a typedef for variable types,
function types and other usages. Other enumeration will be
changed to typedef's in the future.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Signed-off-by: Eric Blake <eblake@redhat.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>
strtoul() is required to parse negative numbers as their
twos-complement positive counterpart. But sometimes we want
to reject negative numbers. Add new functions to do this.
The 'p' suffix is a mnemonic for 'positive' (technically it
also parses 0, but 'non-negative' doesn't lend itself to a
nice one-letter suffix).
* src/util/virstring.h (virStrToLong_uip, virStrToLong_ulp)
(virStrToLong_ullp): New prototypes.
* src/util/virstring.c (virStrToLong_uip, virStrToLong_ulp)
(virStrToLong_ullp): New functions.
* src/libvirt_private.syms (virstring.h): Export them.
* tests/virstringtest.c (testStringToLong): Test them.
Signed-off-by: Eric Blake <eblake@redhat.com>
Commit f22b7899 called to light a long-standing latent bug: the
behavior of virStrToLong_ui was different on 32-bit platforms
than on 64-bit platforms. Curse you, C type promotion and
narrowing rules, and strtoul specification. POSIX says that for
a 32-bit long, strtol handles only 2^32 values [LONG_MIN to
LONG_MAX] while strtoul handles 2^33 - 1 values [-ULONG_MAX to
ULONG_MAX] with twos-complement wraparound for negatives. Thus,
parsing -1 as unsigned long produces ULONG_MAX, rather than a
range error. We WANT[1] this same shortcut for turning -1 into
UINT_MAX when parsing to int; and get it for free with 32-bit
long. But with 64-bit long, ULONG_MAX is outside the range
of int and we were rejecting it as invalid; meanwhile, we were
silently treating -18446744073709551615 as 1 even though it
textually exceeds INT_MIN. Too bad there's not a strtoui() in
libc that does guaranteed parsing to int, regardless of the size
of long.
The bug has been latent since 2007, introduced by Jim Meyering
in commit 5d25419 in the attempt to eradicate unsafe use of
strto[u]l when parsing ints and longs. How embarrassing that we
are only discovering it now - so I'm adding a testsuite to ensure
that it covers all the corner cases we care about.
[1] Ideally, we really want the caller to be able to choose whether
to allow negative numbers to wrap around to their 2s-complement
counterpart, as in strtoul, or to force a stricter input range
of [0 to UINT_MAX] by rejecting negative signs; this will be added
in a later patch for all three int types.
This patch is tested on both 32- and 64-bit; the enhanced
virstringtest passes on both platforms, while virstoragetest now
reliably fails on both platforms instead of just 32-bit platforms.
That test will be fixed later.
* src/util/virstring.c (virStrToLong_ui): Ensure same behavior
regardless of platform long size.
* tests/virstringtest.c (testStringToLong): New function.
(mymain): Comprehensively test string to long parsing.
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 8823272d41a259c1246c05d.
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>
SO_REUSEADDR on Windows is actually akin to SO_REUSEPORT
on Linux/BSD. ie it allows 2 apps to listen to the same
port at once. Thus we must not set it on Win32 platforms
See http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Now that all clients have been adjusted, ensure that no future
misuse of readdir is introduced into the code base.
* cfg.mk (sc_prohibit_readdir): New rule.
* src/util/virfile.c (virDirRead): Exempt the wrapper.
Signed-off-by: Eric Blake <eblake@redhat.com>
In making the conversion to the new API, I fixed a couple bugs:
virSCSIDeviceGetSgName would leak memory if a directory
unexpectedly contained multiple entries;
virNetDevTapGetRealDeviceName could report a spurious error
from a stale errno inherited before starting the readdir search.
The decision on whether to store the result of virDirRead into
a variable is based on whether the end of the loop falls through
to cleanup code automatically. In some cases, we have loops that
are documented to return NULL on failure, and which raise an
error on most failure paths but not in the case where the directory
was unexpectedly empty; it may be worth a followup patch to
explicitly report an error if readdir was successful but the
directory was empty, so that a NULL return always has an error set.
* src/util/vircgroup.c (virCgroupRemoveRecursively): Use new
interface.
(virCgroupKillRecursiveInternal, virCgroupSetOwner): Report
readdir failures.
* src/util/virfile.c (virFileLoopDeviceOpenSearch)
(virFileNBDDeviceFindUnused, virFileDeleteTree): Use new
interface.
* src/util/virnetdevtap.c (virNetDevTapGetRealDeviceName):
Properly check readdir errors.
* src/util/virpci.c (virPCIDeviceIterDevices)
(virPCIDeviceFileIterate, virPCIGetNetName): Report readdir
failures.
(virPCIDeviceAddressIOMMUGroupIterate): Use new interface.
* src/util/virscsi.c (virSCSIDeviceGetSgName): Report readdir
failures, and avoid memory leak.
(virSCSIDeviceGetDevName): Report readdir failures.
* src/util/virusb.c (virUSBDeviceSearch): Report readdir
failures.
* src/util/virutil.c (virGetFCHostNameByWWN)
(virFindFCHostCapableVport): Report readdir failures.
Signed-off-by: Eric Blake <eblake@redhat.com>
Introduce a wrapper for readdir. This helps us make sure that we always
set errno before calling readdir and it will make sure errors are
properly logged.
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
Signed-off-by: Eric Blake <eblake@redhat.com>
The virFirewallAddRuleFull method originally had a single
compulsory virFirewallQueryCallback parameter. During dev
work though the ignoreErrors parameter was added and the
callback parameter made optional. The ATTRIBUTE_NONNULL
annotation was never removed though.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Convert the virebtables.{c,h} files to use the new virFirewall
APIs for changing ebtables rules.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Update the iptablesXXXX methods so that instead of directly
executing iptables commands, they populate rules in an
instance of virFirewallPtr. The bridge driver can thus
construct the ruleset and then invoke it in one operation
having rollback handled automatically.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The network and nwfilter drivers both have a need to update
firewall rules. The currently share no code for interacting
with iptables / firewalld. The nwfilter driver is fairly
tied to the concept of creating shell scripts to execute
which makes it very hard to port to talk to firewalld via
DBus APIs.
This patch introduces a virFirewallPtr object which is able
to represent a complete sequence of rule changes, with the
ability to have multiple transactional checkpoints with
rollbacks. By formally separating the definition of the rules
to be applied from the mechanism used to apply them, it is
also possible to write a firewall engine that uses firewalld
DBus APIs natively instead of via the slow firewalld-cmd.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Instead of hardcoding LIBEXECDIR as the location of the libvirt_iohelper
binary, use virFileFindResource to optionally find it in the current
build directory.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Add virFileFindResource which will try to locate files
in the local build tree if the calling binary (eg libvirtd or
test suite) is being run from the build tree. The corresponding
virFileActivateDirOverride should be called at startup passing
in argv[0]. This will be examined for evidence of libtool magic
binary prefix / sub-directory in order to activate the override.
Signed-off-by: Daniel P. Berrange <berrange@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.
