Re: [libvirt] [PATCH 1/8] snapshots: Avoid term 'checkpoint' for full system snapshot
On 06/13/2018 12:42 PM, Eric Blake wrote: > Upcoming patches plan to introduce virDomainCheckpointPtr as a new > object for use in incremental backups, along with documentation > how incremental backups differ from snapshots. But first, we need > to rename any existing mention of a 'system checkpoint' to instead > be a 'full system state snapshot', so that we aren't overloading > the term checkpoint. > > Signed-off-by: Eric Blake > > --- > Bikeshed suggestions on what to name the new object for use in > backups is welcome, if we would rather keep the term 'checkpoint' > for a disk+memory snapshot. > --- "Naming is hard" and opinions can vary greatly - be careful for what you ask in case you receive something not wanted ;-). I haven't followed the discussions thus far all that closely, but I'll give this a go anyway since it's languishing and saying nothing is akin to implicitly agreeing everything is fine. Fair warning, I'm not all that familiar with snapshot algorithms having largely tried to ignore it since others (Eric and Peter) have far more in depth knowledge. In any case, another option for the proposed "checkpoint" could be a "snapshot reference". One can start or end a reference period and then set or clear a reference point. What I'm not clear on yet is whether the intention is to have this checkpoint (and backup) be integrated in any way to the existing snapshot algorithms. I guess part of me thinks that if I take a full system snapshot, then any backup/checkpoint data should be included so that if/when I go back to that point in time I can start from whence I left as it relates to my backup. Kind of a superset and/or integrated model rather than something bolted onto the side to resolve a specific need. I suppose a reservation I have about separate virDomainCheckpoint* and virDomainBackup* API's is understanding the relationship between the two naming spaces. IIUC though a Checkpoint would be reference point in time within a Backup period. I do have more comments in patch2, but I want to make them coherent before posting. Still I wanted to be sure you got at least "some" feedback for this and well of course an opinion on checkpoint ;-) > docs/formatsnapshot.html.in | 14 +++--- > include/libvirt/libvirt-domain-snapshot.h | 2 +- > src/conf/snapshot_conf.c | 2 +- > src/libvirt-domain-snapshot.c | 4 ++-- > src/qemu/qemu_driver.c| 12 ++-- > tools/virsh-snapshot.c| 2 +- > tools/virsh.pod | 14 +++--- > 7 files changed, 25 insertions(+), 25 deletions(-) > > diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in > index fbbecfd242..f2e51df5ab 100644 > --- a/docs/formatsnapshot.html.in > +++ b/docs/formatsnapshot.html.in > @@ -33,7 +33,7 @@ > resume in a consistent state; but if the disks are modified > externally in the meantime, this is likely to lead to data > corruption. > - system checkpoint > + full system state Is "state" superfluous in this context? IOW: Everywhere that "full system state" exists, it seems "full system" could be used. Other synonyms that came up are complete, entire, integrated, or thorough (hah!). But I think "Full System" conveys enough meaning even though it could convey more meaning than intended. >A combination of disk snapshots for all disks as well as VM > memory state, which can be used to resume the guest from where it > left off with symptoms similar to hibernation (that is, TCP > @@ -55,7 +55,7 @@ >as virDomainSaveImageGetXMLDesc() to work with >those files. > > -System checkpoints are created > +Full system state snapshots are created >by virDomainSnapshotCreateXML() with no flags, and >disk snapshots are created by the same function with >the VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY flag; in BTW: Existing and maybe it's just me, but when I read a conjunctive sentence with only two parts I don't expect to see ", and" or ", or" - it's just "and" or "or" without the comma Also the "flag; in both cases...", I think should be a "flag. Regardless of the flags value provided, restoration of the snapshot is handled by the virDomainRevertToSnapshot() function." But that's just me being "particular". ;-) There's bigger fish to fry here other than grammar issues. There's so many usages of the "; " to join two sentences in this page - it'd probably take more effort than desired to go through each one. > @@ -128,13 +128,13 @@ > what file name is created in an external snapshot. On output, > this is fully populated to show the state of each disk in the > snapshot, including any properties that were generated by the > -hypervisor defaults. For system checkpoints, this field is > -ignored on input and omitted on output (a system
Re: [libvirt] [qemu-s390x] request a revert for "block: Remove deprecated -drive option serial" (was block: Remove deprecated -drive option serial)
On 22.06.2018 14:51, Christian Borntraeger wrote: > adding more CC. > > On 06/22/2018 01:38 PM, Christian Borntraeger wrote: >> >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: >>> The -drive option serial was deprecated in QEMU 2.10. It's time to >>> remove it. >>> >>> Tests need to be updated to set the serial number with -global instead >>> of using the -drive option. >> >> libvirt 4.5 still creates those (at least on s390x) >> >> >> > iothread='1'/> >> >> >> skel >> >> >> >> -> >> [...] >> -drive >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native >> -device >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on >> >> [...] >> >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: >> Block format 'qcow2' does not support the option 'serial' >> 2018-06-22 11:25:21.098+: shutting down, reason=failed >> >> So it seems that this breaks s390x. I wonder why nobody noticed the deprecation messages before? > So what about reverting commit b0083267444a5e0f28391f6c2831a539f878d424 > "block: Remove deprecated -drive option serial" and redo the removal in > qemu 3.1 (or 3.2) ? > Even if we fix libvirt today, this is certainly a too short period of > time to get things fixed in the field. Agreed, reverting that commit is likely the best thing we can do right now, and then kill it in a later QEMU release. Note that you also need to revert 6266e900b8083945cb766b45c124fb3c42932cb3 first. Thomas -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Am 22.06.2018 um 17:40 hat Daniel P. Berrangé geschrieben: > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote: > > This was in fact one release longer than our deprecation policy says. > > Are we serious about the deprecation policy or aren't we? > > > > I might consider reverting a change if it turned out that this requires > > some massive work in libvirt. But I think this one should be rather easy > > to fix in libvirt until 3.0 is released. > > I've got a patch mostly ready that converts libvirt to setting these things > on the frontend device, however, I've got some queries... > > - usb-storage - doesn't appear to support geometry or werror/rerror > >Will we loose functionality by stopping use of -drive for werror > >Loosing geometry feels relevant too, unless it was already ignored >when set opf -drive ? You're right, usb-storage doesn't allow using -device to specify these, and it does use the values from -drive. For werror/rerror, we should clearly implement the option forwarding to scsi-disk so that you can make use of it. I'm not sure how sane specifying a non-standard CHS geometry for a USB stick actually is. As an additional difficulty, usb-storage internally creates a scsi-disk device (not scsi-hd), which is also considered legacy and doesn't support the geometry options either, so it's not just simple forwarding. We removed an actual feature there, but that feature was probably never intended nor used. If someone comes up with a compelling reason why they really need to configure the CHS geometry of their USB sticks, I guess we can do it. My real USB sticks I tested don't even support MODE_PAGE_HD_GEOMETRY (though they have MODE_PAGE_FLEXIBLE_DISK_GEOMETRY). > - SD card - requires -drive with if=sd, no -device support AFAICT > >Will we loose functionality be stopping use of -drive for if=sd, or >is this stuff ignored anyway ? This was already silently ignored. > - ide-cd/scsi-id - doesn't support geometry > >Presumably becuase its irrelevant for CDs Yes, geometry has no effect for CDs. Kevin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 06/22/2018 05:01 PM, Kevin Wolf wrote: > Am 22.06.2018 um 16:38 hat Christian Borntraeger geschrieben: >> >> >> On 06/22/2018 04:25 PM, Kevin Wolf wrote: >>> Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben: On 06/22/2018 02:55 PM, Kevin Wolf wrote: > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: >> >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: >>> The -drive option serial was deprecated in QEMU 2.10. It's time to >>> remove it. >>> >>> Tests need to be updated to set the serial number with -global instead >>> of using the -drive option. >> >> libvirt 4.5 still creates those (at least on s390x) >> >> >> > iothread='1'/> >> >> >> skel >> >> >> >> >> >> -> >> [...] >> -drive >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native >> -device >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on >> >> [...] >> >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: >> Block format 'qcow2' does not support the option 'serial' >> 2018-06-22 11:25:21.098+: shutting down, reason=failed >> >> So it seems that this breaks s390x. To me it seems that this is also broken on x86. > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > released. I think this is definitely too short notice. We should not break existing setups just by insisting that users have to update libvirt when they update QEMU. Yes, this might be our policy, but doing so "just because we can" is certainly a very bad attitude. I see no fundamental technical reason why we should not revert this change. >>> >>> This was in fact one release longer than our deprecation policy says. >>> Are we serious about the deprecation policy or aren't we? >> >> I think it makes more sense to have 2 releases after everything was fixed >> instead of 2 releases after it was announced. > > This means effectively banning feature removal. The only time that > people actually starting fixing things is when it breaks. So if you > never remove it before everything is fixed, you just never remove it at > all. With the proposal that is floating around (like the --no-deprecation option to be used for regression test suites) this could be solved. > > It's unfortunate, but breaking things at some point is necessary. I hope > the breakage will only last a few days because libvirt will fix this. > > Maybe one thing we could look into for the future is a special > deprecation warning function rather than just error_report(), and we > would make that one fatal in non-release builds so that things break > early, but you can still override it with a ./configure option. > >> So if everyone has adopted we can certainly follow our deprecation policy. >> Now if deprecation breaks some real world cases it makes no sense to >> "insist" on that deprecation policy. Really: If latest greatest libvirt >> does not work 2 weeks before soft freeze I consider this too late. >> >> Why: This breaks MY regression test setup before softfreeze. So I will stop >> testing qemu in the most critical point in time. >> >> If you would come up with your statement (taking deprecation policy more >> serious than users) in the Linux kernel I can pretty much guarantee that >> Linus would call you names. > > Users shouldn't use random git snapshots. Developers can revert the > change locally until libvirt is fixed.g > > If contrary to all expectations, libvirt doesn't manage to get this > fixed until 3.0-rc2, I will consider reverting the patch. But not > significantly earlier than that. I think I made it clear that I consider this wrong on so many levels. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote: > Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben: > > > > > > On 06/22/2018 02:55 PM, Kevin Wolf wrote: > > > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > > >> > > >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: > > >>> The -drive option serial was deprecated in QEMU 2.10. It's time to > > >>> remove it. > > >>> > > >>> Tests need to be updated to set the serial number with -global instead > > >>> of using the -drive option. > > >> > > >> libvirt 4.5 still creates those (at least on s390x) > > >> > > >> > > >>> >> iothread='1'/> > > >> > > >> > > >> skel > > >> > > >> > > >> > > >> > > >> > > >> -> > > >> [...] > > >> -drive > > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > > >> -device > > >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > > >> > > >> [...] > > >> > > >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > > >> Block format 'qcow2' does not support the option 'serial' > > >> 2018-06-22 11:25:21.098+: shutting down, reason=failed > > >> > > >> So it seems that this breaks s390x. > > > > To me it seems that this is also broken on x86. > > > > > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > > > released. > > > > I think this is definitely too short notice. We should not break existing > > setups just by insisting that users have to update libvirt when they update > > QEMU. Yes, this might be our policy, but doing so "just because we can" > > is certainly a very bad attitude. I see no fundamental technical reason why > > we should not revert this change. > > This was in fact one release longer than our deprecation policy says. > Are we serious about the deprecation policy or aren't we? > > I might consider reverting a change if it turned out that this requires > some massive work in libvirt. But I think this one should be rather easy > to fix in libvirt until 3.0 is released. I've got a patch mostly ready that converts libvirt to setting these things on the frontend device, however, I've got some queries... - usb-storage - doesn't appear to support geometry or werror/rerror Will we loose functionality by stopping use of -drive for werror Loosing geometry feels relevant too, unless it was already ignored when set opf -drive ? - SD card - requires -drive with if=sd, no -device support AFAICT Will we loose functionality be stopping use of -drive for if=sd, or is this stuff ignored anyway ? - ide-cd/scsi-id - doesn't support geometry Presumably becuase its irrelevant for CDs 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-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Am 22.06.2018 um 16:38 hat Christian Borntraeger geschrieben: > > > On 06/22/2018 04:25 PM, Kevin Wolf wrote: > > Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben: > >> > >> > >> On 06/22/2018 02:55 PM, Kevin Wolf wrote: > >>> Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > > On 06/15/2018 04:21 PM, Kevin Wolf wrote: > > The -drive option serial was deprecated in QEMU 2.10. It's time to > > remove it. > > > > Tests need to be updated to set the serial number with -global instead > > of using the -drive option. > > libvirt 4.5 still creates those (at least on s390x) > > > iothread='1'/> > > > skel > > > > > > -> > [...] > -drive > file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > -device > virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > > [...] > > 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > Block format 'qcow2' does not support the option 'serial' > 2018-06-22 11:25:21.098+: shutting down, reason=failed > > So it seems that this breaks s390x. > >> > >> To me it seems that this is also broken on x86. > >>> > >>> Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > >>> released. > >> > >> I think this is definitely too short notice. We should not break existing > >> setups just by insisting that users have to update libvirt when they update > >> QEMU. Yes, this might be our policy, but doing so "just because we can" > >> is certainly a very bad attitude. I see no fundamental technical reason why > >> we should not revert this change. > > > > This was in fact one release longer than our deprecation policy says. > > Are we serious about the deprecation policy or aren't we? > > I think it makes more sense to have 2 releases after everything was fixed > instead of 2 releases after it was announced. This means effectively banning feature removal. The only time that people actually starting fixing things is when it breaks. So if you never remove it before everything is fixed, you just never remove it at all. It's unfortunate, but breaking things at some point is necessary. I hope the breakage will only last a few days because libvirt will fix this. Maybe one thing we could look into for the future is a special deprecation warning function rather than just error_report(), and we would make that one fatal in non-release builds so that things break early, but you can still override it with a ./configure option. > So if everyone has adopted we can certainly follow our deprecation policy. > Now if deprecation breaks some real world cases it makes no sense to > "insist" on that deprecation policy. Really: If latest greatest libvirt > does not work 2 weeks before soft freeze I consider this too late. > > Why: This breaks MY regression test setup before softfreeze. So I will stop > testing qemu in the most critical point in time. > > If you would come up with your statement (taking deprecation policy more > serious than users) in the Linux kernel I can pretty much guarantee that > Linus would call you names. Users shouldn't use random git snapshots. Developers can revert the change locally until libvirt is fixed. If contrary to all expectations, libvirt doesn't manage to get this fixed until 3.0-rc2, I will consider reverting the patch. But not significantly earlier than that. Kevin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 06/22/2018 09:30 AM, Daniel P. Berrangé wrote: Perhaps we could use a more structured notification, to make detecting use of deprecated features programmatically trivial. A QMP event might do. Libvirt currently has CI that is largely focused on unit testing. We recently did some work, however, to get our functional test suite working properly again (Sys-Virt-TCK) and are trying to get some new CI hardware. So if we get that running, we coud run tests on real QEMU versions and check the /var/log/libvirt/qemu/$GUEST.logs to make sure we're not triggering unexpected warnings from QEMU This could be even easier if there was a --no-deprecations flag to QEMU which triggered abort() whenever mgmt app uses a deprecated feature. Yes, a QMP event (which libvirt could then turn into a hard error if it ever receives the event) or a qemu command line option to make deprecated usage fatal (which libvirt would choose to enable) would both be pragmatic approaches to quickly vetting whether libvirt is using something that qemu has marked deprecated - provided that we are careful to always wire up the event/abort into qemu at each location where we also add a deprecation message. An event might be more flexible than qemu aborting (as libvirt could make programmatic decisions on whether to keep going in spite of the event, rather than the guest unconditionally being lost). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 22 June 2018 at 15:38, Christian Borntraeger wrote: > So if everyone has adopted we can certainly follow our deprecation policy. > Now if deprecation breaks some real world cases it makes no sense to > "insist" on that deprecation policy. Really: If latest greatest libvirt > does not work 2 weeks before soft freeze I consider this too late. > > Why: This breaks MY regression test setup before softfreeze. So I will stop > testing qemu in the most critical point in time. > > If you would come up with your statement (taking deprecation policy more > serious than users) in the Linux kernel I can pretty much guarantee that > Linus would call you names. This is one of those areas where I like to think the QEMU community is a more pleasant place to be than the kernel :-) The fact we have a deprecation policy at all indicates that we are (unlike the kernel) sometimes willing to break things that previously worked for users; but I think we should be a bit pragmatic as well. If one of our largest use cases (libvirt) missed the memo on this one I don't think we do anybody any favours by sticking to the letter of the rules. thanks -- PMM -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 06/22/2018 04:25 PM, Kevin Wolf wrote: > Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben: >> >> >> On 06/22/2018 02:55 PM, Kevin Wolf wrote: >>> Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: On 06/15/2018 04:21 PM, Kevin Wolf wrote: > The -drive option serial was deprecated in QEMU 2.10. It's time to > remove it. > > Tests need to be updated to set the serial number with -global instead > of using the -drive option. libvirt 4.5 still creates those (at least on s390x) >>> iothread='1'/> skel -> [...] -drive file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native -device virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on [...] 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: Block format 'qcow2' does not support the option 'serial' 2018-06-22 11:25:21.098+: shutting down, reason=failed So it seems that this breaks s390x. >> >> To me it seems that this is also broken on x86. >>> >>> Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is >>> released. >> >> I think this is definitely too short notice. We should not break existing >> setups just by insisting that users have to update libvirt when they update >> QEMU. Yes, this might be our policy, but doing so "just because we can" >> is certainly a very bad attitude. I see no fundamental technical reason why >> we should not revert this change. > > This was in fact one release longer than our deprecation policy says. > Are we serious about the deprecation policy or aren't we? I think it makes more sense to have 2 releases after everything was fixed instead of 2 releases after it was announced. So if everyone has adopted we can certainly follow our deprecation policy. Now if deprecation breaks some real world cases it makes no sense to "insist" on that deprecation policy. Really: If latest greatest libvirt does not work 2 weeks before soft freeze I consider this too late. Why: This breaks MY regression test setup before softfreeze. So I will stop testing qemu in the most critical point in time. If you would come up with your statement (taking deprecation policy more serious than users) in the Linux kernel I can pretty much guarantee that Linus would call you names. > > I might consider reverting a change if it turned out that this requires > some massive work in libvirt. But I think this one should be rather easy > to fix in libvirt until 3.0 is released. I have not heard any reason what we gain by removing these features (and no I dont believe that this increases your maintenance burden a lot). But it clearly breaks things. I suggest: revert the removal patches (all of them cyls,secs,serial etc) and redo them for 3.1. > >>> Sadly, it also shows that deprecation warnings in log files go >>> unnoticed. >> >> In fact whoever added the deprication notice should have followed up >> with the libvirt team to implement that change. no? > > I expect the libvirt developers to read the QEMU Changelog at least for > incompatible changes and deprecations. We can't reasonably go and hunt > for developers for every management tool for QEMU that exists. > > And anyway, if you come across a deprecation warning, that's the time > you should act, not only when it finally breaks. > > Kevin > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote: > Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben: > > > > > > On 06/22/2018 02:55 PM, Kevin Wolf wrote: > > > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > > >> > > >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: > > >>> The -drive option serial was deprecated in QEMU 2.10. It's time to > > >>> remove it. > > >>> > > >>> Tests need to be updated to set the serial number with -global instead > > >>> of using the -drive option. > > >> > > >> libvirt 4.5 still creates those (at least on s390x) > > >> > > >> > > >>> >> iothread='1'/> > > >> > > >> > > >> skel > > >> > > >> > > >> > > >> > > >> > > >> -> > > >> [...] > > >> -drive > > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > > >> -device > > >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > > >> > > >> [...] > > >> > > >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > > >> Block format 'qcow2' does not support the option 'serial' > > >> 2018-06-22 11:25:21.098+: shutting down, reason=failed > > >> > > >> So it seems that this breaks s390x. > > > > To me it seems that this is also broken on x86. > > > > > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > > > released. > > > > I think this is definitely too short notice. We should not break existing > > setups just by insisting that users have to update libvirt when they update > > QEMU. Yes, this might be our policy, but doing so "just because we can" > > is certainly a very bad attitude. I see no fundamental technical reason why > > we should not revert this change. > > This was in fact one release longer than our deprecation policy says. > Are we serious about the deprecation policy or aren't we? > > I might consider reverting a change if it turned out that this requires > some massive work in libvirt. But I think this one should be rather easy > to fix in libvirt until 3.0 is released. It is probably even possible for us to fix it in our July 1st release > > > > Sadly, it also shows that deprecation warnings in log files go > > > unnoticed. > > > > In fact whoever added the deprication notice should have followed up > > with the libvirt team to implement that change. no? > > I expect the libvirt developers to read the QEMU Changelog at least for > incompatible changes and deprecations. We can't reasonably go and hunt > for developers for every management tool for QEMU that exists. Yeah, from libvirt side we need todo a better job of checking this and filing bugs against libvirt if there's something we tickle. 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-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Fri, Jun 22, 2018 at 03:25:19PM +0100, Daniel P. Berrangé wrote: > On Fri, Jun 22, 2018 at 04:19:29PM +0200, Markus Armbruster wrote: > > Kevin Wolf writes: > > > > > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > > >> > > >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: > > >> > The -drive option serial was deprecated in QEMU 2.10. It's time to > > >> > remove it. > > >> > > > >> > Tests need to be updated to set the serial number with -global instead > > >> > of using the -drive option. > > >> > > >> libvirt 4.5 still creates those (at least on s390x) > > >> > > >> > > >>> >> iothread='1'/> > > >> > > >> > > >> skel > > >> > > >> > > >> > > >> > > >> > > >> -> > > >> [...] > > >> -drive > > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > > >> -device > > >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > > >> > > >> [...] > > >> > > >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > > >> Block format 'qcow2' does not support the option 'serial' > > >> 2018-06-22 11:25:21.098+: shutting down, reason=failed > > >> > > >> So it seems that this breaks s390x. > > > > > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > > > released. > > > > > > Sadly, it also shows that deprecation warnings in log files go > > > unnoticed. > > > > Nobody reads log files until things have gone belly up, and even then > > unrelated log entries get ignored. > > > > The way to get deprecation warnings noticed it to have the management > > application fail its "make check". > > > > Perhaps we could use a more structured notification, to make detecting > > use of deprecated features programmatically trivial. A QMP event might > > do. > > Libvirt currently has CI that is largely focused on unit testing. We > recently did some work, however, to get our functional test suite > working properly again (Sys-Virt-TCK) and are trying to get some > new CI hardware. So if we get that running, we coud run tests on real > QEMU versions and check the /var/log/libvirt/qemu/$GUEST.logs to > make sure we're not triggering unexpected warnings from QEMU This could be even easier if there was a --no-deprecations flag to QEMU which triggered abort() whenever mgmt app uses a deprecated feature. 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-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Fri, Jun 22, 2018 at 04:19:29PM +0200, Markus Armbruster wrote: > Kevin Wolf writes: > > > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > >> > >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: > >> > The -drive option serial was deprecated in QEMU 2.10. It's time to > >> > remove it. > >> > > >> > Tests need to be updated to set the serial number with -global instead > >> > of using the -drive option. > >> > >> libvirt 4.5 still creates those (at least on s390x) > >> > >> > >>>> iothread='1'/> > >> > >> > >> skel > >> > >> > >> > >> > >> > >> -> > >> [...] > >> -drive > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > >> -device > >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > >> > >> [...] > >> > >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > >> Block format 'qcow2' does not support the option 'serial' > >> 2018-06-22 11:25:21.098+: shutting down, reason=failed > >> > >> So it seems that this breaks s390x. > > > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > > released. > > > > Sadly, it also shows that deprecation warnings in log files go > > unnoticed. > > Nobody reads log files until things have gone belly up, and even then > unrelated log entries get ignored. > > The way to get deprecation warnings noticed it to have the management > application fail its "make check". > > Perhaps we could use a more structured notification, to make detecting > use of deprecated features programmatically trivial. A QMP event might > do. Libvirt currently has CI that is largely focused on unit testing. We recently did some work, however, to get our functional test suite working properly again (Sys-Virt-TCK) and are trying to get some new CI hardware. So if we get that running, we coud run tests on real QEMU versions and check the /var/log/libvirt/qemu/$GUEST.logs to make sure we're not triggering unexpected warnings from QEMU 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-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben: > > > On 06/22/2018 02:55 PM, Kevin Wolf wrote: > > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > >> > >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: > >>> The -drive option serial was deprecated in QEMU 2.10. It's time to > >>> remove it. > >>> > >>> Tests need to be updated to set the serial number with -global instead > >>> of using the -drive option. > >> > >> libvirt 4.5 still creates those (at least on s390x) > >> > >> > >>>> iothread='1'/> > >> > >> > >> skel > >> > >> > >> > >> > >> > >> -> > >> [...] > >> -drive > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > >> -device > >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > >> > >> [...] > >> > >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > >> Block format 'qcow2' does not support the option 'serial' > >> 2018-06-22 11:25:21.098+: shutting down, reason=failed > >> > >> So it seems that this breaks s390x. > > To me it seems that this is also broken on x86. > > > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > > released. > > I think this is definitely too short notice. We should not break existing > setups just by insisting that users have to update libvirt when they update > QEMU. Yes, this might be our policy, but doing so "just because we can" > is certainly a very bad attitude. I see no fundamental technical reason why > we should not revert this change. This was in fact one release longer than our deprecation policy says. Are we serious about the deprecation policy or aren't we? I might consider reverting a change if it turned out that this requires some massive work in libvirt. But I think this one should be rather easy to fix in libvirt until 3.0 is released. > > Sadly, it also shows that deprecation warnings in log files go > > unnoticed. > > In fact whoever added the deprication notice should have followed up > with the libvirt team to implement that change. no? I expect the libvirt developers to read the QEMU Changelog at least for incompatible changes and deprecations. We can't reasonably go and hunt for developers for every management tool for QEMU that exists. And anyway, if you come across a deprecation warning, that's the time you should act, not only when it finally breaks. Kevin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Kevin Wolf writes: > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: >> >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: >> > The -drive option serial was deprecated in QEMU 2.10. It's time to >> > remove it. >> > >> > Tests need to be updated to set the serial number with -global instead >> > of using the -drive option. >> >> libvirt 4.5 still creates those (at least on s390x) >> >> >> > iothread='1'/> >> >> >> skel >> >> >> >> >> >> -> >> [...] >> -drive >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native >> -device >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on >> >> [...] >> >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: >> Block format 'qcow2' does not support the option 'serial' >> 2018-06-22 11:25:21.098+: shutting down, reason=failed >> >> So it seems that this breaks s390x. > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > released. > > Sadly, it also shows that deprecation warnings in log files go > unnoticed. Nobody reads log files until things have gone belly up, and even then unrelated log entries get ignored. The way to get deprecation warnings noticed it to have the management application fail its "make check". Perhaps we could use a more structured notification, to make detecting use of deprecated features programmatically trivial. A QMP event might do. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Fri, Jun 22, 2018 at 03:36:50PM +0200, Christian Borntraeger wrote: > > > On 06/22/2018 02:55 PM, Kevin Wolf wrote: > > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > >> > >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: > >>> The -drive option serial was deprecated in QEMU 2.10. It's time to > >>> remove it. > >>> > >>> Tests need to be updated to set the serial number with -global instead > >>> of using the -drive option. > >> > >> libvirt 4.5 still creates those (at least on s390x) > >> > >> > >>>> iothread='1'/> > >> > >> > >> skel > >> > >> > >> > >> > >> > >> -> > >> [...] > >> -drive > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > >> -device > >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > >> > >> [...] > >> > >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > >> Block format 'qcow2' does not support the option 'serial' > >> 2018-06-22 11:25:21.098+: shutting down, reason=failed > >> > >> So it seems that this breaks s390x. > > To me it seems that this is also broken on x86. Correct, this is not architecture specific. > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > > released. > > I think this is definitely too short notice. We should not break existing > setups just by insisting that users have to update libvirt when they update > QEMU. Yes, this might be our policy, but doing so "just because we can" > is certainly a very bad attitude. I see no fundamental technical reason why > we should not revert this change. > > > Sadly, it also shows that deprecation warnings in log files go > > unnoticed. > > In fact whoever added the deprication notice should have followed up > with the libvirt team to implement that change. no? On libvirt side I thought we had already stopped using the deprecated syntax, but we clearly missed it :-( 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-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 06/22/2018 03:36 PM, Christian Borntraeger wrote: > > > On 06/22/2018 02:55 PM, Kevin Wolf wrote: >> Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: >>> >>> On 06/15/2018 04:21 PM, Kevin Wolf wrote: The -drive option serial was deprecated in QEMU 2.10. It's time to remove it. Tests need to be updated to set the serial number with -global instead of using the -drive option. >>> >>> libvirt 4.5 still creates those (at least on s390x) >>> >>> >>> >> iothread='1'/> >>> >>> >>> skel >>> >>> >>> >>> >>> >>> -> >>> [...] >>> -drive >>> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native >>> -device >>> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on >>> >>> [...] >>> >>> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive >>> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: >>> Block format 'qcow2' does not support the option 'serial' >>> 2018-06-22 11:25:21.098+: shutting down, reason=failed >>> >>> So it seems that this breaks s390x. > > To me it seems that this is also broken on x86. >> >> Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is >> released. > > I think this is definitely too short notice. We should not break existing > setups just by insisting that users have to update libvirt when they update > QEMU. Yes, this might be our policy, but doing so "just because we can" > is certainly a very bad attitude. I see no fundamental technical reason why > we should not revert this change. > > >> >> Sadly, it also shows that deprecation warnings in log files go >> unnoticed. > > In fact whoever added the deprication notice should have followed up > with the libvirt team to implement that change. no? FWIW cyls, heads, secs and trans also seem to be affected by this. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 0/2] Add new mdev type for aggregated resources
On 6/22/2018 1:12 PM, Zhenyu Wang wrote:
> On 2018.06.21 09:00:28 -0600, Alex Williamson wrote:
>> On Thu, 21 Jun 2018 19:57:38 +0530
>> Kirti Wankhede wrote:
>>
>>> On 6/20/2018 1:10 PM, Zhenyu Wang wrote:
Current mdev device create interface depends on fixed mdev type, which get
uuid
from user to create instance of mdev device. If user wants to use
customized
number of resource for mdev device, then only can create new mdev type for
that
which may not be flexible.
To allow to create user defined resources for mdev, this RFC trys
to extend mdev create interface by adding new "instances=xxx" parameter
following uuid, for target mdev type if aggregation is supported, it can
create new mdev device which contains resources combined by number of
instances, e.g
echo ",instances=10" > create
>>>
>>> This seems orthogonal to the way mdev types are meant to be used. Vendor
>>> driver can provide the possible types to provide flexibility to the users.
>>
>> I think the goal here is to define how we create a type that supports
>> arbitrary combinations of resources where the total number of those
>> resources my be sufficiently large that the parent driver enumerating
>> them all is not feasible.
>>
>>> Secondly, not always all resources defined for a particular mdev type
>>> can be multiplied, for example, a mdev type for vGPU that supports 2
>>> heads, that can't be multiplied to use with 20 heads.
>
> yeah, for vGPU we actually can have flexible config for memory size but
> currently fixed by type definition. Although like for display config, we
> are just sticking with 1 head config even for aggregate type.
>
A mdev type is set of parameters. If aggregation is on one parameter,
how can user know which parameter can be aggregated?
>>
>> Not all types need to define themselves this way, aiui this is an
>> optional extension. Userspace can determine if this feature is
>> available with the new attribute and if they're not aware of the new
>> attribute, we operate in a backwards compatible mode where 'echo $UUID >
>> create' consumes one instance of that type. Mdev, like vfio, is device
>> agnostic, so while a vGPU example may have scaling issues that need to
>> be ironed out to implement this, those don't immediately negate the
>> value of this proposal. Thanks,
>>
>
> yep, backward compatible is always ensured by this, so for no
> aggregation attrib type, still keep current behavior, driver should
> refuse "instances=" if set by user. One thing I'm concern is that
> looks currently without changing mdev create ops interface, can't
> carry that option for driver, or should we do with more general parameter?
>
I think, if aggregation is not supported then vendor driver should fail
'create' with instance >1. That means every vendor driver would need a
change to handle this case.
Other way could be, structure mdev_parent_ops can have other function
pointer 'create_with_instances' which has instances as an argument.
Vendor driver which support aggregation will have to provide this
callback and existing vendor driver will need no change. Then in mdev core:
if (instances > 1) {
if (parent->ops->create_with_instances)
ret = parent->ops->create_with_instances(kobj, mdev, instances);
else
return -EINVAL;
} else
ret = parent->ops->create(kobj, mdev);
Thanks,
Kirti
> thanks
>
>>
VM manager e.g libvirt can check mdev type with "aggregation" attribute
which can support this setting. And new sysfs attribute "instances" is
created for each mdev device to show allocated number. Default number
of 1 or no "instances" file can be used for compatibility check.
This RFC trys to create new KVMGT type with minimal vGPU resources which
can be combined with "instances=x" setting to allocate for user wanted
resources.
Zhenyu Wang (2):
vfio/mdev: Add new instances parameters for mdev create
drm/i915/gvt: Add new aggregation type
drivers/gpu/drm/i915/gvt/gvt.c | 26 ---
drivers/gpu/drm/i915/gvt/gvt.h | 14 +---
drivers/gpu/drm/i915/gvt/kvmgt.c | 9 +++--
drivers/gpu/drm/i915/gvt/vgpu.c | 56
drivers/s390/cio/vfio_ccw_ops.c | 3 +-
drivers/vfio/mdev/mdev_core.c| 11 ---
drivers/vfio/mdev/mdev_private.h | 6 +++-
drivers/vfio/mdev/mdev_sysfs.c | 42
include/linux/mdev.h | 3 +-
samples/vfio-mdev/mbochs.c | 3 +-
samples/vfio-mdev/mdpy.c | 3 +-
samples/vfio-mdev/mtty.c | 3 +-
12 files changed, 141 insertions(+), 38 deletions(-)
>>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: formatdomain: Note the caveats for CPU policy option "force"
On Fri, Jun 22, 2018 at 02:19:52PM +0200, Jiri Denemark wrote: > On Tue, Jun 12, 2018 at 10:58:46 +0200, Kashyap Chamarthy wrote: [...] > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > > index 6912762f28..4d6c3892ee 100644 > > --- a/docs/formatdomain.html.in > > +++ b/docs/formatdomain.html.in > > @@ -1566,8 +1566,17 @@ > > > > > >force > > - The virtual CPU will claim the feature is supported > > regardless > > -of it being supported by host CPU. > > The original content was indented. Also I don't think you're supposed to > add two elements after single . Hmm, I made a test locally to see it renders as intented. Because, I wnated the second paragraph to be on a separate line. > > > + The virtual CPU will claim the feature is supported > > + regardless of it being supported by host CPU -- this is only > > + true for QEMU version older than 2.9.0. > > I'd suggest > > Libvirt will ask the hypervisor to enable the feature regardless of > it being supported by host CPU. The hypervisor will likely ignore > this request if the feature is not supported by the host CPU or if > it cannot be emulated. In other words, it is useful for features > which do not exist in real hardware, such as 'virt-ssbd', or which > can be emulated, such as 'x2apic'. Yes, your wording is much clearer. > > I.e. when using the > > + CPU mode 'host-model', libvirt identifies which CPU features > > + to use by looking at host CPUID. For that to take effect, it > > + is mandatory to use force to tell libvirt that a > > + said CPU feature must be used despite it not existing in the > > + host -- this applicable only for a very limited set of CPU > > + features, such as 'x2apic', virt-ssbd' (for AMD CPUs). > > + However, when using QEMU 2.9.0 and above, there should > > + never be any need to use force. > > What was the motivation behind this change? I find it quite confusing > that this is supposed to describe 'force' policy and suddenly you're > talking about host-model CPU mode. Hmm, re-reading, indeed the flow from the previous sentence to the "I.e. when using the CPU mode 'host-model'" seems like a non-sequitur. My intention was to add a note about in which scenarios specifying 'force' makes sense. IIRC, I learnt from Dan Berrangé that if you are using a QEMU version that is older than 2.9.0, then 'force' is required for a guest that needs features that don't exist in the real CPU. If you are using QEMU >= 2.9.0, then 'force' is not required. -- /kashyap -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 06/22/2018 02:55 PM, Kevin Wolf wrote: > Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: >> >> On 06/15/2018 04:21 PM, Kevin Wolf wrote: >>> The -drive option serial was deprecated in QEMU 2.10. It's time to >>> remove it. >>> >>> Tests need to be updated to set the serial number with -global instead >>> of using the -drive option. >> >> libvirt 4.5 still creates those (at least on s390x) >> >> >> > iothread='1'/> >> >> >> skel >> >> >> >> >> >> -> >> [...] >> -drive >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native >> -device >> virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on >> >> [...] >> >> 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive >> file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: >> Block format 'qcow2' does not support the option 'serial' >> 2018-06-22 11:25:21.098+: shutting down, reason=failed >> >> So it seems that this breaks s390x. To me it seems that this is also broken on x86. > > Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is > released. I think this is definitely too short notice. We should not break existing setups just by insisting that users have to update libvirt when they update QEMU. Yes, this might be our policy, but doing so "just because we can" is certainly a very bad attitude. I see no fundamental technical reason why we should not revert this change. > > Sadly, it also shows that deprecation warnings in log files go > unnoticed. In fact whoever added the deprication notice should have followed up with the libvirt team to implement that change. no? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Escape commas for qemuBuildSCSIiSCSIHostdevDrvStr
On 06/21/2018 06:55 PM, John Ferlan wrote:
>
>
> On 06/20/2018 09:17 AM, Anya Harter wrote:
>> Add comma escaping for netsource
>>
>> Signed-off-by: Anya Harter
>> ---
>> src/qemu/qemu_command.c | 4 +++-
>> tests/qemuxml2argvdata/name-escape.args | 6 +-
>> tests/qemuxml2argvdata/name-escape.xml | 7 +++
>> tests/qemuxml2argvtest.c| 5 -
>> 4 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index a99240992a..96c6c08c2d 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -4677,7 +4677,9 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr
>> dev,
>> if (!(netsource = qemuBuildNetworkDriveStr(iscsisrc->src, srcPriv ?
>> srcPriv->secinfo :
>> NULL)))
>> goto cleanup;
>> -virBufferAsprintf(, "file=%s,if=none,format=raw", netsource);
>> +virBufferAddLit(, "file=");
>> +virQEMUBuildBufferEscapeComma(, netsource);
>> +virBufferAddLit(, ",if=none,format=raw");
>> }
>>
>> if (virBufferCheckError() < 0)
>> diff --git a/tests/qemuxml2argvdata/name-escape.args
>> b/tests/qemuxml2argvdata/name-escape.args
>> index aef7c238ca..1cbb1efd66 100644
>> --- a/tests/qemuxml2argvdata/name-escape.args
>> +++ b/tests/qemuxml2argvdata/name-escape.args
>> @@ -22,6 +22,7 @@ bar=2/monitor.sock,server,nowait \
>> -no-shutdown \
>> -no-acpi \
>> -boot c \
>> +-device lsi,id=scsi0,bus=pci.0,addr=0x3 \
>
> [1]
>
>> -device usb-ccid,id=ccid0,bus=usb.0,port=1 \
>> -usb \
>> -drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\
>> @@ -41,4 +42,7 @@
>> cert3=cert3,db=/etc/pki/nssdb,,foo,id=smartcard0,bus=ccid0.0 \
>> -spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock,gl=on,\
>> rendernode=/dev/dri/foo,,bar \
>> -device cirrus-vga,id=video0,bus=pci.0,addr=0x2 \
>> --device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
>> +-drive
>> file=iscsi://example,,foo.org:3260/iqn.1992-01.com.example/0,if=none,\
>> +format=raw,id=drive-hostdev0 \
>> +-device scsi-generic,bus=scsi0.0,scsi-id=4,drive=drive-hostdev0,id=hostdev0
>> \
>> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
>> diff --git a/tests/qemuxml2argvdata/name-escape.xml
>> b/tests/qemuxml2argvdata/name-escape.xml
>> index 70a1ce09d3..24dd248842 100644
>> --- a/tests/qemuxml2argvdata/name-escape.xml
>> +++ b/tests/qemuxml2argvdata/name-escape.xml
>> @@ -27,6 +27,13 @@
>>
>>
>>
>> +
>
> [1] If you change this to:
>
>
>
> Then the lsi doesn't show up...
>
>> +
>> +
>> +
>> +
>
> Something is not quite right about seeing "example,foo.org" as a host
> name. I don't see that as a valid host name regardless of comma
> escaping. Do we escape commas in other "" string
> fields? I don't see it being done for qemuBuildChrChardevStr (search on
> TCP or "host.*name" in qemu_command.c)>
> Now the one thing that perhaps *could* be escaped is the name
> attribute. Currently it's set to "iqn.1992-01.com.example", but the spec
> defines a structure of that name field to have a field that is a "String
> defined by the naming authority" (shorter version, see
> https://en.wikipedia.org/wiki/ISCSI)
>
> So if the name is : 'iqn.1992-01.com.example:storage/2', then the
> "storage/2" piece is the string defined by the naming authority. Not
> that I know whether it would work, but that's where I suppose a comma
> could go.
If you could give me a string with a comma in it, I would be happy to
put it into the test instead of in the host name field. However, if no
one can think of a realistic case where comma escaping would be
necessary, we can just remove the test altogether. I only put it in as a
test that implementation was correct and that the commas were actually
being escaped correctly.
>
>> +
>> +
>>
>>
>>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index 582a9de7bb..2e891a0bea 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -2772,7 +2772,10 @@ mymain(void)
>> QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
>> QEMU_CAPS_DEVICE_ISA_SERIAL,
>> QEMU_CAPS_CHARDEV_FILE_APPEND,
>> -QEMU_CAPS_CCID_EMULATED);
>> +QEMU_CAPS_CCID_EMULATED,
>> +QEMU_CAPS_VIRTIO_SCSI,
>> +QEMU_CAPS_SCSI_LSI,
>
> [1] and the LSI wouldn't be needed here.
>
> John
>
> first patch looks fine, but I'm not so convinced on this one only
> because of the host name w/ a comma.
>
>> +QEMU_CAPS_DEVICE_SCSI_GENERIC);
>> DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS);
>>
>> DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET);
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Am 22.06.2018 um 13:38 hat Christian Borntraeger geschrieben: > > On 06/15/2018 04:21 PM, Kevin Wolf wrote: > > The -drive option serial was deprecated in QEMU 2.10. It's time to > > remove it. > > > > Tests need to be updated to set the serial number with -global instead > > of using the -drive option. > > libvirt 4.5 still creates those (at least on s390x) > > > > > > skel > > > > > > -> > [...] > -drive > file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > -device > virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > > [...] > > 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > Block format 'qcow2' does not support the option 'serial' > 2018-06-22 11:25:21.098+: shutting down, reason=failed > > So it seems that this breaks s390x. Thanks for bringing this up. libvirt should fix this before QEMU 3.0 is released. Sadly, it also shows that deprecation warnings in log files go unnoticed. Kevin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] request a revert for "block: Remove deprecated -drive option serial" (was block: Remove deprecated -drive option serial)
adding more CC. On 06/22/2018 01:38 PM, Christian Borntraeger wrote: > > On 06/15/2018 04:21 PM, Kevin Wolf wrote: >> The -drive option serial was deprecated in QEMU 2.10. It's time to >> remove it. >> >> Tests need to be updated to set the serial number with -global instead >> of using the -drive option. > > libvirt 4.5 still creates those (at least on s390x) > > > > > > skel > > > > > > -> > [...] > -drive > file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native > -device > virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on > > [...] > > 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive > file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: > Block format 'qcow2' does not support the option 'serial' > 2018-06-22 11:25:21.098+: shutting down, reason=failed > > So it seems that this breaks s390x. So what about reverting commit b0083267444a5e0f28391f6c2831a539f878d424 "block: Remove deprecated -drive option serial" and redo the removal in qemu 3.1 (or 3.2) ? Even if we fix libvirt today, this is certainly a too short period of time to get things fixed in the field. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: formatdomain: Note the caveats for CPU policy option "force"
On Tue, Jun 12, 2018 at 10:58:46 +0200, Kashyap Chamarthy wrote: > Eduardo Habkost has pointed out that the current documentation of > libvirt's CPU feature policy "require" vs. "force" does not match > QEMU's behaviour. > > Update the documentation by spelling out the QEMU version dependency and > explain in which scenarios the usage of "policy = 'force'" is applicable > or not. > > Signed-off-by: Kashyap Chamarthy > --- > Wordsmithing / corrections welcome. > --- > docs/formatdomain.html.in | 13 +++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 6912762f28..4d6c3892ee 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -1566,8 +1566,17 @@ > > >force > - The virtual CPU will claim the feature is supported regardless > -of it being supported by host CPU. The original content was indented. Also I don't think you're supposed to add two elements after single . > + The virtual CPU will claim the feature is supported > + regardless of it being supported by host CPU -- this is only > + true for QEMU version older than 2.9.0. I'd suggest Libvirt will ask the hypervisor to enable the feature regardless of it being supported by host CPU. The hypervisor will likely ignore this request if the feature is not supported by the host CPU or if it cannot be emulated. In other words, it is useful for features which do not exist in real hardware, such as 'virt-ssbd', or which can be emulated, such as 'x2apic'. > I.e. when using the > + CPU mode 'host-model', libvirt identifies which CPU features > + to use by looking at host CPUID. For that to take effect, it > + is mandatory to use force to tell libvirt that a > + said CPU feature must be used despite it not existing in the > + host -- this applicable only for a very limited set of CPU > + features, such as 'x2apic', virt-ssbd' (for AMD CPUs). > + However, when using QEMU 2.9.0 and above, there should > + never be any need to use force. What was the motivation behind this change? I find it quite confusing that this is supposed to describe 'force' policy and suddenly you're talking about host-model CPU mode. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 19/20] nwfilter: wire up new APIs for creating and deleting nwfilter bindings
On Mon, Jun 18, 2018 at 04:59:50PM -0400, John Ferlan wrote:
>
>
> On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> > This allows the virsh commands nwfilter-binding-create and
> > nwfilter-binding-delete to be used.
> >
> > Note using these commands lets you delete filters that were
> > previously created automatically by the virt drivers, or add
> > filters for VM nics that were not there before. Generally it
> > is expected these new APIs will only be used by virt drivers.
> > It is the admin's responsibility to not shoot themselves in
> > the foot.
>
> Can't wait to see QE get ahold of this ;-)
>
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > src/nwfilter/nwfilter_driver.c | 79 ++
> > 1 file changed, 79 insertions(+)
> >
>
> I think with a couple of extra comments as described below, then this
> looks fine. Not sure how the other point regarding calling CreateXML
> from the ConfNWFilterInstantiate path (and the reload of load all
> configs) will be handled, but I'm sure it'll be something handled in
> patch 16 and 20, so the comment below is just the "tracing" I did while
> reviewing...
>
> Reviewed-by: John Ferlan
>
> John
>
> > diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> > index 6bfb584b09..2b6856a36c 100644
> > --- a/src/nwfilter/nwfilter_driver.c
> > +++ b/src/nwfilter/nwfilter_driver.c
> > @@ -788,6 +788,83 @@ nwfilterBindingGetXMLDesc(virNWFilterBindingPtr
> > binding,
> > }
> >
> >
>
> Because "not everyone" reads the commit message that added this, I think
> adding a few comments here and for BindingDelete to essentially indicate
> the same as the commit message would be good.
>
> > +static virNWFilterBindingPtr
> > +nwfilterBindingCreateXML(virConnectPtr conn,
> > + const char *xml,
> > + unsigned int flags)
> > +{
> > +virNWFilterBindingObjPtr obj;
> > +virNWFilterBindingDefPtr def;
> > +virNWFilterBindingPtr ret = NULL;
> > +
> > +virCheckFlags(0, NULL);
> > +
> > +def = virNWFilterBindingDefParseString(xml);
> > +if (!def)
> > +return NULL;
> > +
> > +if (virNWFilterBindingCreateXMLEnsureACL(conn, def) < 0)
> > +goto cleanup;
> > +
> > +obj = virNWFilterBindingObjListFindByPortDev(driver->bindings,
> > def->portdevname);
> > +if (obj) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Filter already present for NIC %s"),
> > def->portdevname);
>
> Recall my point from patch 16 regarding the existence of the filter.
> From certain paths it's OK if it exists but once this becomes functional
> for the subsequent patch via virDomainConfNWFilterInstantiate, then the
> issue from patch 16 moves to patch 20 (e.g. guest not restarting).
In this case, I think I'll change the calling code so that it first checks
whether the filter exists or not, instead of unconditionally trying to
recreate it.
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-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 06/15/2018 04:21 PM, Kevin Wolf wrote: > The -drive option serial was deprecated in QEMU 2.10. It's time to > remove it. > > Tests need to be updated to set the serial number with -global instead > of using the -drive option. libvirt 4.5 still creates those (at least on s390x) skel -> [...] -drive file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native -device virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on [...] 2018-06-22T11:25:20.946024Z qemu-system-s390x: -drive file=/var/lib/libvirt/qemu/image.zhyp137,format=qcow2,if=none,id=drive-virtio-disk0,serial=skel,cache=none,aio=native: Block format 'qcow2' does not support the option 'serial' 2018-06-22 11:25:21.098+: shutting down, reason=failed So it seems that this breaks s390x. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 17/20] nwfilter: remove virt driver callback layer for rebuilding filters
On Mon, Jun 18, 2018 at 04:59:37PM -0400, John Ferlan wrote:
>
>
> On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> > Now that the nwfilter driver keeps a list of bindings that it has
> > created, there is no need for the complex virt driver callbacks. It is
> > possible to simply iterate of the list of recorded filter bindings.
> >
> > This means that rebuilding filters no longer has to acquire any locks on
> > the virDomainObj objects, as they're never touched.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > src/conf/nwfilter_conf.c | 134 +++-
> > src/conf/nwfilter_conf.h | 51 +---
> > src/conf/virnwfilterobj.c | 4 +-
> > src/libvirt_private.syms | 7 +-
> > src/lxc/lxc_driver.c | 28 -
> > src/nwfilter/nwfilter_driver.c | 21 ++--
> > src/nwfilter/nwfilter_gentech_driver.c | 167 -
> > src/nwfilter/nwfilter_gentech_driver.h | 4 +-
> > src/qemu/qemu_driver.c | 25
> > src/uml/uml_driver.c | 29 -
> > 10 files changed, 141 insertions(+), 329 deletions(-)
> >
>
> A diffstat that Jano usually applauds! Miracles do happen when you
> close your eyes and say 3 times "I wish this code would just go away"
> ;-). Still this is some of the most "entertaining code" - that now used
> to exist! It seems I can dig up my nwfilter obj/hash code once this is
> in...
>
> There's a couple nits below...
>
> Reviewed-by: John Ferlan
>
> John
>
>
> > diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
> > index de26a6d034..5bb8a0c2e7 100644
> > --- a/src/conf/nwfilter_conf.c
> > +++ b/src/conf/nwfilter_conf.c
>
> [...]
>
> > +
> > +
> > +int
> > +virNWFilterTriggerRebuild(void)
> > +{
> > +return rebuildCallback(rebuildOpaque);
>
> In the better safe than sorry - should we gate on "if
> (rebuildCallback)"? I don't think there's a way into here with it being
> NULL, but those are always "famous last words".
Yeah ok.
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 v3 16/20] nwfilter: keep track of active filter bindings
On Mon, Jun 18, 2018 at 04:59:12PM -0400, John Ferlan wrote:
>
>
> On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> > Currently the nwfilter driver does not keep any record of what filter
> > bindings it has active. This means that when it needs to recreate
> > filters, it has to rely on triggering callbacks provided by the virt
> > drivers. This introduces a hash table recording the virNWFilterBinding
> > objects so the driver has a record of all active filters.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > src/conf/virnwfilterobj.h | 4 ++
> > src/nwfilter/nwfilter_driver.c | 86 --
> > 2 files changed, 65 insertions(+), 25 deletions(-)
> >
>
> [...]
>
> > diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> > index 7202691646..2388e925fc 100644
> > --- a/src/nwfilter/nwfilter_driver.c
> > +++ b/src/nwfilter/nwfilter_driver.c
> > @@ -38,7 +38,6 @@
> > #include "domain_conf.h"
> > #include "domain_nwfilter.h"
> > #include "nwfilter_driver.h"
> > -#include "virnwfilterbindingdef.h"
> > #include "nwfilter_gentech_driver.h"
> > #include "configmake.h"
> > #include "virfile.h"
> > @@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged,
> > virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> > void *opaque ATTRIBUTE_UNUSED)
> > {
>
> [...]
>
> >
> > if (virNWFilterObjListLoadAllConfigs(driver->nwfilters,
> > driver->configDir) < 0)
> > goto error;
> >
> > +if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings,
> > driver->bindingDir) < 0)
> > +goto error;
> > +
>
> Because of this... [1]
>
> > nwfilterDriverUnlock();
> >
> > return 0;
> >
>
> [...]
>
> > @@ -647,13 +656,37 @@ nwfilterInstantiateFilter(const char *vmname,
> >const unsigned char *vmuuid,
> >virDomainNetDefPtr net)
> > {
> > -virNWFilterBindingDefPtr binding;
> > +virNWFilterBindingObjPtr obj;
> > +virNWFilterBindingDefPtr def;
> > int ret;
> >
> > -if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
> > +obj = virNWFilterBindingObjListFindByPortDev(driver->bindings,
> > net->ifname);
> > +if (obj) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Filter already present for NIC %s"),
> > net->ifname);
>
> [1]
> If I stop at this patch, start a domain with a filter, then restart
> libvirtd, then that will cause a guest running with a filter to exit and
> not just disappear - as it can be restarted. The error is:
>
> 2018-06-18 16:49:07.405+: 3978: error :
> nwfilterInstantiateFilter:666 : internal error: Filter already present
> for NIC vnet1
>
> Because once I have this patch built/running the
> /var/run/libvirt/nwfilter-binding/vnet1.xml exists and get's loaded when
> virNWFilterBindingObjListLoadAllConfigs is called at StateInitialize.
>
> I only saw this because I found later in patch 20 the failure comes from
> nwfilterBindingCreateXML instead when virDomainConfNWFilterInstantiate
> is called. I worked my way back to this point.
>
> Not sure which would be the "best" solution at this point. Should we
> wait to do [1] until patch 20 or perhaps just not have an error here.
The current semantics are that nwfilterInstantiateFilter will not
report an error if the filter already exists, so I think I'll just
not report an error here. This method will go away anyway, so no
great loss.
>
> NB: If the guest was running at a point up through patch 15 then it
> won't exit on the first libvirtd restart (since the obj dir doesn't
> exist), but the issue shows up on the 2nd restart.
>
> In general the code is fine to me, but just need to handle this one
> issue in some way.
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 v3 14/20] conf: introduce a virNWFilterBindingObjPtr struct
On Mon, Jun 18, 2018 at 10:08:00AM -0400, John Ferlan wrote:
>
>
> On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> > Introduce a new struct to act as the stateful owner of the
> > virNWFilterBindingDefPtr objects.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > src/conf/Makefile.inc.am | 2 +
> > src/conf/virnwfilterbindingobj.c | 299 +++
> > src/conf/virnwfilterbindingobj.h | 69 +++
> > src/libvirt_private.syms | 14 ++
> > 4 files changed, 384 insertions(+)
> > create mode 100644 src/conf/virnwfilterbindingobj.c
> > create mode 100644 src/conf/virnwfilterbindingobj.h
> >
>
> While continuing, I tripped across this:
>
> > +
> > +static virNWFilterBindingObjPtr
> > +virNWFilterBindingObjParseNode(xmlDocPtr doc,
> > + xmlNodePtr root)
> > +{
> > +xmlXPathContextPtr ctxt = NULL;
> > +virNWFilterBindingObjPtr obj = NULL;
> > +
> > +if (STRNEQ((const char *)root->name, "filterbinding")) {
>
> "filterbindingstatus"
Oh fun, will fix that.
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 v3 11/20] nwfilter: convert IP address learning code to virNWFilterBindingDefPtr
On Sun, Jun 17, 2018 at 08:28:11AM -0400, John Ferlan wrote: > > > On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote: > > Use the virNWFilterBindingDefPTr struct in the IP address learning code > > directly. > > > > Reviewed-by: John Ferlan > > Signed-off-by: Daniel P. Berrangé > > --- > > src/nwfilter/nwfilter_gentech_driver.c | 7 +- > > src/nwfilter/nwfilter_learnipaddr.c| 106 +++-- > > src/nwfilter/nwfilter_learnipaddr.h| 7 +- > > 3 files changed, 32 insertions(+), 88 deletions(-) > > > R-By still applies, but please let's... > > > --- a/src/nwfilter/nwfilter_learnipaddr.c > > +++ b/src/nwfilter/nwfilter_learnipaddr.c > > [...] > > > @@ -737,19 +724,14 @@ learnIPAddressThread(void *arg) > > */ > > Adjust the comments above here to replace the ifname, linkdev, macaddr, > filtername, and filterparams with @binding Ok will do. > > > int > > virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, > > - const char *ifname, > > + virNWFilterBindingDefPtr binding, > >int ifindex, > > - const char *linkdev, > > - const virMacAddr *macaddr, > > - const char *filtername, > > - virHashTablePtr filterparams, > >virNWFilterDriverStatePtr driver, > >int howDetect) > > [...] 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 v3 09/20] virsh: add nwfilter binding commands
On Sun, Jun 17, 2018 at 08:27:27AM -0400, John Ferlan wrote: > > > On 06/14/2018 08:32 AM, Daniel P. Berrangé wrote: > > $ virsh nwfilter-binding-list > > Port Dev Filter > > -- > > vnet0 clean-traffic > > vnet1 clean-traffic > > > > $ virsh nwfilter-binding-dumpxml vnet1 > > > > > > f25arm7 > > 12ac8b8c-4f23-4248-ae42-fdcd50c400fd > > > > > > > > > > > > > > > > > > Signed-off-by: Daniel P. Berrangé > > --- > > tools/virsh-completer.c | 45 ++ > > tools/virsh-completer.h | 4 + > > tools/virsh-nwfilter.c | 317 > > tools/virsh-nwfilter.h | 8 + > > 4 files changed, 374 insertions(+) > > > > Will need virsh.pod changes to describe the new commands. I'm OK if you > just post an update as a reply. I'll send a further patch with POD docs 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] formatdomain.html.in: Amend the 'random' RNG backend section
Since libvirt 1.3.4, any RNG source is accepted for the 'random' backend. However, '/dev/urandom' is the _recommended_ source of entropy. Mention it so in the docs. Suggested-by: Daniel P. Berrangé Signed-off-by: Kashyap Chamarthy --- docs/formatdomain.html.in | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 89672a0486..3dd6a59a01 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7738,12 +7738,16 @@ qemu-kvm -net nic,model=? /dev/null random - This backend type expects a non-blocking character device as - input. The file name is specified as contents of the - backend element. Since 1.3.4 - any path is accepted. Before that /dev/random and /dev/hwrng were - the only accepted paths. When no file name is specified the hypervisor - default is used. For qemu, the default is /dev/random + This backend type expects a non-blocking character device + as input. The file name is specified as contents of the + backend element. Since + 1.3.4 any path is accepted. Before that + /dev/random and /dev/hwrng were + the only accepted paths. When no file name is specified, + the hypervisor default is used. For QEMU, the default is + /dev/random. However, the recommended source + of entropy is /dev/urandom (as it doesn't + have the limitations of /dev/random). egd -- 2.17.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels
The UNIX socket FDs were we passing to QEMU inherited a label based on
libvirtd's context. QEMU is thus denied ability to access the UNIX
socket. We need to use the security manager to change our current
context temporarily when creating the UNIX socket FD.
Signed-off-by: Daniel P. Berrangé
---
src/qemu/qemu_command.c | 86 -
src/qemu/qemu_command.h | 1 +
src/qemu/qemu_process.c | 2 +
3 files changed, 63 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1ffcb5b1ae..146f671663 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4931,6 +4931,7 @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef
*dev)
* host side of the character device */
static char *
qemuBuildChrChardevStr(virLogManagerPtr logManager,
+ virSecurityManagerPtr secManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
@@ -5065,7 +5066,13 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
case VIR_DOMAIN_CHR_TYPE_UNIX:
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) {
+if (qemuSecuritySetSocketLabel(secManager, (virDomainDefPtr)def) <
0)
+goto cleanup;
int fd = qemuOpenChrChardevUNIXSocket(dev);
+if (qemuSecurityClearSocketLabel(secManager, (virDomainDefPtr)def)
< 0) {
+VIR_FORCE_CLOSE(fd);
+goto cleanup;
+}
if (fd < 0)
goto cleanup;
@@ -5404,6 +5411,7 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
static int
qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
+virSecurityManagerPtr secManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
virDomainDefPtr def,
@@ -5414,7 +5422,8 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
if (!priv->monConfig)
return 0;
-if (!(chrdev = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
+if (!(chrdev = qemuBuildChrChardevStr(logManager, secManager,
+ cmd, cfg, def,
priv->monConfig, "monitor",
priv->qemuCaps, true,
priv->chardevStdioLogd)))
@@ -5533,6 +5542,7 @@ qemuBuildSclpDevStr(virDomainChrDefPtr dev)
static int
qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager,
+ virSecurityManagerPtr secManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
@@ -5550,7 +5560,8 @@ qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager,
return 0;
case VIR_DOMAIN_RNG_BACKEND_EGD:
-if (!(*chr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
+if (!(*chr = qemuBuildChrChardevStr(logManager, secManager,
+cmd, cfg, def,
rng->source.chardev,
rng->info.alias, qemuCaps, true,
chardevStdioLogd)))
@@ -5680,6 +5691,7 @@ qemuBuildRNGDevStr(const virDomainDef *def,
static int
qemuBuildRNGCommandLine(virLogManagerPtr logManager,
+virSecurityManagerPtr secManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
@@ -5702,7 +5714,7 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager,
}
/* possibly add character device for backend */
-if (qemuBuildRNGBackendChrdevStr(logManager, cmd, cfg, def,
+if (qemuBuildRNGBackendChrdevStr(logManager, secManager, cmd, cfg, def,
rng, qemuCaps, ,
chardevStdioLogd) < 0)
return -1;
@@ -8135,6 +8147,7 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
static int
qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
virLogManagerPtr logManager,
+ virSecurityManagerPtr secManager,
virCommandPtr cmd,
virDomainDefPtr def,
virDomainNetDefPtr net,
@@ -8157,7 +8170,8 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
switch ((virDomainChrType)net->data.vhostuser->type) {
case VIR_DOMAIN_CHR_TYPE_UNIX:
-if (!(chardev = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
+if (!(chardev = qemuBuildChrChardevStr(logManager, secManager,
+
Re: [libvirt] [PATCH] qemu: fix msg could be a wild pointer in qemuMonitorIOProcess()
On Wed, Jun 20, 2018 at 16:45:27 +0800, Weilun Zhu wrote: > As qemuMonitorJSONIOProcess will call qemuMonitorJSONIOProcessEvent > which unlocks the monitor mutex, there is some extreme situation, > eg qemu send message to monitor twice in a short time, where the > local viriable 'msg' of qemuMonitorIOProcess could be a wild point: > > 1. qemuMonitorSend() assign mon->msg to parameter 'msg', which is alse a > local variable of its caller qemuMonitorJSONCommandWithFd(), cause > eventloop to send message to monitor, then wait condition. > 2. qemu send message to monitor for the first time immediately. > 3. qemuMonitorIOProcess() is called, then wake up the qemuMonitorSend() > thread, but the qemuMonitorSend() thread stuck for a while as cpu pressure > or some other reasons, which means the qemu monitor is still unlocked. > 4. qemu send event message to monitor for the second time, > such as RTC_CHANGE event > 5. qemuMonitorIOProcess() is called again, the local viriable 'msg' is > assigned to mon->msg. > 6. qemuMonitorIOProcess() call qemuMonitorJSONIOProcess() to deal with > the qemu event. > 7. qemuMonitorJSONIOProcess() unlock the qemu monitor in the macro > 'QEMU_MONITOR_CALLBACK', then qemuMonitorSend() thread get the mutex > and free the mon->msg, assign mon->msg to NULL. > > Signed-off-by: Weilun Zhu > --- > src/qemu/qemu_monitor.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) Reviewed-by: Jiri Denemark and pushed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] news: Document recent agent job change
On Thu, Jun 21, 2018 at 03:38:12PM +0200, Michal Privoznik wrote: > Signed-off-by: Michal Privoznik > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vmx: allow an odd number of vCPUs
On Thu, Jun 14, 2018 at 15:34:25 +0200, Pino Toscano wrote: > Most probably this was a limitation in older ESX versions, and it seems > it does not exist anymore in more recent versions; see the following > thread: > https://www.redhat.com/archives/libvir-list/2018-May/msg02159.html > https://www.redhat.com/archives/libvir-list/2018-June/msg00043.html > > Hence, allow an odd number (greater than 1) of vCPUs, since most > probably older versions of ESXi will error out anyway. > > https://bugzilla.redhat.com/show_bug.cgi?id=1584091 > > Signed-off-by: Pino Toscano > --- > src/vmx/vmx.c | 15 +++ > 1 file changed, 7 insertions(+), 8 deletions(-) Polite Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemuDomainObjBeginJobInternal: Report agent job in error message
On Wed, Jun 20, 2018 at 14:32:04 +0200, Michal Privoznik wrote:
> If a thread is unable to acquire a job (e.g. because of timeout)
> an error is reported and the error message contains reference to
> the other thread holding the job. Well, the error message should
> report agent job too as it is yet another source of possible
> failure.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_domain.c | 26 --
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 827597d5f3..4331b95917 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6420,6 +6420,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
> bool async = job == QEMU_JOB_ASYNC;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> const char *blocker = NULL;
> +const char *agentBlocker = NULL;
> int ret = -1;
> unsigned long long duration = 0;
> unsigned long long agentDuration = 0;
> @@ -6549,16 +6550,21 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
> priv->job.apiFlags,
> duration / 1000, agentDuration / 1000, asyncDuration / 1000);
>
> -if (nested || qemuDomainNestedJobAllowed(priv, job))
> -blocker = priv->job.ownerAPI;
> -else
> -blocker = priv->job.asyncOwnerAPI;
> +if (job) {
> +if (nested || qemuDomainNestedJobAllowed(priv, job))
> +blocker = priv->job.ownerAPI;
> +else
> +blocker = priv->job.asyncOwnerAPI;
> +}
> +
> +if (agentJob)
> +agentBlocker = priv->job.agentOwnerAPI;
>
> if (errno == ETIMEDOUT) {
> -if (blocker) {
> +if (blocker || agentBlocker) {
> virReportError(VIR_ERR_OPERATION_TIMEOUT,
> - _("cannot acquire state change lock (held by
> %s)"),
> - blocker);
> + _("cannot acquire state change lock (held by %s
> %s)"),
> + NULLSTR(blocker), NULLSTR(agentBlocker));
Since this is an error message reported to the user I think we should
make it a little bit more user friendly. It would be nice to distinguish
state change lock and agent lock and only print the relevant blocker
(rather than both when one of them is NULL). And when both blockers are
reported, we should separate them better, e.g., "%s and %s".
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 0/2] Add new mdev type for aggregated resources
On 2018.06.21 09:00:28 -0600, Alex Williamson wrote: > On Thu, 21 Jun 2018 19:57:38 +0530 > Kirti Wankhede wrote: > > > On 6/20/2018 1:10 PM, Zhenyu Wang wrote: > > > Current mdev device create interface depends on fixed mdev type, which > > > get uuid > > > from user to create instance of mdev device. If user wants to use > > > customized > > > number of resource for mdev device, then only can create new mdev type > > > for that > > > which may not be flexible. > > > > > > To allow to create user defined resources for mdev, this RFC trys > > > to extend mdev create interface by adding new "instances=xxx" parameter > > > following uuid, for target mdev type if aggregation is supported, it can > > > create new mdev device which contains resources combined by number of > > > instances, e.g > > > > > > echo ",instances=10" > create > > > > This seems orthogonal to the way mdev types are meant to be used. Vendor > > driver can provide the possible types to provide flexibility to the users. > > I think the goal here is to define how we create a type that supports > arbitrary combinations of resources where the total number of those > resources my be sufficiently large that the parent driver enumerating > them all is not feasible. > > > Secondly, not always all resources defined for a particular mdev type > > can be multiplied, for example, a mdev type for vGPU that supports 2 > > heads, that can't be multiplied to use with 20 heads. yeah, for vGPU we actually can have flexible config for memory size but currently fixed by type definition. Although like for display config, we are just sticking with 1 head config even for aggregate type. > > Not all types need to define themselves this way, aiui this is an > optional extension. Userspace can determine if this feature is > available with the new attribute and if they're not aware of the new > attribute, we operate in a backwards compatible mode where 'echo $UUID > > create' consumes one instance of that type. Mdev, like vfio, is device > agnostic, so while a vGPU example may have scaling issues that need to > be ironed out to implement this, those don't immediately negate the > value of this proposal. Thanks, > yep, backward compatible is always ensured by this, so for no aggregation attrib type, still keep current behavior, driver should refuse "instances=" if set by user. One thing I'm concern is that looks currently without changing mdev create ops interface, can't carry that option for driver, or should we do with more general parameter? thanks > > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute > > > which can support this setting. And new sysfs attribute "instances" is > > > created for each mdev device to show allocated number. Default number > > > of 1 or no "instances" file can be used for compatibility check. > > > > > > This RFC trys to create new KVMGT type with minimal vGPU resources which > > > can be combined with "instances=x" setting to allocate for user wanted > > > resources. > > > > > > Zhenyu Wang (2): > > > vfio/mdev: Add new instances parameters for mdev create > > > drm/i915/gvt: Add new aggregation type > > > > > > drivers/gpu/drm/i915/gvt/gvt.c | 26 --- > > > drivers/gpu/drm/i915/gvt/gvt.h | 14 +--- > > > drivers/gpu/drm/i915/gvt/kvmgt.c | 9 +++-- > > > drivers/gpu/drm/i915/gvt/vgpu.c | 56 > > > drivers/s390/cio/vfio_ccw_ops.c | 3 +- > > > drivers/vfio/mdev/mdev_core.c| 11 --- > > > drivers/vfio/mdev/mdev_private.h | 6 +++- > > > drivers/vfio/mdev/mdev_sysfs.c | 42 > > > include/linux/mdev.h | 3 +- > > > samples/vfio-mdev/mbochs.c | 3 +- > > > samples/vfio-mdev/mdpy.c | 3 +- > > > samples/vfio-mdev/mtty.c | 3 +- > > > 12 files changed, 141 insertions(+), 38 deletions(-) > > > > -- Open Source Technology Center, Intel ltd. $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827 signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virDomainSnapshotDefParse: Prefer VIR_STEAL_PTR
On Thu, Jun 21, 2018 at 02:45:43PM +0200, Michal Privoznik wrote: > Signed-off-by: Michal Privoznik > --- Trivial.. Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
