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: NEric Blake <eblake@redhat.com>
Reviewed-by: NJohn 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: NEric Blake <eblake@redhat.com>
Reviewed-by: NJohn 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: NAndrea Bolognani <abologna@redhat.com>
Signed-off-by: NCole 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: NEric Blake <eblake@redhat.com>
Reviewed-by: NJohn 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: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NKaterina 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: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NKaterina 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: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NDaniel 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: NDaniel 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: NDaniel 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: NDaniel 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: NDaniel 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: NDaniel P. Berrangé <berrange@redhat.com>

This is a global variable, but it isn't accessible from outside
of the file its declared in.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Also return an error when VIR_STRDUP fails.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

We can steal the strings instead of creating more copies.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NJán Tomko <jtomko@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: NJá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: NJá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: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NJán Tomko <jtomko@redhat.com>

Move the allocation from vboxDumpNetworks inside vboxDumpNetwork.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

The 'enabled' bool is initialized to false, there is no need to nest the
conditions.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Split out per-adapter code from vboxDumpNetworks.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Free up 'vboxDumpNetwork' for dumping single network.
Signed-off-by: NJá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: NJá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: NJán Tomko <jtomko@redhat.com>

Now that the functions are separate, we no longer need comment
separators.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

s/sharedFoldersCleanup/cleanup/
Signed-off-by: NJá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: NJohn Ferlan <jferlan@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

commit 77a12987 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.

Since it's not longer used to shuttle the @secinfo, let's remove
the private hostdev completely.

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.

So that the function signature matches vboxSnapshotGetReadWriteDisks

Both vboxSnapshotGetReadWriteDisks and vboxSnapshotGetReadWriteDisks do
not need to free the def->disks on cleanup because it's being done by
the caller via virDomainSnaphotDefFree