Re: [libvirt PATCH] qemu: support multiqueue for vdpa net device
On 3/2/22 3:30 AM, Martin Kletzander wrote:
On Tue, Mar 01, 2022 at 05:21:37PM -0600, Jonathon Jongsma wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2024406
Signed-off-by: Jonathon Jongsma
---
src/conf/domain_conf.c | 11 ++
src/qemu/qemu_domain.c | 3 +-
.../net-vdpa-multiqueue.x86_64-latest.args | 36 +++
.../qemuxml2argvdata/net-vdpa-multiqueue.xml | 29 +++
tests/qemuxml2argvtest.c | 1 +
.../net-vdpa-multiqueue.xml | 36 +++
tests/qemuxml2xmltest.c | 1 +
7 files changed, 116 insertions(+), 1 deletion(-)
create mode 100644
tests/qemuxml2argvdata/net-vdpa-multiqueue.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/net-vdpa-multiqueue.xml
create mode 100644 tests/qemuxml2xmloutdata/net-vdpa-multiqueue.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 34fec887a3..87117a2ddb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10552,6 +10552,17 @@ virDomainNetDefParseXML(virDomainXMLOption
*xmlopt,
goto error;
}
def->data.vdpa.devicepath = g_steal_pointer();
+
+ if (!def->model)
+ def->model = VIR_DOMAIN_NET_MODEL_VIRTIO;
+
+ if (!virDomainNetIsVirtioModel(def)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Wrong 'type' attribute specified "
+ "with . "
+ "vdpa requires the virtio-net* frontend"));
+ goto error;
+ }
This looks like it belongs to the verification phase as well so that it
does not reject already-existing domain XMLs.
I guess that brings up the question about whether the model should be
set here at all. It was already being set to 'virtio' in the post-parse
callback but I decided to set it here during parse because the
virtio-specific driver options (notably multiqueue in this case) don't
even get parsed unless the model is explicitly set to virtio at this point.
Note that the same behavior can also occur for other network devices.
qemuDomainDefaultNetModel() returns 'virtio' for some configurations
(e.g. s390, riscv, etc). But since the default model type is not set
until the post-parse callback, any virtio driver options that are
specified in the xml will be silently ignored for configurations that
don't include an explicit (even if the model
defaults to virtio) because the model will be unset during parse.
If that's how it's expected to behave, then I could remove this whole
hunk and just require that the model be explicitly set to virtio in the
xml in order to use multiqueue.
break;
case VIR_DOMAIN_NET_TYPE_BRIDGE:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index acc76c1cd6..6b61fefb8f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4523,7 +4523,8 @@ qemuDomainValidateActualNetDef(const
virDomainNetDef *net,
actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
- actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) {
+ actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER ||
+ actualType == VIR_DOMAIN_NET_TYPE_VDPA)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("interface %s - multiqueue is not
supported for network interfaces of type %s"),
macstr, virDomainNetTypeToString(actualType));
Do I get that we cannot support it for but we do for
? Just asking if I understand it correctly, no need to
change that.
I'm not sure, to be perfectly honest. I'm just dealing with the vdpa
device at the moment.
diff --git
a/tests/qemuxml2argvdata/net-vdpa-multiqueue.x86_64-latest.args
b/tests/qemuxml2argvdata/net-vdpa-multiqueue.x86_64-latest.args
new file mode 100644
index 00..26ef666036
--- /dev/null
+++ b/tests/qemuxml2argvdata/net-vdpa-multiqueue.x86_64-latest.args
@@ -0,0 +1,36 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}'
\
+-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-accel tcg \
+-cpu qemu64 \
+-m 214 \
+-object
'{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev
Re: [PATCH] deprecation: x86 default machine types
On 3/2/22 18:42, Daniel P. Berrangé wrote: Overall I'm just not seeing enough benefit to justify the disruption we'll cause by making this change to existing system emulator binaries. I agree. Paolo
ANNOUNCE: virt-manager 4.0.0 released
I'm happy to announce the release of virt-manager 4.0.0! The release can be downloaded from: http://virt-manager.org/download/ Some notable defaults changes: * virt-install: missing --os-variant/--osinfo is now a hard error in most cases. If you weren't specifying a value, or getting one from install media detection, you were probably getting crappy defaults and didn't realize it. If you hit this case you will see a big descriptive error hopefully guiding you to an easy solution. You can see the error and some more details in this email: https://listman.redhat.com/archives/virt-tools-list/2022-February/msg00021.html * For qemu x86 we now use mode=host-passthrough as the CPU default instead of mode=host-model * We now use video model virtio-gpu/virtio-vga in many cases where we previously used qxl, following the suggestions here: https://www.kraxel.org/blog/2019/09/display-devices-in-qemu/ * We now add an TPM emulated device when the VM will use UEFI * qemu x86 q35 VMs will be created with extra pcie-root-ports to facilitate device hotplug. * We set disk discard=unmap when we know the disk storage sparse, or when storage is a block device. Some other notable changes: - Add 'Enable shared memory' UI checkbox (Lin Ma) - add UI preference to default to UEFI for new VMs (Charles Arnold) - Add virtiofs filesystem driver UI option - Fill in all --cputune, --cpu, --shmem, --input, and --boot suboptions (Hugues Fafard) - virt-* mdev improvements (Shalini Chellathurai Saroja) - bhyve improvments (Roman Bogorodskiy) - Revive network portgroup UI Notes for distro maintainers: * We replaced usage of isoinfo with xorisso * We now depend on python setuptools for build + install * We added an explicit runtime requirement to pygobject >= 3.31.3. This is from June 2014 so probably not relevant for modern distros. Thanks to everyone who has contributed to this release through testing, bug reporting, submitting patches, and otherwise sending in feedback! Thanks, Cole
Re: [PATCH] deprecation: x86 default machine types
* Daniel P. Berrangé (berra...@redhat.com) wrote: > On Tue, Mar 01, 2022 at 07:54:32PM +, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > Declare the intent to require a machine type to be specified on x86 > > system emulation. > > > > Signed-off-by: Dr. David Alan Gilbert > > --- > > docs/about/deprecated.rst | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > > index 85773db631..143c60d105 100644 > > --- a/docs/about/deprecated.rst > > +++ b/docs/about/deprecated.rst > > @@ -324,6 +324,14 @@ machine is hardly emulated at all (e.g. neither the > > LCD nor the USB part had > > been implemented), so there is not much value added by this board. Use the > > ``ref405ep`` machine instead. > > > > +x86 default machine type > > + > > + > > +x86 currently defaults to the ```pc``` machine type which is based on the > > very > > +old ```i440fx``` chipset. This default will be removed and the user will > > be > > +required to specify a machine type explicitly using -M; users are > > encouraged to > > +switch to the not quite as old ```q35``` machine types. > > This will have no impact on anyone using libvirt as a mgmt app, > because it will explicitly set 'pc' if the user doesn't request > a machine type. > > It will, however, break a huge number of users who don't use > libvirt or a similar mgmt app. > > 'q35' is not a drop in replacement for 'pc', and even though > it is slightly newer, the features it brings are not likely > to be important enough for most users who aren't using a mgmt > app to care about switching. I can see it having advantages for those who do things like PCIe pass through of graphics cards. However, my main concern is that there's a split happening where downstream we're working primarily on q35 but a lot of people still use i440fx; eventually that split will mean the i440fx users will have a pretty bad experience instability/features. So I'd like to encourage them onto a35. Ideally I'd like to make that easy; e.g. auto creating some of the PCIe busses. > In the ongoing work to introduce a completely new system > emulator binary that is exclusively runtime QMP configured, > the machine type will almost certainly be mandatory, without > affecting existing users. That would also apply consistently > across all target arches. I'm assuming that will also cause the disruption to those end users. Dave > Overall I'm just not seeing enough benefit to justify the > disruption we'll cause by making this change to existing > system emulator binaries. > > 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 :| > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage
On Wed, Mar 02, 2022 at 05:22:07PM +0100, Erik Skultety wrote: > > that can be used by the local sysadmin to define such a policy. > > true, but I suppose from upstream's perspective it can already be handled > using > gitlab tags only, so it feels redundant to handle the same on multiple places. Right, but you have to keep in mind that the person working on the project might not be the same one managing the machines, and that the latter shouldn't have to rely on the former getting the configuration just right to ensure that their desired usage policy is enforced. Configuration mistakes on the project's side should result in failed jobs, not usage policy violations. > > I guess this is already sort of already implicitly implemented due to > > the fact that a job will fail if the corresponding VM template does > > not exist... > > Yes, it'll throw an error, possibly an ugly exception (I hope not) when this > happens. Perhaps all we really need to do is to make sure that a nice, explanatory error message is produced in this scenario then :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] deprecation: x86 default machine types
On Tue, Mar 01, 2022 at 07:54:32PM +, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" > > Declare the intent to require a machine type to be specified on x86 > system emulation. > > Signed-off-by: Dr. David Alan Gilbert > --- > docs/about/deprecated.rst | 8 > 1 file changed, 8 insertions(+) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 85773db631..143c60d105 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -324,6 +324,14 @@ machine is hardly emulated at all (e.g. neither the LCD > nor the USB part had > been implemented), so there is not much value added by this board. Use the > ``ref405ep`` machine instead. > > +x86 default machine type > + > + > +x86 currently defaults to the ```pc``` machine type which is based on the > very > +old ```i440fx``` chipset. This default will be removed and the user will be > +required to specify a machine type explicitly using -M; users are encouraged > to > +switch to the not quite as old ```q35``` machine types. This will have no impact on anyone using libvirt as a mgmt app, because it will explicitly set 'pc' if the user doesn't request a machine type. It will, however, break a huge number of users who don't use libvirt or a similar mgmt app. 'q35' is not a drop in replacement for 'pc', and even though it is slightly newer, the features it brings are not likely to be important enough for most users who aren't using a mgmt app to care about switching. In the ongoing work to introduce a completely new system emulator binary that is exclusively runtime QMP configured, the machine type will almost certainly be mandatory, without affecting existing users. That would also apply consistently across all target arches. Overall I'm just not seeing enough benefit to justify the disruption we'll cause by making this change to existing system emulator binaries. 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 :|
Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage
On Wed, Mar 02, 2022 at 05:18:26PM +0100, Erik Skultety wrote: > On Wed, Mar 02, 2022 at 04:42:21PM +0100, Jiri Denemark wrote: > > On Wed, Mar 02, 2022 at 09:43:24 +, Daniel P. Berrangé wrote: > > > On Wed, Mar 02, 2022 at 08:42:30AM +0100, Erik Skultety wrote: > > > > ... > > > Ultimately when we switch to using merge requests, the integration tests > > > should be run as a gating job, triggered from the merge train when the > > > code gets applied to git, so that we prevent regressions actually making > > > it into git master at all. > > > > > > Post-merge integration testing always exhibits the problem that people > > > will > > > consider it somebody else's problem to fix the regression. So effectively > > > whoever creates the integration testing system ends up burdened with the > > > job of investigating failures and finding someone to poke to fix it. With > > > it run pre-merge then whoever wants to get their code merged needs to > > > investigate the problems. Now sometimes the problems with of course be > > > with the integration test system itself, not the submitters code, but > > > this is OK because it leads to situation where the job of maintaining > > > the integration tests are more equitably spread across all involved and > > > builds a mindset that functional / integration testing is a critical > > > part of delivering code, which is something we've lacked for too long > > > in libvirt. > > > > This is all fine as long as there's a way to explicitly merge something > > even if CI fails. I don't like waiting for a fix in something completely > > unrelated. For example, a MR with translations or an important bug fix > > I haven't studied GitLab enough to rule it out straight away, but I'd like to > have some more intelligent hook (or whatnot) in place that could detect what > type of change is proposed so that documentation changes and translations > would > not go through the whole pipeline and instead would be merged almost > immediately (after passing e.g. the docs build). Yep, for translations all we really need todo is trigger the msgmerge commands to validate that printf format specifiers aren't messed up, so that one is quite easy. In theory we could do something similar for docs only changes, but not sure how amenable our meson rules are to a our meson rules dont For docs our 'website' gitlab job only builds the HTML docs skipping the code. So we could potentially filter that too > An important bug fix however must go through the whole pipeline IMO whether we > like it or not, that's the point, we need to ensure that we're not introducing > a regression to the best of our capabilities. As for CI infrastructure issues, > well, unfortunately that is an inevitable part of doing any automated tasks. > By > introducing it upstream it should therefore become a collective > responsibility, > so that if a CI issue occurs we need to work together to resolve it ASAP > rather > than going around it and pushing a fix just because it takes long to execute. I think the idea of skipping an unreliable pipeline is the wrong way to approach it. For something to be gating the expectation has to be that it is reliable. If something is reliable and it fails with a genuine problem, then it is collective responsibility to drop everything and focus on fixing that problem, not skip it. Sometimes this may cause a delay in merging stuff but that is a good thing, as we've certainly had enough cases over the years where obvious trivial fixes were pushed but were in fact broken. If something breaks and the fix is going to be difficult or out of our control, then we might have no choice but to disable the test jobs completely. For this it should be a case of setting an env var in the gitlab config to skip certain job names, which we can unset once the infra is fixed. An example of this would be where the Cirrus CI API changed recently and completely broke cirrus-run and took a week to fix, and I unset the CIRRUS CI token in the repo. In the case that some jobs are not consistently not reliable then there are a few relevant actions to consider. For example we have ultimately decided to make the all the bleeding edge distros be non-gating because Rawhide/Sid/Tumbleweed regularly broke due to their maintainers pushing broken packages. If we have a test of our own that is periodically failing in a non-determinstic way, then we should immediately disable that test in the test suite, rather than skipping gating CI. The test should only be re-enabled once its reliability is fixed. Tests which are gating must be reliable without false positives. In the absolute worst case, someone with admin permissions can just toggle the 'pipelines must succeed' option in the gitlab repo admin UI, but if it gets to that point we've really failed badly. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-
Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage
... > > > I could easily see the instance of libvirt-gitlab-executor running on > > > hardware owned and operated by Red Hat returning a failure if a job > > > submitted to it comes with DISTRO=debian-11. > > > > libvirt-gitlab-executor is supposed to be system owner agnostic, I'd even > > like > > to make the project part of the libvirt gitlab namespace umbrella so that > > anyone can use it to prepare their machine to be plugged into and used for > > libvirt upstream integration testing. > > I absolutely agree and it was always my understanding that the > project living under your personal account was only a temporary > measure. > > > Therefore, I don't think the project is > > the right place for such checks, those should IMO live solely in the > > gitlab-ci.yml configuration. > > To be clear, I didn't mean that the software itself should contain > hardcoded limits on what jobs it will process, but rather that it > would make sense for it to offer a configuration setting along the > lines of > > accept_distros: > - "alpine-*" > - "debian-*" It's surely up for a discussion, I haven't made any hard decisions wrt where should the project be headed, right now it's very simple, let's see :). > > that can be used by the local sysadmin to define such a policy. true, but I suppose from upstream's perspective it can already be handled using gitlab tags only, so it feels redundant to handle the same on multiple places. > > I guess this is already sort of already implicitly implemented due to > the fact that a job will fail if the corresponding VM template does > not exist... Yes, it'll throw an error, possibly an ugly exception (I hope not) when this happens. Erik
Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage
On Wed, Mar 02, 2022 at 04:42:21PM +0100, Jiri Denemark wrote: > On Wed, Mar 02, 2022 at 09:43:24 +, Daniel P. Berrangé wrote: > > On Wed, Mar 02, 2022 at 08:42:30AM +0100, Erik Skultety wrote: > > > ... > > Ultimately when we switch to using merge requests, the integration tests > > should be run as a gating job, triggered from the merge train when the > > code gets applied to git, so that we prevent regressions actually making > > it into git master at all. > > > > Post-merge integration testing always exhibits the problem that people will > > consider it somebody else's problem to fix the regression. So effectively > > whoever creates the integration testing system ends up burdened with the > > job of investigating failures and finding someone to poke to fix it. With > > it run pre-merge then whoever wants to get their code merged needs to > > investigate the problems. Now sometimes the problems with of course be > > with the integration test system itself, not the submitters code, but > > this is OK because it leads to situation where the job of maintaining > > the integration tests are more equitably spread across all involved and > > builds a mindset that functional / integration testing is a critical > > part of delivering code, which is something we've lacked for too long > > in libvirt. > > This is all fine as long as there's a way to explicitly merge something > even if CI fails. I don't like waiting for a fix in something completely > unrelated. For example, a MR with translations or an important bug fix I haven't studied GitLab enough to rule it out straight away, but I'd like to have some more intelligent hook (or whatnot) in place that could detect what type of change is proposed so that documentation changes and translations would not go through the whole pipeline and instead would be merged almost immediately (after passing e.g. the docs build). An important bug fix however must go through the whole pipeline IMO whether we like it or not, that's the point, we need to ensure that we're not introducing a regression to the best of our capabilities. As for CI infrastructure issues, well, unfortunately that is an inevitable part of doing any automated tasks. By introducing it upstream it should therefore become a collective responsibility, so that if a CI issue occurs we need to work together to resolve it ASAP rather than going around it and pushing a fix just because it takes long to execute. Erik > should not be blocked by a bug in CI or an infrastructure issue. So > having a way (restricted, of course) to merge even if pipeline failed > would solve this issue. > > Jirka >
Re: [PATCH] libxl: Turn on user aliases
On 3/2/22 3:29 AM, Peter Krempa wrote: On Tue, Mar 01, 2022 at 09:40:30 -0700, Jim Fehlig wrote: On 2/17/22 04:56, Michal Privoznik wrote: When I implemented user aliases I've invented this virDomainDefFeatures flag so that individual drivers can signal support for user provided aliases. The reasoning was that a device alias might be part of guest ABI, or used in a different way then in QEMU. Well, neither applies to the libxl driver, so it's safe to allow user aliases there. I suppose it is safe, but does it make sense since aliases are not used by the driver in any way, and not supported by libxl? In that case at least the libvirt side user-visible aspect of it does make sense. Users can pick a stable alias for their device if they care about that for any reason. ...for example, in the qemu driver, the alias can be used as an index to match when unplugging or updating a device; in the case of an , in the past you could match the device to unplug/update by looking at the MAC address or PCI address, but the MAC address isn't necessarily unique, and PCI address is usually determined by libvirt, so the management application may not know it. But the management application can just explicitly provide an alias in the XML when the domain is defined, and then later use that as the key when unplugging/updating. Matching on alias when searching for an was added for use by the QEMU driver back in commit 114e3b4232, but fortunately this search is done by virDomainNetFindIdx(), which is also used by the libxl interface update and unplug functions, so libxl will get this functionality for free once Michal's patch is pushed. (I haven't looked at whether or not other types of devices are using alias in this manner either in the qemu or libxl drivers, but I assume at least some of them are) In the qemu impl we decided to propagate it to the hypervisor, but that should be considered an implementation detail as the IDs of objects (mostly) don't have guest visible meaning.
Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage
On Wed, Mar 02, 2022 at 03:52:49PM +0100, Erik Skultety wrote: > On Wed, Mar 02, 2022 at 06:27:04AM -0800, Andrea Bolognani wrote: > > I don't think we can expect integration tests to be merged at the > > same time as a feature when new APIs are involved. If tests are > > written in Python, then the Python bindings need to introduce support > > for the new API before the test can exist, and that can't happen > > until the feature has been merged. > > Again, that is a logical conclusion which brings us to an unrelated process > question: How do we change the contribution process so that the contribution > of > a feature doesn't end with it being merged to the C library, IOW we'd ideally > want to have a test introduced with every feature,but given that we'd need the > bindings first to actually do that, but we can't have a binding unless the C > counterpart is already merged, how do we keep the contributors motivated > enough? (Note that it's food for thought, it's only tangential to this effort) Yeah, let's save this massive topic for another time :) > > If the feature or bug fix doesn't require new APIs to be introduced > > this is of course not an issue. Most changes should fall into this > > bucket. > > > > So overall I still think using existing artifacts would be the better > > approach, at least initially. We can always change things later if we > > find that we've outgrown it. > > So, given that https://gitlab.com/libvirt/libvirt-perl/-/merge_requests/55 was > already merged we should not get to a situation where no artifacts would be > available because gitlab documents that even if artifacts expired they won't > be > deleted until new artifacts become available, I think we can depend on the > latest available artifacts without building them. I'll refresh the patch > accordingly and test. Thanks! > > I think it's not unreasonable that when Red Hat, or any other entity, > > provides hardware access to the project there will be some strings > > attached. This is already the case for GitLab and Cirrus CI, for > > example. > > > > I could easily see the instance of libvirt-gitlab-executor running on > > hardware owned and operated by Red Hat returning a failure if a job > > submitted to it comes with DISTRO=debian-11. > > libvirt-gitlab-executor is supposed to be system owner agnostic, I'd even like > to make the project part of the libvirt gitlab namespace umbrella so that > anyone can use it to prepare their machine to be plugged into and used for > libvirt upstream integration testing. I absolutely agree and it was always my understanding that the project living under your personal account was only a temporary measure. > Therefore, I don't think the project is > the right place for such checks, those should IMO live solely in the > gitlab-ci.yml configuration. To be clear, I didn't mean that the software itself should contain hardcoded limits on what jobs it will process, but rather that it would make sense for it to offer a configuration setting along the lines of accept_distros: - "alpine-*" - "debian-*" that can be used by the local sysadmin to define such a policy. I guess this is already sort of already implicitly implemented due to the fact that a job will fail if the corresponding VM template does not exist... -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage
On Wed, Mar 02, 2022 at 09:43:24 +, Daniel P. Berrangé wrote: > On Wed, Mar 02, 2022 at 08:42:30AM +0100, Erik Skultety wrote: > > ... > Ultimately when we switch to using merge requests, the integration tests > should be run as a gating job, triggered from the merge train when the > code gets applied to git, so that we prevent regressions actually making > it into git master at all. > > Post-merge integration testing always exhibits the problem that people will > consider it somebody else's problem to fix the regression. So effectively > whoever creates the integration testing system ends up burdened with the > job of investigating failures and finding someone to poke to fix it. With > it run pre-merge then whoever wants to get their code merged needs to > investigate the problems. Now sometimes the problems with of course be > with the integration test system itself, not the submitters code, but > this is OK because it leads to situation where the job of maintaining > the integration tests are more equitably spread across all involved and > builds a mindset that functional / integration testing is a critical > part of delivering code, which is something we've lacked for too long > in libvirt. This is all fine as long as there's a way to explicitly merge something even if CI fails. I don't like waiting for a fix in something completely unrelated. For example, a MR with translations or an important bug fix should not be blocked by a bug in CI or an infrastructure issue. So having a way (restricted, of course) to merge even if pipeline failed would solve this issue. Jirka
Re: [PATCH 00/23] virsh: various small cleanups
On a Wednesday in 2022, Peter Krempa wrote: A collection of mostly small patches [...] Peter Krempa (23): virsh: cmdBlockcopy: Use virXMLFormatElement virsh: cmdStart: Rewrite ternary operator use to standard if conditions virsh: doSave: Use if-else instead of ternary operator virsh: cmdRestore: Use if-else instead of ternary operator virsh: virshVcpuinfoPrintAffinity: Use if-else instead of ternary operator virsh: Use NULLSTR_EMPTY instead of ternary operator virshEventPrint: Use automatic memory clearing virsh: Move 'virshDomainBlockJobToString' to virsh-util virsh: Move 'cmdEvent' and all of it's machinery to virsh-domain-event.c virsh: cmdEvent: Rewrite questionable event registration virsh: cmdDomDisplay: Extract loop body fetching display URIs into 'virshGetOneDisplay' virsh: cmdDomDisplay: Remove unneeded 'cleanup' label virshGetOneDisplay: Automaticaly free extracted data virshGetOneDisplay: Don't reuse 'xpath' variable virshGetOneDisplay: Refactor formatting of URI params virsh: cmdSchedinfo: Add separate variable for holding flags used for query virsh: cmdDesc: Use separate flags variable for getters vsh: Add helper for auto-removing temporary file virsh: cmdDesc: Use 'vshTempFile' type to simplify cleanup virsh: cmdDesc: Automatically free memory virsh: cmdDesc: Remove unneeded 'cleanup' virsh: domain: Don't use ternaries inside vshPrint/vshError functions virsh: cmdDesc: Fix logic when '-edit' is used along with 'desc' argument po/POTFILES.in |1 + tools/meson.build |1 + tools/virsh-completer-domain.c | 19 - tools/virsh-completer-domain.h |5 - tools/virsh-domain-event.c | 1007 tools/virsh-domain-event.h | 23 + tools/virsh-domain.c | 1623 +++- tools/virsh-util.c | 19 + tools/virsh-util.h |5 + tools/virsh.c |2 + tools/virsh.h |1 + tools/vsh.c| 25 +- tools/vsh.h|3 + 13 files changed, 1412 insertions(+), 1322 deletions(-) create mode 100644 tools/virsh-domain-event.c create mode 100644 tools/virsh-domain-event.h Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 13/23] virshGetOneDisplay: Automaticaly free extracted data
On a Wednesday in 2022, Peter Krempa wrote: Use automatic memory freeing for the temporary variables hodling the *holding data extracted from the XML. The code in this function was originally extracted from a loop so we can also drop pre-clearing of the pointers. Signed-off-by: Peter Krempa --- tools/virsh-domain.c | 49 ++-- 1 file changed, 15 insertions(+), 34 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage
On Wed, Mar 02, 2022 at 06:27:04AM -0800, Andrea Bolognani wrote: > On Wed, Mar 02, 2022 at 01:11:04PM +0100, Erik Skultety wrote: > > > > I gave this more thought. What you suggest is viable, but the following > > > > is worth > > > > considering if we go with your proposal: > > > > > > > > - libvirt-perl jobs build upstream libvirt first in order to build the > > > > bindings > > > > -> generally it takes until right before the release that > > > > APIs/constants > > > >are added to the respective bindings (Perl/Python) > > This is not entirely accurate. While it's true that bindings > generally lag behind the C library, they're usually updated within > days. Changes only getting in at the very end of a development cycle > is an exception, not the norm. > > > > > -> if we rely on the latest libvirt-perl artifacts without actually > > > >triggering the pipeline, yes, the artifacts would be stable, but > > > > fairly > > > >old (unless we schedule a recurrent pipeline in the project to > > > > refresh > > > >them), thus not giving us feedback from the integration stage > > > > that > > > >bindings need to be added first, because the API coverage would > > > > likely > > > >fail, thus failing the whole libvirt-perl pipeline and thus > > > > invalidating > > > >the integration test stage in the libvirt project > > > > => now, I admit this would get pretty annoying because it would > > > > force > > > >contributors (or the maintainer) who add new APIs to add > > > > respective > > > >bindings as well in a timely manner, but then again > > > > ultimately we'd > > > >like our contributors to also introduce an integration test > > > > along > > > >with their feature... > > > > > > Note right now the perl API coverage tests are configured to only be > > > gating > > > when run on nightly scheduled jobs. I stopped them being gating on > > > contributions > > > because if someone if fixing a bug in the bindings it is silly to force > > > their merge request to also add new API bindings. > > I don't think we can expect integration tests to be merged at the > same time as a feature when new APIs are involved. If tests are > written in Python, then the Python bindings need to introduce support > for the new API before the test can exist, and that can't happen > until the feature has been merged. Again, that is a logical conclusion which brings us to an unrelated process question: How do we change the contribution process so that the contribution of a feature doesn't end with it being merged to the C library, IOW we'd ideally want to have a test introduced with every feature,but given that we'd need the bindings first to actually do that, but we can't have a binding unless the C counterpart is already merged, how do we keep the contributors motivated enough? (Note that it's food for thought, it's only tangential to this effort) > > If the feature or bug fix doesn't require new APIs to be introduced > this is of course not an issue. Most changes should fall into this > bucket. > > So overall I still think using existing artifacts would be the better > approach, at least initially. We can always change things later if we > find that we've outgrown it. So, given that https://gitlab.com/libvirt/libvirt-perl/-/merge_requests/55 was already merged we should not get to a situation where no artifacts would be available because gitlab documents that even if artifacts expired they won't be deleted until new artifacts become available, I think we can depend on the latest available artifacts without building them. I'll refresh the patch accordingly and test. > > > > > > Should we make them *less* specific instead? As in, is there any > > > > > reason for having different tags for Fedora and CentOS jobs as > > > > > opposed to using a generic "this needs to run in a VM" tag for both? > > > > > > > > Well, I would not be against, but I feel this is more of a political > > > > issue: > > > > this HW was provided by Red Hat with the intention to be dedicated for > > > > Red Hat > > > > workloads. If another interested 3rd party comes (and I do hope they > > > > will) and > > > > provides HW, we should utilize the resources fairly in a way respectful > > > > to the > > > > donor's/owner's intentions, IOW if party A provides a single machine to > > > > run > > > > CI workloads using Debian VMs, we should not schedule Fedora/CentOS > > > > workloads > > > > in there effectively saturating it. > > > > So if the tags are to be adjusted, then I'd be in favour of recording > > > > the owner > > > > of the runner in the tag. > > > > > > If we have hardware available, we should use to the best of its ability. > > > Nothing is gained by leaving it idle if it has spare capacity to run jobs. > > > > Well, logically there's absolutely no disagreement with you here. > > Personally, > > I would go
Re: [PATCH 09/23] virsh: Move 'cmdEvent' and all of it's machinery to virsh-domain-event.c
*its On a Wednesday in 2022, Peter Krempa wrote: 'cmdEvent' along with all the helper functions it needs is ~950 LOC. Move it out from virsh-domain.c to virsh-domain-event.c along with the completer function so that the new module doesn't have to expose any new types. Semantically this creates a new category in 'virsh help' but all other behaviour stays the same. Signed-off-by: Peter Krempa --- po/POTFILES.in |1 + tools/meson.build |1 + tools/virsh-completer-domain.c | 19 - tools/virsh-completer-domain.h |5 - tools/virsh-domain-event.c | 1007 tools/virsh-domain-event.h | 23 + tools/virsh-domain.c | 946 -- tools/virsh.c |2 + tools/virsh.h |1 + 9 files changed, 1035 insertions(+), 970 deletions(-) create mode 100644 tools/virsh-domain-event.c create mode 100644 tools/virsh-domain-event.h Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage
On Wed, Mar 02, 2022 at 01:11:04PM +0100, Erik Skultety wrote: > > > I gave this more thought. What you suggest is viable, but the following > > > is worth > > > considering if we go with your proposal: > > > > > > - libvirt-perl jobs build upstream libvirt first in order to build the > > > bindings > > > -> generally it takes until right before the release that > > > APIs/constants > > >are added to the respective bindings (Perl/Python) This is not entirely accurate. While it's true that bindings generally lag behind the C library, they're usually updated within days. Changes only getting in at the very end of a development cycle is an exception, not the norm. > > > -> if we rely on the latest libvirt-perl artifacts without actually > > >triggering the pipeline, yes, the artifacts would be stable, but > > > fairly > > >old (unless we schedule a recurrent pipeline in the project to > > > refresh > > >them), thus not giving us feedback from the integration stage that > > >bindings need to be added first, because the API coverage would > > > likely > > >fail, thus failing the whole libvirt-perl pipeline and thus > > > invalidating > > >the integration test stage in the libvirt project > > > => now, I admit this would get pretty annoying because it would > > > force > > >contributors (or the maintainer) who add new APIs to add > > > respective > > >bindings as well in a timely manner, but then again ultimately > > > we'd > > >like our contributors to also introduce an integration test > > > along > > >with their feature... > > > > Note right now the perl API coverage tests are configured to only be gating > > when run on nightly scheduled jobs. I stopped them being gating on > > contributions > > because if someone if fixing a bug in the bindings it is silly to force > > their merge request to also add new API bindings. I don't think we can expect integration tests to be merged at the same time as a feature when new APIs are involved. If tests are written in Python, then the Python bindings need to introduce support for the new API before the test can exist, and that can't happen until the feature has been merged. If the feature or bug fix doesn't require new APIs to be introduced this is of course not an issue. Most changes should fall into this bucket. So overall I still think using existing artifacts would be the better approach, at least initially. We can always change things later if we find that we've outgrown it. > > > > Should we make them *less* specific instead? As in, is there any > > > > reason for having different tags for Fedora and CentOS jobs as > > > > opposed to using a generic "this needs to run in a VM" tag for both? > > > > > > Well, I would not be against, but I feel this is more of a political > > > issue: > > > this HW was provided by Red Hat with the intention to be dedicated for > > > Red Hat > > > workloads. If another interested 3rd party comes (and I do hope they > > > will) and > > > provides HW, we should utilize the resources fairly in a way respectful > > > to the > > > donor's/owner's intentions, IOW if party A provides a single machine to > > > run > > > CI workloads using Debian VMs, we should not schedule Fedora/CentOS > > > workloads > > > in there effectively saturating it. > > > So if the tags are to be adjusted, then I'd be in favour of recording the > > > owner > > > of the runner in the tag. > > > > If we have hardware available, we should use to the best of its ability. > > Nothing is gained by leaving it idle if it has spare capacity to run jobs. > > Well, logically there's absolutely no disagreement with you here. Personally, > I would go about it the same. The problem is that the HW we're talking about > wasn't an official donation, Red Hat still owns and controls the HW, so the > company can very much disagree with running other workloads on it long term. > I'm not saying we shouldn't test the limits, reliability and bandwidth to its > fullest potential. What I'm trying to say is that the major issue here is that > contributing to open source projects is a collaborative effort of all > interested parties (duh, should go without saying) and so we cannot expect a > single party which just happens to have the biggest stake in the project to > run > workloads for everybody else. I mean the situation would have been different > if > the HW were a proper donation, but unfortunately it is not. If we pick and run > workloads on various distros for the sake of getting coverage (which makes > total sense btw), it would later be harder to communicate back to the > community > why the number of distros (or their variety) would need to be decreased once > the HW's capabilities are saturated with demanding workloads, e.g. migration > testing or device assignment, etc. > > Whether I do or do not personally feel comfortable being
[PATCH 04/23] virsh: cmdRestore: Use if-else instead of ternary operator
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 607eb973ac..732690ec44 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5313,6 +5313,7 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd)
const char *xmlfile = NULL;
g_autofree char *xml = NULL;
virshControl *priv = ctl->privData;
+int rc;
if (vshCommandOptStringReq(ctl, cmd, "file", ) < 0)
return false;
@@ -5333,9 +5334,13 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd)
virFileReadAll(xmlfile, VSH_MAX_XML_FILE, ) < 0)
return false;
-if (((flags || xml)
- ? virDomainRestoreFlags(priv->conn, from, xml, flags)
- : virDomainRestore(priv->conn, from)) < 0) {
+if (flags || xml) {
+rc = virDomainRestoreFlags(priv->conn, from, xml, flags);
+} else {
+rc = virDomainRestore(priv->conn, from);
+}
+
+if (rc < 0) {
vshError(ctl, _("Failed to restore domain from %s"), from);
return false;
}
--
2.35.1
[PATCH 06/23] virsh: Use NULLSTR_EMPTY instead of ternary operator
Signed-off-by: Peter Krempa --- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 73f05ce7f9..25097627ac 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7601,7 +7601,7 @@ cmdIOThreadInfo(vshControl *ctl, const vshCmd *cmd) ignore_value(pinInfo = virBitmapDataFormat(info[i]->cpumap, info[i]->cpumaplen)); -if (vshTableRowAppend(table, iothreadIdStr, pinInfo ? pinInfo : "", NULL) < 0) +if (vshTableRowAppend(table, iothreadIdStr, NULLSTR_EMPTY(pinInfo), NULL) < 0) goto cleanup; } @@ -14211,7 +14211,7 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) info[i]->mountpoint, info[i]->name, info[i]->fstype, - targets ? targets : "", + NULLSTR_EMPTY(targets), NULL) < 0) goto cleanup; } -- 2.35.1
[PATCH 21/23] virsh: cmdDesc: Remove unneeded 'cleanup'
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 22 +-
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 4d9722f400..7b4f8638a9 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8336,7 +8336,6 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
g_autofree char *desc = NULL;
const vshCmdOpt *opt = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-bool ret = false;
unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
unsigned int queryflags = 0;
@@ -8354,7 +8353,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
return false;
if ((state = virshDomainState(ctl, dom, NULL)) < 0)
-goto cleanup;
+return false;
if (title)
type = VIR_DOMAIN_METADATA_TITLE;
@@ -8372,7 +8371,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
if (!desc) {
desc = virshGetDomainDescription(ctl, dom, title, queryflags);
if (!desc)
-goto cleanup;
+return false;
}
if (edit) {
@@ -8382,15 +8381,15 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
/* Create and open the temporary file. */
if (!(tmp = vshEditWriteToTempFile(ctl, desc)))
-goto cleanup;
+return false;
/* Start the editor. */
if (vshEditFile(ctl, tmp) == -1)
-goto cleanup;
+return false;
/* Read back the edited file. */
if (!(desc_edited = vshEditReadBackFile(ctl, tmp)))
-goto cleanup;
+return false;
/* strip a possible newline at the end of file; some
* editors enforce a newline, this makes editing the title
@@ -8405,8 +8404,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
vshPrintExtra(ctl, "%s",
title ? _("Domain title not changed\n") :
_("Domain description not changed\n"));
-ret = true;
-goto cleanup;
+return true;
}
VIR_FREE(desc);
@@ -8417,7 +8415,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
vshError(ctl, "%s",
title ? _("Failed to set new domain title") :
_("Failed to set new domain description"));
-goto cleanup;
+return false;
}
vshPrintExtra(ctl, "%s",
title ? _("Domain title updated successfully") :
@@ -8425,7 +8423,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
} else {
desc = virshGetDomainDescription(ctl, dom, title, queryflags);
if (!desc)
-goto cleanup;
+return false;
if (strlen(desc) > 0)
vshPrint(ctl, "%s", desc);
@@ -8436,9 +8434,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
virDomainGetName(dom));
}
-ret = true;
- cleanup:
-return ret;
+return true;
}
--
2.35.1
[PATCH 23/23] virsh: cmdDesc: Fix logic when '-edit' is used along with 'desc' argument
Historically the use of the '-desc' multiple argument parameter was not
forbidden toghether with '-edit', but use of both together has some
unexpected behaviour. Specifically the editor is filled with the
contents passed via '-desc' but if the user doesn't change the text in
any way virsh will claim that the description was not chaged even if it
differs from the currently set description. Similarly, when the user
would edit the description provided via 'desc' so that it's identical
with the one configured for the domain, virsh would claim that it was
updated:
# virsh desc cd
No description for domain: cd
# EDITOR=true virsh desc cd --edit "test desc"
Domain description not changed
After the fix:
# virsh desc cd
No description for domain: cd
# EDITOR=true virsh desc cd --edit "test desc"
Domain description updated successfully
# EDITOR=true virsh desc cd --edit "test desc"
Domain description not changed
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 32 ++--
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 89ad45dbf0..d5fd8be7c3 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8335,7 +8335,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
int state;
int type;
-g_autofree char *desc = NULL;
+g_autofree char *descArg = NULL;
const vshCmdOpt *opt = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
@@ -8367,14 +8367,17 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
virBufferTrim(, " ");
-desc = virBufferContentAndReset();
+descArg = virBufferContentAndReset();
-if (edit || desc) {
-if (!desc) {
-desc = virshGetDomainDescription(ctl, dom, title, queryflags);
-if (!desc)
-return false;
-}
+if (edit || descArg) {
+g_autofree char *descDom = NULL;
+g_autofree char *descNew = NULL;
+
+if (!(descDom = virshGetDomainDescription(ctl, dom, title,
queryflags)))
+return false;
+
+if (!descArg)
+descArg = g_strdup(descDom);
if (edit) {
g_autoptr(vshTempFile) tmp = NULL;
@@ -8382,7 +8385,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
char *tmpstr;
/* Create and open the temporary file. */
-if (!(tmp = vshEditWriteToTempFile(ctl, desc)))
+if (!(tmp = vshEditWriteToTempFile(ctl, descArg)))
return false;
/* Start the editor. */
@@ -8402,7 +8405,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
*tmpstr = '\0';
/* Compare original XML with edited. Has it changed at all? */
-if (STREQ(desc, desc_edited)) {
+if (STREQ(descDom, desc_edited)) {
if (title)
vshPrintExtra(ctl, "%s", _("Domain title not changed\n"));
else
@@ -8411,11 +8414,12 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
return true;
}
-VIR_FREE(desc);
-desc = g_steal_pointer(_edited);
+descNew = g_steal_pointer(_edited);
+} else {
+descNew = g_steal_pointer();
}
-if (virDomainSetMetadata(dom, type, desc, NULL, NULL, flags) < 0) {
+if (virDomainSetMetadata(dom, type, descNew, NULL, NULL, flags) < 0) {
if (title)
vshError(ctl, "%s", _("Failed to set new domain title"));
else
@@ -8430,7 +8434,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
vshPrintExtra(ctl, "%s", _("Domain description updated
successfully"));
} else {
-desc = virshGetDomainDescription(ctl, dom, title, queryflags);
+g_autofree char *desc = virshGetDomainDescription(ctl, dom, title,
queryflags);
if (!desc)
return false;
--
2.35.1
[PATCH 20/23] virsh: cmdDesc: Automatically free memory
Decrease scope of variables and use automatic freeing.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index dcf0f712f6..4d9722f400 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8333,9 +8333,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
int state;
int type;
-char *desc = NULL;
-char *desc_edited = NULL;
-char *tmpstr;
+g_autofree char *desc = NULL;
const vshCmdOpt *opt = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
bool ret = false;
@@ -8379,6 +8377,8 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
if (edit) {
g_autoptr(vshTempFile) tmp = NULL;
+g_autofree char *desc_edited = NULL;
+char *tmpstr;
/* Create and open the temporary file. */
if (!(tmp = vshEditWriteToTempFile(ctl, desc)))
@@ -8438,8 +8438,6 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
ret = true;
cleanup:
-VIR_FREE(desc_edited);
-VIR_FREE(desc);
return ret;
}
--
2.35.1
[PATCH 22/23] virsh: domain: Don't use ternaries inside vshPrint/vshError functions
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 50 ++--
1 file changed, 30 insertions(+), 20 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 7b4f8638a9..89ad45dbf0 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3929,11 +3929,13 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
/* No way to emulate deletion of just snapshot metadata
* without support for the newer flags. Oh well. */
if (has_snapshots_metadata) {
-vshError(ctl,
- snapshots_metadata ?
- _("Unable to remove metadata of %d snapshots") :
- _("Refusing to undefine while %d snapshots exist"),
- has_snapshots_metadata);
+if (snapshots_metadata)
+vshError(ctl, _("Unable to remove metadata of %d snapshots"),
+ has_snapshots_metadata);
+else
+vshError(ctl, _("Refusing to undefine while %d snapshots exist"),
+ has_snapshots_metadata);
+
goto cleanup;
}
@@ -8401,9 +8403,11 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
/* Compare original XML with edited. Has it changed at all? */
if (STREQ(desc, desc_edited)) {
-vshPrintExtra(ctl, "%s",
- title ? _("Domain title not changed\n") :
- _("Domain description not changed\n"));
+if (title)
+vshPrintExtra(ctl, "%s", _("Domain title not changed\n"));
+else
+vshPrintExtra(ctl, "%s", _("Domain description not
changed\n"));
+
return true;
}
@@ -8412,26 +8416,32 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
}
if (virDomainSetMetadata(dom, type, desc, NULL, NULL, flags) < 0) {
-vshError(ctl, "%s",
- title ? _("Failed to set new domain title") :
- _("Failed to set new domain description"));
+if (title)
+vshError(ctl, "%s", _("Failed to set new domain title"));
+else
+vshError(ctl, "%s", _("Failed to set new domain description"));
+
return false;
}
-vshPrintExtra(ctl, "%s",
- title ? _("Domain title updated successfully") :
- _("Domain description updated successfully"));
+
+if (title)
+vshPrintExtra(ctl, "%s", _("Domain title updated successfully"));
+else
+vshPrintExtra(ctl, "%s", _("Domain description updated
successfully"));
+
} else {
desc = virshGetDomainDescription(ctl, dom, title, queryflags);
if (!desc)
return false;
-if (strlen(desc) > 0)
+if (strlen(desc) > 0) {
vshPrint(ctl, "%s", desc);
-else
-vshPrintExtra(ctl,
- title ? _("No title for domain: %s") :
- _("No description for domain: %s"),
- virDomainGetName(dom));
+} else {
+if (title)
+vshPrintExtra(ctl, _("No title for domain: %s"),
virDomainGetName(dom));
+else
+vshPrintExtra(ctl, _("No description for domain: %s"),
virDomainGetName(dom));
+}
}
return true;
--
2.35.1
[PATCH 18/23] vsh: Add helper for auto-removing temporary file
The vsh helpers for user-editing of contents use temporary files.
Introduce 'vshTempFile' type which automatically removes the file.
Signed-off-by: Peter Krempa
---
tools/vsh.c | 25 +++--
tools/vsh.h | 3 +++
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index 4ec5e54b5d..bbde594967 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2378,34 +2378,47 @@ vshAskReedit(vshControl *ctl,
#endif /* WIN32 */
+void
+vshEditUnlinkTempfile(char *file)
+{
+if (!file)
+return;
+
+ignore_value(unlink(file));
+g_free(file);
+}
+
+
/* Common code for the edit / net-edit / pool-edit functions which follow. */
char *
vshEditWriteToTempFile(vshControl *ctl, const char *doc)
{
-g_autofree char *ret = NULL;
+g_autofree char *filename = NULL;
+g_autoptr(vshTempFile) ret = NULL;
const char *tmpdir;
VIR_AUTOCLOSE fd = -1;
tmpdir = getenv("TMPDIR");
-if (!tmpdir) tmpdir = "/tmp";
-ret = g_strdup_printf("%s/virshXX.xml", tmpdir);
-fd = g_mkstemp_full(ret, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR);
+if (!tmpdir)
+tmpdir = "/tmp";
+filename = g_strdup_printf("%s/virshXX.xml", tmpdir);
+fd = g_mkstemp_full(filename, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR);
if (fd == -1) {
vshError(ctl, _("g_mkstemp_full: failed to create temporary file: %s"),
g_strerror(errno));
return NULL;
}
+ret = g_steal_pointer();
+
if (safewrite(fd, doc, strlen(doc)) == -1) {
vshError(ctl, _("write: %s: failed to write to temporary file: %s"),
ret, g_strerror(errno));
-unlink(ret);
return NULL;
}
if (VIR_CLOSE(fd) < 0) {
vshError(ctl, _("close: %s: failed to write or close temporary file:
%s"),
ret, g_strerror(errno));
-unlink(ret);
return NULL;
}
diff --git a/tools/vsh.h b/tools/vsh.h
index e208d957bb..663dc1ffce 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -341,6 +341,9 @@ void vshSaveLibvirtError(void);
void vshSaveLibvirtHelperError(void);
/* file handling */
+void vshEditUnlinkTempfile(char *file);
+typedef char vshTempFile;
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(vshTempFile, vshEditUnlinkTempfile);
char *vshEditWriteToTempFile(vshControl *ctl, const char *doc);
int vshEditFile(vshControl *ctl, const char *filename);
char *vshEditReadBackFile(vshControl *ctl, const char *filename);
--
2.35.1
[PATCH 17/23] virsh: cmdDesc: Use separate flags variable for getters
The getters have a different set of flags. Add a variable for the getter
to avoid having to construct flags when calling the getter.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 2d1889c71e..cac50dba51 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8341,12 +8341,15 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
bool ret = false;
unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
+unsigned int queryflags = 0;
VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
-if (config)
+if (config) {
flags |= VIR_DOMAIN_AFFECT_CONFIG;
+queryflags |= VIR_DOMAIN_XML_INACTIVE;
+}
if (live)
flags |= VIR_DOMAIN_AFFECT_LIVE;
@@ -8370,8 +8373,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
if (edit || desc) {
if (!desc) {
-desc = virshGetDomainDescription(ctl, dom, title,
- config?VIR_DOMAIN_XML_INACTIVE:0);
+desc = virshGetDomainDescription(ctl, dom, title, queryflags);
if (!desc)
goto cleanup;
}
@@ -8420,8 +8422,7 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
title ? _("Domain title updated successfully") :
_("Domain description updated successfully"));
} else {
-desc = virshGetDomainDescription(ctl, dom, title,
- config?VIR_DOMAIN_XML_INACTIVE:0);
+desc = virshGetDomainDescription(ctl, dom, title, queryflags);
if (!desc)
goto cleanup;
--
2.35.1
[PATCH 19/23] virsh: cmdDesc: Use 'vshTempFile' type to simplify cleanup
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cac50dba51..dcf0f712f6 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8335,7 +8335,6 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
int type;
char *desc = NULL;
char *desc_edited = NULL;
-char *tmp = NULL;
char *tmpstr;
const vshCmdOpt *opt = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
@@ -8379,6 +8378,8 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
}
if (edit) {
+g_autoptr(vshTempFile) tmp = NULL;
+
/* Create and open the temporary file. */
if (!(tmp = vshEditWriteToTempFile(ctl, desc)))
goto cleanup;
@@ -8439,10 +8440,6 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd)
cleanup:
VIR_FREE(desc_edited);
VIR_FREE(desc);
-if (tmp) {
-unlink(tmp);
-VIR_FREE(tmp);
-}
return ret;
}
--
2.35.1
[PATCH 15/23] virshGetOneDisplay: Refactor formatting of URI params
Unconditionally format the start of the query ('?') and make delimiters
('&') part of the arguments. At the end we can trim off 1 char from the
end of the buffer unconditionally.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 17 +++--
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index d0f78798b5..ca1145428f 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11678,7 +11678,6 @@ virshGetOneDisplay(vshControl *ctl,
g_autofree char *type_conn = NULL;
g_autofree char *sockpath = NULL;
g_autofree char *passwd = NULL;
-bool params = false;
/* Attempt to get the port number for the current graphics scheme */
xpathPort = g_strdup_printf(xpath_fmt, scheme, "@port");
@@ -11781,22 +11780,20 @@ virshGetOneDisplay(vshControl *ctl,
virBufferAsprintf(, ":%d", port);
}
+/* format the parameters part of the uri */
+virBufferAddLit(, "?");
+
/* TLS Port */
if (tls_port) {
-virBufferAsprintf(,
- "?tls-port=%d",
- tls_port);
-params = true;
+virBufferAsprintf(, "tls-port=%d&", tls_port);
}
if (STREQ(scheme, "spice") && passwd) {
-virBufferAsprintf(,
- "%spassword=%s",
- params ? "&" : "?",
- passwd);
-params = true;
+virBufferAsprintf(, "password=%s&", passwd);
}
+virBufferTrimLen(, 1);
+
return virBufferContentAndReset();
}
--
2.35.1
[PATCH 16/23] virsh: cmdSchedinfo: Add separate variable for holding flags used for query
Instead of having two ad-hoc places which decide whether the original
flags can be used add another variable specifically for flags used for
query.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index ca1145428f..2d1889c71e 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5158,6 +5158,7 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
size_t i;
bool ret_val = false;
unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
+unsigned int queryflags = VIR_DOMAIN_AFFECT_CURRENT;
bool current = vshCommandOptBool(cmd, "current");
bool config = vshCommandOptBool(cmd, "config");
bool live = vshCommandOptBool(cmd, "live");
@@ -5170,6 +5171,14 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
if (live)
flags |= VIR_DOMAIN_AFFECT_LIVE;
+/* We cannot query both live and config at once, so settle
+ on current in that case. If we are setting, then the two values should
+ match when we re-query; otherwise, we report the error later. */
+if (config && live)
+queryflags = VIR_DOMAIN_AFFECT_CURRENT;
+else
+queryflags = flags;
+
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
@@ -5188,12 +5197,7 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
memset(params, 0, sizeof(*params) * nparams);
if (flags || current) {
-/* We cannot query both live and config at once, so settle
- on current in that case. If we are setting, then the
- two values should match when we re-query; otherwise, we
- report the error later. */
-if (virDomainGetSchedulerParametersFlags(dom, params, ,
- ((live && config) ? 0 :
flags)) == -1)
+if (virDomainGetSchedulerParametersFlags(dom, params, ,
queryflags) == -1)
goto cleanup;
} else {
if (virDomainGetSchedulerParameters(dom, params, ) == -1)
@@ -5212,8 +5216,7 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
nupdates, flags) == -1)
goto cleanup;
-if (virDomainGetSchedulerParametersFlags(dom, params, ,
- ((live && config) ? 0 :
flags)) == -1)
+if (virDomainGetSchedulerParametersFlags(dom, params, ,
queryflags) == -1)
goto cleanup;
} else {
if (virDomainSetSchedulerParameters(dom, updates, nupdates) == -1)
--
2.35.1
[PATCH 14/23] virshGetOneDisplay: Don't reuse 'xpath' variable
Add autofreed per-xpath variables to simplify the code.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 74 +++-
1 file changed, 25 insertions(+), 49 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index d5157e4a63..d0f78798b5 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11667,66 +11667,50 @@ virshGetOneDisplay(vshControl *ctl,
{
const char *xpath_fmt = "string(/domain/devices/graphics[@type='%s']/%s)";
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-char *xpath = NULL;
+g_autofree char *xpathPort = NULL;
+g_autofree char *xpathPortTLS = NULL;
+g_autofree char *xpathListen = NULL;
+g_autofree char *xpathType = NULL;
+g_autofree char *xpathPasswd = NULL;
g_autofree char *listen_addr = NULL;
int port = 0;
int tls_port = 0;
g_autofree char *type_conn = NULL;
g_autofree char *sockpath = NULL;
g_autofree char *passwd = NULL;
-int tmp;
bool params = false;
-/* Create our XPATH lookup for the current display's port */
-VIR_FREE(xpath);
-xpath = g_strdup_printf(xpath_fmt, scheme, "@port");
-
/* Attempt to get the port number for the current graphics scheme */
-tmp = virXPathInt(xpath, ctxt, );
-VIR_FREE(xpath);
+xpathPort = g_strdup_printf(xpath_fmt, scheme, "@port");
-/* If there is no port number for this type, then jump to the next
- * scheme */
-if (tmp)
+if (virXPathInt(xpathPort, ctxt, ) < 0)
port = 0;
-/* Create our XPATH lookup for TLS Port (automatically skipped
- * for unsupported schemes */
-xpath = g_strdup_printf(xpath_fmt, scheme, "@tlsPort");
-
/* Attempt to get the TLS port number */
-tmp = virXPathInt(xpath, ctxt, _port);
-VIR_FREE(xpath);
-if (tmp)
-tls_port = 0;
+xpathPortTLS = g_strdup_printf(xpath_fmt, scheme, "@tlsPort");
-/* Create our XPATH lookup for the current display's address */
-xpath = g_strdup_printf(xpath_fmt, scheme, "@listen");
-
-/* Attempt to get the listening addr if set for the current
- * graphics scheme */
-listen_addr = virXPathString(xpath, ctxt);
-VIR_FREE(xpath);
+if (virXPathInt(xpathPortTLS, ctxt, _port) < 0)
+tls_port = 0;
-/* Create our XPATH lookup for the current spice type. */
-xpath = g_strdup_printf(xpath_fmt, scheme, "listen/@type");
+/* Attempt to get the listening addr if set for the current graphics
scheme */
+xpathListen = g_strdup_printf(xpath_fmt, scheme, "@listen");
+listen_addr = virXPathString(xpathListen, ctxt);
/* Attempt to get the type of spice connection */
-type_conn = virXPathString(xpath, ctxt);
-VIR_FREE(xpath);
+xpathType = g_strdup_printf(xpath_fmt, scheme, "listen/@type");
+type_conn = virXPathString(xpathType, ctxt);
if (STREQ_NULLABLE(type_conn, "socket")) {
-xpath = g_strdup_printf(xpath_fmt, scheme, "listen/@socket");
-
-sockpath = virXPathString(xpath, ctxt);
+g_autofree char *xpathSockpath = g_strdup_printf(xpath_fmt, scheme,
"listen/@socket");
-VIR_FREE(xpath);
+sockpath = virXPathString(xpathSockpath, ctxt);
}
if (!port && !tls_port && !sockpath)
-goto cleanup;
+return NULL;
if (!listen_addr) {
+g_autofree char *xpathListenAddress = NULL;
/* The subelement address - -
* *should* have been automatically backfilled into its
* parent (which we just tried to
@@ -11735,10 +11719,9 @@ virshGetOneDisplay(vshControl *ctl,
* subelement (which, by the way, doesn't exist on libvirt
* < 0.9.4, so we really do need to check both places)
*/
-xpath = g_strdup_printf(xpath_fmt, scheme, "listen/@address");
+xpathListenAddress = g_strdup_printf(xpath_fmt, scheme,
"listen/@address");
-listen_addr = virXPathString(xpath, ctxt);
-VIR_FREE(xpath);
+listen_addr = virXPathString(xpathListenAddress, ctxt);
} else {
virSocketAddr addr;
@@ -11759,17 +11742,13 @@ virshGetOneDisplay(vshControl *ctl,
}
}
-/* We can query this info for all the graphics types since we'll
+/* Attempt to get the password.
+ * We can query this info for all the graphics types since we'll
* get nothing for the unsupported ones (just rdp for now).
* Also the parameter '--include-password' was already taken
* care of when getting the XML */
-
-/* Create our XPATH lookup for the password */
-xpath = g_strdup_printf(xpath_fmt, scheme, "@passwd");
-
-/* Attempt to get the password */
-passwd = virXPathString(xpath, ctxt);
-VIR_FREE(xpath);
+xpathPasswd = g_strdup_printf(xpath_fmt, scheme, "@passwd");
+passwd = virXPathString(xpathPasswd, ctxt);
/* Build up the full URI, starting with the scheme */
if (sockpath)
@@ -11818,9 +11797,6 @@
[PATCH 13/23] virshGetOneDisplay: Automaticaly free extracted data
Use automatic memory freeing for the temporary variables hodling the
data extracted from the XML.
The code in this function was originally extracted from a loop so we can
also drop pre-clearing of the pointers.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 49 ++--
1 file changed, 15 insertions(+), 34 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 7c06c3f80d..d5157e4a63 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11668,15 +11668,14 @@ virshGetOneDisplay(vshControl *ctl,
const char *xpath_fmt = "string(/domain/devices/graphics[@type='%s']/%s)";
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
char *xpath = NULL;
-char *listen_addr = NULL;
+g_autofree char *listen_addr = NULL;
int port = 0;
int tls_port = 0;
-char *type_conn = NULL;
-char *sockpath = NULL;
-char *passwd = NULL;
+g_autofree char *type_conn = NULL;
+g_autofree char *sockpath = NULL;
+g_autofree char *passwd = NULL;
int tmp;
bool params = false;
-virSocketAddr addr;
/* Create our XPATH lookup for the current display's port */
VIR_FREE(xpath);
@@ -11706,7 +11705,6 @@ virshGetOneDisplay(vshControl *ctl,
/* Attempt to get the listening addr if set for the current
* graphics scheme */
-VIR_FREE(listen_addr);
listen_addr = virXPathString(xpath, ctxt);
VIR_FREE(xpath);
@@ -11714,18 +11712,15 @@ virshGetOneDisplay(vshControl *ctl,
xpath = g_strdup_printf(xpath_fmt, scheme, "listen/@type");
/* Attempt to get the type of spice connection */
-VIR_FREE(type_conn);
type_conn = virXPathString(xpath, ctxt);
VIR_FREE(xpath);
if (STREQ_NULLABLE(type_conn, "socket")) {
-if (!sockpath) {
-xpath = g_strdup_printf(xpath_fmt, scheme, "listen/@socket");
+xpath = g_strdup_printf(xpath_fmt, scheme, "listen/@socket");
-sockpath = virXPathString(xpath, ctxt);
+sockpath = virXPathString(xpath, ctxt);
-VIR_FREE(xpath);
-}
+VIR_FREE(xpath);
}
if (!port && !tls_port && !sockpath)
@@ -11745,28 +11740,22 @@ virshGetOneDisplay(vshControl *ctl,
listen_addr = virXPathString(xpath, ctxt);
VIR_FREE(xpath);
} else {
+virSocketAddr addr;
+
/* If listen_addr is 0.0.0.0 or [::] we should try to parse URI and set
- * listen_addr based on current URI. */
+ * listen_addr based on current URI. If that fails we'll print
+ * 'localhost' as the address as INADDR_ANY won't help the user. */
if (virSocketAddrParse(, listen_addr, AF_UNSPEC) > 0 &&
virSocketAddrIsWildcard()) {
virConnectPtr conn = ((virshControl *)(ctl->privData))->conn;
-char *uriStr = virConnectGetURI(conn);
-virURI *uri = NULL;
+g_autofree char *uriStr = virConnectGetURI(conn);
+g_autoptr(virURI) uri = NULL;
-if (uriStr) {
-uri = virURIParse(uriStr);
-VIR_FREE(uriStr);
-}
-
-/* It's safe to free the listen_addr even if parsing of URI
- * fails, if there is no listen_addr we will print "localhost". */
-VIR_FREE(listen_addr);
+g_clear_pointer(_addr, g_free);
-if (uri) {
+if (uriStr && (uri = virURIParse(uriStr)))
listen_addr = g_strdup(uri->server);
-virURIFree(uri);
-}
}
}
@@ -11779,7 +11768,6 @@ virshGetOneDisplay(vshControl *ctl,
xpath = g_strdup_printf(xpath_fmt, scheme, "@passwd");
/* Attempt to get the password */
-VIR_FREE(passwd);
passwd = virXPathString(xpath, ctxt);
VIR_FREE(xpath);
@@ -11803,9 +11791,6 @@ virshGetOneDisplay(vshControl *ctl,
else
virBufferAsprintf(, "%s", listen_addr);
-/* Free socket to prepare the pointer for the next iteration */
-VIR_FREE(sockpath);
-
/* Add the port */
if (port) {
if (STREQ(scheme, "vnc")) {
@@ -11835,10 +11820,6 @@ virshGetOneDisplay(vshControl *ctl,
cleanup:
VIR_FREE(xpath);
-VIR_FREE(type_conn);
-VIR_FREE(sockpath);
-VIR_FREE(passwd);
-VIR_FREE(listen_addr);
return virBufferContentAndReset();
}
--
2.35.1
[PATCH 12/23] virsh: cmdDomDisplay: Remove unneeded 'cleanup' label
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index f82aa49745..7c06c3f80d 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11864,17 +11864,17 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
if (!virDomainIsActive(dom)) {
vshError(ctl, _("Domain is not running"));
-goto cleanup;
+return false;
}
if (vshCommandOptBool(cmd, "include-password"))
flags |= VIR_DOMAIN_XML_SECURE;
if (vshCommandOptStringReq(ctl, cmd, "type", ) < 0)
-goto cleanup;
+return false;
if (virshDomainGetXMLFromDom(ctl, dom, flags, , ) < 0)
-goto cleanup;
+return false;
/* Attempt to grab our display info */
for (iter = 0; scheme[iter] != NULL; iter++) {
@@ -11903,7 +11903,6 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
vshError(ctl, _("No graphical display found"));
}
- cleanup:
return ret;
}
--
2.35.1
[PATCH 11/23] virsh: cmdDomDisplay: Extract loop body fetching display URIs into 'virshGetOneDisplay'
Separate the code so that the function is not as massive. Note that this
is a minimal extraction which does not clean up the code meant for
looping.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 352 ++-
1 file changed, 183 insertions(+), 169 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index dc6e3b5020..f82aa49745 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11659,215 +11659,235 @@ static const vshCmdOptDef opts_domdisplay[] = {
{.name = NULL}
};
-static bool
-cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
+
+static char *
+virshGetOneDisplay(vshControl *ctl,
+ const char *scheme,
+ xmlXPathContext *ctxt)
{
-g_autoptr(xmlDoc) xml = NULL;
-g_autoptr(xmlXPathContext) ctxt = NULL;
-g_autoptr(virshDomain) dom = NULL;
+const char *xpath_fmt = "string(/domain/devices/graphics[@type='%s']/%s)";
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-bool ret = false;
char *xpath = NULL;
char *listen_addr = NULL;
-int port, tls_port = 0;
+int port = 0;
+int tls_port = 0;
char *type_conn = NULL;
char *sockpath = NULL;
char *passwd = NULL;
-char *output = NULL;
-const char *scheme[] = { "vnc", "spice", "rdp", NULL };
-const char *type = NULL;
-int iter = 0;
int tmp;
-int flags = 0;
bool params = false;
-bool all = vshCommandOptBool(cmd, "all");
-const char *xpath_fmt = "string(/domain/devices/graphics[@type='%s']/%s)";
virSocketAddr addr;
-VSH_EXCLUSIVE_OPTIONS("all", "type");
+/* Create our XPATH lookup for the current display's port */
+VIR_FREE(xpath);
+xpath = g_strdup_printf(xpath_fmt, scheme, "@port");
-if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
-return false;
+/* Attempt to get the port number for the current graphics scheme */
+tmp = virXPathInt(xpath, ctxt, );
+VIR_FREE(xpath);
-if (!virDomainIsActive(dom)) {
-vshError(ctl, _("Domain is not running"));
-goto cleanup;
-}
+/* If there is no port number for this type, then jump to the next
+ * scheme */
+if (tmp)
+port = 0;
-if (vshCommandOptBool(cmd, "include-password"))
-flags |= VIR_DOMAIN_XML_SECURE;
+/* Create our XPATH lookup for TLS Port (automatically skipped
+ * for unsupported schemes */
+xpath = g_strdup_printf(xpath_fmt, scheme, "@tlsPort");
-if (vshCommandOptStringReq(ctl, cmd, "type", ) < 0)
-goto cleanup;
+/* Attempt to get the TLS port number */
+tmp = virXPathInt(xpath, ctxt, _port);
+VIR_FREE(xpath);
+if (tmp)
+tls_port = 0;
-if (virshDomainGetXMLFromDom(ctl, dom, flags, , ) < 0)
-goto cleanup;
+/* Create our XPATH lookup for the current display's address */
+xpath = g_strdup_printf(xpath_fmt, scheme, "@listen");
-/* Attempt to grab our display info */
-for (iter = 0; scheme[iter] != NULL; iter++) {
-/* Particular scheme requested */
-if (!all && type && STRNEQ(type, scheme[iter]))
-continue;
+/* Attempt to get the listening addr if set for the current
+ * graphics scheme */
+VIR_FREE(listen_addr);
+listen_addr = virXPathString(xpath, ctxt);
+VIR_FREE(xpath);
-/* Create our XPATH lookup for the current display's port */
-VIR_FREE(xpath);
-xpath = g_strdup_printf(xpath_fmt, scheme[iter], "@port");
+/* Create our XPATH lookup for the current spice type. */
+xpath = g_strdup_printf(xpath_fmt, scheme, "listen/@type");
-/* Attempt to get the port number for the current graphics scheme */
-tmp = virXPathInt(xpath, ctxt, );
-VIR_FREE(xpath);
+/* Attempt to get the type of spice connection */
+VIR_FREE(type_conn);
+type_conn = virXPathString(xpath, ctxt);
+VIR_FREE(xpath);
-/* If there is no port number for this type, then jump to the next
- * scheme */
-if (tmp)
-port = 0;
+if (STREQ_NULLABLE(type_conn, "socket")) {
+if (!sockpath) {
+xpath = g_strdup_printf(xpath_fmt, scheme, "listen/@socket");
-/* Create our XPATH lookup for TLS Port (automatically skipped
- * for unsupported schemes */
-xpath = g_strdup_printf(xpath_fmt, scheme[iter], "@tlsPort");
+sockpath = virXPathString(xpath, ctxt);
-/* Attempt to get the TLS port number */
-tmp = virXPathInt(xpath, ctxt, _port);
-VIR_FREE(xpath);
-if (tmp)
-tls_port = 0;
+VIR_FREE(xpath);
+}
+}
+
+if (!port && !tls_port && !sockpath)
+goto cleanup;
-/* Create our XPATH lookup for the current display's address */
-xpath = g_strdup_printf(xpath_fmt, scheme[iter], "@listen");
+if (!listen_addr) {
+/* The subelement address - -
+ * *should* have been
[PATCH 07/23] virshEventPrint: Use automatic memory clearing
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 25097627ac..33984618eb 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -13166,13 +13166,13 @@ static void
virshEventPrint(virshDomEventData *data,
virBuffer *buf)
{
-char *msg;
+g_autofree char *msg = NULL;
if (!(msg = virBufferContentAndReset(buf)))
return;
if (!data->loop && *data->count)
-goto cleanup;
+return;
if (data->timestamp) {
char timestamp[VIR_TIME_STRING_BUFLEN];
@@ -13188,9 +13188,6 @@ virshEventPrint(virshDomEventData *data,
(*data->count)++;
if (!data->loop)
vshEventDone(data->ctl);
-
- cleanup:
-VIR_FREE(msg);
}
static void
--
2.35.1
[PATCH 05/23] virsh: virshVcpuinfoPrintAffinity: Use if-else instead of ternary operator
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 732690ec44..73f05ce7f9 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6721,8 +6721,12 @@ virshVcpuinfoPrintAffinity(vshControl *ctl,
return -1;
vshPrint(ctl, _("%s (out of %d)"), str, maxcpu);
} else {
-for (i = 0; i < maxcpu; i++)
-vshPrint(ctl, "%c", VIR_CPU_USED(cpumap, i) ? 'y' : '-');
+for (i = 0; i < maxcpu; i++) {
+if (VIR_CPU_USED(cpumap, i))
+vshPrint(ctl, "y");
+else
+vshPrint(ctl, "-");
+}
}
vshPrint(ctl, "\n");
--
2.35.1
[PATCH 02/23] virsh: cmdStart: Rewrite ternary operator use to standard if conditions
Rewrite the invocation of the virDomainCreate(WithFiles/Flags) APIs
based on the arguments into if-else instead of (nested) ternary
operators.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 23 +--
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 6f0c063438..4c90f40f86 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4104,10 +4104,15 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
/* We can emulate force boot, even for older servers that reject it. */
if (flags & VIR_DOMAIN_START_FORCE_BOOT) {
-if ((nfds ?
- virDomainCreateWithFiles(dom, nfds, fds, flags) :
- virDomainCreateWithFlags(dom, flags)) == 0)
+if (nfds > 0) {
+rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
+} else {
+rc = virDomainCreateWithFlags(dom, flags);
+}
+
+if (rc == 0)
goto started;
+
if (last_error->code != VIR_ERR_NO_SUPPORT &&
last_error->code != VIR_ERR_INVALID_ARG) {
vshReportError(ctl);
@@ -4128,9 +4133,15 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
}
/* Prefer older API unless we have to pass a flag. */
-if ((nfds ? virDomainCreateWithFiles(dom, nfds, fds, flags) :
- (flags ? virDomainCreateWithFlags(dom, flags)
- : virDomainCreate(dom))) < 0) {
+if (nfds > 0) {
+rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
+} else if (flags != 0) {
+rc = virDomainCreateWithFlags(dom, flags);
+} else {
+rc = virDomainCreate(dom);
+}
+
+if (rc < 0) {
vshError(ctl, _("Failed to start domain '%s'"), virDomainGetName(dom));
return false;
}
--
2.35.1
[PATCH 10/23] virsh: cmdEvent: Rewrite questionable event registration
The code registering the event handlers in 'cmdEvent' had too many
blocks of code conditional on whether just one event is being listened
to or all events.
The code can be greatly simplified by uniting the code paths and having
only one branch when filling the list of events we want to listen for.
Signed-off-by: Peter Krempa
---
tools/virsh-domain-event.c | 70 +++---
1 file changed, 35 insertions(+), 35 deletions(-)
diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c
index 51571dffad..1a2f1cb6e0 100644
--- a/tools/virsh-domain-event.c
+++ b/tools/virsh-domain-event.c
@@ -274,6 +274,7 @@ typedef struct virshDomainEventCallback
virshDomainEventCallback;
struct virshDomEventData {
vshControl *ctl;
+int event;
bool loop;
int *count;
bool timestamp;
@@ -885,10 +886,10 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
g_autoptr(virshDomain) dom = NULL;
bool ret = false;
int timeout = 0;
-virshDomEventData *data = NULL;
+g_autofree virshDomEventData *data = NULL;
+size_t ndata = 0;
size_t i;
const char *eventName = NULL;
-int event = -1;
bool all = vshCommandOptBool(cmd, "all");
bool loop = vshCommandOptBool(cmd, "loop");
bool timestamp = vshCommandOptBool(cmd, "timestamp");
@@ -900,59 +901,59 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
VSH_EXCLUSIVE_OPTIONS("list", "event");
if (vshCommandOptBool(cmd, "list")) {
-for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++)
-vshPrint(ctl, "%s\n", virshDomainEventCallbacks[event].name);
+for (i = 0; i < G_N_ELEMENTS(virshDomainEventCallbacks); i++)
+vshPrint(ctl, "%s\n", virshDomainEventCallbacks[i].name);
return true;
}
if (vshCommandOptStringReq(ctl, cmd, "event", ) < 0)
return false;
-if (eventName) {
-for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++)
-if (STREQ(eventName, virshDomainEventCallbacks[event].name))
-break;
-if (event == VIR_DOMAIN_EVENT_ID_LAST) {
-vshError(ctl, _("unknown event type %s"), eventName);
-return false;
-}
-} else if (!all) {
+
+if (!eventName && !all) {
vshError(ctl, "%s",
_("one of --list, --all, or --event is required"));
return false;
}
-if (all) {
-data = g_new0(virshDomEventData, VIR_DOMAIN_EVENT_ID_LAST);
-for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) {
-data[i].ctl = ctl;
-data[i].loop = loop;
-data[i].count =
-data[i].timestamp = timestamp;
-data[i].cb = [i];
-data[i].id = -1;
-}
-} else {
+if (eventName)
data = g_new0(virshDomEventData, 1);
-data[0].ctl = ctl;
-data[0].loop = vshCommandOptBool(cmd, "loop");
-data[0].count =
-data[0].timestamp = timestamp;
-data[0].cb = [event];
-data[0].id = -1;
+else
+data = g_new0(virshDomEventData,
G_N_ELEMENTS(virshDomainEventCallbacks));
+
+for (i = 0; i < G_N_ELEMENTS(virshDomainEventCallbacks); i++) {
+if (eventName &&
+STRNEQ(eventName, virshDomainEventCallbacks[i].name))
+continue;
+
+data[i].event = i;
+data[i].ctl = ctl;
+data[i].loop = loop;
+data[i].count =
+data[i].timestamp = timestamp;
+data[i].cb = [i];
+data[i].id = -1;
+ndata++;
}
+
+if (ndata == 0) {
+vshError(ctl, _("unknown event type %s"), eventName);
+return false;
+}
+
if (vshCommandOptTimeoutToMs(ctl, cmd, ) < 0)
goto cleanup;
-if (vshCommandOptBool(cmd, "domain"))
+if (vshCommandOptBool(cmd, "domain")) {
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
goto cleanup;
+}
if (vshEventStart(ctl, timeout) < 0)
goto cleanup;
-for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : 1); i++) {
+for (i = 0; i < ndata; i++) {
if ((data[i].id = virConnectDomainEventRegisterAny(priv->conn, dom,
- all ? i : event,
+ data[i].event,
data[i].cb->cb,
[i],
NULL)) < 0) {
@@ -985,12 +986,11 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
cleanup:
vshEventCleanup(ctl);
if (data) {
-for (i = 0; i < (all ? VIR_DOMAIN_EVENT_ID_LAST : 1); i++) {
+for (i = 0; i < ndata; i++) {
if (data[i].id >= 0 &&
virConnectDomainEventDeregisterAny(priv->conn, data[i].id) < 0)
ret = false;
}
-VIR_FREE(data);
}
return ret;
}
--
[PATCH 01/23] virsh: cmdBlockcopy: Use virXMLFormatElement
Rewrite the formatting of the block copy target xml using
virXMLFormatElement.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 9b1b14cdc2..6f0c063438 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2475,14 +2475,20 @@ cmdBlockcopy(vshControl *ctl, const vshCmd *cmd)
if (!xmlstr) {
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-virBufferAsprintf(, "\n",
- blockdev ? "block" : "file");
-virBufferAdjustIndent(, 2);
-virBufferAsprintf(, "\n", dest);
+g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
+g_auto(virBuffer) childBuf = VIR_BUFFER_INITIALIZER;
+
+if (blockdev) {
+virBufferAddLit(, " type='block'");
+virBufferAddLit(, "\n", dest);
virBufferEscapeString(, "\n", format);
-virBufferAdjustIndent(, -2);
-virBufferAddLit(, "\n");
+virXMLFormatElement(, "disk", , );
xmlstr = virBufferContentAndReset();
}
--
2.35.1
[PATCH 09/23] virsh: Move 'cmdEvent' and all of it's machinery to virsh-domain-event.c
'cmdEvent' along with all the helper functions it needs is ~950 LOC.
Move it out from virsh-domain.c to virsh-domain-event.c along with the
completer function so that the new module doesn't have to expose any new
types.
Semantically this creates a new category in 'virsh help' but all other
behaviour stays the same.
Signed-off-by: Peter Krempa
---
po/POTFILES.in |1 +
tools/meson.build |1 +
tools/virsh-completer-domain.c | 19 -
tools/virsh-completer-domain.h |5 -
tools/virsh-domain-event.c | 1007
tools/virsh-domain-event.h | 23 +
tools/virsh-domain.c | 946 --
tools/virsh.c |2 +
tools/virsh.h |1 +
9 files changed, 1035 insertions(+), 970 deletions(-)
create mode 100644 tools/virsh-domain-event.c
create mode 100644 tools/virsh-domain-event.h
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 1fd3afdd6f..0d9adb0758 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -354,6 +354,7 @@
@SRCDIR@tools/virsh-checkpoint.c
@SRCDIR@tools/virsh-completer-host.c
@SRCDIR@tools/virsh-console.c
+@SRCDIR@tools/virsh-domain-event.c
@SRCDIR@tools/virsh-domain-monitor.c
@SRCDIR@tools/virsh-domain.c
@SRCDIR@tools/virsh-edit.c
diff --git a/tools/meson.build b/tools/meson.build
index f4b4a16c29..ac714e6425 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -174,6 +174,7 @@ executable(
'virsh-completer-volume.c',
'virsh-console.c',
'virsh-domain.c',
+'virsh-domain-event.c',
'virsh-domain-monitor.c',
'virsh-host.c',
'virsh-interface.c',
diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
index 321c47ef65..250dd8b21a 100644
--- a/tools/virsh-completer-domain.c
+++ b/tools/virsh-completer-domain.c
@@ -357,25 +357,6 @@ virshDomainBlockjobBaseTopCompleter(vshControl *ctl,
return ret;
}
-char **
-virshDomainEventNameCompleter(vshControl *ctl G_GNUC_UNUSED,
- const vshCmd *cmd G_GNUC_UNUSED,
- unsigned int flags)
-{
-size_t i = 0;
-g_auto(GStrv) tmp = NULL;
-
-virCheckFlags(0, NULL);
-
-tmp = g_new0(char *, VIR_DOMAIN_EVENT_ID_LAST + 1);
-
-for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++)
-tmp[i] = g_strdup(virshDomainEventCallbacks[i].name);
-
-return g_steal_pointer();
-}
-
-
char **
virshDomainInterfaceStateCompleter(vshControl *ctl,
const vshCmd *cmd,
diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
index 044c675842..27cf963912 100644
--- a/tools/virsh-completer-domain.h
+++ b/tools/virsh-completer-domain.h
@@ -41,11 +41,6 @@ virshDomainDiskTargetCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
-char **
-virshDomainEventNameCompleter(vshControl *ctl,
- const vshCmd *cmd,
- unsigned int flags);
-
char **
virshDomainInterfaceStateCompleter(vshControl *ctl,
const vshCmd *cmd,
diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c
new file mode 100644
index 00..51571dffad
--- /dev/null
+++ b/tools/virsh-domain-event.c
@@ -0,0 +1,1007 @@
+/*
+ * virsh-domain-event.c: Domain event listening commands
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ *
[PATCH 08/23] virsh: Move 'virshDomainBlockJobToString' to virsh-util
The helper function is used in virshBlockJobInfo and also in the
callbacks of cmdEvent. Upcoming patch is going to move out the event
code into a helper so this needs to be in a shared place.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 19 ---
tools/virsh-util.c | 19 +++
tools/virsh-util.h | 5 +
3 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 33984618eb..9c304dbf78 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2614,25 +2614,6 @@ static const vshCmdOptDef opts_blockjob[] = {
{.name = NULL}
};
-VIR_ENUM_DECL(virshDomainBlockJob);
-VIR_ENUM_IMPL(virshDomainBlockJob,
- VIR_DOMAIN_BLOCK_JOB_TYPE_LAST,
- N_("Unknown job"),
- N_("Block Pull"),
- N_("Block Copy"),
- N_("Block Commit"),
- N_("Active Block Commit"),
- N_("Backup"),
-);
-
-static const char *
-virshDomainBlockJobToString(int type)
-{
-const char *str = virshDomainBlockJobTypeToString(type);
-return str ? _(str) : _("Unknown job");
-}
-
-
static bool
virshBlockJobInfo(vshControl *ctl,
virDomainPtr dom,
diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index 8fb617fa3c..dc6ed7a86d 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -418,3 +418,22 @@ virshDomainGetXML(vshControl *ctl,
return ret;
}
+
+
+VIR_ENUM_IMPL(virshDomainBlockJob,
+ VIR_DOMAIN_BLOCK_JOB_TYPE_LAST,
+ N_("Unknown job"),
+ N_("Block Pull"),
+ N_("Block Copy"),
+ N_("Block Commit"),
+ N_("Active Block Commit"),
+ N_("Backup"),
+);
+
+
+const char *
+virshDomainBlockJobToString(int type)
+{
+const char *str = virshDomainBlockJobTypeToString(type);
+return str ? _(str) : _("Unknown job");
+}
diff --git a/tools/virsh-util.h b/tools/virsh-util.h
index 838935d5e8..4d4fe6c01e 100644
--- a/tools/virsh-util.h
+++ b/tools/virsh-util.h
@@ -151,3 +151,8 @@ virshDomainGetXML(vshControl *ctl,
xmlXPathContextPtr *ctxt)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4)
ATTRIBUTE_NONNULL(5) G_GNUC_WARN_UNUSED_RESULT;
+
+VIR_ENUM_DECL(virshDomainBlockJob);
+
+const char *
+virshDomainBlockJobToString(int type);
--
2.35.1
[PATCH 00/23] virsh: various small cleanups
A collection of mostly small patches caused by the geopolitically induced attention span shortening. Peter Krempa (23): virsh: cmdBlockcopy: Use virXMLFormatElement virsh: cmdStart: Rewrite ternary operator use to standard if conditions virsh: doSave: Use if-else instead of ternary operator virsh: cmdRestore: Use if-else instead of ternary operator virsh: virshVcpuinfoPrintAffinity: Use if-else instead of ternary operator virsh: Use NULLSTR_EMPTY instead of ternary operator virshEventPrint: Use automatic memory clearing virsh: Move 'virshDomainBlockJobToString' to virsh-util virsh: Move 'cmdEvent' and all of it's machinery to virsh-domain-event.c virsh: cmdEvent: Rewrite questionable event registration virsh: cmdDomDisplay: Extract loop body fetching display URIs into 'virshGetOneDisplay' virsh: cmdDomDisplay: Remove unneeded 'cleanup' label virshGetOneDisplay: Automaticaly free extracted data virshGetOneDisplay: Don't reuse 'xpath' variable virshGetOneDisplay: Refactor formatting of URI params virsh: cmdSchedinfo: Add separate variable for holding flags used for query virsh: cmdDesc: Use separate flags variable for getters vsh: Add helper for auto-removing temporary file virsh: cmdDesc: Use 'vshTempFile' type to simplify cleanup virsh: cmdDesc: Automatically free memory virsh: cmdDesc: Remove unneeded 'cleanup' virsh: domain: Don't use ternaries inside vshPrint/vshError functions virsh: cmdDesc: Fix logic when '-edit' is used along with 'desc' argument po/POTFILES.in |1 + tools/meson.build |1 + tools/virsh-completer-domain.c | 19 - tools/virsh-completer-domain.h |5 - tools/virsh-domain-event.c | 1007 tools/virsh-domain-event.h | 23 + tools/virsh-domain.c | 1623 +++- tools/virsh-util.c | 19 + tools/virsh-util.h |5 + tools/virsh.c |2 + tools/virsh.h |1 + tools/vsh.c| 25 +- tools/vsh.h|3 + 13 files changed, 1412 insertions(+), 1322 deletions(-) create mode 100644 tools/virsh-domain-event.c create mode 100644 tools/virsh-domain-event.h -- 2.35.1
[PATCH 03/23] virsh: doSave: Use if-else instead of ternary operator
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 4c90f40f86..607eb973ac 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4213,6 +4213,7 @@ doSave(void *opaque)
unsigned int flags = 0;
const char *xmlfile = NULL;
g_autofree char *xml = NULL;
+int rc;
#ifndef WIN32
sigset_t sigmask, oldsigmask;
@@ -4244,9 +4245,13 @@ doSave(void *opaque)
goto out;
}
-if (((flags || xml)
- ? virDomainSaveFlags(dom, to, xml, flags)
- : virDomainSave(dom, to)) < 0) {
+if (flags || xml) {
+rc = virDomainSaveFlags(dom, to, xml, flags);
+} else {
+rc = virDomainSave(dom, to);
+}
+
+if (rc < 0) {
vshError(ctl, _("Failed to save domain '%s' to %s"), name, to);
goto out;
}
--
2.35.1
Re: [PATCH] qemu: segmentation fault in virtqemud executing qemuDomainUndefineFlags
On Wed, Mar 02, 2022 at 09:49:35 +0100, Michal Prívozník wrote: > On 3/2/22 00:21, Jim Fehlig wrote: > > On 3/1/22 10:47, Boris Fiuczynski wrote: > >> Commit 5adfb3472342741c443ac91dee0abb18b5a3d038 causes a segmentation > >> fault. > > Pushed now. I believe, this warrants maintenance release. Jirka? Well, the bug definitely deserves to be mentioned in NEWS. We still mention maintenance branches and releases on our web, but we haven't done them for quite some time. The most recent commit in a maintenance branch is almost three years old. So the question is whether anyone actually expects our maintenance releases. I believe pretty much any downstream was doing their own backports and releases and the only user of our maint branches was Fedora ages ago. But I'm not against doing the release if we think it might help. If, however, we decide not to do it, we should remove (or better rewrite) the corresponding part of our docs to better match reality. Jirka
[PATCH] qemu: domainjob: Allow free if cb is not set in qemuDomainObjClearJob
We should allow resetting and freeing qemuDomainJobObj even if
'cb' attribute is not set. This is theoretical for now, but the
attribute must not be always set in the future, so early return
would create memory leaks. It is sufficient to check if 'cb'
exists before dereferencing it in qemuDomainObjClearJob() and
also qemuDomainObjResetAsyncJob() as the latter is called from
the former.
This commit partially reverts af16e754cd4efc3ca1.
Signed-off-by: Kristina Hanicova
---
src/qemu/qemu_domainjob.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
index 3e73eba4ed..587c166d94 100644
--- a/src/qemu/qemu_domainjob.c
+++ b/src/qemu/qemu_domainjob.c
@@ -239,8 +239,10 @@ qemuDomainObjResetAsyncJob(qemuDomainJobObj *job)
job->abortJob = false;
VIR_FREE(job->error);
g_clear_pointer(>current, virDomainJobDataFree);
-job->cb->resetJobPrivate(job->privateData);
job->apiFlags = 0;
+
+if (job->cb)
+job->cb->resetJobPrivate(job->privateData);
}
int
@@ -270,16 +272,15 @@ qemuDomainObjRestoreJob(virDomainObj *obj,
void
qemuDomainObjClearJob(qemuDomainJobObj *job)
{
-if (!job->cb)
-return;
-
qemuDomainObjResetJob(job);
qemuDomainObjResetAsyncJob(job);
-g_clear_pointer(>privateData, job->cb->freeJobPrivate);
g_clear_pointer(>current, virDomainJobDataFree);
g_clear_pointer(>completed, virDomainJobDataFree);
virCondDestroy(>cond);
virCondDestroy(>asyncCond);
+
+if (job->cb)
+g_clear_pointer(>privateData, job->cb->freeJobPrivate);
}
bool
--
2.35.1
Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage
On Wed, Mar 02, 2022 at 09:43:24AM +, Daniel P. Berrangé wrote: ... > > > > > > No, I got that part. My question was whether > > > > > > other-project-pipeline: > > > trigger: > > > project: other-project > > > strategy: depend > > > > > > our-job: > > > needs: > > > - other-project-pipeline > > > - project: other-project > > > job: other-project-job > > > artifacts: true > > > > > > actually guarantees that the instance of other-project-job whose > > > artifacts are available to our-job is the same one that was started > > > as part of the pipeline triggered by the other-project-pipeline job. > > > > Sorry for a delayed response. > > > > I don't think so. We can basically only rely on a fact that the jobs would > > actually be queued in order they arrive which means that jobs submitted > > earlier > > should finish earlier, but that of course is only a premise not a guarantee. > > > > On the other hand I never intended to run the integration CI on every single > > push to the master branch, instead, I wanted to make this a scheduled > > pipeline > > which would effectively alleviate the problem, because with scheduled > > pipelines > > there would very likely not be a concurrent pipeline running in libvirt-perl > > which would make us download artifacts from a pipeline we didn't trigger > > ourselves. > > Ultimately when we switch to using merge requests, the integration tests > should be run as a gating job, triggered from the merge train when the > code gets applied to git, so that we prevent regressions actually making > it into git master at all. > > Post-merge integration testing always exhibits the problem that people will > consider it somebody else's problem to fix the regression. So effectively > whoever creates the integration testing system ends up burdened with the > job of investigating failures and finding someone to poke to fix it. With > it run pre-merge then whoever wants to get their code merged needs to > investigate the problems. Now sometimes the problems with of course be > with the integration test system itself, not the submitters code, but > this is OK because it leads to situation where the job of maintaining > the integration tests are more equitably spread across all involved and > builds a mindset that functional / integration testing is a critical > part of delivering code, which is something we've lacked for too long > in libvirt. Agreed. > > > > > > > Taking a step back, why exactly are we triggering a rebuild of > > > > > libvirt-perl in the first place? Once we change that project's > > > > > pipeline so that RPMs are published as artifacts, can't we just grab > > > > > the ones from the latest successful pipeline? Maybe you've already > > > > > explained why you did things this way and I just missed it! > > > > > > > > ...which brings us here. Well, I adopted the mantra that all > > > > libvirt-friends > > > > projects depend on libvirt and given that we need libvirt-perl bindings > > > > to test > > > > upstream, I'd like to always have the latest bindings available to test > > > > with > > > > the current upstream build. The other reason why I did the way you > > > > commented on > > > > is that during development of the proposal many times I had to make > > > > changes to > > > > both libvirt and libvirt-perl in lockstep and it was tremendously > > > > frustrating > > > > to wait for the pipeline to get to the integration stage only to > > > > realize that > > > > the integration job didn't wait for the latest bindings and instead > > > > picked up > > > > the previous latest artifacts which I knew were either faulty or didn't > > > > contain > > > > the necessary changes yet. > > > > > > Of course that would be annoying when you're making changes to both > > > projects at the same time, but is that a scenario that we can expect > > > to be common once the integration tests are in place? > > > > > > To be clear, I'm not necessarily against the way you're doing things > > > right now, it's just that it feels like using the artifacts from the > > > latest successful libvirt-perl pipeline would lower complexity, avoid > > > burning additional resources and reduce wait times. > > > > > > If the only only downside is having a worse experience when making > > > changes to the pipeline, and we can expect that to be infrequent > > > enough, perhaps that's a reasonable tradeoff. > > > > I gave this more thought. What you suggest is viable, but the following is > > worth > > considering if we go with your proposal: > > > > - libvirt-perl jobs build upstream libvirt first in order to build the > > bindings > > -> generally it takes until right before the release that APIs/constants > >are added to the respective bindings (Perl/Python) > > -> if we rely on the latest libvirt-perl artifacts without actually > >triggering the pipeline, yes, the artifacts would be
Re: [PATCH v5 0/8] Implement detach device related APIs for test driver
On Mon, 2022-02-07 at 15:38 +0800, Luke Yue wrote: > diff to v4: > - Rebase to current master > > diff to v3: > - Add virDomainDeviceDefOperationsCallbacks to xmlopt for de- > duplicating purpose > - Add virDomainDeviceTypeFlags for de-duplicating purpose > - Remove the memballoon helper function > - Squash test_driver commits > - Move test device xmls to generichotplugdata > - Reimplement tests with internal APIs > > link to v4: > https://listman.redhat.com/archives/libvir-list/2021-December/msg00108.html > link to v3: > https://listman.redhat.com/archives/libvir-list/2021-November/msg00288.html > link to CI: https://gitlab.com/lukedyue/libvirt/-/pipelines/464756840 > > Luke Yue (8): > conf: Introduce virDomainInputDefRemove and fix memory leak > conf: Introduce virDomainDeviceDefOperationsCallbacks to xmlopt > conf: Add virDomainDeviceTypeFlags and use it in various drivers > conf: Add tpm helpers for future use > domain_driver: extract DetachXXXDeviceConfig related functions and > use > them > test_driver: Implement virDomainDetachDeviceFlags > examples: xml: test: add xml for testing devices related APIs > tests: Add generichotplugtest > > examples/xml/test/meson.build | 1 + > examples/xml/test/testdomfc5.xml | 54 ++ > examples/xml/test/testnodeinline.xml | 54 ++ > src/bhyve/bhyve_domain.c | 2 +- > src/ch/ch_conf.c | 2 +- > src/conf/domain_conf.c | 572 > +- > src/conf/domain_conf.h | 135 - > src/conf/virconftypes.h | 2 + > src/hyperv/hyperv_driver.c | 3 +- > src/libvirt_private.syms | 21 + > src/libxl/libxl_conf.c | 3 +- > src/libxl/libxl_domain.c | 8 + > src/libxl/libxl_domain.h | 1 + > src/libxl/libxl_driver.c | 73 +-- > src/lxc/lxc_conf.c | 3 +- > src/lxc/lxc_domain.c | 7 + > src/lxc/lxc_domain.h | 1 + > src/lxc/lxc_driver.c | 62 +- > src/openvz/openvz_conf.c | 2 +- > src/qemu/qemu_conf.c | 3 +- > src/qemu/qemu_domain.c | 39 ++ > src/qemu/qemu_domain.h | 6 + > src/qemu/qemu_driver.c | 206 +-- > src/qemu/qemu_process.c | 2 +- > src/security/virt-aa-helper.c | 2 +- > src/test/test_driver.c | 197 +- > src/vbox/vbox_common.c | 2 +- > src/vmware/vmware_driver.c | 2 +- > src/vmx/vmx.c | 2 +- > src/vz/vz_driver.c | 2 +- > tests/bhyveargv2xmltest.c | 2 +- > .../generichotplug-controller.xml | 1 + > .../generichotplug-disk-cdrom.xml | 5 + > .../generichotplug-filesystem.xml | 6 + > .../generichotplug-hostdev.xml | 5 + > .../generichotplug-input.xml | 1 + > .../generichotplug-interface.xml | 6 + > .../generichotplug-lease.xml | 5 + > .../generichotplug-memballoon.xml | 3 + > .../generichotplug-memory.xml | 6 + > .../generichotplugdata/generichotplug-rng.xml | 4 + > .../generichotplug-shmem.xml | 4 + > .../generichotplug-sound.xml | 3 + > .../generichotplugdata/generichotplug-tpm.xml | 5 + > .../generichotplug-vsock.xml | 3 + > .../generichotplug-watchdog.xml | 1 + > tests/generichotplugtest.c | 178 ++ > tests/meson.build | 1 + > tests/testutils.c | 2 +- > 49 files changed, 1378 insertions(+), 332 deletions(-) > create mode 100644 tests/generichotplugdata/generichotplug- > controller.xml > create mode 100644 tests/generichotplugdata/generichotplug-disk- > cdrom.xml > create mode 100644 tests/generichotplugdata/generichotplug- > filesystem.xml > create mode 100644 tests/generichotplugdata/generichotplug- > hostdev.xml > create mode 100644 tests/generichotplugdata/generichotplug-input.xml > create mode 100644 tests/generichotplugdata/generichotplug- > interface.xml > create mode 100644 tests/generichotplugdata/generichotplug-lease.xml > create mode 100644 tests/generichotplugdata/generichotplug- > memballoon.xml > create mode 100644 tests/generichotplugdata/generichotplug- > memory.xml > create mode 100644 tests/generichotplugdata/generichotplug-rng.xml > create mode 100644 tests/generichotplugdata/generichotplug-shmem.xml > create mode 100644
Re: [PATCH 01/17] ci: Drop Ubuntu 1804
On Wed, Mar 02, 2022 at 09:12:55AM +0100, Peter Krempa wrote: > On Tue, Mar 01, 2022 at 17:46:44 +0100, Erik Skultety wrote: > > On Tue, Feb 15, 2022 at 02:47:44PM +0100, Peter Krempa wrote: > > > As of April 23 2022, Ubuntu 20.04 will be out for two years, which per > > > our platform support policy means we no longer have to support > > > Ubuntu 18.04. > > > > Would you mind contributing the patch to libvirt-ci and regenerating the > > gitlab.yml config with lcitool from manifest when the time comes? :) > > So ... is libvirt-ci always fully mirroring what libvirt does? > > AFAIU lcitool is used at least within the qemu project and I didn't > really check to see whether qemu will continue caring about Ubuntu 18.04 > and the READMEs in libvirt-ci aren't clearing up the expectations > either. Accidentally (on purpose), I proposed a platform support matrix for QEMU that has the same rules as libvirt. So broadly speaking both projects will target the same platforms at any given point in time. None the less we should *NOT* remove platforms from libvirt-ci as the first step. We should remove the platforms from usage in all projects first. Removing from libvirt-ci should be the last thing. This is because while projects broadly follow the same goals, the timeframe in which those goals are applied may not line up exactly. There can be constraints from the software release cycles. QEMU is about to enter freeze, but if they encounter problems in CI they still want to be able to pull in updates from libvirt-ci, without Ubuntu 18.04 support being ripped out from under their feet. 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 :|
Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage
On Wed, Mar 02, 2022 at 08:42:30AM +0100, Erik Skultety wrote: > ... > > > > > > +libvirt-perl-bindings: > > > > > + stage: bindings > > > > > + trigger: > > > > > +project: eskultety/libvirt-perl > > > > > +branch: multi-project-ci > > > > > +strategy: depend > > > > > + > > > > > + > > > > > +centos-stream-8-tests: > > > > > + extends: .tests > > > > > + needs: > > > > > +- libvirt-perl-bindings > > > > > +- pipeline: $PARENT_PIPELINE_ID > > > > > + job: x86_64-centos-stream-8 > > > > > +- project: eskultety/libvirt-perl > > > > > + job: x86_64-centos-stream-8 > > > > > + ref: multi-project-ci > > > > > + artifacts: true > > > > > > > > IIUC from the documentation and from reading around, using > > > > strategy:depend will cause the local job to reflect the status of the > > > > triggered pipeline. So far so good. > > > > > > > > What I am unclear about is, is there any guarantee that using > > > > artifact:true with a job from an external project's pipeline will > > > > expose the artifacts from the job that was executed as part of the > > > > specific pipeline that we've just triggered ourselves as opposed to > > > > some other pipeline that might have already been completed in the > > > > past of might have completed in the meantime? > > > > > > Not just by using artifact:true or strategy:depend. The important bit is > > > having > > > 'libvirt-perl-bindings' in the job's needs list. Let me explain, if you > > > don't > > > put the bindings trigger job to the requirements list of another job > > > (centos-stream-8-tests in this case) what will happen is that the trigger > > > job > > > will be waiting for the external pipeline to finish, but > > > centos-stream-8-tests > > > job would execute basically as soon as the container project builds are > > > finished because artifacts:true would download the latest RPM artifacts > > > from an > > > earlier build... > > > > No, I got that part. My question was whether > > > > other-project-pipeline: > > trigger: > > project: other-project > > strategy: depend > > > > our-job: > > needs: > > - other-project-pipeline > > - project: other-project > > job: other-project-job > > artifacts: true > > > > actually guarantees that the instance of other-project-job whose > > artifacts are available to our-job is the same one that was started > > as part of the pipeline triggered by the other-project-pipeline job. > > Sorry for a delayed response. > > I don't think so. We can basically only rely on a fact that the jobs would > actually be queued in order they arrive which means that jobs submitted > earlier > should finish earlier, but that of course is only a premise not a guarantee. > > On the other hand I never intended to run the integration CI on every single > push to the master branch, instead, I wanted to make this a scheduled pipeline > which would effectively alleviate the problem, because with scheduled > pipelines > there would very likely not be a concurrent pipeline running in libvirt-perl > which would make us download artifacts from a pipeline we didn't trigger > ourselves. Ultimately when we switch to using merge requests, the integration tests should be run as a gating job, triggered from the merge train when the code gets applied to git, so that we prevent regressions actually making it into git master at all. Post-merge integration testing always exhibits the problem that people will consider it somebody else's problem to fix the regression. So effectively whoever creates the integration testing system ends up burdened with the job of investigating failures and finding someone to poke to fix it. With it run pre-merge then whoever wants to get their code merged needs to investigate the problems. Now sometimes the problems with of course be with the integration test system itself, not the submitters code, but this is OK because it leads to situation where the job of maintaining the integration tests are more equitably spread across all involved and builds a mindset that functional / integration testing is a critical part of delivering code, which is something we've lacked for too long in libvirt. > > > > Taking a step back, why exactly are we triggering a rebuild of > > > > libvirt-perl in the first place? Once we change that project's > > > > pipeline so that RPMs are published as artifacts, can't we just grab > > > > the ones from the latest successful pipeline? Maybe you've already > > > > explained why you did things this way and I just missed it! > > > > > > ...which brings us here. Well, I adopted the mantra that all > > > libvirt-friends > > > projects depend on libvirt and given that we need libvirt-perl bindings > > > to test > > > upstream, I'd like to always have the latest bindings available to test > > > with > > > the current upstream build. The other reason why I did the way you > > > commented on > > > is that
Re: [libvirt PATCH] qemu: support multiqueue for vdpa net device
On Tue, Mar 01, 2022 at 05:21:37PM -0600, Jonathon Jongsma wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2024406
Signed-off-by: Jonathon Jongsma
---
src/conf/domain_conf.c| 11 ++
src/qemu/qemu_domain.c| 3 +-
.../net-vdpa-multiqueue.x86_64-latest.args| 36 +++
.../qemuxml2argvdata/net-vdpa-multiqueue.xml | 29 +++
tests/qemuxml2argvtest.c | 1 +
.../net-vdpa-multiqueue.xml | 36 +++
tests/qemuxml2xmltest.c | 1 +
7 files changed, 116 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/net-vdpa-multiqueue.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/net-vdpa-multiqueue.xml
create mode 100644 tests/qemuxml2xmloutdata/net-vdpa-multiqueue.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 34fec887a3..87117a2ddb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10552,6 +10552,17 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
goto error;
}
def->data.vdpa.devicepath = g_steal_pointer();
+
+if (!def->model)
+def->model = VIR_DOMAIN_NET_MODEL_VIRTIO;
+
+if (!virDomainNetIsVirtioModel(def)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Wrong 'type' attribute specified "
+ "with . "
+ "vdpa requires the virtio-net* frontend"));
+goto error;
+}
This looks like it belongs to the verification phase as well so that it
does not reject already-existing domain XMLs.
break;
case VIR_DOMAIN_NET_TYPE_BRIDGE:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index acc76c1cd6..6b61fefb8f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4523,7 +4523,8 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
- actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) {
+ actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER ||
+ actualType == VIR_DOMAIN_NET_TYPE_VDPA)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("interface %s - multiqueue is not supported for network
interfaces of type %s"),
macstr, virDomainNetTypeToString(actualType));
Do I get that we cannot support it for but we do for
? Just asking if I understand it correctly, no need to
change that.
diff --git a/tests/qemuxml2argvdata/net-vdpa-multiqueue.x86_64-latest.args
b/tests/qemuxml2argvdata/net-vdpa-multiqueue.x86_64-latest.args
new file mode 100644
index 00..26ef666036
--- /dev/null
+++ b/tests/qemuxml2argvdata/net-vdpa-multiqueue.x86_64-latest.args
@@ -0,0 +1,36 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}'
\
+-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-accel tcg \
+-cpu qemu64 \
+-m 214 \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot strict=on \
+-device
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
+-add-fd set=0,fd=1732,opaque=net0-vdpa \
+-netdev vhost-vdpa,vhostdev=/dev/fdset/0,id=hostnet0 \
+-device
'{"driver":"virtio-net-pci","mq":true,"vectors":6,"netdev":"hostnet0","id":"net0","mac":"52:54:00:95:db:c0","bus":"pci.0","addr":"0x2"}'
\
+-audiodev '{"id":"audio1","driver":"none"}' \
+-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/net-vdpa-multiqueue.xml
b/tests/qemuxml2argvdata/net-vdpa-multiqueue.xml
new file mode 100644
index 00..2e38c6f976
--- /dev/null
+++ b/tests/qemuxml2argvdata/net-vdpa-multiqueue.xml
@@ -0,0 +1,29 @@
+
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 219136
+ 219136
+ 1
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-x86_64
+
+
+
+
+
+
+
+
+
+
+
+
+
diff
Re: [PATCH] qemu: segmentation fault in virtqemud executing qemuDomainUndefineFlags
On 3/2/22 00:21, Jim Fehlig wrote: > On 3/1/22 10:47, Boris Fiuczynski wrote: >> Commit 5adfb3472342741c443ac91dee0abb18b5a3d038 causes a segmentation >> fault. >> >> Stack trace of thread 664419: >> #0 0x03ff62ec553c in qemuDomainUndefineFlags >> (dom=0x3ff6c002810, flags=) at >> ../src/qemu/qemu_driver.c:6618 >> #1 0x03ff876a7e5c in virDomainUndefineFlags >> (domain=domain@entry=0x3ff6c002810, flags=) at >> ../src/libvirt-domain.c:6519 >> #2 0x02aa2b64a808 in remoteDispatchDomainUndefineFlags >> (server=0x2aa2c3d7880, msg=0x2aa2c3d2770, args=, >> rerr=0x3ff8287b950, client=) >> at src/remote/remote_daemon_dispatch_stubs.h:13080 >> #3 remoteDispatchDomainUndefineFlagsHelper (server=0x2aa2c3d7880, >> client=, msg=0x2aa2c3d2770, rerr=0x3ff8287b950, >> args=, ret=0x0) >> at src/remote/remote_daemon_dispatch_stubs.h:13059 >> #4 0x03ff8758bbf4 in virNetServerProgramDispatchCall >> (msg=0x2aa2c3d2770, client=0x2aa2c3e3050, server=0x2aa2c3d7880, >> prog=0x2aa2c3d8010) >> at ../src/rpc/virnetserverprogram.c:428 >> #5 virNetServerProgramDispatch (prog=0x2aa2c3d8010, >> server=server@entry=0x2aa2c3d7880, client=0x2aa2c3e3050, >> msg=0x2aa2c3d2770) at ../src/rpc/virnetserverprogram.c:302 >> #6 0x03ff8758c260 in virNetServerProcessMsg (msg=> out>, prog=, client=, srv=0x2aa2c3d7880) >> at ../src/rpc/virnetserver.c:140 >> #7 virNetServerHandleJob (jobOpaque=0x2aa2c3e2d30, >> opaque=0x2aa2c3d7880) at ../src/rpc/virnetserver.c:160 >> #8 0x03ff874c49aa in virThreadPoolWorker (opaque=> out>) at ../src/util/virthreadpool.c:164 >> #9 0x03ff874c3f62 in virThreadHelper (data=) at >> ../src/util/virthread.c:256 >> #10 0x03ff86c1cf8c in start_thread () from /lib64/libc.so.6 >> #11 0x03ff86c9650e in thread_start () from /lib64/libc.so.6 > > libvirt-tck encountered the same segfault in my test environment. > >> Signed-off-by: Boris Fiuczynski >> --- >> src/qemu/qemu_driver.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Reviewed-by: Jim Fehlig Pushed now. I believe, this warrants maintenance release. Jirka? Michal
Re: [PATCH] cmdQemuMonitorCommandQMPWrap: Reset ignored errors from JSON parsing
On Tue, Mar 01, 2022 at 02:33:28PM +0100, Peter Krempa wrote:
'cmdQemuMonitorCommandQMPWrap' is checking whether the user provided
string is not valid JSON to avoid wrapping it. In cases where it's not
JSON we ignore the error and add the wrapper.
If the caller then reports a different non-libvirt error the error from
the JSON parsing would be printed as well. Reset errors we ignore:
# virsh qemu-monitor-command cd --pass-fds a asdf
error: Unable to parse FD number 'a'
error: internal error: cannot parse json asdf: lexical error: invalid char in
json text.
asdf
(right here) --^
In the above case 'asdf' is not valid JSON, but the code did wrap it
into '{"execute":"asdf"}', the only problem is the argument for
--pass-fds.
Signed-off-by: Peter Krempa
---
tools/virsh-domain.c | 4
1 file changed, 4 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 9b1b14cdc2..743660e794 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9769,6 +9769,8 @@ cmdQemuMonitorCommandQMPWrap(vshControl *ctl,
if (virJSONValueIsObject(fullcmdjson))
return g_steal_pointer();
+vshResetLibvirtError();
+
/* we try to wrap the command and possible arguments into a JSON object, if
* we as fall back we pass through what we've got from the user */
@@ -9787,6 +9789,8 @@ cmdQemuMonitorCommandQMPWrap(vshControl *ctl,
* JSON object wrapper and try using that */
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+vshResetLibvirtError();
+
This is a little bit out of place and could be seen as confusing since
it is here just because the error could be set in a function that's in
an if condition. I, for one, would vote for extracting that and making
it more clear why that is, but this fixes the immediate issue, so
Reviewed-by: Martin Kletzander
virBufferAddLit(, "{");
/* opt points to the _ARGV option bit containing the command so we'll
* iterate through the arguments now */
--
2.35.1
signature.asc
Description: PGP signature
Re: [PATCH] libxl: Turn on user aliases
On Tue, Mar 01, 2022 at 09:40:30 -0700, Jim Fehlig wrote: > On 2/17/22 04:56, Michal Privoznik wrote: > > When I implemented user aliases I've invented this > > virDomainDefFeatures flag so that individual drivers can signal > > support for user provided aliases. The reasoning was that a > > device alias might be part of guest ABI, or used in a different > > way then in QEMU. Well, neither applies to the libxl driver, so > > it's safe to allow user aliases there. > > I suppose it is safe, but does it make sense since aliases are not used by > the driver in any way, and not supported by libxl? In that case at least the libvirt side user-visible aspect of it does make sense. Users can pick a stable alias for their device if they care about that for any reason. In the qemu impl we decided to propagate it to the hypervisor, but that should be considered an implementation detail as the IDs of objects (mostly) don't have guest visible meaning.
[libvirt PATCH][merged][trivial] Fix typo in NEWS
Signed-off-by: Tim Wiederhake --- NEWS.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.rst b/NEWS.rst index 14c4aaf185..a5b6106bc2 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -77,7 +77,7 @@ v8.1.0 (2022-03-01) * Remove unix sockets from filesystem when disabling a '.socket' systemd unit The presence of the socket files is used by our remote driver to determine -which service to access. Since neiter systemd nor the daemons clean up the +which service to access. Since neither systemd nor the daemons clean up the socket file clients were running into problems when a modular deployment was switched to monolithic ``libvirtd``. -- 2.31.1
Re: [PATCH 01/17] ci: Drop Ubuntu 1804
On Tue, Mar 01, 2022 at 17:46:44 +0100, Erik Skultety wrote: > On Tue, Feb 15, 2022 at 02:47:44PM +0100, Peter Krempa wrote: > > As of April 23 2022, Ubuntu 20.04 will be out for two years, which per > > our platform support policy means we no longer have to support > > Ubuntu 18.04. > > Would you mind contributing the patch to libvirt-ci and regenerating the > gitlab.yml config with lcitool from manifest when the time comes? :) So ... is libvirt-ci always fully mirroring what libvirt does? AFAIU lcitool is used at least within the qemu project and I didn't really check to see whether qemu will continue caring about Ubuntu 18.04 and the READMEs in libvirt-ci aren't clearing up the expectations either.
