[libvirt] GSoC student introduction

[Prafullkumar Tale]

Hello,

I'm Prafullkumar, a student participant from Google Summer of Code program.
I'll be working to convert OCI-formatted containers to libvirt lxc
containers[1] over the summer. I'm excited to be working with libvirt
community and hope to make valuable contribution.

I hangout on IRC with the handle *talep158.* I'm available in IST working
hours.

[1] https://wiki.libvirt.org/page/Google_Summer_of_Code_Ideas

Regards
-- 
Prafullkumar
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/3] sample: vfio mdev display devices.

[Alex Williamson]

On Wed, 25 Apr 2018 01:20:08 +0530
Kirti Wankhede  wrote:

> On 4/24/2018 3:10 AM, Alex Williamson wrote:
> > On Wed, 18 Apr 2018 12:31:53 -0600
> > Alex Williamson  wrote:
> >   
> >> On Mon,  9 Apr 2018 12:35:10 +0200
> >> Gerd Hoffmann  wrote:
> >>  
> >>> This little series adds three drivers, for demo-ing and testing vfio
> >>> display interface code.  There is one mdev device for each interface
> >>> type (mdpy.ko for region and mbochs.ko for dmabuf).
> >>
> >> Erik Skultety brought up a good question today regarding how libvirt is
> >> meant to handle these different flavors of display interfaces and
> >> knowing whether a given mdev device has display support at all.  It
> >> seems that we cannot simply use the default display=auto because
> >> libvirt needs to specifically configure gl support for a dmabuf type
> >> interface versus not having such a requirement for a region interface,
> >> perhaps even removing the emulated graphics in some cases (though I
> >> don't think we have boot graphics through either solution yet).
> >> Additionally, GVT-g seems to need the x-igd-opregion support
> >> enabled(?), which is a non-starter for libvirt as it's an experimental
> >> option!
> >>
> >> Currently the only way to determine display support is through the
> >> VFIO_DEVICE_QUERY_GFX_PLANE ioctl, but for libvirt to probe that on
> >> their own they'd need to get to the point where they could open the
> >> vfio device and perform the ioctl.  That means opening a vfio
> >> container, adding the group, setting the iommu type, and getting the
> >> device.  I was initially a bit appalled at asking libvirt to do that,
> >> but the alternative is to put this information in sysfs, but doing that
> >> we risk that we need to describe every nuance of the mdev device
> >> through sysfs and it becomes a dumping ground for every possible
> >> feature an mdev device might have.
> >>  
> 
> One or two sysfs file for each feature shouldn't be that much of over
> head? In kernel, other subsystem modules expose capability through
> sysfs, like PCI subsystem adds 'boot_vga' file for VGA device which
> returns 0/1 depending on if its boot VGA device. Similarly
> 'd3cold_allowed', 'msi_bus'...

Obviously we could add sysfs files, but unlike properties that the PCI
core exposes about struct pci_dev fields, the idea of a vfio_device is
much more abstract.  Each bus driver creates its own device
representation, so we have a top level vfio_device referencing through
an opaque pointer a vfio_pci_device, vfio_platform_device, or
mdev_device, and each mdev vendor driver creates its own private data
structure below the mdev_device.  So it's not quite a simple as one new
attribute "show" function to handle all devices of that bus_type.  We
need a consistent implementation in each bus driver and vendor driver
or we need to figure out how to percolate the information up to the
vfio core.  Your idea below seems to take the percolate approach.
 
> >> So I was ready to return and suggest that maybe libvirt should probe
> >> the device to know about these ancillary configuration details, but
> >> then I remembered that both mdev vGPU vendors had external dependencies
> >> to even allow probing the device.  KVMGT will fail to open the device
> >> if it's not associated with an instance of KVM and NVIDIA vGPU, I
> >> believe, will fail if the vGPU manager process cannot find the QEMU
> >> instance to extract the VM UUID.  (Both of these were bad ideas)  
> > 
> > Here's another proposal that's really growing on me:
> > 
> >  * Fix the vendor drivers!  Allow devices to be opened and probed
> >without these external dependencies.
> >  * Libvirt uses the existing vfio API to open the device and probe the
> >necessary ioctls, if it can't probe the device, the feature is
> >unavailable, ie. display=off, no migration.
> >   
> 
> I'm trying to think simpler mechanism using sysfs that could work for
> any feature and knowing source-destination migration compatibility check
> by libvirt before initiating migration.
> 
> I have another proposal:
> * Add a ioctl VFIO_DEVICE_PROBE_FEATURES
> struct vfio_device_features {
> __u32 argsz;
> __u32 features;
> }
> 
> Define bit for each feature:
> #define VFIO_DEVICE_FEATURE_DISPLAY_REGION(1 << 0)
> #define VFIO_DEVICE_FEATURE_DISPLAY_DMABUF(1 << 1)
> #define VFIO_DEVICE_FEATURE_MIGRATION (1 << 2)
> 
> * Vendor driver returns bitmask of supported features during
> initialization phase.
> 
> * In vfio core module, trap this ioctl for each device  in
> vfio_device_fops_unl_ioctl(),

Whoops, chicken and egg problem, VFIO_GROUP_GET_DEVICE_FD is our
blocking point with mdev drivers, we can't get a device fd, so we can't
call an ioctl on the device fd.

> check features bitmask returned by vendor
> driver and add a sysfs file if feature is supported that device. This
> sysfs file would return 0/1.

I 

Re: [libvirt] [PATCH] qemu: use target.port for isa-serial

[John Ferlan]

On 04/11/2018 07:22 AM, Thilo Cestonaro wrote:

Not entirely my area of expertise, console serial ports, but I'll
provide some feedback. Maybe someone else will chime in too...

> A configured target.port is currently totaly ignored, while constructing

s/totaly/totally
s/,//

> qemu commandline, for all types of serial devices. This patch adds a -device

s/,//

> parameter "index" for the target model isa-serial.

Please add blank lines between paragraphs - tough to read... Needed for
each of the 4 paragraphs...

> This enables the user to specify which serial device will end in which ttySX
> device.

But shouldn't they then specify that's what they want to happen rather
than appending for every one?  Perhaps I'm missing something subtle.

Still you have a very specific usage model, the ttySX console, but
you're trying to model a generic solution. Is there really a need to
supply the index when there's only 1 serial console device/port?

> Updated test results which failed because of this change.

They failed perhaps because you took this too far?

> Added two tests serial-dev-without-target-port, serial-dev-with-target-port
> which test generating qemu command with multiple serial devices and different
> ports.

Ahhh and this is what I'll key off mostly...

> 
> Signed-off-by: Thilo Cestonaro 
> ---
> v2: added tests which create qemu commandline with and without specified 
> target
> ports. Updated existing tests to pass again.
> 

Noting v2 here is nice, but the subject line should have PATCHv2...

>  src/qemu/qemu_command.c   | 16 
>  tests/qemuxml2argvdata/bios.args  |  2 +-
>  .../qemuxml2argvdata/console-compat-auto.args |  2 +-
>  .../console-compat-chardev.args   |  2 +-
>  tests/qemuxml2argvdata/console-compat.args|  2 +-
>  .../qemuxml2argvdata/console-virtio-many.args |  2 +-
>  tests/qemuxml2argvdata/controller-order.args  |  2 +-
>  .../q35-virt-manager-basic.args   |  2 +-
>  .../serial-dev-chardev-iobase.args|  2 +-
>  .../qemuxml2argvdata/serial-dev-chardev.args  |  2 +-
>  tests/qemuxml2argvdata/serial-dev-chardev.xml |  4 +-
>  .../serial-dev-with-target-port.args  | 31 +++
>  .../serial-dev-with-target-port.xml   | 38 +++
>  .../serial-dev-without-target-port.args   | 31 +++
>  .../serial-dev-without-target-port.xml| 35 +
>  .../qemuxml2argvdata/serial-file-chardev.args |  2 +-
>  tests/qemuxml2argvdata/serial-file-log.args   |  2 +-
>  .../qemuxml2argvdata/serial-many-chardev.args |  4 +-
>  .../qemuxml2argvdata/serial-pty-chardev.args  |  2 +-
>  tests/qemuxml2argvdata/serial-spiceport.args  |  2 +-
>  .../qemuxml2argvdata/serial-tcp-chardev.args  |  2 +-
>  .../serial-tcp-telnet-chardev.args|  2 +-
>  .../serial-tcp-tlsx509-chardev-notls.args |  4 +-
>  .../serial-tcp-tlsx509-chardev-verify.args|  4 +-
>  .../serial-tcp-tlsx509-chardev.args   |  4 +-
>  .../serial-tcp-tlsx509-secret-chardev.args|  4 +-
>  .../qemuxml2argvdata/serial-udp-chardev.args  |  4 +-
>  .../qemuxml2argvdata/serial-unix-chardev.args |  2 +-
>  tests/qemuxml2argvdata/serial-vc-chardev.args |  2 +-
>  tests/qemuxml2argvdata/user-aliases.args  |  4 +-
>  tests/qemuxml2argvtest.c  |  8 +++-
>  31 files changed, 191 insertions(+), 34 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/serial-dev-with-target-port.args
>  create mode 100644 tests/qemuxml2argvdata/serial-dev-with-target-port.xml
>  create mode 100644 tests/qemuxml2argvdata/serial-dev-without-target-port.args
>  create mode 100644 tests/qemuxml2argvdata/serial-dev-without-target-port.xml
> 

I tried to 'git am -3' on top of current head - suffice to say with the
volume of change in the last couple of weeks - your patch won't apply
completely.  So I need to ask you to regenerate and post as a v3 with a
couple of other thoughts as described below...

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 514c3ab2e..5f770404b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10291,6 +10291,22 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
>
> virDomainChrSerialTargetModelTypeToString(serial->targetModel),
>serial->info.alias, serial->info.alias);
>  
> +switch ((virDomainChrSerialTargetModel) serial->targetModel) {
> +case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL:
> +if (serial->target.port != -1)
> +virBufferAsprintf(, ",index=%d", serial->target.port);
> +break;

This has resulted in a *lot* of changed to .args files and it's really
not clear if "all" are really necessary...

> +case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE:
> +case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL:
> +case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL:
> +case 

Re: [libvirt] [PATCH 29/29] Require space after cast

[Laine Stump]

On 04/23/2018 10:16 AM, Martin Kletzander wrote:
> On Mon, Apr 23, 2018 at 02:25:26PM +0100, Daniel P. Berrangé wrote:
>> Personally I'm not a fan of adding the extra space - the cast is
>> associated
>> with the variable, so I don't think it needs it.
>>
>
> I know we've discussed it earlier, and I don't want to repeat myself,
> but for
> the sake of keeping it in the conversation; there are spaces around
> everything
> except function calls, but in the end it's just a clearer separation. 
> I used to
> prefer non-spaced casts too, but then I misread some and realized it's
> similar
> to arithmetics for example.
>
> And before anyone starts shouting at me that it is very subjective and
> it all
> depeds on x, y, and z, including your terminal font, I agree, it for
> sure is.
> And I also agree that we should not be spending almost any of out time
> with
> something that's very close to bikeshedding =)  I just want this to be
> unified
> across the codebase, and if the consensus is that there should be no
> spaces,
> then I'll resend the series with the spaces removed.
>
> Just let me know so that I don't send ton of patches for no reason.

I would also vote for no spaces - I've always seen "lack of a space
after a single word in parens" as the "this is a typecast" operator (it
makes sense in my addled brain).


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 10/11] qemu: Add VM Generation ID to qemu command line

[John Ferlan]

On 04/24/2018 03:28 AM, Peter Krempa wrote:
> On Mon, Apr 23, 2018 at 20:00:04 -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1149445
>>
>> If the domain requests usage of the genid functionality,
>> then add the QEMU '-device vmgenid' to the command line
>> providing either the supplied or generated GUID value.
>>
>> Add tests for both a generated and supplied GUID value.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_command.c| 31 +++
>>  tests/qemuxml2argvdata/genid-auto.args | 24 
>>  tests/qemuxml2argvdata/genid.args  | 24 
>>  tests/qemuxml2argvtest.c   |  4 
>>  4 files changed, 83 insertions(+)
>>  create mode 100644 tests/qemuxml2argvdata/genid-auto.args
>>  create mode 100644 tests/qemuxml2argvdata/genid.args
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index b666f3715f..1f5e79d86a 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -5942,6 +5942,34 @@ qemuBuildSmbiosCommandLine(virCommandPtr cmd,
>>  
>>  
>>  static int
>> +qemuBuildVMGenIDCommandLine(virCommandPtr cmd,
>> +const virDomainDef *def,
>> +virQEMUCapsPtr qemuCaps)
>> +{
>> +virBuffer opts = VIR_BUFFER_INITIALIZER;
>> +char guid[VIR_UUID_STRING_BUFLEN];
>> +
>> +if (!def->genidRequested)
>> +return 0;
>> +
>> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID)) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +   _("genid is not supported with this QEMU binary"));
>> +return -1;
> 
> This is already checked in qemuProcessGenID.
> 

Oh right.

>> +}
>> +
>> +virUUIDFormat(def->genid, guid);
>> +virBufferAsprintf(, "vmgenid,guid=%s,id=vmgenid0", guid);
> 
> [...]
> 
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index 74d930ebe2..0dd0850036 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -805,6 +805,10 @@ mymain(void)
>>  QEMU_CAPS_SECCOMP_BLACKLIST);
>>  DO_TEST_PARSE_ERROR("minimal-no-memory", NONE);
>>  DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP);
>> +
>> +DO_TEST("genid", QEMU_CAPS_DEVICE_VMGENID);
>> +DO_TEST("genid-auto", QEMU_CAPS_DEVICE_VMGENID);
> 
> Please use DO_TEST_CAPS_LATEST/DO_TEST_CAPS_VER
> 

OK - That was added after I originally posted. Happy, happy, joy, joy,
genid is the guinea pig! Still, it's not clear what would be expected
since the comments indicate desired usage for positive test cases.

Since pre 2.9 would be a failure scenario, the only difference is works
or doesn't work, which honestly doesn't appear to be the intended
purpose of the new test macros.

So do you expect to see _VER for all versions since 2.9 as well or just
a DO_TEST_CAPS_LATEST("genid"); (and "genid-auto") with adjusted result
file names instead of the DO_TEST results?  Wish there were more
examples other than just disk-drive-write-cache...

The 2.9 output is different from 2.10 and 2.12, but does that matter for
this code? We don't have a comparable 2.11 output. Seems like an
excessive amount of *.args files could be generated with the only
difference being the -machine output. Although I do note that the
*disk-drive-write-cache*.*.args files all have the same machine id value
while my branch has different ones for each version file. That's because
you define the machine in the XML file while mine just used 'pc'.
Leaves me wondering about the validity of what was being done /-|.

Thinking forward to when/if the next set of qemu capabilities are
created, does that mean we have to go back and create 2.12 specific
files for the current "x86_64-latest.args" file(s) since the latest will
invariably and eventually get some new stuff that will cause a
difference?  Who gets that task? The first unlucky/unfortunate sole that
creates the caps_2.13.0.x86_64.xml file? Or whomever has a change
causing the difference?

To a degree it's fortunate that genid is only in the x86_64 capabilities
I guess; otherwise, I'd be wondering again asking/pondering about
aarch64, s390x, ppc64 too...

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[John Snow]

On 04/23/2018 06:38 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 21.04.2018 00:26, Eric Blake wrote:
>> On 04/20/2018 01:24 PM, John Snow wrote:
>>
> Why is option 3 unworkable, exactly?:
>
> (3) Checkpoints exist as structures only with libvirt. They are saved
> and remembered in the XML entirely.
>
> Or put another way:
>
> Can you explain to me why it's important for libvirt to be able to
> reconstruct checkpoint information from a qcow2 file?
>

 In short it take extra effort for metadata to be consistent when 
 libvirtd crashes occurs. See for more detailed explanation 
 in [1] starting from words "Yes it is possible".

 [1] https://www.redhat.com/archives/libvir-list/2018-April/msg01001.html
>>
>> I'd argue the converse. Libvirt already knows how to do atomic updates
>> of XML files that it tracks.  If libvirtd crashes/restarts in the middle
>> of an API call, you already have indeterminate results of whether the
>> API worked or failed; once libvirtd is restarted, you'll have to
>> probably retry the command.  For all other cases, the API call
>> completes, and either no XML changes were made (the command failed and
>> reports the failure properly), or all XML changes were made (the command
>> created the appropriate changes to track the new checkpoint, including
>> whatever bitmap names have to be recorded to map the relation between
>> checkpoints and bitmaps).
> 
> We can fail to save XML... Consider we have B1, B2 and create B3 bitmap
> in the process of creating checkpoint C3. Next qemu creates snapshot
> and bitmap successfully then libvirt fail to update XML and after some
> time libvirt restarts (not even crashes). Now libvirt nows of B1 and B2 but 
> not B3.
> What can be the consequences? For example if we ask bitmap from C2 we
> miss all changes from C3 as we don't know of B3. This will lead to corrupted
> backups.
> 
> This can be fixed:
> 
> - in qemu. If bitmaps have child/parent realtionship then on libvirt restart
>   we can recover (we ask qemu for bitmaps, discover B3 and then discover
>   B3 is child of B2). This is how basically implementation with naming
>   scheme works. Well on this way we don't need special metadata in 
>   libvirt (besides may be domain xml attached to checkpoiint etc)
> 
> - in libvirt. If we save XML before creating a snapshot with checkpoint.
>   This fixes the issue with successful operation but saving XML failure.
>   But now we have another issue :) We can save XML successfully but then 
> operation
>   itself can fail and we fail to revert XML back. Well we can recover
>   even without child/parent metadata in qemu in this case. Just ask
>   qemu for bitmaps on libvirt restart and if bitmap is missing kick
>   it out as it is a case described above (successful saving XML then
>   unsuccessfull qemu operation)
> 

This option seems perfectly workable to me...

> So it is possible to track bitmaps in libvirt. We just need to be extra 
> carefull
> not to produce invalid backups.
> 
>>
>> Consider the case of internal snapshots.  Already, we have the case
>> where qemu itself does not track enough useful metadata about internal
>> snapshots (right now, just a name and timestamp of creation); so libvirt
>> additionally tracks further information in : the name,
>> timestamp, relationship to any previous snapshot (libvirt can then
>> reconstruct a tree relationship between all snapshots; where a parent
>> can have more than one child if you roll back to a snapshot and then
>> execute the guest differently), the set of disks participating in the
>> snapshot, and the  description at the time of the snapshot (if
>> you hotplug devices, or even the fact that creating external snapshots
>> changes which file is the active qcow2 in a backing chain, you'll need
>> to know how to roll back to the prior domain state as part of
>> reverting).  This is approximately the same set of information that a
>>  will need to track.
> 
> I would differentiate checkpoints and backups. For example in case
> of push backups we can store additional metadata in 
> so later we can revert back to previous state. But checkpoints 
> (bitmaps technically) are only to make incremental backups(restores?). 
> We can attach extra metadata to checkpoints but it looks accidental just 
> because
> bitmaps and backups relate to some same point in time. To me a backup (push)
> can carry all the metadata and as to checkpoints a backup can have
> associated checkpoint or not. For example if we choose to always
> make full backups we don't need checkpoints at all (at least if we are
> not going to use them for restore).
> 

Well ... if we create checkpoints alongside full backups, then you have
points to reference to create future incremental backups. You don't need
checkpoints if you *NEVER* use an incremental backup. If we want the
feature enabled, so to speak, you likely need to be making checkpoints
alongside full backups.

I'd 

Re: [libvirt] [PATCH 0/3] sample: vfio mdev display devices.

[Kirti Wankhede]

On 4/24/2018 3:10 AM, Alex Williamson wrote:
> On Wed, 18 Apr 2018 12:31:53 -0600
> Alex Williamson  wrote:
> 
>> On Mon,  9 Apr 2018 12:35:10 +0200
>> Gerd Hoffmann  wrote:
>>
>>> This little series adds three drivers, for demo-ing and testing vfio
>>> display interface code.  There is one mdev device for each interface
>>> type (mdpy.ko for region and mbochs.ko for dmabuf).  
>>
>> Erik Skultety brought up a good question today regarding how libvirt is
>> meant to handle these different flavors of display interfaces and
>> knowing whether a given mdev device has display support at all.  It
>> seems that we cannot simply use the default display=auto because
>> libvirt needs to specifically configure gl support for a dmabuf type
>> interface versus not having such a requirement for a region interface,
>> perhaps even removing the emulated graphics in some cases (though I
>> don't think we have boot graphics through either solution yet).
>> Additionally, GVT-g seems to need the x-igd-opregion support
>> enabled(?), which is a non-starter for libvirt as it's an experimental
>> option!
>>
>> Currently the only way to determine display support is through the
>> VFIO_DEVICE_QUERY_GFX_PLANE ioctl, but for libvirt to probe that on
>> their own they'd need to get to the point where they could open the
>> vfio device and perform the ioctl.  That means opening a vfio
>> container, adding the group, setting the iommu type, and getting the
>> device.  I was initially a bit appalled at asking libvirt to do that,
>> but the alternative is to put this information in sysfs, but doing that
>> we risk that we need to describe every nuance of the mdev device
>> through sysfs and it becomes a dumping ground for every possible
>> feature an mdev device might have.
>>

One or two sysfs file for each feature shouldn't be that much of over
head? In kernel, other subsystem modules expose capability through
sysfs, like PCI subsystem adds 'boot_vga' file for VGA device which
returns 0/1 depending on if its boot VGA device. Similarly
'd3cold_allowed', 'msi_bus'...

>> So I was ready to return and suggest that maybe libvirt should probe
>> the device to know about these ancillary configuration details, but
>> then I remembered that both mdev vGPU vendors had external dependencies
>> to even allow probing the device.  KVMGT will fail to open the device
>> if it's not associated with an instance of KVM and NVIDIA vGPU, I
>> believe, will fail if the vGPU manager process cannot find the QEMU
>> instance to extract the VM UUID.  (Both of these were bad ideas)
> 
> Here's another proposal that's really growing on me:
> 
>  * Fix the vendor drivers!  Allow devices to be opened and probed
>without these external dependencies.
>  * Libvirt uses the existing vfio API to open the device and probe the
>necessary ioctls, if it can't probe the device, the feature is
>unavailable, ie. display=off, no migration.
> 

I'm trying to think simpler mechanism using sysfs that could work for
any feature and knowing source-destination migration compatibility check
by libvirt before initiating migration.

I have another proposal:
* Add a ioctl VFIO_DEVICE_PROBE_FEATURES
struct vfio_device_features {
__u32 argsz;
__u32 features;
}

Define bit for each feature:
#define VFIO_DEVICE_FEATURE_DISPLAY_REGION  (1 << 0)
#define VFIO_DEVICE_FEATURE_DISPLAY_DMABUF  (1 << 1)
#define VFIO_DEVICE_FEATURE_MIGRATION   (1 << 2)

* Vendor driver returns bitmask of supported features during
initialization phase.

* In vfio core module, trap this ioctl for each device  in
vfio_device_fops_unl_ioctl(), check features bitmask returned by vendor
driver and add a sysfs file if feature is supported that device. This
sysfs file would return 0/1.

For migration this bit will only indicate if host driver supports
migration feature.

For source and destination compatibility check libvirt would need more
data/variables to check like,
* if same type of 'mdev_type' device create-able at destination,
   i.e. if ('mdev_type'->available_instances > 0)

* if host_driver_version at source and destination are compatible.
Host driver from same release branch should be mostly compatible, but if
there are major changes in structures or APIs, host drivers from
different branches might not be compatible, for example, if source and
destination are from different branches and one of the structure had
changed, then data collected at source might not be compatible with
structures at destination and typecasting it to changed structures would
mess up migrated data during restoration.

* if guest_driver_version is compatible with host driver at destination.
For mdev devices, guest driver communicates with host driver in some
form. If there are changes in structures/APIs of such communication,
guest driver at source might not be compatible with host driver at
destination.

'available_instances' sysfs already exist, later 

Re: [libvirt] [RFC v2] external (pull) backup API

[John Snow]

On 04/23/2018 05:31 AM, Vladimir Sementsov-Ogievskiy wrote:
> 21.04.2018 00:26, Eric Blake wrote:
>> On 04/20/2018 01:24 PM, John Snow wrote:
>>
> Why is option 3 unworkable, exactly?:
>
> (3) Checkpoints exist as structures only with libvirt. They are saved
> and remembered in the XML entirely.
>
> Or put another way:
>
> Can you explain to me why it's important for libvirt to be able to
> reconstruct checkpoint information from a qcow2 file?
>
 In short it take extra effort for metadata to be consistent when
 libvirtd crashes occurs. See for more detailed explanation
 in [1] starting from words "Yes it is possible".

 [1]
 https://www.redhat.com/archives/libvir-list/2018-April/msg01001.html
>> I'd argue the converse. Libvirt already knows how to do atomic updates
>> of XML files that it tracks.  If libvirtd crashes/restarts in the middle
>> of an API call, you already have indeterminate results of whether the
>> API worked or failed; once libvirtd is restarted, you'll have to
>> probably retry the command.  For all other cases, the API call
>> completes, and either no XML changes were made (the command failed and
>> reports the failure properly), or all XML changes were made (the command
>> created the appropriate changes to track the new checkpoint, including
>> whatever bitmap names have to be recorded to map the relation between
>> checkpoints and bitmaps).
>>
>> Consider the case of internal snapshots.  Already, we have the case
>> where qemu itself does not track enough useful metadata about internal
>> snapshots (right now, just a name and timestamp of creation); so libvirt
>> additionally tracks further information in : the name,
>> timestamp, relationship to any previous snapshot (libvirt can then
>> reconstruct a tree relationship between all snapshots; where a parent
>> can have more than one child if you roll back to a snapshot and then
>> execute the guest differently), the set of disks participating in the
>> snapshot, and the  description at the time of the snapshot (if
>> you hotplug devices, or even the fact that creating external snapshots
>> changes which file is the active qcow2 in a backing chain, you'll need
>> to know how to roll back to the prior domain state as part of
>> reverting).  This is approximately the same set of information that a
>>  will need to track.
>>
>> I'm slightly tempted to just overload  to track three
>> modes instead of two (internal, external, and now checkpoint); but think
>> that will probably be a bit too confusing, so more likely I will create
>>  as a new object, but copy a lot of coding paradigms
>> from .
>>
>> So, from that point of view, libvirt tracking the relationship between
>> qcow2 bitmaps in order to form checkpoint information can be done ALL
>> with libvirt, and without NEEDING the qcow2 file to track any relations
>> between bitmaps.  BUT, libvirt's job can probably be made easier if
>> qcow2 would, at the least, allow bitmaps to track their parent, and/or
>> provide APIs to easily merge a parent..intermediate..child chain of
>> related bitmaps to be merged into a single bitmap, for easy runtime
>> creation of the temporary bitmap used to express the delta between two
>> checkpoints.
> 
> I don't think this is a good idea:
> https://www.redhat.com/archives/libvir-list/2018-April/msg01306.html
> 
> In short, I think, if we do something to support checkpoints in qemu
> (updated BdrvDirtyBitmap, qapi, qcow2 and migration stream, new nbd meta
> context), we'd better implement checkpoints, than .parent relationship.
> 

[I'm going to answer this in response to the thread you've referenced.]

>>
>>> OK; I can't speak to the XML design (I'll leave that to Eric and other
>>> libvirt engineers) but the data consistency issues make sense.
>> And I'm still trying to figure out exactly what is needed, to capture
>> everything needed to create checkpoints and take backups (both push and
>> pull model).  Reverting to data from an external backup may be a bit
>> more manual, at least at first (after all, we STILL don't have decent
>> libvirt support for rolling back to external snapshots, several years
>> later).  In other words, my focus right now is "how can we safely track
>> checkpoints for capturing of point-in-time incremental backups with
>> minimal guest downtime", rather than "given an incremental backup
>> captured previously, how do we roll a guest back to that point in time".
>>
>>> ATM I am concerned that by shifting the snapshots into bitmap names that
>>> you still leave yourself open for data corruption if these bitmaps are
>>> modified outside of libvirt -- these third party tools can't possibly
>>> understand the schema that they were created under.
>>>
>>> (Though I suppose very simply that if a bitmap is missing you'd be able
>>> to detect that in libvirt and signal an error, but it's not very nice.)
>> Well, we also have to realize that third-party tools shouldn't 

Re: [libvirt] [PATCH v2 09/11] Remove check for duplicitous GenID in another VM

[John Ferlan]

On 04/24/2018 02:39 AM, Peter Krempa wrote:
> On Mon, Apr 23, 2018 at 20:00:03 -0400, John Ferlan wrote:
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_process.c | 68 
>> ++---
>>  1 file changed, 2 insertions(+), 66 deletions(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 06ec4ddeb9..bfc22b7d07 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -5880,73 +5880,14 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
>>  
>>  
>>  /**
>> - * qemuProcessCheckGenID:
>> - * @vm: Domain to be checked
>> - * @opaque: Domain about to be started
> 
> Did you forget to squash this into the previous patch?
> 

Yeah - I got totally distracted by all the capabilities adjustments that
I forgot to merge this one!

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 06/11] conf: Add VM Generation ID parse/format support

[John Ferlan]

On 04/24/2018 03:21 AM, Peter Krempa wrote:
> On Mon, Apr 23, 2018 at 20:00:00 -0400, John Ferlan wrote:
>> The VM Generation ID is a mechanism to provide a unique 128-bit,
>> cryptographically random, and integer value identifier known as
>> the GUID (Globally Unique Identifier) to the guest OS. The value
>> is used to help notify the guest operating system when the virtual
>> machine is executed with a different configuration.
>>
>> This patch adds support for a new "genid" XML element similar to
>> the "uuid" element. The "genid" element can have two forms ""
>> or "$GUID". If the $GUID is not provided, libvirt
>> will generate one.
>>
>> For the ABI checks add avoidance for the genid comparison if the
>> appropriate flag is set.
>>
>> Since adding support for a generated GUID (or UUID like) value to
>> be displayed only for an active guest, modifying the xml2xml test
>> to include virrandommock.so is necessary since it will generate a
>> "known" UUID value that can be compared against for the active test.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  docs/formatdomain.html.in| 29 
>>  docs/schemas/domaincommon.rng|  8 
>>  src/conf/domain_conf.c   | 59 
>> 
>>  src/conf/domain_conf.h   |  3 ++
>>  tests/qemuxml2argvdata/genid-auto.xml| 32 +
>>  tests/qemuxml2argvdata/genid.xml | 32 +
>>  tests/qemuxml2xmloutdata/genid-active.xml| 32 +
>>  tests/qemuxml2xmloutdata/genid-auto-active.xml   | 32 +
>>  tests/qemuxml2xmloutdata/genid-auto-inactive.xml | 32 +
>>  tests/qemuxml2xmloutdata/genid-inactive.xml  | 32 +
>>  tests/qemuxml2xmltest.c  |  5 +-
>>  11 files changed, 295 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/qemuxml2argvdata/genid-auto.xml
>>  create mode 100644 tests/qemuxml2argvdata/genid.xml
>>  create mode 100644 tests/qemuxml2xmloutdata/genid-active.xml
>>  create mode 100644 tests/qemuxml2xmloutdata/genid-auto-active.xml
>>  create mode 100644 tests/qemuxml2xmloutdata/genid-auto-inactive.xml
>>  create mode 100644 tests/qemuxml2xmloutdata/genid-inactive.xml
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index ada0df227f..6a140f3e40 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -34,6 +34,7 @@
>>  domain type='kvm' id='1'
>>nameMyGuest/name
>>uuid4dea22b3-1d52-d8f3-2516-782e98ab3fa0/uuid
>> +  genid43dc0cf8-809b-4adb-9bea-a9abb5f3d90e/genid
> 
> Since we encourage to use the variant with this being empty I'd show
> that here.
> 

I'd agree except for the fact this is a "live" XML example since
"id='1'", so it should stay as is unless of course it's desired to never
show the GUID for the running VM.

>>titleA short description - title - of the domain/title
>>descriptionSome human readable description/description
>>metadata
>> @@ -61,6 +62,34 @@
>>  specification. Since 0.0.1, sysinfo
>>  since 0.8.7
>>  
>> +  genid
>> +  Since 4.3.0, the genid
>> +element can be used to add a Virtual Machine Generation ID which
>> +exposes a 128-bit, cryptographically random, integer value 
>> identifier,
>> +referred to as a Globally Unique Identifier (GUID) using the same
>> +format as the uuid. The value is used to help notify
>> +the guest operating system when the virtual machine is executed
>> +with a different configuration, such as:
>> +
>> +
>> +  snapshot execution
>> +  backup recovery
>> +  failover in a disaster recovery environment
>> +  creation from template (import, copy, clone)
>> +
>> +
>> +The guest operating system notices the change and is then able to
>> +react as appropriate by marking its copies of distributed databases
>> +as dirty, re-initializing its random number generator, etc.
>> +
>> +
>> +When a GUID value is not provided, e.g. using the XML syntax
>> +genid/, then libvirt will automatically generate a GUID.
>> +This is the recommended configuration since the hypervisor then
>> +can handle changing the GUID value for specific state transitions.
>> +Using a static GUID value may result in failures for starting from
>> +snapshot, restoring from backup, starting a cloned domain, 
>> etc.
>> +
>>title
>>The optional element title provides space for a
>>  short description of the domain. The title should not contain
> 
> [...]
> 
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 6d4db50998..8c30adf850 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -18793,6 +18793,34 @@ virDomainDefParseXML(xmlDocPtr xml,
>>  

[libvirt] [PATCH] remote: disable unused function on win32 platform build

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---

Pushed as trivial build fix

 src/remote/remote_driver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 3973b37d5f..95437b4365 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -712,6 +712,7 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn,
 }
 
 
+#ifndef WIN32
 static char *remoteGetUNIXSocketNonRoot(void)
 {
 char *sockname = NULL;
@@ -729,6 +730,7 @@ static char *remoteGetUNIXSocketNonRoot(void)
 VIR_DEBUG("Chosen UNIX sockname %s", sockname);
 return sockname;
 }
+#endif /* WIN32 */
 
 static char *remoteGetUNIXSocketRoot(unsigned int flags)
 {
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 05/11] conf: Add flag to regenerate genid for virDomainDefCopy

[John Ferlan]

On 04/24/2018 03:24 AM, Peter Krempa wrote:
> On Mon, Apr 23, 2018 at 19:59:59 -0400, John Ferlan wrote:
>> Add the VIR_DOMAIN_DEF_COPY_NEWGENID to indicate that the generated
>> domain definition should adjust the genid value before returning the
>> @def to the caller.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/conf/domain_conf.c | 12 
>>  src/conf/domain_conf.h |  5 +
>>  src/qemu/qemu_driver.c |  3 ++-
>>  src/test/test_driver.c |  3 ++-
>>  4 files changed, 21 insertions(+), 2 deletions(-)
> 
> IMO all this code should not be necessary. Code paths which knowingly
> need to change the GENID should do so explicitly rather than pulling the
> logic here.
> 

Honestly, I found it easier to have the code in one place rather than
cut-copy-paste in two places... But I can move it out.

However, I may have forgotten a transition though w/r/t domain copy.
Unfortunately clone could be a bugger since it uses parse and format,
but if startup always generates one, then that's perhaps covered.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/3] sample: vfio mdev display devices.

[Alex Williamson]

On Tue, 24 Apr 2018 09:17:37 +0200
Gerd Hoffmann  wrote:

>   Hi,
> 
> > Here's another proposal that's really growing on me:
> > 
> >  * Fix the vendor drivers!  Allow devices to be opened and probed
> >without these external dependencies.  
> 
> Hmm.  If you try use gvt with tcg then, wouldn't qemu think "device
> probed ok, all green" then even though that isn't the case?

Well, is there a way to make it work with tcg?  That would be the best
solution.  Perhaps KVM could be handled as an accelerator rather than a
required component.  I don't really understand how the page tracking
interface is used and why it's not required by NVIDIA if it's so
fundamental to GVT-g.  Otherwise, are there other points at which the
device could refuse to be enabled, for instance what if the write to
enable bus-master in the PCI command register returned an error if the
device isn't fully configured.  Paolo had suggested offline that maybe
there could be a read-only mode of the device that allows probing.  I
think that would be a fair bit of work and complexity to support, but
I'm open to those sorts of ideas.  I can't be sure the NVIDIA
requirement isn't purely for accounting purposes within their own
proprietary userspace manager, without any real technical requirement.
Hoping Intel and NVIDIA can comment on these so we really understand
why these are in place before we bend over backwards for a secondary API
interface. Thanks,

Alex

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: improve virNetDevTapGetRealDeviceName

[Roman Bogorodskiy]

  Michal Privoznik wrote:

> On 04/08/2018 05:40 PM, Roman Bogorodskiy wrote:
> > virNetDevTapGetRealDeviceName() is used on FreeBSD because interface
> > names (such as one sees in output of tools like ifconfig(8)) might not
> > match their /dev entity names, and for bhyve we need the latter.
> > 
> > Current implementation is not very efficient because in order to find
> > /dev name, it goes through all /dev/tap* entries and tries to issue
> > TAPGIFNAME ioctl on it. Not only this is slow, but also there's a bug in
> > this implementation when more than one NIC is passed to a VM: once we
> > find the tap interface we're looking for, we set its state to UP because
> > opening it for issuing ioctl sets it DOWN, even if it was UP before.
> > When we have more than 1 NIC for a VM, we have only last one UP because
> > others remain DOWN after unsuccessful attempts to match interface name.
> > 
> > New implementation just uses sysctl(3), so it should be faster and
> > won't make interfaces go down to get name.
> > ---
> >  src/util/virnetdevtap.c | 71 
> > +
> >  1 file changed, 30 insertions(+), 41 deletions(-)
> 
> ACK with this squashed in:
> 
> diff --git i/src/util/virnetdevtap.c w/src/util/virnetdevtap.c
> index afe4f0b3c1..bd0710ad2e 100644
> --- i/src/util/virnetdevtap.c
> +++ w/src/util/virnetdevtap.c
> @@ -40,7 +40,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -129,13 +128,14 @@ virNetDevTapGetRealDeviceName(char *ifname 
> ATTRIBUTE_UNUSED)
>  return NULL;
>  }
>  
> -ret = malloc(len);
> +if (VIR_ALLOC_N(ret, len) < 0)
> +return NULL;
>  
>  if (sysctl(name, 6, ret, , 0, 0) < 0) {
>  virReportSystemError(errno,
>   _("Unable to get driver name for '%s'"),
>   ifname);
> -free(ret);
> +VIR_FREE(ret);
>  return NULL;
>  }
>  
> 
> 
> Michal

Pushed, thanks!

Roman Bogorodskiy


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/2] po: clean up some language translations

[Daniel P . Berrangé]

On Tue, Apr 24, 2018 at 04:15:16PM +0100, Daniel P. Berrangé wrote:
> First we purge some translated strings that are no longer required, then
> we refresh from Zanata to purge bogus non-translations.
> 
> Daniel P. Berrangé (2):
>   Refresh translations to drop unused strings
>   po: delete bogus translations from various languages

Hmm, I guess even with the minimization these still come out rather
large, so get blocked on the list. In the unlikely event someone
wants to "review" it, they're here:

  https://github.com/berrange/libvirt/commits/po-file


otherwise I'll push the changes tomorrow


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/2] po: clean up some language translations

[Daniel P . Berrangé]

First we purge some translated strings that are no longer required, then
we refresh from Zanata to purge bogus non-translations.

Daniel P. Berrangé (2):
  Refresh translations to drop unused strings
  po: delete bogus translations from various languages

 po/af.mini.po   |   2 +-
 po/am.mini.po   |   2 +-
 po/anp.mini.po  |   2 +-
 po/ar.mini.po   | 744 +---
 po/as.mini.po   | 520 +---
 po/ast.mini.po  |   2 +-
 po/bal.mini.po  |   2 +-
 po/be.mini.po   |   2 +-
 po/bg.mini.po   |  26 +-
 po/bn.mini.po   | 743 +--
 po/bn_IN.mini.po| 287 +---
 po/bo.mini.po   |   2 +-
 po/br.mini.po   |   2 +-
 po/brx.mini.po  |   2 +-
 po/bs.mini.po   |  20 +-
 po/ca.mini.po   |  23 +-
 po/cs.mini.po   |  67 +
 po/cy.mini.po   | 744 +---
 po/da.mini.po   |  20 +-
 po/de.mini.po   | 549 +-
 po/de_CH.mini.po|   2 +-
 po/el.mini.po   |   2 +-
 po/en_GB.mini.po| 525 +---
 po/eo.mini.po   |   2 +-
 po/es.mini.po   | 548 +-
 po/et.mini.po   | 743 +--
 po/eu.mini.po   | 743 +--
 po/fa.mini.po   |   2 +-
 po/fi.mini.po   |  23 +-
 po/fil.mini.po  |   2 +-
 po/fr.mini.po   |  67 +
 po/fur.mini.po  |   2 +-
 po/ga.mini.po   |   2 +-
 po/gl.mini.po   | 743 +--
 po/gu.mini.po   | 524 +---
 po/he.mini.po   | 743 +--
 po/hi.mini.po   | 362 +
 po/hr.mini.po   |   2 +-
 po/hu.mini.po   |  20 +-
 po/ia.mini.po   |   2 +-
 po/id.mini.po   |   8 +-
 po/ilo.mini.po  |   2 +-
 po/is.mini.po   | 743 +--
 po/it.mini.po   | 270 +--
 po/ja.mini.po   | 553 +-
 po/ka.mini.po   | 743 +--
 po/kk.mini.po   |   2 +-
 po/km.mini.po   |   2 +-
 po/kn.mini.po   | 532 +
 po/ko.mini.po   | 323 +--
 po/kw.mini.po   |   2 +-
 po/k...@kkcor.mini.po |   2 +-
 po/k...@uccor.mini.po |   2 +-
 po/kw_GB.mini.po|   2 +-
 po/ky.mini.po   |   2 +-
 po/lt.mini.po   | 743 +--
 po/lv.mini.po   | 743 +--
 po/mai.mini.po  |   2 +-
 po/mk.mini.po   |  20 +-
 po/ml.mini.po   | 526 +
 po/mn.mini.po   |   2 +-
 po/mr.mini.po   | 544 +-
 po/ms.mini.po   |   2 +-
 po/my.mini.po   |   2 +-
 po/nb.mini.po   |   2 +-
 po/nds.mini.po  |   2 +-
 po/ne.mini.po   |   2 +-
 po/nl.mini.po   | 349 +---
 po/nn.mini.po   | 743 +--
 po/nso.mini.po  | 743 +--
 po/or.mini.po   | 446 +--
 po/pa.mini.po   | 515 +---
 po/pl.mini.po   | 356 +
 po/pt.mini.po   |  29 +-
 po/pt_BR.mini.po| 545 +-
 po/ro.mini.po   | 743 +--
 po/ru.mini.po   | 522 +---
 po/si.mini.po   | 743 +--
 po/sk.mini.po   | 743 +--
 po/sl.mini.po   | 743 +--
 po/sq.mini.po   |   2 +-
 po/sr.mini.po   | 125 +
 po/s...@latin.mini.po | 125 +
 po/sv.mini.po   |  27 +-
 po/ta.mini.po   | 545 +-
 po/te.mini.po   | 516 +---
 po/tg.mini.po   |   2 +-
 po/th.mini.po   | 743 +--
 po/tr.mini.po   | 743 +--
 po/tw.mini.po   |   2 +-
 po/uk.mini.po   | 628 +---
 po/ur.mini.po   | 743 +--
 po/vi.mini.po   | 315 +-
 po/wba.mini.po  |   2 +-
 po/yo.mini.po   |   2 +-
 po/zh_CN.mini.po| 519 +---
 po/zh_HK.mini.po|   2 +-
 po/zh_TW.mini.po|  20 +-
 po/zu.mini.po   | 743 

Re: [libvirt] [PATCH rust] git: add config file telling git-publish how to send patches

[Ján Tomko]

On Mon, Apr 23, 2018 at 02:32:11PM +0100, Daniel P. Berrangé wrote:

On Mon, Apr 23, 2018 at 03:20:53PM +0200, Ján Tomko wrote:

I found the reverse order, i.e.
'rust PATCH'
to be both nicer and more useful. (not to mention more common)

For example, in some downstream lists I use a simple text match
against 'libvirt PATCH', which also catches any subsequent version


I'm not really seeing why it would make any functional difference
to ability to match subjects.



I assume in v2 it would become 'PATCHv2 libvirt', which does not contain
the 'PATCH libvirt' string, unlike 'libvirt PATCHv2' and 'libvirt
PATCH'

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [rust PATCH] travis: fix syntax for script commands

[Daniel P . Berrangé]

On Tue, Apr 24, 2018 at 02:48:29PM +0200, Sahid Orentino Ferdjaoui wrote:
> On Tue, Apr 24, 2018 at 01:33:20PM +0100, Daniel P. Berrangé wrote:
> > The script: commands were listed without a leading '-' which caused
> > travis to concatenate them into a single command. This meant the second
> > command became a set of arguments to the first command. Historically
> > cargo ignored these extra args so the mistake was not noticed, but it
> > now generates a fatal error.
> >
> 
> Thanks for this Daniel. Do you want me to merge it or are you going to
> take care of that too?

I'll push it - was just posting for review

> 
> Reviewed-by: Sahid Orentino Ferdjaoui 
> 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  .travis.yml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/.travis.yml b/.travis.yml
> > index f9844bb..c52f745 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -40,5 +40,5 @@ install:
> >- echo "pass" | sudo saslpasswd2 -p -a libvirt user
> >  
> >  script:
> > -  cargo test --verbose
> > -  cargo test --verbose -- --ignored
> > +  - cargo test --verbose
> > +  - cargo test --verbose -- --ignored
> > -- 
> > 2.14.3

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [rust PATCH] travis: fix syntax for script commands

[Sahid Orentino Ferdjaoui]

On Tue, Apr 24, 2018 at 01:33:20PM +0100, Daniel P. Berrangé wrote:
> The script: commands were listed without a leading '-' which caused
> travis to concatenate them into a single command. This meant the second
> command became a set of arguments to the first command. Historically
> cargo ignored these extra args so the mistake was not noticed, but it
> now generates a fatal error.
>

Thanks for this Daniel. Do you want me to merge it or are you going to
take care of that too?

Reviewed-by: Sahid Orentino Ferdjaoui 

> Signed-off-by: Daniel P. Berrangé 
> ---
>  .travis.yml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index f9844bb..c52f745 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -40,5 +40,5 @@ install:
>- echo "pass" | sudo saslpasswd2 -p -a libvirt user
>  
>  script:
> -  cargo test --verbose
> -  cargo test --verbose -- --ignored
> +  - cargo test --verbose
> +  - cargo test --verbose -- --ignored
> -- 
> 2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2] qemu: Add I/O thread support info into domain capabilities

[Martin Kletzander]

Signed-off-by: Martin Kletzander 
---
 docs/schemas/domaincaps.rng   | 10 ++
 src/conf/domain_capabilities.c|  3 +++
 src/conf/domain_capabilities.h|  1 +
 src/qemu/qemu_capabilities.c  | 11 +++
 tests/domaincapsschemadata/basic.xml  |  1 +
 tests/domaincapsschemadata/full.xml   |  1 +
 tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml  |  1 +
 .../domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml |  1 +
 tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.12.0.s390x.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml |  1 +
 .../domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml |  1 +
 tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml   |  1 +
 tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.7.0.s390x.xml   |  1 +
 tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.8.0.s390x.xml   |  1 +
 tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml  |  1 +
 22 files changed, 43 insertions(+)

diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
index 39053181eb9a..049b2ae7b64f 100644
--- a/docs/schemas/domaincaps.rng
+++ b/docs/schemas/domaincaps.rng
@@ -28,6 +28,9 @@
 
   
 
+
+  
+
 
   
 
@@ -53,6 +56,13 @@
 
   
 
+  
+
+  
+  
+
+  
+
   
 
   
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index bebbaf44d00e..f18bea99d63f 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -562,6 +562,9 @@ virDomainCapsFormat(virDomainCapsPtr const caps)
 if (caps->maxvcpus)
 virBufferAsprintf(, "\n", caps->maxvcpus);
 
+virBufferAsprintf(, "\n",
+  caps->iothreads ? "yes" : "no");
+
 virDomainCapsOSFormat(, >os);
 virDomainCapsCPUFormat(, >cpu);
 
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index fa4c1e442f57..3b5ce214d636 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -147,6 +147,7 @@ struct _virDomainCaps {
 
 /* Some machine specific info */
 int maxvcpus;
+bool iothreads;  /* Whether I/O threads are supported or not. */
 
 virDomainCapsOS os;
 virDomainCapsCPU cpu;
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 833c75514c25..00634abb1649 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4632,6 +4632,16 @@ virQEMUCapsFillDomainCPUCaps(virCapsPtr caps,
 }
 
 
+static int
+virQEMUCapsFillDomainIOThreadCaps(virQEMUCapsPtr qemuCaps,
+  virDomainCapsPtr domCaps)
+{
+domCaps->iothreads = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD);
+
+return 0;
+}
+
+
 static int
 virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps,
 const char *machine,
@@ -4866,6 +4876,7 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps,
 
 if (virQEMUCapsFillDomainOSCaps(os, firmwares, nfirmwares) < 0 ||
 virQEMUCapsFillDomainCPUCaps(caps, qemuCaps, domCaps) < 0 ||
+virQEMUCapsFillDomainIOThreadCaps(qemuCaps, domCaps) < 0 ||
 virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps,
 domCaps->machine, disk) < 0 ||
 virQEMUCapsFillDomainDeviceGraphicsCaps(qemuCaps, graphics) < 0 ||
diff --git a/tests/domaincapsschemadata/basic.xml 
b/tests/domaincapsschemadata/basic.xml
index 6b788d9144c8..7f9582430a7a 100644
--- a/tests/domaincapsschemadata/basic.xml
+++ b/tests/domaincapsschemadata/basic.xml
@@ -3,6 +3,7 @@
   uml
   my-machine-type
   x86_64
+  
   
   
 
diff --git a/tests/domaincapsschemadata/full.xml 
b/tests/domaincapsschemadata/full.xml
index ab6ef9f2ef17..b97bc883262a 100644
--- a/tests/domaincapsschemadata/full.xml
+++ b/tests/domaincapsschemadata/full.xml
@@ -4,6 +4,7 @@
   my-machine-type
   x86_64
   
+  
   
 
   /foo/bar
diff --git a/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml 
b/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml
index 8d1ad865703e..d71d948477ef 100644
--- a/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml
@@ -4,6 +4,7 @@
   pc-i440fx-1.7
   x86_64
   
+  
   
 
   /usr/share/AAVMF/AAVMF_CODE.fd
diff --git a/tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml 
b/tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml
index 9cba942fbf8e..9feceeea74c6 

[libvirt] [rust PATCH] travis: fix syntax for script commands

[Daniel P . Berrangé]

The script: commands were listed without a leading '-' which caused
travis to concatenate them into a single command. This meant the second
command became a set of arguments to the first command. Historically
cargo ignored these extra args so the mistake was not noticed, but it
now generates a fatal error.

Signed-off-by: Daniel P. Berrangé 
---
 .travis.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index f9844bb..c52f745 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -40,5 +40,5 @@ install:
   - echo "pass" | sudo saslpasswd2 -p -a libvirt user
 
 script:
-  cargo test --verbose
-  cargo test --verbose -- --ignored
+  - cargo test --verbose
+  - cargo test --verbose -- --ignored
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 6/6] conf: Clean up object referencing for Add and Remove

[John Ferlan]

When adding a new object to the domain object list, there should
have been 2 virObjectRef calls made one for each list into which
the object was placed to match the 2 virObjectUnref calls that
would occur during Remove as part of virHashRemoveEntry when
virObjectFreeHashData is called when the element is removed from
the hash table as set up in virDomainObjListNew.

Some drivers (libxl, lxc, qemu, and vz) handled this inconsistency
by calling virObjectRef upon successful return from virDomainObjListAdd
in order to use virDomainObjEndAPI when done with the returned @vm.
While others (bhyve, openvz, test, and vmware) handled this via only
calling virObjectUnlock upon successful return from virDomainObjListAdd.

This patch will "unify" the approach to use virDomainObjEndAPI
for any @vm successfully returned from virDomainObjListAdd.

Because list removal is so tightly coupled with list addition,
this patch fixes the list removal algorithm to return the object
as entered - "locked and reffed".  This way, the callers can then
decide how to uniformly handle add/remove success and failure.
This removes the onus on the caller to "specially handle" the
@vm during removal processing.

The Add/Remove logic allows for some logic simplification such
as in libxl where we can Remove the @vm directly rather than
needing to set a @remove_dom boolean and removing after the
libxlDomainObjEndJob completes as the @vm is locked/reffed.

Signed-off-by: John Ferlan 
---
 src/bhyve/bhyve_driver.c| 21 +--
 src/conf/virdomainobjlist.c | 33 +++
 src/libxl/libxl_driver.c| 64 +
 src/libxl/libxl_migration.c | 31 +-
 src/lxc/lxc_driver.c| 21 ---
 src/lxc/lxc_process.c   | 17 ++--
 src/openvz/openvz_conf.c|  2 +-
 src/openvz/openvz_driver.c  | 20 +-
 src/qemu/qemu_domain.c  | 17 +---
 src/qemu/qemu_driver.c  |  6 +
 src/qemu/qemu_migration.c   |  3 +--
 src/test/test_driver.c  | 56 ---
 src/uml/uml_driver.c| 37 +++---
 src/vmware/vmware_conf.c|  3 +--
 src/vmware/vmware_driver.c  | 17 
 src/vz/vz_driver.c  |  1 -
 src/vz/vz_sdk.c |  3 ---
 17 files changed, 99 insertions(+), 253 deletions(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 768578a43f..6001f0806c 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -550,7 +550,6 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flag
 if (virDomainSaveConfig(BHYVE_CONFIG_DIR, caps,
 vm->newDef ? vm->newDef : vm->def) < 0) {
 virDomainObjListRemove(privconn->domains, vm);
-vm = NULL;
 goto cleanup;
 }
 
@@ -566,8 +565,7 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flag
 virObjectUnref(caps);
 virDomainDefFree(def);
 virDomainDefFree(oldDef);
-if (vm)
-virObjectUnlock(vm);
+virDomainObjEndAPI();
 if (event)
 virObjectEventStateQueue(privconn->domainEventState, event);
 
@@ -609,12 +607,10 @@ bhyveDomainUndefine(virDomainPtr domain)
   VIR_DOMAIN_EVENT_UNDEFINED,
   
VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
 
-if (virDomainObjIsActive(vm)) {
+if (virDomainObjIsActive(vm))
 vm->persistent = 0;
-} else {
+else
 virDomainObjListRemove(privconn->domains, vm);
-virObjectLock(vm);
-}
 
 ret = 0;
 
@@ -958,10 +954,8 @@ bhyveDomainCreateXML(virConnectPtr conn,
  VIR_DOMAIN_RUNNING_BOOTED,
  start_flags) < 0) {
 /* If domain is not persistent, remove its data */
-if (!vm->persistent) {
+if (!vm->persistent)
 virDomainObjListRemove(privconn->domains, vm);
-vm = NULL;
-}
 goto cleanup;
 }
 
@@ -974,8 +968,7 @@ bhyveDomainCreateXML(virConnectPtr conn,
  cleanup:
 virObjectUnref(caps);
 virDomainDefFree(def);
-if (vm)
-virObjectUnlock(vm);
+virDomainObjEndAPI();
 if (event)
 virObjectEventStateQueue(privconn->domainEventState, event);
 
@@ -1007,10 +1000,8 @@ bhyveDomainDestroy(virDomainPtr dom)
   VIR_DOMAIN_EVENT_STOPPED,
   
VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
 
-if (!vm->persistent) {
+if (!vm->persistent)
 virDomainObjListRemove(privconn->domains, vm);
-virObjectLock(vm);
-}
 
  cleanup:
 virDomainObjEndAPI();
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 5725040552..a30b3908f0 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -226,9 +226,13 @@ 

[libvirt] [PATCH 3/6] conf: Move and use virDomainObjListRemoveLocked

[John Ferlan]

Rather than open code within virDomainObjListRemove, just call
the *Locked function.

Additionally, add comments to virDomainObjListRemove to describe
the usage model.

Signed-off-by: John Ferlan 
---
 src/conf/virdomainobjlist.c | 64 +
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 6752f6c572..5725040552 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -355,26 +355,50 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr 
doms,
 }
 
 
-/*
- * The caller must hold a lock on the driver owning 'doms',
- * and must also have locked 'dom', to ensure no one else
- * is either waiting for 'dom' or still using it
+/* The caller must hold lock on 'doms' in addition to 'virDomainObjListRemove'
+ * requirements
+ *
+ * Can be used to remove current element while iterating with
+ * virDomainObjListForEach
  */
-void virDomainObjListRemove(virDomainObjListPtr doms,
-virDomainObjPtr dom)
+void
+virDomainObjListRemoveLocked(virDomainObjListPtr doms,
+ virDomainObjPtr dom)
 {
 char uuidstr[VIR_UUID_STRING_BUFLEN];
 
-dom->removing = true;
 virUUIDFormat(dom->def->uuid, uuidstr);
-virObjectRef(dom);
-virObjectUnlock(dom);
 
-virObjectRWLockWrite(doms);
-virObjectLock(dom);
 virHashRemoveEntry(doms->objs, uuidstr);
 virHashRemoveEntry(doms->objsName, dom->def->name);
 virObjectUnlock(dom);
+}
+
+
+/**
+ * @doms: Pointer to the domain object list
+ * @dom: Domain pointer from either after Add or FindBy* API where the
+ *   @dom was successfully added to both the doms->objs and ->objsName
+ *   hash tables that now would need to be removed.
+ *
+ * The caller must hold a lock on the driver owning 'doms',
+ * and must also have locked and ref counted 'dom', to ensure
+ * no one else is either waiting for 'dom' or still using it.
+ *
+ * When this function returns, @dom will be removed from the hash
+ * tables, unlocked, and returned with the refcnt that was present
+ * upon entry.
+ */
+void
+virDomainObjListRemove(virDomainObjListPtr doms,
+   virDomainObjPtr dom)
+{
+dom->removing = true;
+virObjectRef(dom);
+virObjectUnlock(dom);
+virObjectRWLockWrite(doms);
+virObjectLock(dom);
+virDomainObjListRemoveLocked(doms, dom);
 virObjectUnref(dom);
 virObjectRWUnlock(doms);
 }
@@ -446,24 +470,6 @@ virDomainObjListRename(virDomainObjListPtr doms,
 return ret;
 }
 
-/* The caller must hold lock on 'doms' in addition to 'virDomainObjListRemove'
- * requirements
- *
- * Can be used to remove current element while iterating with
- * virDomainObjListForEach
- */
-void virDomainObjListRemoveLocked(virDomainObjListPtr doms,
-  virDomainObjPtr dom)
-{
-char uuidstr[VIR_UUID_STRING_BUFLEN];
-
-virUUIDFormat(dom->def->uuid, uuidstr);
-
-virHashRemoveEntry(doms->objs, uuidstr);
-virHashRemoveEntry(doms->objsName, dom->def->name);
-virObjectUnlock(dom);
-}
-
 
 static virDomainObjPtr
 virDomainObjListLoadConfig(virDomainObjListPtr doms,
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/6] conf: Use virDomainObjListFindBy*Locked for virDomainObjListAdd

[John Ferlan]

Use the FindBy{UUID|Name}Locked helpers which will return a locked
and ref counted object rather than the direct virHashLookup and
virObjectLock of the returned object. We'll need to temporarily
virObjectUnref when we assign a new domain @def, but that will
change shortly when virDomainObjListAddObjLocked returns the
correct reference counted object.

Use the virDomainObjEndAPI in the error path to Unref/Unlock for
the corresponding Unref/Unlock of either the FindBy* return or
the virDomainObjNew since both return a reffed/locked object.

Signed-off-by: John Ferlan 
---
 src/conf/virdomainobjlist.c | 22 +-
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 9aa2abd8c3..6752f6c572 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -280,11 +280,8 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
 if (oldDef)
 *oldDef = NULL;
 
-virUUIDFormat(def->uuid, uuidstr);
-
 /* See if a VM with matching UUID already exists */
-if ((vm = virHashLookup(doms->objs, uuidstr))) {
-virObjectLock(vm);
+if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) {
 /* UUID matches, but if names don't match, refuse it */
 if (STRNEQ(vm->def->name, def->name)) {
 virUUIDFormat(vm->def->uuid, uuidstr);
@@ -314,10 +311,12 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
   def,
   !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE),
   oldDef);
+/* XXX: Temporary until this API is fixed to return a locked and
+ *  refcnt'd object */
+virObjectUnref(vm);
 } else {
 /* UUID does not match, but if a name matches, refuse it */
-if ((vm = virHashLookup(doms->objsName, def->name))) {
-virObjectLock(vm);
+if ((vm = virDomainObjListFindByNameLocked(doms, def->name))) {
 virUUIDFormat(vm->def->uuid, uuidstr);
 virReportError(VIR_ERR_OPERATION_FAILED,
_("domain '%s' already exists with uuid %s"),
@@ -329,18 +328,15 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
 goto cleanup;
 vm->def = def;
 
-if (virDomainObjListAddObjLocked(doms, vm) < 0) {
-virDomainObjEndAPI();
-return NULL;
-}
+if (virDomainObjListAddObjLocked(doms, vm) < 0)
+goto error;
 }
  cleanup:
 return vm;
 
  error:
-virObjectUnlock(vm);
-vm = NULL;
-goto cleanup;
+virDomainObjEndAPI();
+return NULL;
 }
 
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration

[John Ferlan]

Since the @dconn reference via args->conn will be used via a thread
or callback, let's make sure memory associated with it isn't free'd
unexpectedly before we use it. The Unref will be done when the object
is Dispose'd.

Signed-off-by: John Ferlan 
---
 src/libxl/libxl_migration.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 7fe352306c..d37a4a687a 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
 
 libxlMigrationCookieFree(args->migcookie);
 VIR_FREE(args->socks);
+virObjectUnref(args->conn);
 virObjectUnref(args->vm);
 }
 
@@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
 if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
 goto error;
 
-args->conn = dconn;
+args->conn = virObjectRef(dconn);
 args->vm = virObjectRef(vm);
 args->flags = flags;
 args->migcookie = mig;
@@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
 if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
 goto error;
 
-args->conn = dconn;
+args->conn = virObjectRef(dconn);
 args->vm = virObjectRef(vm);
 args->flags = flags;
 args->socks = socks;
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/6] Complete cleanup of domain object usage

[John Ferlan]

This should be the "last time" (famous last words) needing to alter
this processing. I hope to get more than one pair of eyes looking
at the series. I've looked at it long enough to feel comfortable
with the changes, but I also know I probably have looked at it
so long that I could have missed something!

The first 3 patches set up the necessary infrastructure. Patches
4 & 5 are just something I found in libxl while doing this - they
could be extracted and considered separately if need be; however,
since I was thinking about @vm referencing - it just seemed to fit.
Since I altered @vm it only seemed right to do the same for @conn.
While it could be said that neither condition could happen - I
figured better safe than sorry and I think it follows how this type
of logic has been handled elsewhere in qemu.

Patch 6 is where all the magic happens. I tried a few different ways
to rework and split the Add/Remove processing, but either way I was
left with rather ugly patches that required touching the same places,
so I capitulated and combined them since there really is a delicate
balance between those paths that have added a virObjectRef after the
virDomainObjListAdd and those that just use virObjectUnlock on the
returned ListAdd objects. Converting the latter to use Ref caused more
"busy work" and equally large patches. The busy work was mosting around
the processing required during ListAdd if something happened which
resulted in ListRemove needing to be called.

The "general change" is that instead of having ListAdd return an object
with 2 refs and 1 lock it will now return an object with 3 refs and 1
lock. With that returned object, some drivers would perform an ObjectRef
while others did not. For those that did not, they would use ObjectUnlock
when done with the object so that when leaving whatever Create routine
there was there would be at least 2 Refs on the object. The change here
is to have them all be consistent. What follows is a "general description"
of what was done for each.

For bhyve, openvz, test, uml, and vmware (e.g. domain drivers that
do not use virObjectRef on the returned virDomainObjListAdd object):

  Create/Add processing:

-> Use EndAPI instead of ObjectUnlock
-> Remove setting vm = NULL if the ListRemove was called to allow
   EndAPI processing to perform last Unref and Unlock

   NB: ListRemove would remove 2 refs and unlock, so it "worked",
   but would "consume" the returned object

  Undefine/Destroy processing:

-> Alter ListRemove and ObjectLock to just be ListRemove allowing the
   EndAPI processing from a FindBy* to perform last Unref and Unlock

For libxl, lxc, qemu, and vz (e.g. domain drivers that use virObjectRef
on the returned virDomainObjListAdd object):

  Create/Add processing:

-> Remove the ObjectRef
-> Remove the ObjectLock when needing to call ListRemove to allow
   EndAPI processing to perform the last Unref and Unlock

   NB: ListRemove no longer Unlock's the returned object

  Undefine/Destroy processing:

-> Alter ListRemove and ObjectLock to just be ListRemove allowing the
   EndAPI processing from a FindBy* to perform last Unref and Unlock

NB: For the AutoDestroy type logic, no change is required since in the
long run the object was left with the correct number of refs after
create processing ran. The issue is more the direct mgmt of object
during Add/Remove rather than mgmt of object once defined.

John Ferlan (6):
  conf: Split FindBy{UUID|Name} into locked helpers
  conf: Use virDomainObjListFindBy*Locked for virDomainObjListAdd
  conf: Move and use virDomainObjListRemoveLocked
  libxl: Add refcnt for args->vm during migration
  libxl: Add refcnt for args->conn during migration
  conf: Clean up object referencing for Add and Remove

 src/bhyve/bhyve_driver.c|  21 ++
 src/conf/virdomainobjlist.c | 165 
 src/libxl/libxl_driver.c|  64 -
 src/libxl/libxl_migration.c |  41 ---
 src/lxc/lxc_driver.c|  21 ++
 src/lxc/lxc_process.c   |  17 +++--
 src/openvz/openvz_conf.c|   2 +-
 src/openvz/openvz_driver.c  |  20 ++
 src/qemu/qemu_domain.c  |  17 +
 src/qemu/qemu_driver.c  |   6 +-
 src/qemu/qemu_migration.c   |   3 +-
 src/test/test_driver.c  |  56 +--
 src/uml/uml_driver.c|  37 +++---
 src/vmware/vmware_conf.c|   3 +-
 src/vmware/vmware_driver.c  |  17 ++---
 src/vz/vz_driver.c  |   1 -
 src/vz/vz_sdk.c |   3 -
 17 files changed, 192 insertions(+), 302 deletions(-)

-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/6] conf: Split FindBy{UUID|Name} into locked helpers

[John Ferlan]

Create helpers virDomainObjListFindByUUIDLocked and
virDomainObjListFindByNameLocked to avoid the need
to lock the domain object list leaving that task
for the caller.

Signed-off-by: John Ferlan 
---
 src/conf/virdomainobjlist.c | 58 ++---
 1 file changed, 49 insertions(+), 9 deletions(-)

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index d57ed10a5f..9aa2abd8c3 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -133,19 +133,16 @@ virDomainObjListFindByID(virDomainObjListPtr doms,
 }
 
 
-virDomainObjPtr
-virDomainObjListFindByUUID(virDomainObjListPtr doms,
-   const unsigned char *uuid)
+static virDomainObjPtr
+virDomainObjListFindByUUIDLocked(virDomainObjListPtr doms,
+ const unsigned char *uuid)
 {
 char uuidstr[VIR_UUID_STRING_BUFLEN];
 virDomainObjPtr obj;
 
-virObjectRWLockRead(doms);
 virUUIDFormat(uuid, uuidstr);
-
 obj = virHashLookup(doms->objs, uuidstr);
 virObjectRef(obj);
-virObjectRWUnlock(doms);
 if (obj) {
 virObjectLock(obj);
 if (obj->removing) {
@@ -158,15 +155,36 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms,
 }
 
 
-virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
-   const char *name)
+/**
+ * @doms: Locked domain object list
+ * @uuid: UUID to search the doms->objs table
+ *
+ * Lookup the @uuid in the doms->objs hash table and return a
+ * locked and ref counted domain object if found. Caller is
+ * expected to use the virDomainObjEndAPI when done with the object.
+ */
+virDomainObjPtr
+virDomainObjListFindByUUID(virDomainObjListPtr doms,
+   const unsigned char *uuid)
 {
 virDomainObjPtr obj;
 
 virObjectRWLockRead(doms);
+obj = virDomainObjListFindByUUIDLocked(doms, uuid);
+virObjectRWUnlock(doms);
+
+return obj;
+}
+
+
+static virDomainObjPtr
+virDomainObjListFindByNameLocked(virDomainObjListPtr doms,
+ const char *name)
+{
+virDomainObjPtr obj;
+
 obj = virHashLookup(doms->objsName, name);
 virObjectRef(obj);
-virObjectRWUnlock(doms);
 if (obj) {
 virObjectLock(obj);
 if (obj->removing) {
@@ -180,6 +198,28 @@ virDomainObjPtr 
virDomainObjListFindByName(virDomainObjListPtr doms,
 
 
 /**
+ * @doms: Locked domain object list
+ * @name: Name to search the doms->objsName table
+ *
+ * Lookup the @name in the doms->objsName hash table and return a
+ * locked and ref counted domain object if found. Caller is expected
+ * to use the virDomainObjEndAPI when done with the object.
+ */
+virDomainObjPtr
+virDomainObjListFindByName(virDomainObjListPtr doms,
+   const char *name)
+{
+virDomainObjPtr obj;
+
+virObjectRWLockRead(doms);
+obj = virDomainObjListFindByNameLocked(doms, name);
+virObjectRWUnlock(doms);
+
+return obj;
+}
+
+
+/**
  * @doms: Domain object list pointer
  * @vm: Domain object to be added
  *
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 4/6] libxl: Add refcnt for args->vm during migration

[John Ferlan]

When adding the @vm to the @args for usage during a thread or
callback, let's add the reference to it at the time of adding to
ensure nothing else deletes it. The corresponding Unref is then
added to the Dispose function.

Signed-off-by: John Ferlan 
---
 src/libxl/libxl_migration.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index d7b494b392..7fe352306c 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
 
 libxlMigrationCookieFree(args->migcookie);
 VIR_FREE(args->socks);
+virObjectUnref(args->vm);
 }
 
 static int
@@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
 goto error;
 
 args->conn = dconn;
-args->vm = vm;
+args->vm = virObjectRef(vm);
 args->flags = flags;
 args->migcookie = mig;
 /* Receive from pipeOut */
@@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
 goto error;
 
 args->conn = dconn;
-args->vm = vm;
+args->vm = virObjectRef(vm);
 args->flags = flags;
 args->socks = socks;
 args->nsocks = nsocks;
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/14] remote: stop trying to load Xen driver module

[Daniel P . Berrangé]

On Tue, Apr 24, 2018 at 02:16:59PM +0200, Michal Privoznik wrote:
> On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> > The Xen driver was recently deleted, but libvirtd has left over code
> > that tries to use it. Fortunately this is dead code because WITH_XEN
> > will never be defined anymore.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/remote/remote_daemon.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> > index 31c6ce1b61..3e02297eee 100644
> > --- a/src/remote/remote_daemon.c
> > +++ b/src/remote/remote_daemon.c
> > @@ -328,9 +328,6 @@ static void daemonInitialize(void)
> >  #ifdef WITH_NWFILTER
> >  VIR_DAEMON_LOAD_MODULE(nwfilterRegister, "nwfilter");
> >  #endif
> > -#ifdef WITH_XEN
> > -VIR_DAEMON_LOAD_MODULE(xenRegister, "xen");
> > -#endif
> >  #ifdef WITH_LIBXL
> >  VIR_DAEMON_LOAD_MODULE(libxlRegister, "libxl");
> >  #endif
> > 
> 
> ACK please. Also might be worth removing the following from virsh (in a
> separate patch perhaps?)

Yes, I'll push that as a trivial patch too

> 
> diff --git i/tools/virsh.c w/tools/virsh.c
> index 5f8352e861..62226eea4c 100644
> --- i/tools/virsh.c
> +++ w/tools/virsh.c
> @@ -526,9 +526,6 @@ virshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED)
>  #ifdef WITH_UML
>  vshPrint(ctl, " UML");
>  #endif
> -#ifdef WITH_XEN
> -vshPrint(ctl, " Xen");
> -#endif
>  #ifdef WITH_LIBXL
>  vshPrint(ctl, " LibXL");
>  #endif
> 
> Michal

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/14] build: prevent unloading of all public libraries

[Daniel P . Berrangé]

On Tue, Apr 24, 2018 at 02:17:00PM +0200, Michal Privoznik wrote:
> On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> > We previously added "-z nodelete" to the build of libvirt.so to prevent
> > crashes when thread local destructors run which point to a code that
> > has been dlclose()d:
> > 
> >   commit 384b9a76a5e387f64cfe8f83f4a518bb302e80f7
> >   Author: Daniel P. Berrangé 
> >   Date:   Thu Apr 19 11:42:22 2018 +0100
> > 
> > driver: prevent unloading of dlopen'd modules
> 
> This commit does not exist. However 8e44e5593eb does.

Yes, yes, copied in a local commit by mistake - the hash you mention
is indeed the right one.

> 
> > 
> > We forgot to copy this protection into the libvirt-qemu.so, libvirt-lxc.so
> > and libvirt-admin.so libraries when we introduced them.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/Makefile.am | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> ACK
> 
> Michal

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/14] build: prevent unloading of dlopen'd modules

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> We previously added "-z nodelete" to the build of libvirt.so to prevent
> crashes when thread local destructors run which point to a code that
> has been dlclose()d:
> 
>   commit 384b9a76a5e387f64cfe8f83f4a518bb302e80f7
>   Author: Daniel P. Berrangé 
>   Date:   Thu Apr 19 11:42:22 2018 +0100
> 
> driver: prevent unloading of dlopen'd modules

As I've pointed earlier no such commit exists.

> 
> The libvirtd loadable modules can suffer from the same problem if they
> were ever unloaded. Fortunately we don't ever call dlclose() on them,
> but lets add a second layer of protection by linking them with the
> "-z nodelete" flag. While we're doing this, lets add a third layer of
> protection by passing RTLD_NODELETE to dlopen().
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/Makefile.am | 6 +-
>  src/driver.c| 7 ++-
>  2 files changed, 11 insertions(+), 2 deletions(-)

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/14] remote: stop trying to load Xen driver module

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> The Xen driver was recently deleted, but libvirtd has left over code
> that tries to use it. Fortunately this is dead code because WITH_XEN
> will never be defined anymore.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/remote/remote_daemon.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index 31c6ce1b61..3e02297eee 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -328,9 +328,6 @@ static void daemonInitialize(void)
>  #ifdef WITH_NWFILTER
>  VIR_DAEMON_LOAD_MODULE(nwfilterRegister, "nwfilter");
>  #endif
> -#ifdef WITH_XEN
> -VIR_DAEMON_LOAD_MODULE(xenRegister, "xen");
> -#endif
>  #ifdef WITH_LIBXL
>  VIR_DAEMON_LOAD_MODULE(libxlRegister, "libxl");
>  #endif
> 

ACK please. Also might be worth removing the following from virsh (in a
separate patch perhaps?)

diff --git i/tools/virsh.c w/tools/virsh.c
index 5f8352e861..62226eea4c 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -526,9 +526,6 @@ virshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED)
 #ifdef WITH_UML
 vshPrint(ctl, " UML");
 #endif
-#ifdef WITH_XEN
-vshPrint(ctl, " Xen");
-#endif
 #ifdef WITH_LIBXL
 vshPrint(ctl, " LibXL");
 #endif

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 08/14] driver: add option to make missing drivers a fatal problem

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> Currently the driver module loading code does not report an error if the
> driver module is physically missing on disk. This is useful for distro
> packaging optional pieces. When the daemons are split up into one daemon
> per driver, we will expect module loading to always succeed. If a driver
> is not desired, the entire daemon should not be installed.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/driver.c  | 37 ++---
>  src/driver.h  |  6 --
>  src/remote/remote_daemon.c|  2 +-
>  src/storage/storage_backend.c |  9 +
>  tests/virdrivermoduletest.c   |  2 +-
>  5 files changed, 33 insertions(+), 23 deletions(-)

ACK with this squashed in:

diff --git i/src/security/virt-aa-helper.c w/src/security/virt-aa-helper.c
index a1bc1090bf..ee5e3b0701 100644
--- i/src/security/virt-aa-helper.c
+++ w/src/security/virt-aa-helper.c
@@ -964,7 +964,7 @@ get_files(vahControl * ctl)
 
 /* load the storage driver so that backing store can be accessed */
 #ifdef WITH_STORAGE
-virDriverLoadModule("storage", "storageRegister");
+virDriverLoadModule("storage", "storageRegister", false);
 #endif
 
 for (i = 0; i < ctl->def->ndisks; i++) {

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 04/14] driver: don't keep a pointer to the loaded library handle

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> Now that we've activated two hacks to prevent unloading of modules,
> there is no point passing back a pointer to the loaded library handle.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/driver.c  | 23 +++
>  src/driver.h  |  5 ++---
>  src/storage/storage_backend.c |  2 +-
>  3 files changed, 10 insertions(+), 20 deletions(-)

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 06/14] driver: tighten check for whether loadable module exists or not

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> Currently we do a access(R_OK) check to see whether a loadable module
> exists, treating failure as non-fatal. This is unreasonably loose, as a
> module which exists but has had incorrect permissions set will turn into
> a silent skip. We only want to skip loading if the module genuinely does
> not exist on disk, due to the optional package not being installed.
> 
> Furthermore, checking the return value of virDriverLoadModuleFile() is
> not a suitable witness that the module does not exist. This method can
> return NULL if dlopen() fails, for example due to being unable to
> resolve symbols in the library. This is should always be reported as an
> error because it is a sign of the bad installation where either the
> module build doesn't match the libvirtd build, or where some 3rd party
> libraries are missing or broken.
> 
> Both these problems can be fixed by using virFileExists in the caller
> instead.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/driver.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 09/14] remote: honour errors from virDriverLoadModule

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> The libvirtd daemon currently ignores the return status of
> virDriverLoadModule entirely. This is way too loose, resulting in many
> important problems going undiagnosed, resulting in a libvirtd that may
> never work correctly. We should only ignore a non-existant module, and
> pass back any fatal errors.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/remote/remote_daemon.c | 61 
> +++---
>  1 file changed, 36 insertions(+), 25 deletions(-)

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 07/14] driver: use normal error reporting APIs when loading modules

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> The driver module loading code is one of the few places that still uses
> VIR_ERROR for reporting failures. Convert it to normal error reporting
> APIs.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/driver.c | 34 +++---
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/src/driver.c b/src/driver.c
> index e02efe2615..9b137c39e4 100644
> --- a/src/driver.c
> +++ b/src/driver.c
> @@ -33,6 +33,7 @@
>  
>  VIR_LOG_INIT("driver");
>  
> +#define VIR_FROM_THIS VIR_FROM_NONE
>  
>  /* XXX re-implement this for other OS, or use libtools helper lib ? */
>  #define DEFAULT_DRIVER_DIR LIBDIR "/libvirt/connection-driver"
> @@ -55,8 +56,11 @@ virDriverLoadModuleFile(const char *file)
>  
>  virUpdateSelfLastChanged(file);
>  
> -if (!(handle = dlopen(file, flags)))
> -VIR_ERROR(_("failed to load module %s %s"), file, dlerror());
> +if (!(handle = dlopen(file, flags))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Failed to load module '%s': %s"), file, dlerror());

Sweet. dlerror() returns 'char *' instead of 'const char *' even though
it's returning a pointer to statically allocated buffer.

> +return NULL;
> +}
>  
>  return handle;
>  }

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/14] build: prevent unloading of all public libraries

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> We previously added "-z nodelete" to the build of libvirt.so to prevent
> crashes when thread local destructors run which point to a code that
> has been dlclose()d:
> 
>   commit 384b9a76a5e387f64cfe8f83f4a518bb302e80f7
>   Author: Daniel P. Berrangé 
>   Date:   Thu Apr 19 11:42:22 2018 +0100
> 
> driver: prevent unloading of dlopen'd modules

This commit does not exist. However 8e44e5593eb does.

> 
> We forgot to copy this protection into the libvirt-qemu.so, libvirt-lxc.so
> and libvirt-admin.so libraries when we introduced them.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/Makefile.am | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/14] driver: fix handling of error return from finding resource

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> The virFileFindResource method merely builds up the expected fully
> qualified path to the resource. It does not actually check if it exists
> on disk. The loadable module callers were mistakenly thinking a NULL
> indicates the file doesn't exist on disk, whereas it in fact indicates
> an out of memory error.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/driver.c  | 2 +-
>  src/storage/storage_backend.c | 2 +-
>  src/util/virfile.c| 4 
>  3 files changed, 6 insertions(+), 2 deletions(-)

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/14] remote: refactor code for building UNIX socket paths

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> The code for building UNIX socket paths will be getting more complex to
> cope with accessing various different daemons. Refactor it to eliminate
> the code duplication and isolation the logic for constructing paths.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/remote/remote_driver.c | 69 
> --
>  1 file changed, 42 insertions(+), 27 deletions(-)

ACK

Up until and including this patch, you can merge these as they fix real
problems / improve things.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 10/14] remote: split URI scheme into driver and transport upfront

[Michal Privoznik]

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
> Currently the remote driver extracts the transport from URI scheme and
> plays games to temporarily hide the driver part when formatting URIs.
> Refactor the code to split the URI scheme upfront so the two pieces are
> easily available where needed.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/remote/remote_driver.c | 73 
> ++
>  1 file changed, 42 insertions(+), 31 deletions(-)

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Add I/O thread support info into domain capabilities

[Martin Kletzander]

On Tue, Apr 24, 2018 at 01:03:46PM +0200, Michal Privoznik wrote:

On 04/23/2018 04:50 PM, Martin Kletzander wrote:

Signed-off-by: Martin Kletzander 
---
 docs/schemas/domaincaps.rng   |  9 +
 src/conf/domain_capabilities.c|  3 +++
 src/conf/domain_capabilities.h|  1 +
 src/qemu/qemu_capabilities.c  | 11 +++
 .../domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml |  1 +
 tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.12.0.s390x.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml |  1 +
 .../domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml |  1 +
 tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml   |  1 +
 tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.7.0.s390x.xml   |  1 +
 tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.8.0.s390x.xml   |  1 +
 tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml  |  1 +
 tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml  |  1 +
 19 files changed, 39 insertions(+)

diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
index 39053181eb9a..dca5fa1c8fa0 100644
--- a/docs/schemas/domaincaps.rng
+++ b/docs/schemas/domaincaps.rng
@@ -28,6 +28,9 @@
 
   
 
+
+  
+
 
   
 
@@ -53,6 +56,12 @@
 
   

+  
+
+  
+
+  


Shouldn't this be  to be consistent with the
rest of 'devices'?



I guess it makes sense to differentiate between QEMU not supporting I/O
threads and libvirt not being able to expose the support info in
capabilities.  v2 it is


Michal


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Add I/O thread support info into domain capabilities

[Michal Privoznik]

On 04/23/2018 04:50 PM, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander 
> ---
>  docs/schemas/domaincaps.rng   |  9 +
>  src/conf/domain_capabilities.c|  3 +++
>  src/conf/domain_capabilities.h|  1 +
>  src/qemu/qemu_capabilities.c  | 11 +++
>  .../domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml |  1 +
>  tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml  |  1 +
>  tests/domaincapsschemadata/qemu_2.12.0.s390x.xml  |  1 +
>  tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml |  1 +
>  .../domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml  |  1 +
>  tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml |  1 +
>  tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml   |  1 +
>  tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml  |  1 +
>  tests/domaincapsschemadata/qemu_2.7.0.s390x.xml   |  1 +
>  tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml  |  1 +
>  tests/domaincapsschemadata/qemu_2.8.0.s390x.xml   |  1 +
>  tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml  |  1 +
>  tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml  |  1 +
>  tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml  |  1 +
>  tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml  |  1 +
>  19 files changed, 39 insertions(+)
> 
> diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
> index 39053181eb9a..dca5fa1c8fa0 100644
> --- a/docs/schemas/domaincaps.rng
> +++ b/docs/schemas/domaincaps.rng
> @@ -28,6 +28,9 @@
>  
>
>  
> +
> +  
> +
>  
>
>  
> @@ -53,6 +56,12 @@
>  
>
>  
> +  
> +
> +  
> +
> +  

Shouldn't this be  to be consistent with the
rest of 'devices'?

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[Nikolay Shirokovskiy]

On 23.04.2018 16:50, Eric Blake wrote:
> On 04/23/2018 05:38 AM, Nikolay Shirokovskiy wrote:
> 
>>> I'd argue the converse. Libvirt already knows how to do atomic updates
>>> of XML files that it tracks.  If libvirtd crashes/restarts in the middle
>>> of an API call, you already have indeterminate results of whether the
>>> API worked or failed; once libvirtd is restarted, you'll have to
>>> probably retry the command.  For all other cases, the API call
>>> completes, and either no XML changes were made (the command failed and
>>> reports the failure properly), or all XML changes were made (the command
>>> created the appropriate changes to track the new checkpoint, including
>>> whatever bitmap names have to be recorded to map the relation between
>>> checkpoints and bitmaps).
>>
>> We can fail to save XML... Consider we have B1, B2 and create B3 bitmap
>> in the process of creating checkpoint C3. Next qemu creates snapshot
>> and bitmap successfully then libvirt fail to update XML and after some
>> time libvirt restarts (not even crashes). Now libvirt nows of B1 and B2 but 
>> not B3.
> 
> Libvirt is in charge of tracking ALL state internally that it requires
> to restore state properly across a libvirtd restart, so that it presents
> the illusion of a libvirt API atomically completing or failing.  If
> libvirt creates bitmap B3 but does not create checkpoint C3 prior to it
> restarting, then on restart, it should be able to correctly see that B3
> is stranded and delete it (rather, merge it back into B2 so that B2
> remains the only live bitmap) as part of an incomplete API that failed.
> 
>> What can be the consequences? For example if we ask bitmap from C2 we
>> miss all changes from C3 as we don't know of B3. This will lead to corrupted
>> backups.
> 
> Checkpoint C3 does not exist if libvirt API did not complete correctly
> (even if bitmap B3 exists).  It should merely be a matter of libvirt
> making proper annotations of what it plans to do prior to calling into
> qemu, so that if it restarts, it can recover from an intermediate state
> of failure to follow those plans.
> 
>>
>> This can be fixed:
>>
>> - in qemu. If bitmaps have child/parent realtionship then on libvirt restart
>>   we can recover (we ask qemu for bitmaps, discover B3 and then discover
>>   B3 is child of B2). This is how basically implementation with naming
>>   scheme works. Well on this way we don't need special metadata in 
>>   libvirt (besides may be domain xml attached to checkpoiint etc)
>>
>> - in libvirt. If we save XML before creating a snapshot with checkpoint.
>>   This fixes the issue with successful operation but saving XML failure.
>>   But now we have another issue :) We can save XML successfully but then 
>> operation
>>   itself can fail and we fail to revert XML back. Well we can recover
>>   even without child/parent metadata in qemu in this case. Just ask
>>   qemu for bitmaps on libvirt restart and if bitmap is missing kick
>>   it out as it is a case described above (successful saving XML then
>>   unsuccessfull qemu operation)
>>
>> So it is possible to track bitmaps in libvirt. We just need to be extra 
>> carefull
>> not to produce invalid backups.
> 
> Yes, but that's true of any interface where a single libvirt API
> controls multiple steps in qemu.
> 
>>
>>>
>>> Consider the case of internal snapshots.  Already, we have the case
>>> where qemu itself does not track enough useful metadata about internal
>>> snapshots (right now, just a name and timestamp of creation); so libvirt
>>> additionally tracks further information in : the name,
>>> timestamp, relationship to any previous snapshot (libvirt can then
>>> reconstruct a tree relationship between all snapshots; where a parent
>>> can have more than one child if you roll back to a snapshot and then
>>> execute the guest differently), the set of disks participating in the
>>> snapshot, and the  description at the time of the snapshot (if
>>> you hotplug devices, or even the fact that creating external snapshots
>>> changes which file is the active qcow2 in a backing chain, you'll need
>>> to know how to roll back to the prior domain state as part of
>>> reverting).  This is approximately the same set of information that a
>>>  will need to track.
>>
>> I would differentiate checkpoints and backups. For example in case
>> of push backups we can store additional metadata in 
>> so later we can revert back to previous state. But checkpoints 
>> (bitmaps technically) are only to make incremental backups(restores?). 
>> We can attach extra metadata to checkpoints but it looks accidental just 
>> because
>> bitmaps and backups relate to some same point in time. To me a backup (push)
>> can carry all the metadata and as to checkpoints a backup can have
>> associated checkpoint or not. For example if we choose to always
>> make full backups we don't need checkpoints at all (at least if we are
>> not going to use them for restore).
> 
> It's still nice to 

Re: [libvirt] [PATCH] virNumaGetHugePageInfo: Return page_avail and page_free as ULL

[Michal Privoznik]

On 04/24/2018 10:42 AM, Daniel P. Berrangé wrote:
> On Mon, Apr 23, 2018 at 05:44:38PM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1569678
>>
>> On some large systems (with ~400GB of RAM) it is possible for
>> unsigned int to overflow in which case we report invalid number
>> of 4K pages pool size. Switch to unsigned long long.
> 
> It isn't very obvious from the code diff what the actual problem
> is, but it is mentioned in the bug that we hit overflow in
> virNumaGetPages when doing:
> 
> huge_page_sum += 1024 * page_size * page_avail;
> 
> because although 'huge_page_sum' is an unsigned long long, the
> page_size and page_avail are both unsigned int, so the promotion
> to unsigned long long doesn't happen until the sum has been
> calculated, by which time we've already overflowed.  Can you
> mention that we're specifically solving this huge_page_sum
> overflow in the commit message
> 
> Turning page_avail into a unsigned long long is not strictly
> needed until we need ability to represent more than 2^32
> 4k pages, which IIUC equates to 16 TB of RAM. That's not
> outside the realm of possibility, so makes sense that we
> change it to unsigned long long to avoid future problems.
> Can you also mention that we're solving this limit too
> in the commit message.
> 
> Reviewed-by: Daniel P. Berrangé 

Adjusted and pushed. Thank you.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/8] vfio-ccw passthrough support

[Shalini Chellathurai Saroja]

Polite ping.


On 04/11/2018 05:49 PM, Shalini Chellathurai Saroja wrote:

Let us support the basic channel I/O passthrough infrastructure based on
vfio, which have been introduced in QEMU 2.10. The current focus is to
support dasd-eckd (cu_type/dev_type = 0x3990/0x3390) as the target
device.

Shalini Chellathurai Saroja (8):
   qemu: introduce capability for virtual-css-bridge
   qemu: introduce vfio-ccw capability
   util: virhostdev: add virHostdevIsMdevDevice()
   qemu: vfio-ccw device address generation
   qemu: command line generation for vfio-ccw device
   tests: tests for vfio-ccw passthrough
   docs: documentation for vfio-ccw passthrough
   news: documentation of new feature

  docs/drvnodedev.html.in|  21 -
  docs/formatdomain.html.in  |  20 +++-
  docs/news.xml  |   9 ++
  docs/schemas/domaincommon.rng  |   5 +-
  src/libvirt_private.syms   |   1 +
  src/qemu/qemu_capabilities.c   |  23 +
  src/qemu/qemu_capabilities.h   |   5 +
  src/qemu/qemu_command.c|  37 ++--
  src/qemu/qemu_domain.c |   2 +-
  src/qemu/qemu_domain_address.c |  32 +--
  src/qemu/qemu_hostdev.c|   3 +-
  src/qemu/qemu_hotplug.c|   4 +-
  src/util/virhostdev.c  |  26 --
  src/util/virhostdev.h  |   3 +
  src/util/virmdev.c |   3 +-
  src/util/virmdev.h |   1 +
  .../qemucapabilitiesdata/caps_2.10.0.s390x.replies |  28 --
  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |   3 +-
  .../qemucapabilitiesdata/caps_2.11.0.s390x.replies |  28 --
  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |   3 +-
  .../qemucapabilitiesdata/caps_2.12.0.s390x.replies |  31 +--
  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   |   3 +
  .../qemucapabilitiesdata/caps_2.7.0.s390x.replies  |  24 +++--
  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml|   3 +-
  .../qemucapabilitiesdata/caps_2.8.0.s390x.replies  |  28 --
  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml|   3 +-
  .../qemucapabilitiesdata/caps_2.9.0.s390x.replies  |  28 --
  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml|   3 +-
  tests/qemuhotplugtest.c|   2 +-
  ...tdev-subsys-mdev-vfio-ccw-duplicate-address.xml |  29 ++
  ...ostdev-subsys-mdev-vfio-ccw-invalid-address.xml |  23 +
  .../hostdev-subsys-mdev-vfio-ccw.args  |  23 +
  .../hostdev-subsys-mdev-vfio-ccw.xml   |  22 +
  tests/qemuxml2argvtest.c   | 102 -
  .../hostdev-subsys-mdev-vfio-ccw.xml   |  28 ++
  tests/qemuxml2xmltest.c|  31 ---
  36 files changed, 491 insertions(+), 149 deletions(-)
  create mode 100644 
tests/qemuxml2argvdata/hostdev-subsys-mdev-vfio-ccw-duplicate-address.xml
  create mode 100644 
tests/qemuxml2argvdata/hostdev-subsys-mdev-vfio-ccw-invalid-address.xml
  create mode 100644 tests/qemuxml2argvdata/hostdev-subsys-mdev-vfio-ccw.args
  create mode 100644 tests/qemuxml2argvdata/hostdev-subsys-mdev-vfio-ccw.xml
  create mode 100644 tests/qemuxml2xmloutdata/hostdev-subsys-mdev-vfio-ccw.xml



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/2] fix parallel shutdown in libvirt-guests.sh

[Michal Privoznik]

On 04/24/2018 10:23 AM, Andrea Bolognani wrote:
> On Thu, 2018-04-19 at 08:41 +0200, Christian Ehrhardt wrote:
>> Working on a few issues in parallel shutdown. The first is fixing a real 
>> issue,
>> while the latter avoids printing a message with an empty variable.
>>
>> *Updates since V1*
>>  - added Reviewed-by to patch #1
>>  - added second patch to skip the message without a guest nama
>>
>> Christian Ehrhardt (2):
>>   tools: fix check_guests_shutdown loop
>>   tools: do not report unknown guests in print_guests_shutdown
>>
>>  tools/libvirt-guests.sh.in | 10 ++
>>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> Series:
> 
>   Reviewed-by: Andrea Bolognani 
> 

Thank you all. I've pushed this.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virNumaGetHugePageInfo: Return page_avail and page_free as ULL

[Daniel P . Berrangé]

On Mon, Apr 23, 2018 at 05:44:38PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1569678
> 
> On some large systems (with ~400GB of RAM) it is possible for
> unsigned int to overflow in which case we report invalid number
> of 4K pages pool size. Switch to unsigned long long.

It isn't very obvious from the code diff what the actual problem
is, but it is mentioned in the bug that we hit overflow in
virNumaGetPages when doing:

huge_page_sum += 1024 * page_size * page_avail;

because although 'huge_page_sum' is an unsigned long long, the
page_size and page_avail are both unsigned int, so the promotion
to unsigned long long doesn't happen until the sum has been
calculated, by which time we've already overflowed.  Can you
mention that we're specifically solving this huge_page_sum
overflow in the commit message

Turning page_avail into a unsigned long long is not strictly
needed until we need ability to represent more than 2^32
4k pages, which IIUC equates to 16 TB of RAM. That's not
outside the realm of possibility, so makes sense that we
change it to unsigned long long to avoid future problems.
Can you also mention that we're solving this limit too
in the commit message.

Reviewed-by: Daniel P. Berrangé 


> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/capabilities.c |  5 +++--
>  src/conf/capabilities.h |  2 +-
>  src/util/virhostmem.c   |  2 +-
>  src/util/virnuma.c  | 32 ++--
>  src/util/virnuma.h  |  8 
>  tests/virnumamock.c |  4 ++--
>  6 files changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index c4ee7efb5f..dd2fc77f91 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -816,7 +816,7 @@ virCapabilitiesFormatNUMATopology(virBufferPtr buf,
>cells[i]->mem);
>  
>  for (j = 0; j < cells[i]->npageinfo; j++) {
> -virBufferAsprintf(buf, " size='%u'>%zu\n",
> +virBufferAsprintf(buf, " size='%u'>%llu\n",
>cells[i]->pageinfo[j].size,
>cells[i]->pageinfo[j].avail);
>  }
> @@ -1351,7 +1351,8 @@ virCapabilitiesGetNUMAPagesInfo(int node,
>  int *npageinfo)
>  {
>  int ret = -1;
> -unsigned int *pages_size = NULL, *pages_avail = NULL;
> +unsigned int *pages_size = NULL;
> +unsigned long long *pages_avail = NULL;
>  size_t npages, i;
>  
>  if (virNumaGetPages(node, _size, _avail, NULL, ) < 0)
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index 694a3590bf..f0a06a24df 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -107,7 +107,7 @@ typedef struct _virCapsHostNUMACellPageInfo 
> virCapsHostNUMACellPageInfo;
>  typedef virCapsHostNUMACellPageInfo *virCapsHostNUMACellPageInfoPtr;
>  struct _virCapsHostNUMACellPageInfo {
>  unsigned int size;  /* page size in kibibytes */
> -size_t avail;   /* the size of pool */
> +unsigned long long avail;   /* the size of pool */
>  };
>  
>  typedef struct _virCapsHostNUMACell virCapsHostNUMACell;
> diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c
> index 11efe8c502..c923a1edf5 100644
> --- a/src/util/virhostmem.c
> +++ b/src/util/virhostmem.c
> @@ -783,7 +783,7 @@ virHostMemGetFreePages(unsigned int npages,
>  for (cell = startCell; cell <= lastCell; cell++) {
>  for (i = 0; i < npages; i++) {
>  unsigned int page_size = pages[i];
> -unsigned int page_free;
> +unsigned long long page_free;
>  
>  if (virNumaGetPageInfo(cell, page_size, 0, NULL, _free) < 0)
>  goto cleanup;
> diff --git a/src/util/virnuma.c b/src/util/virnuma.c
> index bebe301f8d..784db0a7ce 100644
> --- a/src/util/virnuma.c
> +++ b/src/util/virnuma.c
> @@ -563,8 +563,8 @@ virNumaGetHugePageInfoDir(char **path, int node)
>  static int
>  virNumaGetHugePageInfo(int node,
> unsigned int page_size,
> -   unsigned int *page_avail,
> -   unsigned int *page_free)
> +   unsigned long long *page_avail,
> +   unsigned long long *page_free)
>  {
>  int ret = -1;
>  char *path = NULL;
> @@ -579,7 +579,7 @@ virNumaGetHugePageInfo(int node,
>  if (virFileReadAll(path, 1024, ) < 0)
>  goto cleanup;
>  
> -if (virStrToLong_ui(buf, , 10, page_avail) < 0 ||
> +if (virStrToLong_ull(buf, , 10, page_avail) < 0 ||
>  *end != '\n') {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("unable to parse: %s"),
> @@ -598,7 +598,7 @@ virNumaGetHugePageInfo(int node,
>  if (virFileReadAll(path, 1024, ) < 0)
>  goto cleanup;
>  
> -if 

Re: [libvirt] [PATCH v2 0/2] fix parallel shutdown in libvirt-guests.sh

[Andrea Bolognani]

On Thu, 2018-04-19 at 08:41 +0200, Christian Ehrhardt wrote:
> Working on a few issues in parallel shutdown. The first is fixing a real 
> issue,
> while the latter avoids printing a message with an empty variable.
> 
> *Updates since V1*
>  - added Reviewed-by to patch #1
>  - added second patch to skip the message without a guest nama
> 
> Christian Ehrhardt (2):
>   tools: fix check_guests_shutdown loop
>   tools: do not report unknown guests in print_guests_shutdown
> 
>  tools/libvirt-guests.sh.in | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)

Series:

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH rust] git: add config file telling git-publish how to send patches

[Daniel P . Berrangé]

On Mon, Apr 23, 2018 at 03:39:13PM +0200, Andrea Bolognani wrote:
> On Mon, 2018-04-23 at 15:20 +0200, Ján Tomko wrote: > On Mon, Apr 23, 2018 at 
> 01:54:32PM +0100, Daniel P. Berrangé wrote:
> > > +[gitpublishprofile "default"]
> > > +base = master
> > > +to = libvir-list@redhat.com
> > > +prefix = PATCH rust
> > 
> > I found the reverse order, i.e.
> > 'rust PATCH'
> > to be both nicer and more useful. (not to mention more common)
> 
> Agreed, 'module PATCH' seems to be the de-facto standard, so we
> should codify that instead of the opposite.

Ok, since consensus seems to be for this, I'll swap these patches around
before pushing them.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC v3 1/3] qapi: add SysEmuTarget to "common.json"

[Kashyap Chamarthy]

On Sat, Apr 21, 2018 at 01:12:44AM +0200, Laszlo Ersek wrote:
> We'll soon need an enumeration type that lists all the softmmu targets
> that QEMU (the project) supports. Introduce @SysEmuTarget to
> "common.json".

[...]

> Notes:
> RFCv3:
> 
> - The patch is new in this version. [Dan, Markus]
> 
> - The original idea was to call the new enum @Target; however, @Target
>   generates exactly the TARGET_AARCH64, TARGET_ALPHA, TARGET_ARM, ...
>   enumeration constants that conflict with the poisoned preprocessing
>   macros of the same names. Hence @SysEmuTarget -- it's more accurate
>   anyway, since we want it to stand for system emulation targets.
> 
> - Also, we discussed defining the new type in either "common.json" or
>   "misc.json". "misc.json" turned out to be a problem: "firmware.json"
>   would then include "misc.json" for the new type's sake, but that
>   inclusion would become the first appearance of "misc.json" -- within
>   "firmware.json". That messed up the generated documentation. By adding
>   the new type to "common.json", "misc.json" (see the 2nd patch) and
>   "firmware.json" (see the 3rd patch) can both consume the new type
>   without problems.

The last two points are interesting enough (for me) to still to keep in
the commit message, but that's minor :-)

>  qapi/common.json | 19 +++
>  1 file changed, 19 insertions(+)

FWIW:

Reviewed-by: Kashyap Chamarthy 

[...]

-- 
/kashyap

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 10/11] qemu: Add VM Generation ID to qemu command line

[Peter Krempa]

On Mon, Apr 23, 2018 at 20:00:04 -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1149445
> 
> If the domain requests usage of the genid functionality,
> then add the QEMU '-device vmgenid' to the command line
> providing either the supplied or generated GUID value.
> 
> Add tests for both a generated and supplied GUID value.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_command.c| 31 +++
>  tests/qemuxml2argvdata/genid-auto.args | 24 
>  tests/qemuxml2argvdata/genid.args  | 24 
>  tests/qemuxml2argvtest.c   |  4 
>  4 files changed, 83 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/genid-auto.args
>  create mode 100644 tests/qemuxml2argvdata/genid.args
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b666f3715f..1f5e79d86a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5942,6 +5942,34 @@ qemuBuildSmbiosCommandLine(virCommandPtr cmd,
>  
>  
>  static int
> +qemuBuildVMGenIDCommandLine(virCommandPtr cmd,
> +const virDomainDef *def,
> +virQEMUCapsPtr qemuCaps)
> +{
> +virBuffer opts = VIR_BUFFER_INITIALIZER;
> +char guid[VIR_UUID_STRING_BUFLEN];
> +
> +if (!def->genidRequested)
> +return 0;
> +
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("genid is not supported with this QEMU binary"));
> +return -1;

This is already checked in qemuProcessGenID.

> +}
> +
> +virUUIDFormat(def->genid, guid);
> +virBufferAsprintf(, "vmgenid,guid=%s,id=vmgenid0", guid);

[...]

> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 74d930ebe2..0dd0850036 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -805,6 +805,10 @@ mymain(void)
>  QEMU_CAPS_SECCOMP_BLACKLIST);
>  DO_TEST_PARSE_ERROR("minimal-no-memory", NONE);
>  DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP);
> +
> +DO_TEST("genid", QEMU_CAPS_DEVICE_VMGENID);
> +DO_TEST("genid-auto", QEMU_CAPS_DEVICE_VMGENID);

Please use DO_TEST_CAPS_LATEST/DO_TEST_CAPS_VER


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 05/11] conf: Add flag to regenerate genid for virDomainDefCopy

[Peter Krempa]

On Mon, Apr 23, 2018 at 19:59:59 -0400, John Ferlan wrote:
> Add the VIR_DOMAIN_DEF_COPY_NEWGENID to indicate that the generated
> domain definition should adjust the genid value before returning the
> @def to the caller.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_conf.c | 12 
>  src/conf/domain_conf.h |  5 +
>  src/qemu/qemu_driver.c |  3 ++-
>  src/test/test_driver.c |  3 ++-
>  4 files changed, 21 insertions(+), 2 deletions(-)

IMO all this code should not be necessary. Code paths which knowingly
need to change the GENID should do so explicitly rather than pulling the
logic here.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 06/11] conf: Add VM Generation ID parse/format support

[Peter Krempa]

On Mon, Apr 23, 2018 at 20:00:00 -0400, John Ferlan wrote:
> The VM Generation ID is a mechanism to provide a unique 128-bit,
> cryptographically random, and integer value identifier known as
> the GUID (Globally Unique Identifier) to the guest OS. The value
> is used to help notify the guest operating system when the virtual
> machine is executed with a different configuration.
> 
> This patch adds support for a new "genid" XML element similar to
> the "uuid" element. The "genid" element can have two forms ""
> or "$GUID". If the $GUID is not provided, libvirt
> will generate one.
> 
> For the ABI checks add avoidance for the genid comparison if the
> appropriate flag is set.
> 
> Since adding support for a generated GUID (or UUID like) value to
> be displayed only for an active guest, modifying the xml2xml test
> to include virrandommock.so is necessary since it will generate a
> "known" UUID value that can be compared against for the active test.
> 
> Signed-off-by: John Ferlan 
> ---
>  docs/formatdomain.html.in| 29 
>  docs/schemas/domaincommon.rng|  8 
>  src/conf/domain_conf.c   | 59 
> 
>  src/conf/domain_conf.h   |  3 ++
>  tests/qemuxml2argvdata/genid-auto.xml| 32 +
>  tests/qemuxml2argvdata/genid.xml | 32 +
>  tests/qemuxml2xmloutdata/genid-active.xml| 32 +
>  tests/qemuxml2xmloutdata/genid-auto-active.xml   | 32 +
>  tests/qemuxml2xmloutdata/genid-auto-inactive.xml | 32 +
>  tests/qemuxml2xmloutdata/genid-inactive.xml  | 32 +
>  tests/qemuxml2xmltest.c  |  5 +-
>  11 files changed, 295 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxml2argvdata/genid-auto.xml
>  create mode 100644 tests/qemuxml2argvdata/genid.xml
>  create mode 100644 tests/qemuxml2xmloutdata/genid-active.xml
>  create mode 100644 tests/qemuxml2xmloutdata/genid-auto-active.xml
>  create mode 100644 tests/qemuxml2xmloutdata/genid-auto-inactive.xml
>  create mode 100644 tests/qemuxml2xmloutdata/genid-inactive.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index ada0df227f..6a140f3e40 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -34,6 +34,7 @@
>  domain type='kvm' id='1'
>nameMyGuest/name
>uuid4dea22b3-1d52-d8f3-2516-782e98ab3fa0/uuid
> +  genid43dc0cf8-809b-4adb-9bea-a9abb5f3d90e/genid

Since we encourage to use the variant with this being empty I'd show
that here.

>titleA short description - title - of the domain/title
>descriptionSome human readable description/description
>metadata
> @@ -61,6 +62,34 @@
>  specification. Since 0.0.1, sysinfo
>  since 0.8.7
>  
> +  genid
> +  Since 4.3.0, the genid
> +element can be used to add a Virtual Machine Generation ID which
> +exposes a 128-bit, cryptographically random, integer value 
> identifier,
> +referred to as a Globally Unique Identifier (GUID) using the same
> +format as the uuid. The value is used to help notify
> +the guest operating system when the virtual machine is executed
> +with a different configuration, such as:
> +
> +
> +  snapshot execution
> +  backup recovery
> +  failover in a disaster recovery environment
> +  creation from template (import, copy, clone)
> +
> +
> +The guest operating system notices the change and is then able to
> +react as appropriate by marking its copies of distributed databases
> +as dirty, re-initializing its random number generator, etc.
> +
> +
> +When a GUID value is not provided, e.g. using the XML syntax
> +genid/, then libvirt will automatically generate a GUID.
> +This is the recommended configuration since the hypervisor then
> +can handle changing the GUID value for specific state transitions.
> +Using a static GUID value may result in failures for starting from
> +snapshot, restoring from backup, starting a cloned domain, 
> etc.
> +
>title
>The optional element title provides space for a
>  short description of the domain. The title should not contain

[...]

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6d4db50998..8c30adf850 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18793,6 +18793,34 @@ virDomainDefParseXML(xmlDocPtr xml,
>  VIR_FREE(tmp);
>  }
>  
> +/* Extract domain genid - a genid can either be provided or generated */
> +if ((n = virXPathNodeSet("./genid", ctxt, )) < 0)
> +goto error;
> +
> +if (n > 0) {
> +if (n != 1) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +  _("element 'genid' 

Re: [libvirt] [PATCH 0/3] sample: vfio mdev display devices.

[Gerd Hoffmann]

  Hi,

> Here's another proposal that's really growing on me:
> 
>  * Fix the vendor drivers!  Allow devices to be opened and probed
>without these external dependencies.

Hmm.  If you try use gvt with tcg then, wouldn't qemu think "device
probed ok, all green" then even though that isn't the case?

cheers,
  Gerd

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 01/11] Check return status for virUUIDGenerate

[Peter Krempa]

On Mon, Apr 23, 2018 at 19:59:55 -0400, John Ferlan wrote:
> Although legal, a few paths were not checking a return value < 0
> for failure instead they checked a non zero failure.
> 
> Clean them all up to be consistent.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_conf.c | 2 +-
>  src/conf/network_conf.c| 2 +-
>  src/conf/secret_conf.c | 2 +-
>  src/openvz/openvz_conf.c   | 2 +-
>  src/xenconfig/xen_common.c | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)

ACK


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 09/11] Remove check for duplicitous GenID in another VM

[Peter Krempa]

On Mon, Apr 23, 2018 at 20:00:03 -0400, John Ferlan wrote:
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_process.c | 68 
> ++---
>  1 file changed, 2 insertions(+), 66 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 06ec4ddeb9..bfc22b7d07 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5880,73 +5880,14 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
>  
>  
>  /**
> - * qemuProcessCheckGenID:
> - * @vm: Domain to be checked
> - * @opaque: Domain about to be started

Did you forget to squash this into the previous patch?


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [qemu RFC v3 2/3] qapi: change the type of TargetInfo.arch from string to enum SysEmuTarget

[Markus Armbruster]

Laszlo Ersek  writes:

> On 04/23/18 11:50, Markus Armbruster wrote:
>> Laszlo Ersek  writes:
>> 
>>> Now that we have @SysEmuTarget, it makes sense to restict
>>> @TargetInfo.@arch to valid sysemu targets at the schema level.
>> 
>> We could mention that supported targets become visible in QMP
>> introspection.  But I can't see what QMP clients could do with this
>> information, so let's not bother.
>> 
>>>
>>> Cc: "Daniel P. Berrange" 
>>> Cc: David Gibson 
>>> Cc: Eric Blake 
>>> Cc: Gerd Hoffmann 
>>> Cc: Kashyap Chamarthy 
>>> Cc: Markus Armbruster 
>>> Cc: Paolo Bonzini 
>>> Cc: Thomas Huth 
>>> Signed-off-by: Laszlo Ersek 
>>> ---
>>>
>>> Notes:
>>> RFCv3:
>>> 
>>> - The patch is new in this version. [Markus]
>>>
>>>  qapi/misc.json |  6 --
>>>  arch_init.c| 18 +-
>>>  2 files changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qapi/misc.json b/qapi/misc.json
>>> index 5636f4a14997..3b4fbc534089 100644
>>> --- a/qapi/misc.json
>>> +++ b/qapi/misc.json
>>> @@ -5,6 +5,8 @@
>>>  # = Miscellanea
>>>  ##
>>>  
>>> +{ 'include': 'common.json' }
>>> +
>>>  ##
>>>  # @qmp_capabilities:
>>>  #
>>> @@ -2449,12 +2451,12 @@
>>>  #
>>>  # Information describing the QEMU target.
>>>  #
>>> -# @arch: the target architecture (eg "x86_64", "i386", etc)
>>> +# @arch: the target architecture
>>>  #
>>>  # Since: 1.2.0
>>>  ##
>>>  { 'struct': 'TargetInfo',
>>> -  'data': { 'arch': 'str' } }
>>> +  'data': { 'arch': 'SysEmuTarget' } }
>>>  
>>>  ##
>>>  # @query-target:
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 6ee07478bd11..4367f30291f8 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -29,6 +29,7 @@
>>>  #include "hw/pci/pci.h"
>>>  #include "hw/audio/soundhw.h"
>>>  #include "qapi/qapi-commands-misc.h"
>>> +#include "qapi/error.h"
>>>  #include "qemu/config-file.h"
>>>  #include "qemu/error-report.h"
>>>  #include "hw/acpi/acpi.h"
>>> @@ -111,8 +112,23 @@ int xen_available(void)
>>>  TargetInfo *qmp_query_target(Error **errp)
>>>  {
>>>  TargetInfo *info = g_malloc0(sizeof(*info));
>>> +Error *local_err = NULL;
>>>  
>>> -info->arch = g_strdup(TARGET_NAME);
>>> +/*
>>> + * The fallback enum value is irrelevant here (TARGET_NAME is a
>>> + * macro and can never be NULL), so simply pass zero. Also, the
>> 
>> We pass -1 in similar cases elsewhere.
>> 
>>> + * lookup should never fail -- if it fails, then @SysEmuTarget needs
>>> + * extending. Catch that with an assertion, but also handle it
>>> + * gracefully, in case someone adventurous disables assertions.
>>> + */
>>> +info->arch = qapi_enum_parse(_lookup, TARGET_NAME, 0,
>>> + _err);
>>> +g_assert(!local_err);
>>> +if (local_err) {
>>> +error_propagate(errp, local_err);
>>> +qapi_free_TargetInfo(info);
>>> +return NULL;
>>> +}
>> 
>> Simpler:
>> 
>>info->arch = qapi_enum_parse(_lookup, TARGET_NAME, 0,
>> _abort);
>
> OK, I'll use this form -- but should I use -1 or 0, after all, for
> default value?

I count more than a dozen instances of -1, and none of zero.  Please use
-1.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list