To ease converting the net->model value to an enum, add
the wrapper functions:
virDomainNetGetModelString
virDomainNetSetModelString
virDomainNetStreqModelString
virDomainNetStrcaseeqModelString
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Pull out the common parts of virDomainSnapshotDef that will be reused
for virDomainCheckpointDef into a new base class. Adjust all callers
that use the direct fields (some of it is churn that disappears when
the next patch refactors virDomainSnapshotObj; oh well...).
Someday, I hope to switch this type to be a subclass of virObject, but
that requires a more thorough audit of cleanup paths, and besides
minimal incremental changes are easier to review.
As for the choice of naming:
I promised my teenage daughter Evelyn that I'd give her credit for her
contribution to this commit. I asked her "What would be a good name
for a base class for DomainSnapshot and DomainCheckpoint". After
explaining what a base class was (using the classic OOB Square and
Circle inherit from Shape), she came up with "DomainMoment", which is
way better than my initial thought of "DomainPointInTime" or
"DomainPIT".
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The only use for the 'current' member of virDomainSnapshotDef was with
the PARSE/FORMAT_INTERNAL flag for controlling an internal-use
<active> element marking whether a particular snapshot definition was
current, and even then, only by the qemu driver on output, and by qemu
and test driver on input. But this duplicates vm->snapshot_current,
and gets in the way of potential simplifications to have qemu store a
single file for all snapshots rather than one file per snapshot. Get
rid of the member by adding a bool* parameter during parse (ignored if
the PARSE_INTERNAL flag is not set), and by adding a new flag during
format (if FORMAT_INTERNAL is set, the value printed in <active>
depends on the new FORMAT_CURRENT).
Then update the qemu driver accordingly, which involves hoisting
assignments to vm->current_snapshot to occur prior to any point where
a snapshot XML file is written (although qemu kept
vm->current_snapshot and snapshot->def_current in sync by the end of
the function, they were not always identical in the middle of
functions, so the shuffling gets a bit interesting). Later patches
will clean up some of that confusing churn to vm->current_snapshot.
Note: even if later patches refactor qemu to no longer use
FORMAT_INTERNAL for output (by storing bulk snapshot XML instead), we
will always need PARSE_INTERNAL for input (because on upgrade, a new
libvirt still has to parse XML left from a previous libvirt).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
An upcoming patch will be reworking virDomainSnapshotDef to have a
base class; minimize the churn by using a local variable to reduce the
number of dereferences required when acessing the domain definition
associated with the snapshot.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Commit 09eb1ae0 added a new enum type for xenbus, and adjusted
affected switch statements in the qemu driver, but failed to notice
that the vbox driver had a similar switch statement.
Signed-off-by: Eric Blake <eblake@redhat.com>
virDomainSnapshotDefFormat currently takes two sets of knobs:
an 'unsigned int flags' argument that can currently just be
VIR_DOMAIN_DEF_FORMAT_SECURE, and an 'int internal' argument used as
a bool to determine whether to output an additional element. It
then reuses the 'flags' knob to call into virDomainDefFormatInternal(),
which takes a different set of flags. In fact, prior to commit 0ecd6851
(1.2.12), the 'flags' argument actually took the public
VIR_DOMAIN_XML_SECURE, which was even more confusing. Let's borrow
from the style of that earlier commit, by introducing a function
for translating from the public flags (VIR_DOMAIN_SNAPSHOT_XML_SECURE
was just recently introduced) into a new enum specific to snapshot
formatting, and adjust all callers to use snapshot-specific enum
values when formatting, and where the formatter now uses a new
variable 'domainflags' to make it obvious when we are translating
from snapshot flags back to domain flags. We don't even have to
use the conversion function for drivers that don't accept the
public VIR_DOMAIN_SNAPSHOT_XML_SECURE flag.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The existing virDomainSnapshotState is a superset of virDomainState,
adding one more state (disk-snapshot) on top of valid domain states.
But as written, the enum cannot be used for gcc validation that all
enum values are covered in a strongly-typed switch condition, because
the enum does not explicitly include the values it is adding to.
Copy the style used in qemu_blockjob.h of creating new enum names
for every inherited value, and update most clients to use the new
enum names anywhere snapshot state is referenced. The exception is
two switch statements in qemu code, which instead gain a fixme
comment about odd type usage (which will be cleaned up in the next
patch). The rest of the patch is fairly mechanical (I actually did
it by temporarily s/state/xstate/ in snapshot_conf.h to let the
compiler find which spots in the code used the field, did the
obvious search and replace in those functions, then undid the rename).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Add <controller type='scsi' model handling for virtio transitional
devices. Ex:
<controller type='scsi' model='virtio-transitional'/>
* "virtio-transitional" maps to qemu "virtio-scsi-pci-transitional"
* "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"
The naming here doesn't match the pre-existing model=virtio-scsi.
The prescence of '-scsi' there seems kind of redundant as we have
type='scsi' already, so I decided to follow the pattern of other
patches and use virtio-transitional etc.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Many drivers had a comment that they did not validate the incoming
'flags' to virDomainGetXMLDesc() because they were relying on
virDomainDefFormat() to do it instead. This used to be the case
(at least since 461e0f1a and friends in 0.9.4 added unknown flag
checking in general), but regressed in commit 0ecd6851 (1.2.12),
when all of the drivers were changed to pass 'flags' through the
new helper virDomainDefFormatConvertXMLFlags(). Since this helper
silently ignores unknown flags, we need to implement flag checking
in each driver instead.
Annoyingly, this means that any new flag values added will silently
be ignored when targeting an older libvirt, rather than our usual
practice of loudly diagnosing an unsupported flag. Add comments
in domain_conf.[ch] to remind us to be extra vigilant about the
impact when adding flags (a new flag to add data is safe if the
older server omitting the requested data doesn't break things in
the newer client; a new flag to suppress data rather than enhancing
the existing VIR_DOMAIN_XML_SECURE may form a data leak or even a
security hole).
In the qemu driver, there are multiple callers all funnelling to
qemuDomainDefFormatBufInternal(); many of them already validated
flags (and often only a subset of the full set of possible flags),
but for ease of maintenance, we can also check flags at the common
helper function.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The @disk was allocated, filled in, and consumed on the normal path,
but for error/cleanup paths it would be leaked. Rename to newHardDisk
and manage properly.
Found by Coverity
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Katerina Koukiou <kkoukiou@redhat.com>
Need to free the allocated hardDiskToOpen array. The contents of the
array are just pointers returned by virVBoxSnapshotConfHardDiskByLocation
and not allocated AFAICT so they don't need to also be freed as well.
Found by Coverity
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Katerina Koukiou <kkoukiou@redhat.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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Avoid the need for the drivers to explicitly check for a NULL path by
making sure it is at least the empty string.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Ensuring that we don't call the virDrvConnectOpen method with a NULL URI
means that the drivers can drop various checks for NULL URIs. These were
not needed anymore since the probe functionality was split
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Declare what URI schemes a driver supports in its virConnectDriver
struct. This allows us to skip trying to open the driver entirely
if the URI scheme doesn't match.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Add a localOnly flag to the virConnectDriver struct which allows a
driver to indicate whether it is local-only, or permits remote
connections. Stateful drivers running inside libvirtd are generally
local only. This allows us to remote the check for uri->server != NULL
from most drivers.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently the virDrvConnectOpen method is supposed to handle both
opening an explicit URI and auto-probing a driver if no URI is
given. Introduce a dedicated virDrvConnectURIProbe method to enable the
probing functionality to be split from the driver opening functionality.
It is still possible for NULL to be passed to the virDrvConnectOpen
method after this change, because the remote driver needs special
handling to enable probing of the URI against a remote libvirtd daemon.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
There is a pattern of using two temporary utf16/utf8 variables
for every value we get from VirtualBox and put in the domain
definition right away.
Reuse the same variable name to improve the chances of getting
the function on one screen.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Use the virMacAddrParse helper that does not require colon-separated
values instead of using extra code to format it that way.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Use VIR_APPEND_ELEMENT instead and change the return type
to int to catch allocation errors.
This removes the need to figure out the adapter count
upfront.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Instead of using def->nets every time, use a temporary pointer.
This will allow splitting out the per-adapter code.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
The allocation errors in this function are already handled by jumping
to a cleanup label.
Change the return type from void to int and return -1 on error.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
The controller model is slightly unusual in that the default value is
-1, not 0. As a result the default value is not covered by any of the
existing enum cases. This in turn means that any switch() statements
that think they have covered all cases, will in fact not match the
default value at all. In the qemuDomainDeviceCalculatePCIConnectFlags()
method this has caused a serious mistake where we fallthrough from the
SCSI controller case, to the VirtioSerial controller case, and from
the USB controller case to the IDE controller case.
By adding explicit enum constant starting at -1, we can ensure switches
remember to handle the default case.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
commit 77a12987a4 changed the "virDomainChrSourceDef source" inside
virDomainChrDef to "virDomainChrSourceDefPtr source", and started
allocating source inside virDomainChrDefNew(), but vboxDumpSerial()
was allocating a virDomainChrDef with a simple VIR_ALLOC() (i.e. never
calling virDomainChrDefNew()), so source was never initialized,
leading to a SEGV any time a serial port was present. The same problem
was created in vboxDumpParallel().
This patch changes vboxDumpSerial() and vboxDumpParallel() to use
virDomainChrDefNew() instead of VIR_ALLOC(), and changes both of those
functions to return an error if virDomainChrDef() (or any other
allocation) fails.
This resolves: https://bugzilla.redhat.com/1536649
As it turns out virDomainDeviceFindControllerModel was only ever
called for SCSI controllers using VIR_DOMAIN_CONTROLLER_TYPE_SCSI
as a parameter.
So rename to virDomainDeviceFindSCSIController and rather than
return a model, let's return a virDomainControllerDefPtr to let
the caller reference whatever it wants.
In VirtualBox SAS and SCSI are separate controller types whereas libvirt
does not make such distinction. This patch adds support for attaching
the VBOX SAS controllers by mapping the 'lsisas1068' controller model in
libvirt XML to VBOX SAS controller type. If VBOX VM has disks attached
to both SCSI and SAS controller libvirt domain XML will have two
<controller type='scsci'> elements with index and model attributes set
accordingly. In this case, each respective <disk> element must have
<address> element specified to assign it to respective SCSI controller.
This patch adds <address> element to each <disk> device since device
names alone won't adequately reflect the storage device layout in the
VM. With this patch, the ouput produced by dumpxml will faithfully
reproduce the storage layout of the VM if used with define.
Previously any removable storage device without media attached was
omitted from domain XML dump. They're still (rightfully) omitted in
snapshot XML dump but need to be accounted properly to for the device
names to stay in 'sync' between domain and snapshot XML dumps.
Primer the code for further changes:
* move variable declarations to the top of the function
* group together free/release statements
* error check and report VBOX API calls used
If a VBOX VM has e.g. a SATA and SCSI disk attached, the XML generated
by dumpxml used to produce "sda" for both of those disks. This is an
invalid domain XML as libvirt does not allow duplicate device names. To
address this, keep the running total of disks that will use "sd" prefix
for device name and pass it to the vboxGenerateMediumName which no
longer tries to "compute" the value based only on current and max
port and slot values. After this the vboxGetMaxPortSlotValues is not
needed and was deleted.
Both vboxSnapshotGetReadWriteDisks and vboxSnapshotGetReadWriteDisks do
not need to free the def->disks on cleanup because it's being done by
the caller via virDomainSnaphotDefFree
This patch prepares the vboxSnapshotGetReadOnlyDisks and
vboxSnapshotGetReadWriteDisks functions for further changes so that
the code movement does not obstruct the gist of those future changes.
This is done primarily because we'll need to know the type of vbox
storage controller as early as possible and make decisions based on
that info.
With this patch, the vbox driver will no longer attach all supported
storage controllers by default even if no disk devices are associated
with them. Instead, it will attach only those that are implicitly added
by virDomainDefAddImplicitController based on <disk> element or if
explicitly specified via the <controller> element.
Since the VBOX API requires to register an initial VM before proceeding
to attach any remaining devices to it, any failure to attach such
devices should result in automatic cleanup of the initially registered
VM so that the state of VBOX registry remains clean without any leftover
"aborted" VMs in it. Failure to cleanup of such partial VMs results in a
warning log so that actual define error stays on the top of the error
stack.
