Re: [libvirt] [PATCH] qemu: Introduce state_lock_timeout to qemu.conf
Yi Wang [2018-09-13, 10:39AM +0800]: > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 886e3fb..306772a 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -6652,9 +6652,6 @@ qemuDomainObjCanSetJob(qemuDomainObjPrivatePtr priv, > priv->job.agentActive == QEMU_AGENT_JOB_NONE)); > } > > -/* Give up waiting for mutex after 30 seconds */ > -#define QEMU_JOB_WAIT_TIME (1000ull * 30) > - > /** > * qemuDomainObjBeginJobInternal: > * @driver: qemu driver > @@ -6714,7 +6711,9 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, > } > > priv->jobs_queued++; > -then = now + QEMU_JOB_WAIT_TIME; > + > +cfg->stateLockTimeout *= 1000; This doesn't look right. Each time qemuDomainObjBeginJobInternal is called, the global config value gets multiplied. > +then = now + cfg->stateLockTimeout; Why not just then = now + cfg->stateLockTimeout * 1000; ? -- IBM Systems Linux on Z & Virtualization Development IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] tools: fix incorrect indentation or empty for first line in function body
On 2018-09-12 at 19:59, Ján Tomko wrote: >This commit only fixes empty lines, why mention incorrect indentation? > >Jano Oh, sorry. I would change this commit message. Shi Lei > >On Wed, Sep 12, 2018 at 11:58:23AM +0800, Shi Lei wrote: >>Signed-off-by: Shi Lei >>--- >> tools/virsh-volume.c | 1 - >> tools/virt-admin.c | 1 - >> 2 files changed, 2 deletions(-) >> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] src: fix incorrect indentation or empty for first line in function body
On 2018-09-12 at 20:02, Ján Tomko wrote: >It would be nice not to mix line removals with re-indentation. > >Jano > It would divide into two patches in v2 series. Shi Lei -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] cfg.mk: Introduce syntax-check rule for incorrect indentation or empty for first line in function body
On 2018-09-12 at 19:58, Ján Tomko wrote:
>On Wed, Sep 12, 2018 at 11:58:20AM +0800, Shi Lei wrote:
>>Signed-off-by: Shi Lei
>>---
>> cfg.mk | 11 +++
>> 1 file changed, 11 insertions(+)
>>
>>diff --git a/cfg.mk b/cfg.mk
>>index 609ae86..a43cb1c 100644
>>--- a/cfg.mk
>>+++ b/cfg.mk
>>@@ -702,6 +702,17 @@ sc_preprocessor_indentation:
>> echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
>> fi
>>
>>+# Use 4 spaces rather than TAB for indentation for those top-level
>>+# function bodies. Doesn't catch all cases, but it helps.
>>+sc_indentation_for_function_body:
>>+ @for f in $$($(VC_LIST_EXCEPT) | grep -E '\.[ch](\.in)?$$'); do \
>
>Doing it this way is very inefficient. This spawns 5 greps and one sed
>for each .c file. It might be bearable for spec files, when we only have
>two, but we have nearly a thousand .c and .h files
>
>I spent quite some time on making syntax-check as fast as possible, to
>encourage people to run it more often.
>
>Adding this to build-aux/check-spacing.pl would let you take advantage
>of the existing code for opening the files and get rid of all the line
>number regexes, making the check more readable.
>
>Also, it would let you split the error message and only output the
>relevant one. Grouping indentation and empty lines in one error message
>is not very friendly.
Okay, I see.
>
>>+ grep -n -A1 '^{$$' $$f | grep '^[0-9]\{1,\}-' \
>>+ | grep -v '^[0-9]\{1,\}-[ ]\{4\}\S' |
>
>> grep -v '^[0-9]\{1,\}-\([#}/]\|.*:\)' \
>
>There is no need to exempt '/', the following is also wrong:
>src/qemu/qemu_domain_address.c:2303:/* Try to find a nice default for busNr
>for a new pci-expander-bus.
Okay.
>
>Jano
>
Thanks for your directions,
Shi Lei
>>+ | $(SED) -ne "s#\(^[0-9]\{1,\}\)-#$$f:\1:#gp" | grep . && \
>>+ { echo '$(ME): incorrect indentation or empty for first line in
>>function body' 1>&2; \
>>+ exit 1; } \
>>+ done || :
>>+
>> # Enforce similar spec file indentation style, by running cppi on a
>> # (comment-only) C file that mirrors the same layout as the spec file.
>> sc_spec_indentation:
>>--
>>2.17.1
>>
>>
>>--
>>libvir-list mailing list
>>libvir-list@redhat.com
>>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Introduce state_lock_timeout to qemu.conf
When doing some job holding state lock for a long time,
we may come across error:
"Timed out during operation: cannot acquire state change lock"
Well, sometimes it's not a problem and users want to continue
to wait, and this patch allow users decide how long time they
can wait the state lock.
Signed-off-by: Yi Wang
Reviewed-by: Xi Xu
---
changes in v6:
- modify the description in qemu.conf
- move the multiplication to BeginJobInternal
changes in v5:
- adjust position of state lock in aug file
- fix state lock time got from conf file from milliseconds to
seconds
changes in v4:
- fix syntax-check error
changes in v3:
- add user-friendly description and nb of state lock
- check validity of stateLockTimeout
changes in v2:
- change default value to 30 in qemu.conf
- set the default value in virQEMUDriverConfigNew()
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 7 +++
src/qemu/qemu_conf.c | 14 ++
src/qemu/qemu_conf.h | 2 ++
src/qemu/qemu_domain.c | 7 +++
src/qemu/test_libvirtd_qemu.aug.in | 1 +
6 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index ddc4bbf..a5601e1 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -100,6 +100,7 @@ module Libvirtd_qemu =
| str_entry "lock_manager"
let rpc_entry = int_entry "max_queued"
+ | int_entry "state_lock_timeout"
| int_entry "keepalive_interval"
| int_entry "keepalive_count"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index cd57b3c..f5e34f1 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -667,6 +667,13 @@
#
#max_queued = 0
+
+# It is strongly recommended to not touch this setting
+#
+# Default is 30
+#
+#state_lock_timeout = 60
+
###
# Keepalive protocol:
# This allows qemu driver to detect broken connections to remote
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a4f545e..5be37dc 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -129,6 +129,9 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def)
#endif
+/* Give up waiting for mutex after 30 seconds */
+#define QEMU_JOB_WAIT_TIME (30)
+
virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
{
virQEMUDriverConfigPtr cfg;
@@ -346,6 +349,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
privileged)
cfg->glusterDebugLevel = 4;
cfg->stdioLogD = true;
+cfg->stateLockTimeout = QEMU_JOB_WAIT_TIME;
+
if (!(cfg->namespaces = virBitmapNew(QEMU_DOMAIN_NS_LAST)))
goto error;
@@ -863,6 +868,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (virConfGetValueUInt(conf, "keepalive_count", >keepAliveCount) < 0)
goto cleanup;
+if (virConfGetValueInt(conf, "state_lock_timeout", >stateLockTimeout)
< 0)
+goto cleanup;
+
if (virConfGetValueInt(conf, "seccomp_sandbox", >seccompSandbox) < 0)
goto cleanup;
@@ -1055,6 +1063,12 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg)
return -1;
}
+if (cfg->stateLockTimeout <= 0) {
+virReportError(VIR_ERR_CONF_SYNTAX, "%s",
+ _("state_lock_timeout must be larger than zero"));
+return -1;
+}
+
return 0;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index a8d84ef..97cf2e1 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -190,6 +190,8 @@ struct _virQEMUDriverConfig {
int keepAliveInterval;
unsigned int keepAliveCount;
+int stateLockTimeout;
+
int seccompSandbox;
char *migrateHost;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 886e3fb..306772a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6652,9 +6652,6 @@ qemuDomainObjCanSetJob(qemuDomainObjPrivatePtr priv,
priv->job.agentActive == QEMU_AGENT_JOB_NONE));
}
-/* Give up waiting for mutex after 30 seconds */
-#define QEMU_JOB_WAIT_TIME (1000ull * 30)
-
/**
* qemuDomainObjBeginJobInternal:
* @driver: qemu driver
@@ -6714,7 +6711,9 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
}
priv->jobs_queued++;
-then = now + QEMU_JOB_WAIT_TIME;
+
+cfg->stateLockTimeout *= 1000;
+then = now + cfg->stateLockTimeout;
retry:
if ((!async && job != QEMU_JOB_DESTROY) &&
diff --git a/src/qemu/test_libvirtd_qemu.aug.in
b/src/qemu/test_libvirtd_qemu.aug.in
index f1e8806..8e072d0 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -82,6 +82,7 @@ module Test_libvirtd_qemu =
{ "relaxed_acs_check" = "1" }
{ "lock_manager" = "lockd" }
{ "max_queued" = "0" }
+{ "state_lock_timeout" = "60" }
{ "keepalive_interval" = "5" }
{ "keepalive_count" = "5" }
{ "seccomp_sandbox" =
Re: [libvirt] [PATCH v5] qemu: Introduce state_lock_timeout toqemu.conf
Thank you both for your patience and discussion, John and Michal.
I will send a new version to fix the issues referred in the discussion.
> On 09/10/2018 10:22 PM, John Ferlan wrote:
> >
> >
> > On 09/05/2018 11:09 PM, Yi Wang wrote:
> >> When doing some job holding state lock for a long time,
> >> we may come across error:
> >
> > blank line
> >
> >> "Timed out during operation: cannot acquire state change lock"
> >
> > blank line
> >
> >> Well, sometimes it's not a problem and users want to continue
> >> to wait, and this patch allow users decide how long time they
> >> can wait the state lock.
> >
> > As I noted in a previous review... With this patch it's not the
> > individual user or command that can decide how long to wait, rather it's
> > set by qemu.conf policy. The duration of the wait just becomes
> > customize-able. Not that it's any different the current implementation,
> > since the value is set in qemuDomainObjBeginJobInternal that means
> > there's no distinguishing between job, asyncjob, and agentjob. There's
> > also no attempt to allow changing the value via virt-admin.
>
> I'm not sure how we would implement per API timeout. We could perhaps do
> per connection but that's also not desired.
>
> And regarding "how does one know how long to wait" - well, they don't.
> That's the thing with timeouts. It's not an argument against them
> though. Even primitive functions (e.g. those from pthread) have
> XXX_timed() variants.
>
> >
> > A "strong recommendation" of longer than 30 seconds doesn't provide much
> > guidance. The setting of value generally depends on the configuration
> > and what causes an API to hold the lock for longer than 30 seconds.
> > That's probably limited to primarily virDomainListGetStats and/or
> > virConnectGetAllDomainStats. There's other factors in play including CPU
> > speed, CPU quota allowed, network throughput/latency, disk speed, etc.
> > Tough to create a value the works for every command/purpose.
>
> Agreed, "strong recommendation" is use case dependant (although it's
> perhaps what initiated this patch). If anything, we should discourage
> people from touching this knob.
>
> >
> > Ironically (perhaps) allowing a value of 0 would essentially make it so
> > no one waited - such as how qemuDomainObjBeginJobNowait operates
> > essentially. OTOH, in order to have everything wait "as long as
> > possible" passing -1 would be a lot easier than providing 2147483647 (or
> > if properly typed to uint, then 4294967295U).
>
> Passing 0 is explicitly forbidden (although in a bit clumsy way - we
> need to check the value right after it's parsed because multiplying it
> by 1000 can lead to overflow and a negative value becomes positive or
> vice versa).
[..]
> > If this were an unsigned int, then the < 0 check wouldn't necessary.
> >
> > So then the question becomes, should 0 (zero) be special cased as wait
> > forever.
>
> This check needs to be done before multiplication otherwise an overflow
> might occur. Also, the multiplication can be done in BeginJobInternal so
> that cfg->stateLockTimeout holds value entered by user.
>
> >
> >> +virReportError(VIR_ERR_CONF_SYNTAX, "%s",
> >> + _("state_lock_timeout should larger than zero"));
> >
> > If 0 were left as invalid, then s/should/must be/
> >
> >> +return -1;
> >> +}
> >> +
> >> return 0;
> >> }
> >>
> Michal
---
Best wishes
Yi Wang--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Re：Re: [PATCH] add nodeset='all' and default for interleavemode
>On 09/11/2018 04:28 PM, Peng Hao wrote:
>> For interleave mode,sometimes we want to allocate mmeory regularly
>> in all nodes on the host. But different hosts has different node number.
>> So we add nodeset='all' for interleave mode and if nodeset=NULL default
>> nodeset is 'all' for interleave mode.
>>
>> Signed-off-by: Peng Hao
>> ---
>> src/conf/numa_conf.c | 73
>>
>> 1 file changed, 57 insertions(+), 16 deletions(-)
>
>Firstly, this patch does not pass 'syntax-check'. Secondly, it breaks
>qemuxml2argvtest.
>
I will pay attention to this.
>> +numa->memory.allnode = true;
>> +} else {
>Any patch that changes accepted XML needs to go hand in hand with
>documentation and RNG update and a test case.
I will add next.
>> +if (placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC &&
>> +mode == VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE &&
>> +numa->memory.allnode) {
>> +if ((bitmap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN)) == NULL)
>> +goto cleanup;
>> +virBitmapClearAll(bitmap);
>> +maxnode = numa_max_node();
>So, you're including numa.h to get this function. What if:
>a) numa.h is not available?
>b) what is wrong with virNumaGetMaxNode()?
>> +for (i = 0; i <= maxnode; i++) {
>> +if (virBitmapSetBit(bitmap, i) < 0) {
>> +virBitmapFree(bitmap);
>> +goto cleanup;
>> +}
>> +}
>> +if (numa->memory.nodeset)
>> +virBitmapFree(numa->memory.nodeset);
>> +numa->memory.nodeset = bitmap;
>> }
>>
>> /* setting nodeset when placement auto is invalid */
>>
>But more importantly, why is this patch needed? I might be missing
>something, but:
>a) you can just not pin the memory to avoid mismatch of NUMA nodes on
>migration,
>b) supply new domain XML on migration where NUMA nodes match the destination
>Isn't pinning memory to all NUMA nodes equivalent to no pinning at all?
I would use 'interlaeve' to let virtual machine's memory distribute evenly in
all nodes. And
'interleave' setting ask for providing 'nodeset'. I think it is not so
convenient.
>Michal--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 0/5] Introduce VIR_AUTOCLOSE macro for closing fd automatically
On 2018-09-12 at 23:38, Michal Privoznik wrote: >On 09/12/2018 11:46 AM, Shi Lei wrote: >> v1 here: >> https://www.redhat.com/archives/libvir-list/2018-September/msg00319.html >> >> Diff from v1: >> - Change VIR_AUTOCLOSE macro (comments from Michal) >> - Remove others except for patches in util (comments from Erik) >> - Change sc_require_attribute_cleanup_initialization for this macro >> >> Shi Lei (5): >> util: file: introduce VIR_AUTOCLOSE macro to close fd of the file >> automatically >> cfg.mk: Change syntax-check rule for VIR_AUTOCLOSE variable >> initialization >> util: file: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE >> util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE >> util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE >> >> cfg.mk | 2 +- >> src/util/virfile.c | 21 ++-- >> src/util/virfile.h | 18 ++- >> src/util/virnetdev.c | 251 + >> src/util/virnetdevbridge.c | 120 ++ >> 5 files changed, 146 insertions(+), 266 deletions(-) >> > >ACKed and pushed. > >Michal Thanks, Shi Lei -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] qemu: Drop some cruft
On 09/12/2018 06:24 PM, Ján Tomko wrote: > On Wed, Sep 12, 2018 at 09:45:32AM -0400, John Ferlan wrote: >> >> >> On 09/12/2018 09:35 AM, Andrea Bolognani wrote: >>> On Wed, 2018-09-12 at 09:09 -0400, John Ferlan wrote: Any chance this can wait? Would be nice to have other series posted upstream that have changes to .xml and .replies files to add new functionality be processed first. There's a series to use - memfd from Marc-Andre Laurent that gets impacted. > > * Lureau > right, my fingers not typing what I'm thinking, mea culpa. >>> >>> I see you've already had to rebase locally to apply the patches at >>> all, due to other changes being pushed in the meantime, so I don't >>> see how pushing this series too would make it any worse. >>> >>> Also IIUC you've asked Marc-André to make some changes that depend >>> on *another series* of yours, that still hasn't been pushed, which >>> means a respin will be necessary either way, won't it? >>> >>> All in all I see no reason to dealy pushing this. >>> >> >> OK - go ahead. It was just a "would be nice" type inquiry. > > I don't see how that negates the need to post another version, > because the intermediate diffs got too hard to follow. > I don't see them as too hard to follow. In fact they're pretty simple as long as you have seen the code. I saw no need to request a repost unless it was because of the .replies differences. The hardest part is the move of the code from domain_conf to qemu_domain and since I requested and made those changes, I can see 'reason' to assist with adjusting the patches I was reviewing. Managing the capabilities conflict was less trivial and I certainly don't remember all the names of the various tools that were created that are supposed to help in the endeavor. >> It's not the >> first time capability changes cause conflicts and it won't be the last >> unless we come up with a better process for them. > > What's wrong with the current process? 1. It's not orderly. Review of upstream series is "ad-hoc" at best. Some series get immediate review, ack, and push. Sometimes certain series languish for no apparent reason. When those series contain capabilities changes it can quickly creates conflict. 2. At certain times there seems to be a "run" or "rush" to make capabilities changes based on priorities perhaps not related to upstream that create conflicts for patches that are languishing. 3. I think if we agree something is "good to add" capability wise and even though all the code isn't ready, the capability changes could go forward while perhaps the resolution of all review comments is handled. Yes, it's more of a problem when .replies changes. > The capabilities conflicts are trivial to resolve and we have automated > tools for most of it (VIR_TEST_REGENRATE_OUTPUT, group-qemu-caps.pl, > qemucapsfixreplies) > OK sometimes trivial to resolve... If one can keep up to date with changes or 'git am' doesn't have conflicts trying to apply changes. One has to go back in time with git checkout to a series before the update that caused the conflict, git apply patches, then --set-upstream-to=origin/master, and then git pull --rebase. And then you get to try to use any of the tools to help resolve conflicts - if you know about them and if they work; otherwise, it's a hand-edit or request to repost. Unless you know of some other neat trick or two... How would qemucapsfixreplies fix the environment where git am fails to do the merge. I didn't recall the name when processing the .replies files. I'm also not sure having taken a quick peak at the code whether it would have helped since there was "conflict" as a result of 4 replies being removed and 1 reply being added in a very similar spot in the file. As noted in the review - whenever I've created .replies files from scratch using the tools - something in my process doesn't create output in the exact format some seem to expect, so I've given up creating them and just wait for someone else to do it. > Also, Andrea's series seems to be innocent in this - the removed > capabilities do not alter .replies and the .[ch] file changes are > contained to the middle of the capabilities array, so they should > be resolved trivially by git. > True... These were, but considering they were reviewed 16 minutes after posting and the previous series was reviewed similarly as quick and those caused merge conflict issues - I reacted to just the fact that capabilities changes were taking place and asked to consider the "orderly processing" of posted patches. I didn't dig into them because I was in the middle of something else. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 4/5] util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE
On 2018-09-12 at 23:38, Michal Privoznik wrote:
>On 09/12/2018 11:46 AM, Shi Lei wrote:
>> Signed-off-by: Shi Lei
>> ---
>> src/util/virnetdevbridge.c | 120 -
>> 1 file changed, 37 insertions(+), 83 deletions(-)
>>
>> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
>> index ed2db27..e058898 100644
>> --- a/src/util/virnetdevbridge.c
>> +++ b/src/util/virnetdevbridge.c
>
>
>> @@ -723,19 +687,14 @@ int virNetDevBridgeRemovePort(const char *brname,
>> int virNetDevBridgeSetSTPDelay(const char *brname,
>> int delay)
>> {
>> - int fd = -1;
>> - int ret = -1;
>> struct ifreq ifr;
>> + VIR_AUTOCLOSE fd = -1;
>>
>> if ((fd = virNetDevSetupControl(brname, )) < 0)
>> - goto cleanup;
>> + return -1;
>>
>> - ret = virNetDevBridgeSet(brname, "forward_delay", MS_TO_JIFFIES(delay),
>> + return virNetDevBridgeSet(brname, "forward_delay", MS_TO_JIFFIES(delay),
>> fd, );
>
>Misaligned arguments.
Sorry for it!
>
>> -
>> - cleanup:
>> - VIR_FORCE_CLOSE(fd);
>> - return ret;
>> }
>>
>>
>> @@ -776,19 +735,14 @@ int virNetDevBridgeGetSTPDelay(const char *brname,
>> int virNetDevBridgeSetSTP(const char *brname,
>> bool enable)
>> {
>> - int fd = -1;
>> - int ret = -1;
>> struct ifreq ifr;
>> + VIR_AUTOCLOSE fd = -1;
>>
>> if ((fd = virNetDevSetupControl(brname, )) < 0)
>> - goto cleanup;
>> + return -1;
>>
>> - ret = virNetDevBridgeSet(brname, "stp_state", enable ? 1 : 0,
>> + return virNetDevBridgeSet(brname, "stp_state", enable ? 1 : 0,
>> fd, );
>> -
>
>And again.
Sorry. I would pay attention.
>
>Michal
Thanks,
Shi Lei
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC
On Wed, Sep 12, 2018 at 03:18:26PM +0200, Michal Privoznik wrote:
On 09/12/2018 12:46 PM, Pavel Hrdina wrote:
On Wed, Sep 12, 2018 at 10:57:32AM +0200, Fabiano Fidêncio wrote:
virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup() and
virLXCCgroupSetupCpuTune() and qemuSetupCpuCgroup() are the most similar
functions between QEMU and LXC code.
Let's move their common code to virCgroup.
Mind that the first two patches are basically preparing the ground for
the changes introduced in the last two patches.
Hi, definitely good idea to remove code duplication!
We have similar issue with the virDomainSetBlkioParameters for QEMU and
LXC drivers. The code to set cgroup values is the same.
Since the common object is domain how about introducing
virDomainSetupBlkioTune and virDomainSetupMemTune and move it into
domain_conf.c, that way we don't have to extract domain specific data
into src/util.
I'd rather remove stuff from doman_conf.c than add something there. It's
already the biggest file by far margin:
libvirt.git $ find . -type f -iname \*.c -exec du -h {} \; | sort -h
416K./src/qemu/qemu_domain.c
696K./src/qemu/qemu_driver.c
956K./src/conf/domain_conf.c
And moving code from domain_conf is something we do from time to time.
Just look at all those virnet* includes at the beginning of domain_conf.h.
Another reason for moving the code out is that blkio tune is not domain
specific. In the future, we might want to limit say iohelper and thus
move it into its specific cgroup.
I'm not that convinced about 2/4 to be honest. Memory specification is
pretty much domain centric.
Both seem to follow cgroups to a point, so they're process-centric, not
domain-centric. So if they're needed on lower (src/util) layers,
separating them makes sense.
(Not that domain_conf does not deserve to get both the parser and the
formatter to be split out at this line count)
But it follows my first point - moving code
out from domain_conf.c. And it de-duplicates the code.
Another benefit is that it will not cause me merge conflicts because I'm
rewriting cgroup code and adding support for cgroup v2.
Such downstream branches should not interfere with upstream libvirt
development. IOW: it's your opportunity as a maintainer to offer to
merge this patch on top of your changes if merging them now would cause
you too much trouble. ;)
Jano
I don't think that code movement should cause that big of a trouble.
You'll get some context conflicts but that's always the case with 30+
unmerged patches.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Add functions for checking if user or group exists
On Wed, Sep 12, 2018 at 05:09:12PM +0200, Martin Kletzander wrote:
Instead of duplicating the code from virGet{User,Group}IDByName(), which are
static anyway, extend those functions to accept NULL pointers for the result and
a boolean for controlling the error reporting.
Signed-off-by: Martin Kletzander
---
Feel free to suggest any other way of doing this, I just felt like this is the
most readable way of doing things
src/libvirt_private.syms | 2 ++
src/util/virutil.c | 40
src/util/virutil.h | 4
3 files changed, 38 insertions(+), 8 deletions(-)
+/* Silently checks if Group @name exists.
+ * Returns if the group exists and fallbacks to false on error.
+ */
+int
+virDoesGroupExist(const char *name)
util/virutil.c:1142:42: error: unused parameter 'exists'
[-Werror,-Wunused-parameter]
virDoesUserExist(const char *name, bool *exists)
^
util/virutil.c:1151:1: error: conflicting types for 'virDoesGroupExist'
virDoesGroupExist(const char *name)
+{
+return virGetGroupIDByName(name, NULL, true) == 0;
+}
+
/* Compute the list of primary and supplementary groups associated
* with @uid, and including @gid in the list (unless it is -1),
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Remove some unnecessary warnings
On Wed, Sep 12, 2018 at 05:09:11PM +0200, Martin Kletzander wrote: cd . ./blurb Martin Kletzander (2): Add functions for checking if user or group exists qemu: Report less errors on driver startup src/libvirt_private.syms | 2 ++ src/qemu/qemu_conf.c | 6 -- src/util/virutil.c | 40 src/util/virutil.h | 4 4 files changed, 42 insertions(+), 10 deletions(-) Thanks-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] tests: add qemuxml2argv memfd-memory-numa test
[...] >> >> So all that's "left": >> >> 1. "Add" a check in qemuDomainABIStabilityCheck to ensure we're not >> changing from memory-backend-ram to memory-backend-memfd. We already >> check that "(src->mem.source != dst->mem.source)" - so we know we're >> already anonymous or not. >> >> Any suggestions? If source is anonymous, then what? I think we can use >> the qemuDomainObjPrivatePtr in some way to determine that we were >> started with -memfd (or not started that way). > > No idea how we could save that information across various restarts / > version changes. I think it'd be ugly... I think migration cookies would have to be used... I considered other mechanisms, but each wouldn't quite work. Without writing the code, if we cared to do this, then we'd have: 1. Add a field to qemuDomainObjPrivatePtr that indicates what got started (none, memfd, file, or ram). Add a typedef enum that has unknown, none, memfd, file, and ram. Add the Parse/Format code to handle the field. 2. Modify the qemu_command code to set the field in priv based on what got started, if something got started. The value would be > 0... 3. Mess with the migration cookie logic to add checks for what the source started. On the destination side of that cookie if we had the "right capabilities", then check the source cookie to see what it has. If it didn't have that field, then I think one could assume the source with anonymous memory backing would be using -ram. We'd already fail the src/dst mem.source check if one used -file. I'm not all the versed in the cookies, but I think that'd work "logically thinking" at least. The devil would be in the details. Assuming your 3.1 patches do something to handle the condition, I guess it comes does to how much of a problem it's believed this could be in 2.12 and 3.0 if someone is running -ram and migrates to a host that would default to -memfd. > > Tbh, I would try to migrate, and let qemu fail if something is > incompatible (such as incompatible memory backends or memory region > name mismatch). See also my qemu series "[PATCH 0/9] hostmem-ram: use > whole path for region name with >= 3.1". It feels like libvirt > duplicates some qemu logic/error otherwise. > I'm sure there's lots of duplication, but generally doing the checks in libvirt allow for a bit "easier" (in least in terms of libvirt) backout logic. Once the qemu process starts - if the process eventually dies because of something, then the logging only goes to libvirt log files. If the process fails to start, libvirt does capture and give that information back to the consumer. So call it preventative duplication. I think historically some qemu error messages have been a bit too vague to figure out why something didn't work. >> >> 2. Get the patches I posted today to cleanup/move the memory backing >> checks from domain_conf to qemu_domain: >> >>https://www.redhat.com/archives/libvir-list/2018-September/msg00463.html >> >> reviewed and pushed so that patch4 can use the qemu_domain API to alter >> it's hugepages check. > > done > Thanks - I pushed that... > feel free to update & resend my series, or else I will rebase and resend it > > thanks > OK - I adjusted your changes to handle the previously agreed upon "issues" and was ready to push the series when it dawned on me that the MEMFD and MEMFD_HUGETLB capabilities both use the 2.12 release - so realistically would the latter really be necessary? Again if something doesn't quite work in 2.12 and 3.0 for hugetlb, then perhaps there's something in 3.1 that can be checked. I can remove or keep patch 2. If removed, then just use MEMFD as the basis. Your call. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] qemu: Drop some cruft
On Wed, Sep 12, 2018 at 09:45:32AM -0400, John Ferlan wrote: On 09/12/2018 09:35 AM, Andrea Bolognani wrote: On Wed, 2018-09-12 at 09:09 -0400, John Ferlan wrote: Any chance this can wait? Would be nice to have other series posted upstream that have changes to .xml and .replies files to add new functionality be processed first. There's a series to use - memfd from Marc-Andre Laurent that gets impacted. * Lureau I see you've already had to rebase locally to apply the patches at all, due to other changes being pushed in the meantime, so I don't see how pushing this series too would make it any worse. Also IIUC you've asked Marc-André to make some changes that depend on *another series* of yours, that still hasn't been pushed, which means a respin will be necessary either way, won't it? All in all I see no reason to dealy pushing this. OK - go ahead. It was just a "would be nice" type inquiry. I don't see how that negates the need to post another version, because the intermediate diffs got too hard to follow. It's not the first time capability changes cause conflicts and it won't be the last unless we come up with a better process for them. What's wrong with the current process? The capabilities conflicts are trivial to resolve and we have automated tools for most of it (VIR_TEST_REGENRATE_OUTPUT, group-qemu-caps.pl, qemucapsfixreplies) Also, Andrea's series seems to be innocent in this - the removed capabilities do not alter .replies and the .[ch] file changes are contained to the middle of the capabilities array, so they should be resolved trivially by git. Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] hw/vfio/display: add ramfb support
On Tue, 11 Sep 2018 06:38:43 +0200 Gerd Hoffmann wrote: > Hi, > > > > type_register_static(_pci_dev_info); > > > +type_register_static(_pci_ramfb_dev_info); > > > My concern here is still all of the extra tooling that needs to be > > added to management layers above QEMU for this device that exists only > > because we can't hotplug the primary display in QEMU. What happens when > > we can hotplug the primary display? > > Ramfb uses fw_cfg, and fw_cfg files can't be added or removed at > runtime, the interface simply isn't designed for that. > > > Aren't disabling hotplug of a > > vfio-pci device and supporting ramfb two separate things? I think > > we're leaking current implementation issues out to the device options > > when really we'd rather have a "ramfb" (or perhaps "console") option on > > the vfio-pci device and the hotplug capability determined automatically > > and available through introspection of the device. > > Well, I don't think libvirt will have too much trouble handling this. > We have two variants (with and without vga compatibility) of other > devices: qxl-vga and qxl, virtio-vga and virtio-gpu-pci. libvirt copes > just fine and picks the right one (I think depending on video model > 'primary' property). > > Also libvirt manages hotpluggability per device *class*, not per device > *instance*. So a device being hotpluggable or not depending on some > device property is a problem for libvirt ... > > I'm open to suggestions how to handle this better, as long as the > libvirt people are on board with the approach. Ok, so we need a new class to handle making a device non-hotpluggable, but I'm still not sure whether we should make: -device vfio-pci-ramfb or -device vfio-pci-nohotplug,ramfb=on Where ramfb would be a property only available on the nohotplug class variant. The latter seems to provide a lot more flexibility, but which is more practical for libvirt? Thanks, Alex -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 1/2] vl.c deprecate incorrect CPUs topology
On 9/12/18 11:19 AM, Igor Mammedov wrote:
-smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
so that total number of logical CPUs [sockets * cores * threads]
would be equal to [maxcpus], however historically we didn't have
such check in QEMU and it is possible to start VM with an invalid
topology.
Deprecate invalid options combination so we can make sure that
the topology VM started with is always correct in the future.
Users with an invalid sockets/cores/threads/maxcpus values should
fix their CLI to make sure that
[sockets * cores * threads] == [maxcpus]
Signed-off-by: Igor Mammedov
Reviewed-by: Andrew Jones
Reviewed-by: Eduardo Habkost
---
+++ b/qemu-deprecated.texi
@@ -139,6 +139,19 @@ The 'file' driver for drives is no longer appropriate for
character or host
devices and will only accept regular files (S_IFREG). The correct driver
for these file types is 'host_cdrom' or 'host_device' as appropriate.
+@subsection -smp (invalid topologies) (since 3.1)
+
+CPU topology properties should describe whole machine topology including
+possible CPUs. In other words: @var{maxcpus} should be equal to
+@math{@var{sockets} * @var{cores} * @var{threads}}.
This sentence...
+
+However, historically it was possible to start QEMU with an incorrect topology
+where @math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} <
@var{maxcpus}},
+which could lead to an incorrect topology enumeration by the guest.
+Support for invalid topologies will be removed, the user must ensure
+topologies described with -smp include all possible cpus, i.e.
+ @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
...and this recommendation sound repetitive. I'd be okay if you removed
the "In other words" sentence above, leaving just the intro statement
(what -cpu is supposed to do), then the second paragraph (what we also
allowed in the past, and how you should fix it before we stop allowing
that).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 2/2] vl:c: make sure that sockets are calculated correctly in '-smp X' case
On 9/12/18 11:19 AM, Igor Mammedov wrote:
commit
(5cdc9b76e3 vl.c: Remove dead assignment)
removed sockets calculation when 'sockets' weren't provided on CLI
since there wasn't any users for it back then. Exiting checks
are neither reachable
} else if (sockets * cores * threads < cpus) {
or nor triggable
s/triggable/triggerable/
if (sockets * cores * threads > max_cpus)
so we weren't noticing wrong topology since then, since users
recalculate sockets adhoc on their own.
However with deprecation check it becomes noticable, for example
-smp 2
will start printing warning:
"warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1)
!= maxcpus (2)"
calculating sockets if they weren't specified.
Fix it by returning back sockets calculation if it's omited on CLI.
s/omited/omitted/
Signed-off-by: Igor Mammedov
Reviewed-by: Andrew Jones
---
vl.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index 7fd700e..9e56696 100644
--- a/vl.c
+++ b/vl.c
@@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
/* compute missing values, prefer sockets over cores over threads */
if (cpus == 0 || sockets == 0) {
-sockets = sockets > 0 ? sockets : 1;
cores = cores > 0 ? cores : 1;
threads = threads > 0 ? threads : 1;
if (cpus == 0) {
+sockets = sockets > 0 ? sockets : 1;
cpus = cores * threads * sockets;
+} else {
+max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
+sockets = max_cpus / (cores * threads);
}
} else if (cores == 0) {
threads = threads > 0 ? threads : 1;
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v7 1/2] vl.c deprecate incorrect CPUs topology
-smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
so that total number of logical CPUs [sockets * cores * threads]
would be equal to [maxcpus], however historically we didn't have
such check in QEMU and it is possible to start VM with an invalid
topology.
Deprecate invalid options combination so we can make sure that
the topology VM started with is always correct in the future.
Users with an invalid sockets/cores/threads/maxcpus values should
fix their CLI to make sure that
[sockets * cores * threads] == [maxcpus]
Signed-off-by: Igor Mammedov
Reviewed-by: Andrew Jones
Reviewed-by: Eduardo Habkost
---
v7:
- add corrections to deprication doc formulas/math/section title
- drop Note section
(Eduardo Habkost )
v6:
- s/socket/sockets/, the same for core, thread in subsection
(Andrew Jones )
v5:
- extend deprecation doc, adding that maxcpus should be multiple of
present on CLI [sockets/cores/threads] options
(Eduardo Habkost )
v4:
- missed dot comment, fix it with s/./,/ (Andrew Jones )
v3:
- more spelling fixes (Andrew Jones )
- place deprecation check after (sockets * cores * threads > max_cpus) check
(Eduardo Habkost )
v2:
- spelling& fixes (Andrew Jones )
---
qemu-deprecated.texi | 13 +
vl.c | 7 +++
2 files changed, 20 insertions(+)
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 1b9c007..31dc1b3 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -139,6 +139,19 @@ The 'file' driver for drives is no longer appropriate for
character or host
devices and will only accept regular files (S_IFREG). The correct driver
for these file types is 'host_cdrom' or 'host_device' as appropriate.
+@subsection -smp (invalid topologies) (since 3.1)
+
+CPU topology properties should describe whole machine topology including
+possible CPUs. In other words: @var{maxcpus} should be equal to
+@math{@var{sockets} * @var{cores} * @var{threads}}.
+
+However, historically it was possible to start QEMU with an incorrect topology
+where @math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} <
@var{maxcpus}},
+which could lead to an incorrect topology enumeration by the guest.
+Support for invalid topologies will be removed, the user must ensure
+topologies described with -smp include all possible cpus, i.e.
+ @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
+
@section QEMU Machine Protocol (QMP) commands
@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
diff --git a/vl.c b/vl.c
index 5ba06ad..7fd700e 100644
--- a/vl.c
+++ b/vl.c
@@ -1246,6 +1246,13 @@ static void smp_parse(QemuOpts *opts)
exit(1);
}
+if (sockets * cores * threads != max_cpus) {
+warn_report("Invalid CPU topology deprecated: "
+"sockets (%u) * cores (%u) * threads (%u) "
+"!= maxcpus (%u)",
+sockets, cores, threads, max_cpus);
+}
+
smp_cpus = cpus;
smp_cores = cores;
smp_threads = threads;
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v7 2/2] vl:c: make sure that sockets are calculated correctly in '-smp X' case
commit
(5cdc9b76e3 vl.c: Remove dead assignment)
removed sockets calculation when 'sockets' weren't provided on CLI
since there wasn't any users for it back then. Exiting checks
are neither reachable
} else if (sockets * cores * threads < cpus) {
or nor triggable
if (sockets * cores * threads > max_cpus)
so we weren't noticing wrong topology since then, since users
recalculate sockets adhoc on their own.
However with deprecation check it becomes noticable, for example
-smp 2
will start printing warning:
"warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads
(1) != maxcpus (2)"
calculating sockets if they weren't specified.
Fix it by returning back sockets calculation if it's omited on CLI.
Signed-off-by: Igor Mammedov
Reviewed-by: Andrew Jones
---
vl.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index 7fd700e..9e56696 100644
--- a/vl.c
+++ b/vl.c
@@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
/* compute missing values, prefer sockets over cores over threads */
if (cpus == 0 || sockets == 0) {
-sockets = sockets > 0 ? sockets : 1;
cores = cores > 0 ? cores : 1;
threads = threads > 0 ? threads : 1;
if (cpus == 0) {
+sockets = sockets > 0 ? sockets : 1;
cpus = cores * threads * sockets;
+} else {
+max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
+sockets = max_cpus / (cores * threads);
}
} else if (cores == 0) {
threads = threads > 0 ? threads : 1;
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v7 0/2] deprecate incorrect CPUs topolog
Changelog since v5: * add(v6) and then remove(v7) Notes section to/from deprication doc (Eduardo Habkost ) * fix up wording and math formating in deprication doc (Eduardo Habkost ) * drop !socket check as it always evaluates to true at that point (Eduardo Habkost ) Changelog since v4: * extend deprication doc, adding that maxcpus should be multiple of present on CLI [sockets/cores/threads] options (Eduardo Habkost ) series bundles together 2 related patches posted separately earlier: vl.c deprecate incorrect CPUs topology vl:c: make sure that sockets are calculated correctly in '-smp X' case Goal is to depricate invalid topologies so we could make sure that topology configuration is correct by forbidding invalid input once deprecation period ends. --- CC: libvir-list@redhat.com CC: ehabk...@redhat.com Igor Mammedov (2): vl.c deprecate incorrect CPUs topology vl:c: make sure that sockets are calculated correctly in '-smp X' case qemu-deprecated.texi | 13 + vl.c | 12 +++- 2 files changed, 24 insertions(+), 1 deletion(-) -- 2.7.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology
On Mon, 10 Sep 2018 14:49:23 -0300
Eduardo Habkost wrote:
> On Thu, Sep 06, 2018 at 10:02:13AM +0200, Igor Mammedov wrote:
> > On Wed, 5 Sep 2018 10:45:12 -0300
> > Eduardo Habkost wrote:
> >
> > > On Wed, Sep 05, 2018 at 11:25:11AM +0200, Igor Mammedov wrote:
> > > > On Tue, 4 Sep 2018 23:12:55 -0300
> > > > Eduardo Habkost wrote:
> > > >
> > > > > On Tue, Sep 04, 2018 at 03:22:35PM +0200, Igor Mammedov wrote:
> > > > > > -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> > > > > > so that total number of logical CPUs [sockets * cores * threads]
> > > > > > would be equal to [maxcpus], however historically we didn't have
> > > > > > such check in QEMU and it is possible to start VM with an invalid
> > > > > > topology.
> > > > > > Deprecate invalid options combination so we can make sure that
> > > > > > the topology VM started with is always correct in the future.
> > > > > > Users with an invalid sockets/cores/threads/maxcpus values should
> > > > > > fix their CLI to make sure that
> > > > > >[sockets * cores * threads] == [maxcpus]
> > > > > >
> > > > > > Signed-off-by: Igor Mammedov
> > > > > > ---
> > > > > > v5:
> > > > > > - extend deprecation doc, adding that maxcpus should be multiple
> > > > > > of
> > > > > > present on CLI [sockets/cores/threads] options
> > > > > > (Eduardo Habkost )
> > > > > > v4:
> > > > > > - missed dot comment, fix it with s/./,/ (Andrew Jones
> > > > > > )
> > > > > > v3:
> > > > > > - more spelling fixes (Andrew Jones )
> > > > > > - place deprecation check after (sockets * cores * threads >
> > > > > > max_cpus) check
> > > > > > (Eduardo Habkost )
> > > > > > v2:
> > > > > > - spelling& fixes (Andrew Jones )
> > > > > > ---
> > > > > > qemu-deprecated.texi | 19 +++
> > > > > > vl.c | 7 +++
> > > > > > 2 files changed, 26 insertions(+)
> > > > > >
> > > > > > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > > > > > index 87212b6..827c3ce 100644
> > > > > > --- a/qemu-deprecated.texi
> > > > > > +++ b/qemu-deprecated.texi
> > > > > > @@ -159,6 +159,25 @@ The 'file' driver for drives is no longer
> > > > > > appropriate for character or host
> > > > > > devices and will only accept regular files (S_IFREG). The correct
> > > > > > driver
> > > > > > for these file types is 'host_cdrom' or 'host_device' as
> > > > > > appropriate.
> > > > > >
> > > > > > +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since
> > > > > > 3.1)
> > > > >
> > > > > Minor: I suggest using @var markup, or maybe just use
> > > > > "-smp (invalid topologies) (since 3.1)" as subsection title for
> > > > > simplicity?
> > > > >
> > > > > > +
> > > > > > +CPU topology properties should describe whole machine topology
> > > > > > including
> > > > > > +possible CPUs, but historically it was possible to start QEMU with
> > > > > > +an incorrect topology where
> > > > > > + sockets * cores * threads >= X && X < maxcpus
> > > > >
> > > > > Minor: this line formatting is lost on the HTML output. I
> > > > > suggest using @var and/or @math.
> > > > >
> > > > > Minor: I suggest not using C syntax.
> > > > >
> > > > > i.e. use something like:
> > > > >
> > > > > @math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} <
> > > > > @var{maxcpus}},
> > > > >
> > > > > > +which could lead to an incorrect topology enumeration by the guest.
> > > > > > +Support for invalid topologies will be removed, the user must
> > > > > > ensure
> > > > > > +topologies described with -smp include all possible cpus, i.e.
> > > > > > + sockets * cores * threads == maxcpus
> > > > >
> > > > > Minor: same as above. I suggest:
> > > > >
> > > > > @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
> > > > >
> > > > >
> > > > > > +Note: it's assumed that maxcpus value must be multiple of the
> > > > > > topology
> > > > > > +options present on command line to avoid creating an invalid
> > > > > > topology.
> > > > > > +If maxcpus isn't be multiple of present topology options then the
> > > > > > condition
> > > > > > +(sockets * cores * threads == maxcpus) can't be satisfied and it
> > > > > > will
> > > > > > +trigger deprecation warning which later will be converted to a
> > > > > > error.
> > > > > > +If you get deprecation warning it's recommended to explicitly
> > > > > > specify
> > > > > > +a correct topology to make warning go away and ensure that it will
> > > > > > +continue working in the future.
> > > > >
> > > > > I don't understand the purpose of the "Note:" section. It seems to
> > > > > duplicate
> > > > > what was already said in the lines above it. Can we just remove it?
> > > > >
> > > > Well, didn't I say that no additional explanation needed in v4?
> > > > Note section was added per your suggestion to explicitly say that
> > > > maxcpus
> > > > should be multiple of options present on
[libvirt] [PATCH 0/5] Cleanup some storage driver paths
See the patches for details - everything leads to the last one. John Ferlan (5): storage: Clean up stateFile if refreshPool fails storage: Clean up storagePoolUpdateStateCallback processing storage: Create error label path for storagePoolCreateXML storage: Introduce storagePoolRefreshFailCleanup storage: Save error during refresh failure processing src/storage/storage_driver.c | 73 1 file changed, 40 insertions(+), 33 deletions(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] storage: Clean up stateFile if refreshPool fails
If the virStoragePoolRefresh fails and we call stopPool, the
code neglected to clean up the state file leading to the next
libvirtd restart attempting to start the pool. For a transient
pool this could make it unexpectedly reappear.
Signed-off-by: John Ferlan
---
src/storage/storage_driver.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 8943df1f84..1dbeb213e3 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1174,8 +1174,13 @@ storagePoolRefresh(virStoragePoolPtr pool,
virStoragePoolObjClearVols(obj);
if (backend->refreshPool(obj) < 0) {
+char *stateFile = virFileBuildPath(driver->stateDir, def->name,
".xml");
+
+if (stateFile)
+unlink(stateFile);
if (backend->stopPool)
backend->stopPool(obj);
+VIR_FREE(stateFile);
event = virStoragePoolEventLifecycleNew(def->name,
def->uuid,
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] storage: Introduce storagePoolRefreshFailCleanup
Create a common pool refresh failure handling method as the
same code is repeated multiple times.
Signed-off-by: John Ferlan
---
src/storage/storage_driver.c | 38 +---
1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 5a8871bd07..8aa3191f7b 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -79,6 +79,18 @@ static void storageDriverUnlock(void)
}
+static void
+storagePoolRefreshFailCleanup(virStorageBackendPtr backend,
+ virStoragePoolObjPtr obj,
+ const char *stateFile)
+{
+if (stateFile)
+ignore_value(unlink(stateFile));
+if (backend->stopPool)
+backend->stopPool(obj);
+}
+
+
/**
* virStoragePoolUpdateInactive:
* @poolptr: pointer to a variable holding the pool object pointer
@@ -127,6 +139,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to initialize storage pool '%s': %s"),
def->name, virGetLastErrorMessage());
+ignore_value(unlink(stateFile));
active = false;
}
@@ -137,8 +150,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj,
if (active) {
virStoragePoolObjClearVols(obj);
if (backend->refreshPool(obj) < 0) {
-if (backend->stopPool)
-backend->stopPool(obj);
+storagePoolRefreshFailCleanup(backend, obj, stateFile);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to restart storage pool '%s': %s"),
def->name, virGetLastErrorMessage());
@@ -151,8 +163,6 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj,
if (!virStoragePoolObjIsActive(obj))
virStoragePoolUpdateInactive();
-if (!active)
-ignore_value(unlink(stateFile));
VIR_FREE(stateFile);
return;
@@ -199,10 +209,7 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
if (!stateFile ||
virStoragePoolSaveState(stateFile, def) < 0 ||
backend->refreshPool(obj) < 0) {
-if (stateFile)
-unlink(stateFile);
-if (backend->stopPool)
-backend->stopPool(obj);
+storagePoolRefreshFailCleanup(backend, obj, stateFile);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to autostart storage pool '%s': %s"),
def->name, virGetLastErrorMessage());
@@ -738,10 +745,7 @@ storagePoolCreateXML(virConnectPtr conn,
virStoragePoolObjClearVols(obj);
if (!stateFile || virStoragePoolSaveState(stateFile, def) < 0 ||
backend->refreshPool(obj) < 0) {
-if (stateFile)
-unlink(stateFile);
-if (backend->stopPool)
-backend->stopPool(obj);
+storagePoolRefreshFailCleanup(backend, obj, stateFile);
goto error;
}
@@ -939,10 +943,7 @@ storagePoolCreate(virStoragePoolPtr pool,
virStoragePoolObjClearVols(obj);
if (!stateFile || virStoragePoolSaveState(stateFile, def) < 0 ||
backend->refreshPool(obj) < 0) {
-if (stateFile)
-unlink(stateFile);
-if (backend->stopPool)
-backend->stopPool(obj);
+storagePoolRefreshFailCleanup(backend, obj, stateFile);
goto cleanup;
}
@@ -1174,10 +1175,7 @@ storagePoolRefresh(virStoragePoolPtr pool,
if (backend->refreshPool(obj) < 0) {
char *stateFile = virFileBuildPath(driver->stateDir, def->name,
".xml");
-if (stateFile)
-unlink(stateFile);
-if (backend->stopPool)
-backend->stopPool(obj);
+storagePoolRefreshFailCleanup(backend, obj, stateFile);
VIR_FREE(stateFile);
event = virStoragePoolEventLifecycleNew(def->name,
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] storage: Clean up storagePoolUpdateStateCallback processing
Alter the code path to remove the need to to go cleanup and thus
remove the label completely.
Signed-off-by: John Ferlan
---
src/storage/storage_driver.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 1dbeb213e3..d0e7e6904c 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -111,15 +111,15 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj,
virStorageBackendPtr backend;
char *stateFile;
-if (!(stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml")))
-goto cleanup;
-
if ((backend = virStorageBackendForType(def->type)) == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Missing backend %d"), def->type);
-goto cleanup;
+return;
}
+if (!(stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml")))
+return;
+
/* Backends which do not support 'checkPool' are considered
* inactive by default. */
if (backend->checkPool &&
@@ -151,8 +151,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj,
if (!virStoragePoolObjIsActive(obj))
virStoragePoolUpdateInactive();
- cleanup:
-if (!active && stateFile)
+if (!active)
ignore_value(unlink(stateFile));
VIR_FREE(stateFile);
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] storage: Save error during refresh failure processing
https://bugzilla.redhat.com/show_bug.cgi?id=1614283
Save the error from the refresh failure because the stopPool
processing may overwrite the error or even worse clear it
due to calling an external libvirt API that resets the last
error such as is the case with the SCSI pool which may call
virGetConnectNodeDev (see commit decaeb288) in order to
process deleting an NPIV vport.
Signed-off-by: John Ferlan
---
src/storage/storage_driver.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 8aa3191f7b..f032d0dfdd 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -84,10 +84,16 @@ storagePoolRefreshFailCleanup(virStorageBackendPtr backend,
virStoragePoolObjPtr obj,
const char *stateFile)
{
+virErrorPtr orig_err = virSaveLastError();
+
if (stateFile)
ignore_value(unlink(stateFile));
if (backend->stopPool)
backend->stopPool(obj);
+if (orig_err) {
+virSetError(orig_err);
+virFreeError(orig_err);
+}
}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/5] storage: Create error label path for storagePoolCreateXML
Rather than duplicate the error code, let's create an error
label to keep code common.
Signed-off-by: John Ferlan
---
src/storage/storage_driver.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index d0e7e6904c..5a8871bd07 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -730,12 +730,8 @@ storagePoolCreateXML(virConnectPtr conn,
}
if (backend->startPool &&
-backend->startPool(obj) < 0) {
-virStoragePoolObjRemove(driver->pools, obj);
-virObjectUnref(obj);
-obj = NULL;
-goto cleanup;
-}
+backend->startPool(obj) < 0)
+goto error;
stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml");
@@ -746,10 +742,7 @@ storagePoolCreateXML(virConnectPtr conn,
unlink(stateFile);
if (backend->stopPool)
backend->stopPool(obj);
-virStoragePoolObjRemove(driver->pools, obj);
-virObjectUnref(obj);
-obj = NULL;
-goto cleanup;
+goto error;
}
event = virStoragePoolEventLifecycleNew(def->name,
@@ -768,6 +761,12 @@ storagePoolCreateXML(virConnectPtr conn,
virObjectEventStateQueue(driver->storageEventState, event);
virStoragePoolObjEndAPI();
return pool;
+
+ error:
+virStoragePoolObjRemove(driver->pools, obj);
+virObjectUnref(obj);
+obj = NULL;
+goto cleanup;
}
static virStoragePoolPtr
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 0/5] Introduce VIR_AUTOCLOSE macro for closing fd automatically
On 09/12/2018 11:46 AM, Shi Lei wrote: > v1 here: > https://www.redhat.com/archives/libvir-list/2018-September/msg00319.html > > Diff from v1: > - Change VIR_AUTOCLOSE macro (comments from Michal) > - Remove others except for patches in util (comments from Erik) > - Change sc_require_attribute_cleanup_initialization for this macro > > Shi Lei (5): > util: file: introduce VIR_AUTOCLOSE macro to close fd of the file > automatically > cfg.mk: Change syntax-check rule for VIR_AUTOCLOSE variable > initialization > util: file: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE > util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE > util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE > > cfg.mk | 2 +- > src/util/virfile.c | 21 ++-- > src/util/virfile.h | 18 ++- > src/util/virnetdev.c | 251 + > src/util/virnetdevbridge.c | 120 ++ > 5 files changed, 146 insertions(+), 266 deletions(-) > ACKed and pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 4/5] util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE
On 09/12/2018 11:46 AM, Shi Lei wrote:
> Signed-off-by: Shi Lei
> ---
> src/util/virnetdevbridge.c | 120 -
> 1 file changed, 37 insertions(+), 83 deletions(-)
>
> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> index ed2db27..e058898 100644
> --- a/src/util/virnetdevbridge.c
> +++ b/src/util/virnetdevbridge.c
> @@ -723,19 +687,14 @@ int virNetDevBridgeRemovePort(const char *brname,
> int virNetDevBridgeSetSTPDelay(const char *brname,
> int delay)
> {
> -int fd = -1;
> -int ret = -1;
> struct ifreq ifr;
> +VIR_AUTOCLOSE fd = -1;
>
> if ((fd = virNetDevSetupControl(brname, )) < 0)
> -goto cleanup;
> +return -1;
>
> -ret = virNetDevBridgeSet(brname, "forward_delay", MS_TO_JIFFIES(delay),
> +return virNetDevBridgeSet(brname, "forward_delay", MS_TO_JIFFIES(delay),
> fd, );
Misaligned arguments.
> -
> - cleanup:
> -VIR_FORCE_CLOSE(fd);
> -return ret;
> }
>
>
> @@ -776,19 +735,14 @@ int virNetDevBridgeGetSTPDelay(const char *brname,
> int virNetDevBridgeSetSTP(const char *brname,
>bool enable)
> {
> -int fd = -1;
> -int ret = -1;
> struct ifreq ifr;
> +VIR_AUTOCLOSE fd = -1;
>
> if ((fd = virNetDevSetupControl(brname, )) < 0)
> -goto cleanup;
> +return -1;
>
> -ret = virNetDevBridgeSet(brname, "stp_state", enable ? 1 : 0,
> +return virNetDevBridgeSet(brname, "stp_state", enable ? 1 : 0,
> fd, );
> -
And again.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Remove some unnecessary warnings
On 09/12/2018 05:09 PM, Martin Kletzander wrote: > cd . > ./blurb > > Martin Kletzander (2): > Add functions for checking if user or group exists > qemu: Report less errors on driver startup > > src/libvirt_private.syms | 2 ++ > src/qemu/qemu_conf.c | 6 -- > src/util/virutil.c | 40 > src/util/virutil.h | 4 > 4 files changed, 42 insertions(+), 10 deletions(-) > ACK series. --help I mean Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 0/7] qemuxml2argvtest cleanups
On 09/12/2018 03:30 PM, Ján Tomko wrote:
> Patch 1 and patch 3 were not acked in v1.
> The changes to the ACKed ones are minimal.
> Complete diff to v2:
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 867e0d569f..0e43320b96 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -460,20 +460,20 @@ testCompareXMLToStartupXML(const void *data)
> }
>
>
> -# define TEST_EXCLUSIVE_FLAGS(FLAG1, FLAG2) \
> -if ((testFlags & FLAG1) && (testFlags & FLAG2)) { \
> -VIR_TEST_DEBUG("Flags %s and %s are mutually exclusive\n", \
> - #FLAG1, #FLAG2); \
> -return -1; \
> -}
> -
> -
> static int
> -testCheckExclusiveFlags(int testFlags ATTRIBUTE_UNUSED)
> +testCheckExclusiveFlags(int flags)
> {
> -TEST_EXCLUSIVE_FLAGS(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE);
> -TEST_EXCLUSIVE_FLAGS(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR);
> -TEST_EXCLUSIVE_FLAGS(FLAG_REAL_CAPS, FLAG_SKIP_LEGACY_CPUS);
> +virCheckFlags(FLAG_EXPECT_FAILURE |
> + FLAG_EXPECT_PARSE_ERROR |
> + FLAG_FIPS |
> + FLAG_STEAL_VM |
> + FLAG_REAL_CAPS |
> + FLAG_SKIP_LEGACY_CPUS |
> + 0, -1);
> +
> +VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE, -1);
> +VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR, -1);
> +VIR_EXCLUSIVE_FLAGS_RET(FLAG_REAL_CAPS, FLAG_SKIP_LEGACY_CPUS, -1);
> return 0;
> }
>
> Ján Tomko (7):
> tests: add a function for checking exclusive flags
> tests: introduce macro for qemu XML->startup XML
> tests: only run startup XML tests if requested
> tests: report errors in QEMU XML->startup XML tests
> tests: do not mangle real qemu caps in xml2argvtest
> tests: turn skipLegacyCPUs into a flag
> qemu: remove unnecessary virQEMUCapsFreeHostCPUModel
>
> src/qemu/qemu_capabilities.c | 25 ++-
> src/qemu/qemu_capspriv.h | 5 ---
> tests/qemuxml2argvtest.c | 74
>
> 3 files changed, 49 insertions(+), 55 deletions(-)
>
ACK series.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Remove some unnecessary warnings
cd . ./blurb Martin Kletzander (2): Add functions for checking if user or group exists qemu: Report less errors on driver startup src/libvirt_private.syms | 2 ++ src/qemu/qemu_conf.c | 6 -- src/util/virutil.c | 40 src/util/virutil.h | 4 4 files changed, 42 insertions(+), 10 deletions(-) -- 2.18.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] Add functions for checking if user or group exists
Instead of duplicating the code from virGet{User,Group}IDByName(), which are
static anyway, extend those functions to accept NULL pointers for the result and
a boolean for controlling the error reporting.
Signed-off-by: Martin Kletzander
---
Feel free to suggest any other way of doing this, I just felt like this is the
most readable way of doing things
src/libvirt_private.syms | 2 ++
src/util/virutil.c | 40
src/util/virutil.h | 4
3 files changed, 38 insertions(+), 8 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a0d229a79f0c..e7a4d61f25fd 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3090,6 +3090,8 @@ virUSBDeviceSetUsedBy;
# util/virutil.h
+virDoesGroupExist;
+virDoesUserExist;
virDoubleToStr;
virEnumFromString;
virEnumToString;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index a908422feb7f..2290020e494e 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -971,9 +971,11 @@ char *virGetGroupName(gid_t gid)
/* Search in the password database for a user id that matches the user name
* `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
+ *
+ * Warns if @missing_ok is false
*/
static int
-virGetUserIDByName(const char *name, uid_t *uid)
+virGetUserIDByName(const char *name, uid_t *uid, bool missing_ok)
{
char *strbuf = NULL;
struct passwd pwbuf;
@@ -996,7 +998,7 @@ virGetUserIDByName(const char *name, uid_t *uid)
}
if (!pw) {
-if (rc != 0) {
+if (rc != 0 && !missing_ok) {
char buf[1024];
/* log the possible error from getpwnam_r. Unfortunately error
* reporting from this function is bad and we can't really
@@ -1009,7 +1011,8 @@ virGetUserIDByName(const char *name, uid_t *uid)
goto cleanup;
}
-*uid = pw->pw_uid;
+if (uid)
+*uid = pw->pw_uid;
ret = 0;
cleanup:
@@ -1032,7 +1035,7 @@ virGetUserID(const char *user, uid_t *uid)
if (*user == '+') {
user++;
} else {
-int rc = virGetUserIDByName(user, uid);
+int rc = virGetUserIDByName(user, uid, false);
if (rc <= 0)
return rc;
}
@@ -1051,9 +1054,11 @@ virGetUserID(const char *user, uid_t *uid)
/* Search in the group database for a group id that matches the group name
* `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
+ *
+ * Warns if @missing_ok is false
*/
static int
-virGetGroupIDByName(const char *name, gid_t *gid)
+virGetGroupIDByName(const char *name, gid_t *gid, bool missing_ok)
{
char *strbuf = NULL;
struct group grbuf;
@@ -1076,7 +1081,7 @@ virGetGroupIDByName(const char *name, gid_t *gid)
}
if (!gr) {
-if (rc != 0) {
+if (rc != 0 && !missing_ok) {
char buf[1024];
/* log the possible error from getgrnam_r. Unfortunately error
* reporting from this function is bad and we can't really
@@ -1089,7 +1094,8 @@ virGetGroupIDByName(const char *name, gid_t *gid)
goto cleanup;
}
-*gid = gr->gr_gid;
+if (gid)
+*gid = gr->gr_gid;
ret = 0;
cleanup:
@@ -1112,7 +1118,7 @@ virGetGroupID(const char *group, gid_t *gid)
if (*group == '+') {
group++;
} else {
-int rc = virGetGroupIDByName(group, gid);
+int rc = virGetGroupIDByName(group, gid, false);
if (rc <= 0)
return rc;
}
@@ -1129,6 +1135,24 @@ virGetGroupID(const char *group, gid_t *gid)
return 0;
}
+/* Silently checks if User @name exists.
+ * Returns if the user exists and fallbacks to false on error.
+ */
+int
+virDoesUserExist(const char *name, bool *exists)
+{
+return virGetUserIDByName(name, NULL, true) == 0;
+}
+
+/* Silently checks if Group @name exists.
+ * Returns if the group exists and fallbacks to false on error.
+ */
+int
+virDoesGroupExist(const char *name)
+{
+return virGetGroupIDByName(name, NULL, true) == 0;
+}
+
/* Compute the list of primary and supplementary groups associated
* with @uid, and including @gid in the list (unless it is -1),
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 1ba9635bd991..8a27cca523c8 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -152,6 +152,10 @@ int virGetUserID(const char *name,
int virGetGroupID(const char *name,
gid_t *gid) ATTRIBUTE_RETURN_CHECK;
+int virDoesUserExist(const char *name, bool *exists);
+int virDoesGroupExist(const char *name, bool *exists);
+
+
bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1);
bool virValidateWWN(const char *wwn);
--
2.18.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] qemu: Report less errors on driver startup
It is not a problem at all if the `tss` user/group does not exist, the code
fallbacks to the `root` user/group. However we report a warning for no reason
on every start-up. Fix this by checking if the user/group actually exists.
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_conf.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a4f545ef9243..4d69b599fed8 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -197,9 +197,11 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
privileged)
if (virAsprintf(>swtpmStorageDir, "%s/lib/libvirt/swtpm",
LOCALSTATEDIR) < 0)
goto error;
-if (virGetUserID("tss", >swtpm_user) < 0)
+if (virDoesUserExist("tss") != 0 ||
+virGetUserID("tss", >swtpm_user) < 0)
cfg->swtpm_user = 0; /* fall back to root */
-if (virGetGroupID("tss", >swtpm_group) < 0)
+if (virDoesGroupExist("tss") != 0 ||
+virGetGroupID("tss", >swtpm_group) < 0)
cfg->swtpm_group = 0; /* fall back to root */
} else {
char *rundir;
--
2.18.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] qemu: Drop some cruft
On 09/12/2018 09:35 AM, Andrea Bolognani wrote: > On Wed, 2018-09-12 at 09:09 -0400, John Ferlan wrote: >> Any chance this can wait? Would be nice to have other series posted >> upstream that have changes to .xml and .replies files to add new >> functionality be processed first. >> >> There's a series to use - memfd from Marc-Andre Laurent that gets impacted. > > I see you've already had to rebase locally to apply the patches at > all, due to other changes being pushed in the meantime, so I don't > see how pushing this series too would make it any worse. > > Also IIUC you've asked Marc-André to make some changes that depend > on *another series* of yours, that still hasn't been pushed, which > means a respin will be necessary either way, won't it? > > All in all I see no reason to dealy pushing this. > OK - go ahead. It was just a "would be nice" type inquiry. It's not the first time capability changes cause conflicts and it won't be the last unless we come up with a better process for them. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] qemu: Drop some cruft
On Wed, 2018-09-12 at 09:09 -0400, John Ferlan wrote: > Any chance this can wait? Would be nice to have other series posted > upstream that have changes to .xml and .replies files to add new > functionality be processed first. > > There's a series to use - memfd from Marc-Andre Laurent that gets impacted. I see you've already had to rebase locally to apply the patches at all, due to other changes being pushed in the meantime, so I don't see how pushing this series too would make it any worse. Also IIUC you've asked Marc-André to make some changes that depend on *another series* of yours, that still hasn't been pushed, which means a respin will be necessary either way, won't it? All in all I see no reason to dealy pushing this. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 6/7] tests: turn skipLegacyCPUs into a flag
Make it obvious when it is used intentionally and error
out when used in combination with real capabilities.
Signed-off-by: Ján Tomko
---
tests/qemuxml2argvtest.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1318ac78f0..166f94575f 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -295,6 +295,7 @@ typedef enum {
FLAG_FIPS = 1 << 2,
FLAG_STEAL_VM = 1 << 3,
FLAG_REAL_CAPS = 1 << 4,
+FLAG_SKIP_LEGACY_CPUS = 1 << 5,
} virQemuXML2ArgvTestFlags;
struct testInfo {
@@ -305,7 +306,6 @@ struct testInfo {
int migrateFd;
unsigned int flags;
unsigned int parseFlags;
-bool skipLegacyCPUs;
virDomainObjPtr vm;
};
@@ -414,7 +414,8 @@ testUpdateQEMUCaps(const struct testInfo *info,
virQEMUCapsInitQMPBasicArch(info->qemuCaps);
-if (testAddCPUModels(info->qemuCaps, info->skipLegacyCPUs) < 0)
+if (testAddCPUModels(info->qemuCaps,
+ !!(info->flags & FLAG_SKIP_LEGACY_CPUS)) < 0)
goto cleanup;
virQEMUCapsFreeHostCPUModel(info->qemuCaps, caps->host.arch,
@@ -472,10 +473,12 @@ testCheckExclusiveFlags(int flags)
FLAG_FIPS |
FLAG_STEAL_VM |
FLAG_REAL_CAPS |
+ FLAG_SKIP_LEGACY_CPUS |
0, -1);
VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE, -1);
VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR, -1);
+VIR_EXCLUSIVE_FLAGS_RET(FLAG_REAL_CAPS, FLAG_SKIP_LEGACY_CPUS, -1);
return 0;
}
@@ -672,7 +675,6 @@ mymain(void)
{
int ret = 0, i;
char *fakerootdir;
-bool skipLegacyCPUs = false;
const char *archs[] = {
"aarch64",
"ppc64",
@@ -786,9 +788,8 @@ mymain(void)
do { \
static struct testInfo info = { \
name, "." suffix, NULL, migrateFrom, migrateFrom ? 7 : -1,\
-(flags | FLAG_REAL_CAPS), parseFlags, false, NULL \
+(flags | FLAG_REAL_CAPS), parseFlags, NULL \
}; \
-info.skipLegacyCPUs = skipLegacyCPUs; \
if (!(info.qemuCaps =
qemuTestParseCapabilitiesArch(virArchFromString(arch), \
capsfile))) \
return EXIT_FAILURE; \
@@ -835,9 +836,8 @@ mymain(void)
do { \
static struct testInfo info = { \
name, NULL, NULL, migrateFrom, migrateFd, (flags), parseFlags, \
-false, NULL \
+NULL \
}; \
-info.skipLegacyCPUs = skipLegacyCPUs; \
if (testInitQEMUCaps(, gic) < 0) \
return EXIT_FAILURE; \
virQEMUCapsSetList(info.qemuCaps, __VA_ARGS__, QEMU_CAPS_LAST); \
@@ -1748,10 +1748,12 @@ mymain(void)
DO_TEST("cpu-numa-memshared", QEMU_CAPS_OBJECT_MEMORY_FILE);
DO_TEST("cpu-host-model", NONE);
DO_TEST("cpu-host-model-vendor", NONE);
-skipLegacyCPUs = true;
-DO_TEST("cpu-host-model-fallback", NONE);
-DO_TEST_FAILURE("cpu-host-model-nofallback", NONE);
-skipLegacyCPUs = false;
+DO_TEST_FULL("cpu-host-model-fallback", NULL, -1,
+ FLAG_SKIP_LEGACY_CPUS, 0,
+ GIC_NONE, NONE);
+DO_TEST_FULL("cpu-host-model-nofallback", NULL, -1,
+ FLAG_SKIP_LEGACY_CPUS | FLAG_EXPECT_FAILURE,
+ 0, GIC_NONE, NONE);
DO_TEST("cpu-host-passthrough", QEMU_CAPS_KVM);
DO_TEST_FAILURE("cpu-qemu-host-passthrough", QEMU_CAPS_KVM);
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 5/7] tests: do not mangle real qemu caps in xml2argvtest
None of the things testUpdateQEMUCaps adjusts are applicable
for tests that use the DO_TEST_CAPS macros, i.e.
real QEMU capabilities parsed from the XML files:
The architecture must be chosen before we even open the caps
file, CPU models are already present and the expensive HostModel
computation was already done in virQEMUCapsLoadCache.
Introduce FLAG_REAL_CAPS and skip the whole testUpdateQEMUCaps
function for DO_TEST_CAPS.
This speeds up the test by 25 %
Signed-off-by: Ján Tomko
---
tests/qemuxml2argvtest.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index fe6e38e61a..1318ac78f0 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -294,6 +294,7 @@ typedef enum {
FLAG_EXPECT_PARSE_ERROR = 1 << 1,
FLAG_FIPS = 1 << 2,
FLAG_STEAL_VM = 1 << 3,
+FLAG_REAL_CAPS = 1 << 4,
} virQemuXML2ArgvTestFlags;
struct testInfo {
@@ -470,6 +471,7 @@ testCheckExclusiveFlags(int flags)
FLAG_EXPECT_PARSE_ERROR |
FLAG_FIPS |
FLAG_STEAL_VM |
+ FLAG_REAL_CAPS |
0, -1);
VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE, -1);
@@ -566,7 +568,8 @@ testCompareXMLToArgv(const void *data)
if (qemuProcessPrepareMonitorChr(_chr, priv->libDir) < 0)
goto cleanup;
-if (testUpdateQEMUCaps(info, vm, driver.caps) < 0)
+if (!(info->flags & FLAG_REAL_CAPS) &&
+testUpdateQEMUCaps(info, vm, driver.caps) < 0)
goto cleanup;
log = virTestLogContentAndReset();
@@ -783,7 +786,7 @@ mymain(void)
do { \
static struct testInfo info = { \
name, "." suffix, NULL, migrateFrom, migrateFrom ? 7 : -1,\
-(flags), parseFlags, false, NULL \
+(flags | FLAG_REAL_CAPS), parseFlags, false, NULL \
}; \
info.skipLegacyCPUs = skipLegacyCPUs; \
if (!(info.qemuCaps =
qemuTestParseCapabilitiesArch(virArchFromString(arch), \
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 1/7] tests: add a function for checking exclusive flags
We can reject some non-sensical combinations with an error
message, once we add flags for them.
Signed-off-by: Ján Tomko
---
tests/qemuxml2argvtest.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 816a3055a2..65fab6c077 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -465,6 +465,18 @@ testCompareXMLToStartupXML(const void *data)
}
+static int
+testCheckExclusiveFlags(int flags)
+{
+virCheckFlags(FLAG_EXPECT_FAILURE |
+ FLAG_EXPECT_PARSE_ERROR |
+ FLAG_FIPS |
+ 0, -1);
+
+return 0;
+}
+
+
static int
testCompareXMLToArgv(const void *data)
{
@@ -507,6 +519,9 @@ testCompareXMLToArgv(const void *data)
if (virQEMUCapsGet(info->qemuCaps, QEMU_CAPS_ENABLE_FIPS))
flags |= FLAG_FIPS;
+if (testCheckExclusiveFlags(info->flags) < 0)
+goto cleanup;
+
if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps) < 0)
goto cleanup;
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 3/7] tests: only run startup XML tests if requested
Use the recently introduced flag as a witness.
This reduces the apparent number of test cases
to the real number of test cases.
Note that this does not suffer from the same problem
as commit 70255fa was fixing, because the condition
for running virTestRun does not depend on results
of previous tests.
Signed-off-by: Ján Tomko
---
tests/qemuxml2argvtest.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 0c82540ced..ac6df86618 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -475,6 +475,8 @@ testCheckExclusiveFlags(int flags)
FLAG_STEAL_VM |
0, -1);
+VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE, -1);
+VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR, -1);
return 0;
}
@@ -842,7 +844,8 @@ mymain(void)
if (virTestRun("QEMU XML-2-ARGV " name, \
testCompareXMLToArgv, ) < 0) \
ret = -1; \
-if (virTestRun("QEMU XML-2-startup-XML " name, \
+if (((flags) & FLAG_STEAL_VM) && \
+virTestRun("QEMU XML-2-startup-XML " name, \
testCompareXMLToStartupXML, ) < 0) \
ret = -1; \
virObjectUnref(info.qemuCaps); \
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 4/7] tests: report errors in QEMU XML->startup XML tests
Now that the function is only run if requested by
the FLAG_STEAL_VM flag, we know that missing data
is an error, not a request to skip the test.
The existence of the output file is now checked by
virTestCompareToFile, which allows usage of
the VIR_TEST_REGENERATE_OUTPUT=1 env variable
to generate new test cases.
Signed-off-by: Ján Tomko
---
tests/qemuxml2argvtest.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index ac6df86618..fe6e38e61a 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -442,18 +442,15 @@ testCompareXMLToStartupXML(const void *data)
char *actual = NULL;
int ret = -1;
-if (!info->vm)
-return EXIT_AM_SKIP;
+if (!info->vm) {
+VIR_TEST_DEBUG("VM object missing. Did the args conversion succeed?");
+return -1;
+}
if (virAsprintf(, "%s/qemuxml2startupxmloutdata/%s.xml",
abs_srcdir, info->name) < 0)
goto cleanup;
-if (!virFileExists(xml)) {
-ret = EXIT_AM_SKIP;
-goto cleanup;
-}
-
if (!(actual = virDomainDefFormat(info->vm->def, NULL, format_flags)))
goto cleanup;
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 7/7] qemu: remove unnecessary virQEMUCapsFreeHostCPUModel
After removing the host CPU model re-computation,
this function is no longer necessary.
This reverts commits:
commit d0498881a04772f9f63b03de80fb4c33d090
virQEMUCapsFreeHostCPUModel: Don't always free host cpuData
commit 5276ec712a44b3680569a096e8fe56a925f0d495
testUpdateQEMUCaps: Don't leak host cpuData
Signed-off-by: Ján Tomko
---
src/qemu/qemu_capabilities.c | 25 ++---
src/qemu/qemu_capspriv.h | 5 -
tests/qemuxml2argvtest.c | 5 -
3 files changed, 2 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 308f5602ca..7e6d8a71b7 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1526,19 +1526,12 @@ virQEMUCapsHostCPUDataCopy(virQEMUCapsHostCPUDataPtr
dst,
static void
-virQEMUCapsHostCPUDataClearModels(virQEMUCapsHostCPUDataPtr cpuData)
+virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData)
{
+qemuMonitorCPUModelInfoFree(cpuData->info);
virCPUDefFree(cpuData->reported);
virCPUDefFree(cpuData->migratable);
virCPUDefFree(cpuData->full);
-}
-
-
-static void
-virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData)
-{
-qemuMonitorCPUModelInfoFree(cpuData->info);
-virQEMUCapsHostCPUDataClearModels(cpuData);
memset(cpuData, 0, sizeof(*cpuData));
}
@@ -2986,20 +2979,6 @@ virQEMUCapsNewHostCPUModel(void)
}
-void
-virQEMUCapsFreeHostCPUModel(virQEMUCapsPtr qemuCaps,
-virArch hostArch,
-virDomainVirtType type)
-{
-virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps,
type);
-
-if (!virQEMUCapsGuestIsNative(hostArch, qemuCaps->arch))
-return;
-
-virQEMUCapsHostCPUDataClearModels(cpuData);
-}
-
-
void
virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
virArch hostArch,
diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h
index cb5e0dd9a9..8d1a40fe74 100644
--- a/src/qemu/qemu_capspriv.h
+++ b/src/qemu/qemu_capspriv.h
@@ -56,11 +56,6 @@ void
virQEMUCapsSetArch(virQEMUCapsPtr qemuCaps,
virArch arch);
-void
-virQEMUCapsFreeHostCPUModel(virQEMUCapsPtr qemuCaps,
-virArch hostArch,
-virDomainVirtType type);
-
void
virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
virArch hostArch,
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 166f94575f..0e43320b96 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -418,11 +418,6 @@ testUpdateQEMUCaps(const struct testInfo *info,
!!(info->flags & FLAG_SKIP_LEGACY_CPUS)) < 0)
goto cleanup;
-virQEMUCapsFreeHostCPUModel(info->qemuCaps, caps->host.arch,
-VIR_DOMAIN_VIRT_KVM);
-virQEMUCapsFreeHostCPUModel(info->qemuCaps, caps->host.arch,
-VIR_DOMAIN_VIRT_QEMU);
-
virQEMUCapsInitHostCPUModel(info->qemuCaps, caps->host.arch,
VIR_DOMAIN_VIRT_KVM);
virQEMUCapsInitHostCPUModel(info->qemuCaps, caps->host.arch,
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 2/7] tests: introduce macro for qemu XML->startup XML
Use this macro to indicate the intention to also
run the XML->startup XML test.
It sets the newly introduced FLAG_STEAL_VM flag,
which is the new witness for the XML->argv test
to leave the VM object behind.
This will allow us to report proper errors in
XML->startup tests.
Signed-off-by: Ján Tomko
---
tests/qemuxml2argvtest.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 65fab6c077..0c82540ced 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -293,6 +293,7 @@ typedef enum {
FLAG_EXPECT_FAILURE = 1 << 0,
FLAG_EXPECT_PARSE_ERROR = 1 << 1,
FLAG_FIPS = 1 << 2,
+FLAG_STEAL_VM = 1 << 3,
} virQemuXML2ArgvTestFlags;
struct testInfo {
@@ -471,6 +472,7 @@ testCheckExclusiveFlags(int flags)
virCheckFlags(FLAG_EXPECT_FAILURE |
FLAG_EXPECT_PARSE_ERROR |
FLAG_FIPS |
+ FLAG_STEAL_VM |
0, -1);
return 0;
@@ -643,7 +645,7 @@ testCompareXMLToArgv(const void *data)
ret = 0;
}
-if (!(flags & FLAG_EXPECT_FAILURE) && ret == 0)
+if (flags & FLAG_STEAL_VM)
VIR_STEAL_PTR(info->vm, vm);
cleanup:
@@ -853,6 +855,9 @@ mymain(void)
# define DO_TEST_GIC(name, gic, ...) \
DO_TEST_FULL(name, NULL, -1, 0, 0, gic, __VA_ARGS__)
+# define DO_TEST_WITH_STARTUP(name, ...) \
+DO_TEST_FULL(name, NULL, -1, FLAG_STEAL_VM, 0, GIC_NONE, __VA_ARGS__)
+
# define DO_TEST_FAILURE(name, ...) \
DO_TEST_FULL(name, NULL, -1, FLAG_EXPECT_FAILURE, \
0, GIC_NONE, __VA_ARGS__)
@@ -1092,7 +1097,7 @@ mymain(void)
DO_TEST_PARSE_ERROR("disk-fmt-cow", NONE);
DO_TEST_PARSE_ERROR("disk-fmt-dir", NONE);
DO_TEST_PARSE_ERROR("disk-fmt-iso", NONE);
-DO_TEST("disk-shared", NONE);
+DO_TEST_WITH_STARTUP("disk-shared", NONE);
DO_TEST_CAPS_VER("disk-shared", "2.12.0");
DO_TEST_CAPS_LATEST("disk-shared");
DO_TEST_PARSE_ERROR("disk-shared-qcow", NONE);
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 0/7] qemuxml2argvtest cleanups
Patch 1 and patch 3 were not acked in v1.
The changes to the ACKed ones are minimal.
Complete diff to v2:
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 867e0d569f..0e43320b96 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -460,20 +460,20 @@ testCompareXMLToStartupXML(const void *data)
}
-# define TEST_EXCLUSIVE_FLAGS(FLAG1, FLAG2) \
-if ((testFlags & FLAG1) && (testFlags & FLAG2)) { \
-VIR_TEST_DEBUG("Flags %s and %s are mutually exclusive\n", \
- #FLAG1, #FLAG2); \
-return -1; \
-}
-
-
static int
-testCheckExclusiveFlags(int testFlags ATTRIBUTE_UNUSED)
+testCheckExclusiveFlags(int flags)
{
-TEST_EXCLUSIVE_FLAGS(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE);
-TEST_EXCLUSIVE_FLAGS(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR);
-TEST_EXCLUSIVE_FLAGS(FLAG_REAL_CAPS, FLAG_SKIP_LEGACY_CPUS);
+virCheckFlags(FLAG_EXPECT_FAILURE |
+ FLAG_EXPECT_PARSE_ERROR |
+ FLAG_FIPS |
+ FLAG_STEAL_VM |
+ FLAG_REAL_CAPS |
+ FLAG_SKIP_LEGACY_CPUS |
+ 0, -1);
+
+VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_FAILURE, -1);
+VIR_EXCLUSIVE_FLAGS_RET(FLAG_STEAL_VM, FLAG_EXPECT_PARSE_ERROR, -1);
+VIR_EXCLUSIVE_FLAGS_RET(FLAG_REAL_CAPS, FLAG_SKIP_LEGACY_CPUS, -1);
return 0;
}
Ján Tomko (7):
tests: add a function for checking exclusive flags
tests: introduce macro for qemu XML->startup XML
tests: only run startup XML tests if requested
tests: report errors in QEMU XML->startup XML tests
tests: do not mangle real qemu caps in xml2argvtest
tests: turn skipLegacyCPUs into a flag
qemu: remove unnecessary virQEMUCapsFreeHostCPUModel
src/qemu/qemu_capabilities.c | 25 ++-
src/qemu/qemu_capspriv.h | 5 ---
tests/qemuxml2argvtest.c | 74
3 files changed, 49 insertions(+), 55 deletions(-)
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v2 1/4] domain_conf: split out virBlkioDevice and virDomainBlkiotune definitions
On 09/12/2018 10:57 AM, Fabiano Fidêncio wrote:
> Let's move those to their own newly created files
> (src/util/virblkio.{c,h}) as this will help us to easily start sharing
> the cgroup code that's duplicated between QEMU and LXC.
>
> Signed-off-by: Fabiano Fidêncio
> ---
> src/Makefile.am | 1 +
> src/conf/domain_conf.c | 11 +
> src/conf/domain_conf.h | 27 ++---
> src/util/Makefile.inc.am | 2 ++
> src/util/virblkio.c | 37
> src/util/virblkio.h | 52
> 6 files changed, 95 insertions(+), 35 deletions(-)
> create mode 100644 src/util/virblkio.c
> create mode 100644 src/util/virblkio.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 2a3ed0d42d..926085ff2d 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -671,6 +671,7 @@ libvirt_setuid_rpc_client_la_SOURCES = \
> util/viratomic.c \
> util/viratomic.h \
> util/virbitmap.c \
> + util/virblkio.c \
> util/virbuffer.c \
> util/vircgroup.c \
> util/vircommand.c \
This is not needed. setuid_rpc_client is trying to be very minimalistic
library which is then statically linked to virt-login-shell. The aim is
to have something small, thus auditable and yet capable of talking to
libvirtd through RPC.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7e14cea128..6ce50f712a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -59,6 +59,7 @@
> #include "virnetdevmacvlan.h"
> #include "virhostdev.h"
> #include "virmdev.h"
> +#include "virblkio.h"
>
> #define VIR_FROM_THIS VIR_FROM_DOMAIN
>
> @@ -1205,16 +1206,6 @@ virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr
> xmlopt)
> }
>
>
> -void
> -virBlkioDeviceArrayClear(virBlkioDevicePtr devices,
> - int ndevices)
> -{
> -size_t i;
> -
> -for (i = 0; i < ndevices; i++)
> -VIR_FREE(devices[i].path);
> -}
> -
> /**
> * virDomainBlkioDeviceParseXML
> *
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index e30a4b2fe7..e9e6b6d6c4 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -57,6 +57,7 @@
> # include "virtypedparam.h"
> # include "virsavecookie.h"
> # include "virresctrl.h"
> +# include "virblkio.h"
>
> /* forward declarations of all device types, required by
> * virDomainDeviceDef
> @@ -2084,17 +2085,6 @@ struct _virDomainClockDef {
> };
>
>
> -typedef struct _virBlkioDevice virBlkioDevice;
> -typedef virBlkioDevice *virBlkioDevicePtr;
> -struct _virBlkioDevice {
> -char *path;
> -unsigned int weight;
> -unsigned int riops;
> -unsigned int wiops;
> -unsigned long long rbps;
> -unsigned long long wbps;
> -};
> -
> typedef enum {
> VIR_DOMAIN_RNG_MODEL_VIRTIO,
>
> @@ -2184,9 +2174,6 @@ struct _virDomainPanicDef {
> };
>
>
> -void virBlkioDeviceArrayClear(virBlkioDevicePtr deviceWeights,
> - int ndevices);
> -
> typedef struct _virDomainResourceDef virDomainResourceDef;
> typedef virDomainResourceDef *virDomainResourceDefPtr;
> struct _virDomainResourceDef {
> @@ -2260,16 +2247,6 @@ struct _virDomainVcpuDef {
> virObjectPtr privateData;
> };
>
> -typedef struct _virDomainBlkiotune virDomainBlkiotune;
> -typedef virDomainBlkiotune *virDomainBlkiotunePtr;
> -
> -struct _virDomainBlkiotune {
> -unsigned int weight;
> -
> -size_t ndevices;
> -virBlkioDevicePtr devices;
> -};
> -
> typedef struct _virDomainMemtune virDomainMemtune;
> typedef virDomainMemtune *virDomainMemtunePtr;
>
> @@ -2402,7 +2379,7 @@ struct _virDomainDef {
> char *title;
> char *description;
>
> -virDomainBlkiotune blkio;
> +virBlkioTune blkio;
> virDomainMemtune mem;
>
> virDomainVcpuDefPtr *vcpus;
> diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
> index a22265606c..13f415b23c 100644
> --- a/src/util/Makefile.inc.am
> +++ b/src/util/Makefile.inc.am
> @@ -17,6 +17,8 @@ UTIL_SOURCES = \
> util/virauthconfig.h \
> util/virbitmap.c \
> util/virbitmap.h \
> + util/virblkio.c \
> + util/virblkio.h \
> util/virbuffer.c \
> util/virbuffer.h \
> util/virperf.c \
> diff --git a/src/util/virblkio.c b/src/util/virblkio.c
> new file mode 100644
> index 00..9711077ee8
> --- /dev/null
> +++ b/src/util/virblkio.c
> @@ -0,0 +1,37 @@
> +/*
> + * virblkio.c: Block IO helpers
> + *
> + * Copyright (C) 2018 Red Hat, Inc.
> + *
> + * 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
Re: [libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC
On 09/12/2018 12:46 PM, Pavel Hrdina wrote:
> On Wed, Sep 12, 2018 at 10:57:32AM +0200, Fabiano Fidêncio wrote:
>> virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup() and
>> virLXCCgroupSetupCpuTune() and qemuSetupCpuCgroup() are the most similar
>> functions between QEMU and LXC code.
>>
>> Let's move their common code to virCgroup.
>>
>> Mind that the first two patches are basically preparing the ground for
>> the changes introduced in the last two patches.
>
> Hi, definitely good idea to remove code duplication!
>
> We have similar issue with the virDomainSetBlkioParameters for QEMU and
> LXC drivers. The code to set cgroup values is the same.
>
> Since the common object is domain how about introducing
> virDomainSetupBlkioTune and virDomainSetupMemTune and move it into
> domain_conf.c, that way we don't have to extract domain specific data
> into src/util.
I'd rather remove stuff from doman_conf.c than add something there. It's
already the biggest file by far margin:
libvirt.git $ find . -type f -iname \*.c -exec du -h {} \; | sort -h
416K./src/qemu/qemu_domain.c
696K./src/qemu/qemu_driver.c
956K./src/conf/domain_conf.c
And moving code from domain_conf is something we do from time to time.
Just look at all those virnet* includes at the beginning of domain_conf.h.
Another reason for moving the code out is that blkio tune is not domain
specific. In the future, we might want to limit say iohelper and thus
move it into its specific cgroup.
I'm not that convinced about 2/4 to be honest. Memory specification is
pretty much domain centric. But it follows my first point - moving code
out from domain_conf.c. And it de-duplicates the code.
>
> Another benefit is that it will not cause me merge conflicts because I'm
> rewriting cgroup code and adding support for cgroup v2.
I don't think that code movement should cause that big of a trouble.
You'll get some context conflicts but that's always the case with 30+
unmerged patches.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] qemu: Drop some cruft
On Wed, Sep 12, 2018 at 09:09:26AM -0400, John Ferlan wrote: On 09/12/2018 08:48 AM, Andrea Bolognani wrote: No longer needed since we bumped our minimum QEMU version to 1.5.0. Andrea Bolognani (3): qemu: Drop QEMU_CAPS_VNC_WEBSOCKET qemu: Drop QEMU_CAPS_CHARDEV_SPICEPORT qemu: Drop redundant version checks src/qemu/qemu_capabilities.c | 27 +-- src/qemu/qemu_capabilities.h | 4 +-- src/qemu/qemu_command.c | 14 +- .../caps_1.5.3.x86_64.xml | 2 -- .../caps_1.6.0.x86_64.xml | 2 -- .../caps_1.7.0.x86_64.xml | 2 -- .../caps_2.1.1.x86_64.xml | 2 -- .../caps_2.10.0.aarch64.xml | 2 -- .../caps_2.10.0.ppc64.xml | 2 -- .../caps_2.10.0.s390x.xml | 2 -- .../caps_2.10.0.x86_64.xml| 2 -- .../caps_2.11.0.s390x.xml | 2 -- .../caps_2.11.0.x86_64.xml| 2 -- .../caps_2.12.0.aarch64.xml | 2 -- .../caps_2.12.0.ppc64.xml | 2 -- .../caps_2.12.0.s390x.xml | 2 -- .../caps_2.12.0.x86_64.xml| 2 -- .../caps_2.4.0.x86_64.xml | 2 -- .../caps_2.5.0.x86_64.xml | 2 -- .../caps_2.6.0.aarch64.xml| 2 -- .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml | 2 -- .../caps_2.6.0.x86_64.xml | 2 -- .../qemucapabilitiesdata/caps_2.7.0.s390x.xml | 2 -- .../caps_2.7.0.x86_64.xml | 2 -- .../qemucapabilitiesdata/caps_2.8.0.s390x.xml | 2 -- .../caps_2.8.0.x86_64.xml | 2 -- .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 2 -- .../qemucapabilitiesdata/caps_2.9.0.s390x.xml | 2 -- .../caps_2.9.0.x86_64.xml | 2 -- .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 -- .../caps_3.0.0.riscv32.xml| 2 -- .../caps_3.0.0.riscv64.xml| 2 -- .../caps_3.0.0.x86_64.xml | 2 -- tests/qemuxml2argvtest.c | 6 ++--- 34 files changed, 12 insertions(+), 99 deletions(-) Any chance this can wait? Would be nice to have other series posted upstream that have changes to .xml and .replies files to add new functionality be processed first. This series does not touch the .replies files. The .xml conflicts only adding new capabilities are trivial to resolve: http://repo.or.cz/tomko-tools.git/blob/HEAD:/rcc Jano There's a series to use - memfd from Marc-Andre Laurent that gets impacted. Yeah - I know - first to merge wins... John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] tests: add qemuxml2argv memfd-memory-numa test
Hi
On Wed, Sep 12, 2018 at 4:01 AM, John Ferlan wrote:
>
>
> On 09/11/2018 04:48 AM, Marc-André Lureau wrote:
>> Hi
>>
>> On Tue, Sep 11, 2018 at 2:57 AM, John Ferlan wrote:
>>>
>>>
>>> On 09/07/2018 07:32 AM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
Check anonymous memory is backed by memfd if qemu is capable.
Signed-off-by: Marc-André Lureau
---
tests/qemuxml2argvdata/memfd-memory-numa.args | 28 +++
tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 +++
tests/qemuxml2argvtest.c | 5 +++
3 files changed, 69 insertions(+)
create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.args
create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml
diff --git a/tests/qemuxml2argvdata/memfd-memory-numa.args
b/tests/qemuxml2argvdata/memfd-memory-numa.args
new file mode 100644
index 00..b26c476196
--- /dev/null
+++ b/tests/qemuxml2argvdata/memfd-memory-numa.args
@@ -0,0 +1,28 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name instance-0092 \
+-S \
+-machine pc-i440fx-wily,accel=kvm,usb=off,dump-guest-core=off \
+-m 14336 \
+-mem-prealloc \
+-smp 20,sockets=1,cores=8,threads=1 \
+-object
memory-backend-memfd,id=ram-node0,hugetlb=yes,hugetlbsize=2097152,share=yes,\
+size=15032385536,host-nodes=3,policy=preferred \
>>>
>>> Another syntax-check error here, needed to move the "share=yes," to the
>>> subsequent line.
>>>
>>
>> ok
>>
+-numa node,nodeid=0,cpus=0-7,memdev=ram-node0 \
+-uuid 126f2720-6f8e-45ab-a886-ec9277079a67 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,\
+path=/tmp/lib/domain--1-instance-0092/monitor.sock,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-usb \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memfd-memory-numa.xml
b/tests/qemuxml2argvdata/memfd-memory-numa.xml
new file mode 100644
index 00..abe93e8c4b
--- /dev/null
+++ b/tests/qemuxml2argvdata/memfd-memory-numa.xml
@@ -0,0 +1,36 @@
+
+instance-0092
+126f2720-6f8e-45ab-a886-ec9277079a67
+14680064
+14680064
+
+
+
+
+
+
+
+
+
+
+
+20
+
+ hvm>> +
+
+
+
+
+
+
+
+
+destroy
+restart
+destroy
+
+ /usr/bin/qemu-system-x86_64
+
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 35df63b2ac..76008a8d07 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2928,6 +2928,11 @@ mymain(void)
DO_TEST("fd-memory-no-numa-topology", QEMU_CAPS_OBJECT_MEMORY_FILE,
QEMU_CAPS_KVM);
+DO_TEST("memfd-memory-numa",
+QEMU_CAPS_OBJECT_MEMORY_MEMFD,
+QEMU_CAPS_OBJECT_MEMORY_MEMFD_HUGETLB,
+QEMU_CAPS_KVM);
+
>>>
>>> Theoretically, if we have 3.1 capabilties to test against, then this
>>> would use a DO_TEST_CAPS_LATEST, while a "pre-3.1" would still be using
>>> -ramfd, right? That is, using DO_TEST_CAPS_VER w/ "3.0.0" would
>>> generate different results.
>>>
>>> I'm conflicted if we should wait for someone to generate the 3.1 caps or
>>> not. For whatever reason, when I post them they're not quite right for
>>> someone else's tastes...
>>>
>>> Let's see if anyone else has strong feelings one way or another.
>>>
>>
>> -memfd is available since 2.12. After patch 1 & 2 are applied, we
>> should probably switch to use DO_TEST_CAPS_LATEST.
>>
>
> hrmph - tried using CAPS_LATEST, and got the error
>
> "CPU topology doesn't match maximum vcpu count"
>
> well *that's* helpful /-|...
>
> The only libvirt test that cares about it currently is
> cpu-hotplug-startup and yes, the maxvcpus matches the cpu topology
> calculation...
>
> So, as long as I change vcpu count from 20 to 8, rename the
> tests/qemuxml2argvdata/memfd-memory-numa.args to
> memfd-memory-numa.x86_64-latest.args, and regenerate the output to:
>
> LC_ALL=C \
> PATH=/bin \
> HOME=/home/test \
> USER=test \
> LOGNAME=test \
> QEMU_AUDIO_DRV=none \
> /usr/bin/qemu-system-x86_64 \
> -name guest=instance-0092,debug-threads=on \
> -S \
> -object secret,id=masterKey0,format=raw,\
> file=/tmp/lib/domain--1-instance-0092/master-key.aes \
> -machine
Re: [libvirt] [PATCH 2/2] conf: Move hypervisor specific nhugepage checks
Hi
On Tue, Sep 11, 2018 at 5:36 PM John Ferlan wrote:
>
> Commit 82327038 moved a couple of checks out of the XML parser
> into the domain validation; however, those checks seem to be more
> useful as hypervisor specific checks rather than the more general
> domain conf checks (nothing in the docs indicate a specific error).
>
> Fortunately only QEMU was processing the memoryBacking, thus
> add the changes to qemuDomainDefValidateMemory and change the
> code a bit to make usage of the similar deref to def->mem and
> the mem->nhugepages filter.
>
> Signed-off-by: John Ferlan
Reviewed-by: Marc-André Lureau
> ---
> src/conf/domain_conf.c | 14 --
> src/qemu/qemu_domain.c | 27 ++-
> 2 files changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7e14cea128..cbc3497c47 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6182,20 +6182,6 @@ virDomainDefMemtuneValidate(const virDomainDef *def)
> if (mem->nhugepages == 0)
> return 0;
That check could be removed now.
>
> -if (mem->allocation == VIR_DOMAIN_MEMORY_ALLOCATION_ONDEMAND) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("hugepages are not allowed with memory "
> - "allocation ondemand"));
> -return -1;
> -}
> -
> -if (mem->source == VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("hugepages are not allowed with anonymous "
> - "memory source"));
> -return -1;
> -}
> -
> for (i = 0; i < mem->nhugepages; i++) {
> size_t j;
> ssize_t nextBit;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5329899b13..30fd21dcdf 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3953,18 +3953,35 @@ static int
> qemuDomainDefValidateMemory(const virDomainDef *def)
> {
> const long system_page_size = virGetSystemPageSizeKB();
> +const virDomainMemtune *mem = &(def->mem);
> +
extra parenthesis?
> +if (mem->nhugepages == 0)
> +return 0;
> +
> +if (mem->allocation == VIR_DOMAIN_MEMORY_ALLOCATION_ONDEMAND) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("hugepages are not allowed with memory "
> + "allocation ondemand"));
> +return -1;
> +}
> +
> +if (mem->source == VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("hugepages are not allowed with anonymous "
> + "memory source"));
> +return -1;
> +}
>
> /* We can't guarantee any other mem.access
> * if no guest NUMA nodes are defined. */
> -if (def->mem.nhugepages != 0 &&
> -def->mem.hugepages[0].size != system_page_size &&
> +if (mem->hugepages[0].size != system_page_size &&
> virDomainNumaGetNodeCount(def->numa) == 0 &&
> -def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT &&
> -def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) {
> +mem->access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT &&
> +mem->access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("memory access mode '%s' not supported "
> "without guest numa node"),
> - virDomainMemoryAccessTypeToString(def->mem.access));
> + virDomainMemoryAccessTypeToString(mem->access));
> return -1;
> }
>
> --
looks good otherwise
Reviewed-by: Marc-André Lureau
> 2.17.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] qemu: Drop some cruft
On 09/12/2018 08:48 AM, Andrea Bolognani wrote: > No longer needed since we bumped our minimum QEMU version > to 1.5.0. > > Andrea Bolognani (3): > qemu: Drop QEMU_CAPS_VNC_WEBSOCKET > qemu: Drop QEMU_CAPS_CHARDEV_SPICEPORT > qemu: Drop redundant version checks > > src/qemu/qemu_capabilities.c | 27 +-- > src/qemu/qemu_capabilities.h | 4 +-- > src/qemu/qemu_command.c | 14 +- > .../caps_1.5.3.x86_64.xml | 2 -- > .../caps_1.6.0.x86_64.xml | 2 -- > .../caps_1.7.0.x86_64.xml | 2 -- > .../caps_2.1.1.x86_64.xml | 2 -- > .../caps_2.10.0.aarch64.xml | 2 -- > .../caps_2.10.0.ppc64.xml | 2 -- > .../caps_2.10.0.s390x.xml | 2 -- > .../caps_2.10.0.x86_64.xml| 2 -- > .../caps_2.11.0.s390x.xml | 2 -- > .../caps_2.11.0.x86_64.xml| 2 -- > .../caps_2.12.0.aarch64.xml | 2 -- > .../caps_2.12.0.ppc64.xml | 2 -- > .../caps_2.12.0.s390x.xml | 2 -- > .../caps_2.12.0.x86_64.xml| 2 -- > .../caps_2.4.0.x86_64.xml | 2 -- > .../caps_2.5.0.x86_64.xml | 2 -- > .../caps_2.6.0.aarch64.xml| 2 -- > .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml | 2 -- > .../caps_2.6.0.x86_64.xml | 2 -- > .../qemucapabilitiesdata/caps_2.7.0.s390x.xml | 2 -- > .../caps_2.7.0.x86_64.xml | 2 -- > .../qemucapabilitiesdata/caps_2.8.0.s390x.xml | 2 -- > .../caps_2.8.0.x86_64.xml | 2 -- > .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 2 -- > .../qemucapabilitiesdata/caps_2.9.0.s390x.xml | 2 -- > .../caps_2.9.0.x86_64.xml | 2 -- > .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 -- > .../caps_3.0.0.riscv32.xml| 2 -- > .../caps_3.0.0.riscv64.xml| 2 -- > .../caps_3.0.0.x86_64.xml | 2 -- > tests/qemuxml2argvtest.c | 6 ++--- > 34 files changed, 12 insertions(+), 99 deletions(-) > Any chance this can wait? Would be nice to have other series posted upstream that have changes to .xml and .replies files to add new functionality be processed first. There's a series to use - memfd from Marc-Andre Laurent that gets impacted. Yeah - I know - first to merge wins... John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] doc: Update the wording around the backingStore
On Tue, Sep 11, 2018 at 5:36 PM John Ferlan wrote: > > Commit bc6d3121a was far too terse when describing the new > elements, attributes, and allow values. Provide a few more > words to help describe. > > Signed-off-by: John Ferlan Reviewed-by: Marc-André Lureau > --- > docs/formatdomain.html.in | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index eb619a1656..1f12ab5b42 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -1150,13 +1150,16 @@ > suitable for the specific environment at the same time to mitigate > the risks described above. Since > 1.0.6 > source > - In this attribute you can switch to file memorybacking or keep > - default anonymous. > + Using the type attribute, it's possible to provide > + "file" to utilize file memorybacking or keep the default > + "anonymous". > access > - Specify if memory is shared or private. This can be overridden per > - numa node by memAccess > + Using the mode attribute, specify if the memory is > + to be "shared" or "private". This can be overridden per numa node by > + memAccess. > allocation > - Specify when allocate the memory > + Using the mode attribute, specify when to allocate > + the memory by supplying either "immediate" or "ondemand". > discard > When set and supported by hypervisor the memory > content is discarded just before guest shuts down (or > -- > 2.17.1 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- Marc-André Lureau -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] qemu: Drop some cruft
On Wed, Sep 12, 2018 at 02:48:51PM +0200, Andrea Bolognani wrote: No longer needed since we bumped our minimum QEMU version to 1.5.0. Andrea Bolognani (3): qemu: Drop QEMU_CAPS_VNC_WEBSOCKET qemu: Drop QEMU_CAPS_CHARDEV_SPICEPORT qemu: Drop redundant version checks Series: Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemu: Drop QEMU_CAPS_CHARDEV_SPICEPORT
On Wed, Sep 12, 2018 at 02:48:53PM +0200, Andrea Bolognani wrote: The capability was introduced in QEMU 1.5.0, which it's our *is minimum supported QEMU version these days. Signed-off-by: Andrea Bolognani --- src/qemu/qemu_capabilities.c | 6 -- src/qemu/qemu_capabilities.h | 2 +- src/qemu/qemu_command.c| 5 - tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 - tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 - tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 - tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 - tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 - tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 - tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 - tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml | 1 - tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml| 1 - tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml| 1 - tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 1 - tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml| 1 - tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 1 - tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 - tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 - tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 - tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 - tests/qemuxml2argvtest.c | 3 +-- 34 files changed, 2 insertions(+), 44 deletions(-) Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH v2 0/2] Enable libosinfo tests that require network connectivity
On Wed, 2018-09-12 at 14:23 +0200, Fabiano Fidêncio wrote: > On Wed, Sep 12, 2018 at 12:29 PM, Andrea Bolognani > wrote: > > Assuming that's the case and that you squash the two patches > > together[1], > > As I don't have commit rights, would you mind squashing the patches efore > pushing them? Squashed, pushed, and changes applied to Jenkins :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] qemu: Avoid duplicate resume events and state changes
The only place where VIR_DOMAIN_EVENT_RESUMED is generated is the RESUME
event handler to make sure we don't generate duplicate events or state
changes. In the worse case the duplicity can revert or cover changes
done by other event handlers.
For example, after QEMU sent RESUME, BLOCK_IO_ERROR, and STOP events
we could happily mark the domain as running and report
VIR_DOMAIN_EVENT_RESUMED to registered clients.
https://bugzilla.redhat.com/show_bug.cgi?id=1612943
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_driver.c| 13 -
src/qemu/qemu_migration.c | 36 ++--
src/qemu/qemu_process.c | 10 --
3 files changed, 10 insertions(+), 49 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5e3f7297e4..49e9fd1233 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1863,7 +1863,6 @@ static int qemuDomainResume(virDomainPtr dom)
virQEMUDriverPtr driver = dom->conn->privateData;
virDomainObjPtr vm;
int ret = -1;
-virObjectEventPtr event = NULL;
int state;
int reason;
virQEMUDriverConfigPtr cfg = NULL;
@@ -1902,9 +1901,6 @@ static int qemuDomainResume(virDomainPtr dom)
"%s", _("resume operation failed"));
goto endjob;
}
-event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_RESUMED,
- VIR_DOMAIN_EVENT_RESUMED_UNPAUSED);
}
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) <
0)
goto endjob;
@@ -1915,7 +1911,6 @@ static int qemuDomainResume(virDomainPtr dom)
cleanup:
virDomainObjEndAPI();
-virObjectEventStateQueue(driver->domainEventState, event);
virObjectUnref(cfg);
return ret;
}
@@ -15978,7 +15973,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
virDomainDefPtr config = NULL;
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
-bool was_running = false;
bool was_stopped = false;
qemuDomainSaveCookiePtr cookie;
virCPUDefPtr origCPU = NULL;
@@ -16169,7 +16163,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
priv = vm->privateData;
if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
/* Transitions 5, 6 */
-was_running = true;
if (qemuProcessStopCPUs(driver, vm,
VIR_DOMAIN_PAUSED_FROM_SNAPSHOT,
QEMU_ASYNC_JOB_START) < 0)
@@ -16266,12 +16259,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
event = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_STARTED,
detail);
-} else if (!was_running) {
-/* Transition 8 */
-detail = VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT;
-event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_RESUMED,
- detail);
}
}
break;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 825a9d399b..4771f26938 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2982,14 +2982,10 @@ qemuMigrationSrcConfirmPhase(virQEMUDriverPtr driver,
virFreeError(orig_err);
if (virDomainObjGetState(vm, ) == VIR_DOMAIN_PAUSED &&
-reason == VIR_DOMAIN_PAUSED_POSTCOPY) {
+reason == VIR_DOMAIN_PAUSED_POSTCOPY)
qemuMigrationAnyPostcopyFailed(driver, vm);
-} else if (qemuMigrationSrcRestoreDomainState(driver, vm)) {
-event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_RESUMED,
-
VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
-virObjectEventStateQueue(driver->domainEventState, event);
-}
+else
+qemuMigrationSrcRestoreDomainState(driver, vm);
qemuMigrationParamsReset(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
priv->job.migParams, priv->job.apiFlags);
@@ -4624,11 +4620,7 @@ qemuMigrationSrcPerformJob(virQEMUDriverPtr driver,
qemuMigrationParamsReset(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
priv->job.migParams, priv->job.apiFlags);
-if (qemuMigrationSrcRestoreDomainState(driver, vm)) {
-event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_RESUMED,
- VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
-}
+qemuMigrationSrcRestoreDomainState(driver, vm);
[libvirt] [PATCH 2/5] qemu: Report more appropriate running reasons
This patch replaces some rather generic VIR_DOMAIN_RUNNING_UNPAUSED
reasons when changing domain state to running with more specific ones.
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index eb9904b7ba..2f3f52e2a5 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3284,7 +3284,7 @@ qemuProcessRecoverMigrationIn(virQEMUDriverPtr driver,
VIR_DEBUG("Incoming migration finished, resuming domain %s",
vm->def->name);
if (qemuProcessStartCPUs(driver, vm,
- VIR_DOMAIN_RUNNING_UNPAUSED,
+ VIR_DOMAIN_RUNNING_MIGRATED,
QEMU_ASYNC_JOB_NONE) < 0) {
VIR_WARN("Could not resume domain %s", vm->def->name);
}
@@ -3391,7 +3391,7 @@ qemuProcessRecoverMigrationOut(virQEMUDriverPtr driver,
(reason == VIR_DOMAIN_PAUSED_MIGRATION ||
reason == VIR_DOMAIN_PAUSED_UNKNOWN)) {
if (qemuProcessStartCPUs(driver, vm,
- VIR_DOMAIN_RUNNING_UNPAUSED,
+ VIR_DOMAIN_RUNNING_MIGRATION_CANCELED,
QEMU_ASYNC_JOB_NONE) < 0) {
VIR_WARN("Could not resume domain %s", vm->def->name);
}
@@ -3449,7 +3449,7 @@ qemuProcessRecoverJob(virQEMUDriverPtr driver,
reason == VIR_DOMAIN_PAUSED_MIGRATION)) ||
reason == VIR_DOMAIN_PAUSED_UNKNOWN)) {
if (qemuProcessStartCPUs(driver, vm,
- VIR_DOMAIN_RUNNING_UNPAUSED,
+ VIR_DOMAIN_RUNNING_SAVE_CANCELED,
QEMU_ASYNC_JOB_NONE) < 0) {
VIR_WARN("Could not resume domain '%s' after migration to
file",
vm->def->name);
--
2.19.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] qemu: Map running reason to resume event detail
Thanks to the previous commit the RESUME event handler knows what reason
should be used when changing the domain state to VIR_DOMAIN_RUNNING, but
the emitted VIR_DOMAIN_EVENT_RESUMED event still uses a generic
VIR_DOMAIN_EVENT_RESUMED_UNPAUSED detail. Luckily, the event detail can
be easily deduced from the running reason, which saves us from having to
pass one more value to the handler.
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_domain.c | 29 +
src/qemu/qemu_domain.h | 3 +++
src/qemu/qemu_process.c | 11 +++
3 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 15325aa4c1..66cf2a1040 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -13438,3 +13438,32 @@ qemuDomainStorageIdReset(qemuDomainObjPrivatePtr priv)
{
priv->nodenameindex = 0;
}
+
+
+virDomainEventResumedDetailType
+qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason)
+{
+switch (reason) {
+case VIR_DOMAIN_RUNNING_RESTORED:
+case VIR_DOMAIN_RUNNING_FROM_SNAPSHOT:
+return VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT;
+
+case VIR_DOMAIN_RUNNING_MIGRATED:
+case VIR_DOMAIN_RUNNING_MIGRATION_CANCELED:
+return VIR_DOMAIN_EVENT_RESUMED_MIGRATED;
+
+case VIR_DOMAIN_RUNNING_POSTCOPY:
+return VIR_DOMAIN_EVENT_RESUMED_POSTCOPY;
+
+case VIR_DOMAIN_RUNNING_UNKNOWN:
+case VIR_DOMAIN_RUNNING_SAVE_CANCELED:
+case VIR_DOMAIN_RUNNING_BOOTED:
+case VIR_DOMAIN_RUNNING_UNPAUSED:
+case VIR_DOMAIN_RUNNING_WAKEUP:
+case VIR_DOMAIN_RUNNING_CRASHED:
+case VIR_DOMAIN_RUNNING_LAST:
+break;
+}
+
+return VIR_DOMAIN_EVENT_RESUMED_UNPAUSED;
+}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 3f3f7ccf18..d40998e293 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1079,4 +1079,7 @@ char *
qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivatePtr priv);
unsigned int qemuDomainStorageIdNew(qemuDomainObjPrivatePtr priv);
void qemuDomainStorageIdReset(qemuDomainObjPrivatePtr priv);
+virDomainEventResumedDetailType
+qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason);
+
#endif /* __QEMU_DOMAIN_H__ */
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 40ffb23dbb..6be3d9e4da 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -714,6 +714,7 @@ qemuProcessHandleResume(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
qemuDomainObjPrivatePtr priv;
virDomainRunningReason reason = VIR_DOMAIN_RUNNING_UNPAUSED;
+virDomainEventResumedDetailType eventDetail;
virObjectLock(vm);
@@ -729,14 +730,16 @@ qemuProcessHandleResume(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
goto unlock;
}
+eventDetail = qemuDomainRunningReasonToResumeEvent(reason);
VIR_DEBUG("Transitioned guest %s out of paused into resumed state, "
- "reason '%s'",
- vm->def->name, virDomainRunningReasonTypeToString(reason));
+ "reason '%s', event detail %d",
+ vm->def->name, virDomainRunningReasonTypeToString(reason),
+ eventDetail);
virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason);
event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_RESUMED,
- VIR_DOMAIN_EVENT_RESUMED_UNPAUSED);
+ VIR_DOMAIN_EVENT_RESUMED,
+ eventDetail);
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
driver->caps) < 0) {
VIR_WARN("Unable to save status on vm %s after state change",
--
2.19.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/5] qemu: Change the way we generate VIR_DOMAIN_EVENT_RESUMED
https://bugzilla.redhat.com/show_bug.cgi?id=1612943 Jiri Denemark (5): qemu: Properly report VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT qemu: Report more appropriate running reasons qemu: Pass running reason to RESUME event handler qemu: Map running reason to resume event detail qemu: Avoid duplicate resume events and state changes src/qemu/qemu_domain.c| 29 src/qemu/qemu_domain.h| 7 ++ src/qemu/qemu_driver.c| 13 --- src/qemu/qemu_migration.c | 36 +- src/qemu/qemu_process.c | 46 --- 5 files changed, 71 insertions(+), 60 deletions(-) -- 2.19.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/5] qemu: Pass running reason to RESUME event handler
Whenever we get the RESUME event from QEMU, we change the state of the
affected domain to VIR_DOMAIN_RUNNING with VIR_DOMAIN_RUNNING_UNPAUSED
reason. This is fine if the domain is resumed unexpectedly, but when we
sent "cont" to QEMU we usually have a better reason for the state
change. The better reason is used in qemuProcessStartCPUs which also
sets the domain state to running if qemuMonitorStartCPUs reports
success. Thus we may end up with two state updates in a row, but the
final reason is correct.
This patch is a preparation for dropping the state change done in
qemuMonitorStartCPUs for which we need to pass the actual running reason
to the RESUME event handler and use it there instead of
VIR_DOMAIN_RUNNING_UNPAUSED.
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_domain.h | 4
src/qemu/qemu_process.c | 23 +--
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 914c9a6a8d..3f3f7ccf18 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -366,6 +366,10 @@ struct _qemuDomainObjPrivate {
/* counter for generating node names for qemu disks */
unsigned long long nodenameindex;
+
+/* qemuProcessStartCPUs stores the reason for starting vCPUs here for the
+ * RESUME event handler to use it */
+virDomainRunningReason runningReason;
};
# define QEMU_DOMAIN_PRIVATE(vm) \
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 2f3f52e2a5..40ffb23dbb 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -712,21 +712,28 @@ qemuProcessHandleResume(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
virQEMUDriverPtr driver = opaque;
virObjectEventPtr event = NULL;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+qemuDomainObjPrivatePtr priv;
+virDomainRunningReason reason = VIR_DOMAIN_RUNNING_UNPAUSED;
virObjectLock(vm);
-if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
-qemuDomainObjPrivatePtr priv = vm->privateData;
+priv = vm->privateData;
+if (priv->runningReason != VIR_DOMAIN_RUNNING_UNKNOWN) {
+reason = priv->runningReason;
+priv->runningReason = VIR_DOMAIN_RUNNING_UNKNOWN;
+}
+
+if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
if (priv->gotShutdown) {
VIR_DEBUG("Ignoring RESUME event after SHUTDOWN");
goto unlock;
}
-VIR_DEBUG("Transitioned guest %s out of paused into resumed state",
- vm->def->name);
+VIR_DEBUG("Transitioned guest %s out of paused into resumed state, "
+ "reason '%s'",
+ vm->def->name, virDomainRunningReasonTypeToString(reason));
-virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
- VIR_DOMAIN_RUNNING_UNPAUSED);
+virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason);
event = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_RESUMED,
VIR_DOMAIN_EVENT_RESUMED_UNPAUSED);
@@ -3088,6 +3095,8 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver,
virDomainObjPtr vm,
}
VIR_FREE(priv->lockState);
+priv->runningReason = reason;
+
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto release;
@@ -3105,6 +3114,7 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver,
virDomainObjPtr vm,
return ret;
release:
+priv->runningReason = VIR_DOMAIN_RUNNING_UNKNOWN;
if (virDomainLockProcessPause(driver->lockManager, vm, >lockState) <
0)
VIR_WARN("Unable to release lease on %s", vm->def->name);
VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState));
@@ -5982,6 +5992,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
priv->monError = false;
priv->monStart = 0;
priv->gotShutdown = false;
+priv->runningReason = VIR_DOMAIN_RUNNING_UNKNOWN;
VIR_DEBUG("Updating guest CPU definition");
if (qemuProcessUpdateGuestCPU(vm->def, priv->qemuCaps, caps, flags) < 0)
--
2.19.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] qemu: Properly report VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT
VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT was defined but not used anywhere
in our event generation code. This fixes qemuDomainRevertToSnapshot to
properly report why the domain was resumed.
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2f8d6915e1..5e3f7297e4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16268,7 +16268,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
detail);
} else if (!was_running) {
/* Transition 8 */
-detail = VIR_DOMAIN_EVENT_RESUMED;
+detail = VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT;
event = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_RESUMED,
detail);
--
2.19.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 0/4] block/rbd: enable filename parsing on open
On Tue, Sep 11, 2018 at 06:32:29PM -0400, Jeff Cody wrote:
> Changes from v3:
>
>
> Patch 4: Typo fixed [Eric]
> Added examples [Eric]
>
> Changes from v2:
> =
>
> Patch 4: New, document deprecation. [Eric]
> Patch 3,2: Add r-b's
>
>
> Changes from v1:
> =
>
> Patch 1: Don't pass unused BlockDriverState to helper function
>
> Patch 2: Do not allow mixed usage; fail if keyvalue is present [Eric]
> Add deprecation warning [John]
> Pull legacy parsing code into function [John]
> Fixed filename leak
>
> Patch 3: New; iotest 231. [Eric]
>
>
> iotest failure on current master:
>
> QA output created by 231
> -qemu-img: RBD options encoded in the filename as keyvalue pairs is
> deprecated. Future versions may cease to parse these options in the future.
> -unable to get monitor info from DNS SRV with service name: ceph-mon
> -no monitors specified to connect to.
> -qemu-img: Could not open
> 'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
> error connecting: No such file or directory
> +qemu-img: Could not open
> 'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
> Parameter 'pool' is missing
> unable to get monitor info from DNS SRV with service name: ceph-mon
> no monitors specified to connect to.
> qemu-img: Could not open
> 'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
> error connecting: No such file or directory
> Failures: 231
> Failed 1 of 1 tests
>
> Jeff Cody (4):
> block/rbd: pull out qemu_rbd_convert_options
> block/rbd: Attempt to parse legacy filenames
> block/rbd: add iotest for rbd legacy keyvalue filename parsing
> block/rbd: add deprecation documentation for filename keyvalue pairs
>
> block/rbd.c| 89 --
> qemu-deprecated.texi | 15 +++
> tests/qemu-iotests/231 | 62 ++
> tests/qemu-iotests/231.out | 9
> tests/qemu-iotests/group | 1 +
> 5 files changed, 162 insertions(+), 14 deletions(-)
> create mode 100755 tests/qemu-iotests/231
> create mode 100644 tests/qemu-iotests/231.out
>
> --
> 2.17.1
>
Thanks,
Applied to my block branch:
git://github.com/codyprime/qemu-kvm-jtc block
-Jeff
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] qemu: Drop redundant version checks
We require QEMU 1.5.0 these days, so checking for versions
older than that is pointless.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_capabilities.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 499befb2cf..96a65046ba 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1724,10 +1724,10 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
*
*ref405ep: no pci
* taihu: no pci
- * bamboo: 1.1.0
+ * bamboo: 1.1.0 (<= 1.5.0, so basically forever)
* mac99: 2.0.0
* g3beige: 2.0.0
- *prep: 1.4.0
+ *prep: 1.4.0 (<= 1.5.0, so basically forever)
* pseries: 2.0.0
* mpc8544ds: forever
* virtex-m507: no pci
@@ -1749,16 +1749,11 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
STREQ(def->os.machine, "ppce500"))
return true;
-if (qemuCaps->version >= 1004000 &&
-STREQ(def->os.machine, "prep"))
-return true;
-
-if (qemuCaps->version >= 1001000 &&
-STREQ(def->os.machine, "bamboo"))
-return true;
-
-if (STREQ(def->os.machine, "mpc8544ds"))
+if (STREQ(def->os.machine, "bamboo") ||
+STREQ(def->os.machine, "mpc8544ds") ||
+STREQ(def->os.machine, "prep")) {
return true;
+}
return false;
}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] qemu: Drop QEMU_CAPS_VNC_WEBSOCKET
The capability was introduced in QEMU 1.3.1 and we require
QEMU 1.5.0 these days.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_capabilities.c | 4
src/qemu/qemu_capabilities.h | 2 +-
src/qemu/qemu_command.c| 9 +
tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 -
tests/qemuxml2argvtest.c | 3 ++-
34 files changed, 4 insertions(+), 44 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 08cf822b88..e0c847f00a 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4146,10 +4146,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
virQEMUCapsInitQMPBasicArch(qemuCaps);
-/* WebSockets were introduced between 1.3.0 and 1.3.1 */
-if (qemuCaps->version >= 1003001)
-virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_WEBSOCKET);
-
/* -chardev spiceport is supported from 1.4.0, but usable through
* qapi only since 1.5.0, however, it still cannot be queried
* for as a capability */
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 0f69c69136..fad223eab7 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -253,7 +253,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
/* 145 */
X_QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX, /* -device
scsi-generic.bootindex */
QEMU_CAPS_MEM_MERGE, /* -machine mem-merge */
-QEMU_CAPS_VNC_WEBSOCKET, /* -vnc x:y,websocket */
+X_QEMU_CAPS_VNC_WEBSOCKET, /* -vnc x:y,websocket */
QEMU_CAPS_DRIVE_DISCARD, /* -drive discard=off(ignore)|on(unmap) */
QEMU_CAPS_REALTIME_MLOCK, /* -realtime mlock=on|off */
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ff9589f593..95d79235a4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7843,15 +7843,8 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr
cfg,
virBufferAsprintf(, ":%d",
graphics->data.vnc.port - 5900);
-if (graphics->data.vnc.websocket) {
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_WEBSOCKET)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("VNC WebSockets are not supported "
- "with this QEMU binary"));
-goto error;
-}
+if (graphics->data.vnc.websocket)
virBufferAsprintf(, ",websocket=%d",
graphics->data.vnc.websocket);
-}
break;
case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
diff --git a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
index b791b13830..34239f1ecc 100644
--- a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
@@ -64,7 +64,6 @@
-
diff --git a/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
index 3a0df4d417..17b430878e 100644
--- a/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
@@ -65,7 +65,6 @@
-
diff --git
[libvirt] [PATCH 2/3] qemu: Drop QEMU_CAPS_CHARDEV_SPICEPORT
The capability was introduced in QEMU 1.5.0, which it's our
minimum supported QEMU version these days.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_capabilities.c | 6 --
src/qemu/qemu_capabilities.h | 2 +-
src/qemu/qemu_command.c| 5 -
tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 -
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 1 -
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 -
tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 -
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 -
tests/qemuxml2argvtest.c | 3 +--
34 files changed, 2 insertions(+), 44 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e0c847f00a..499befb2cf 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4146,12 +4146,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
virQEMUCapsInitQMPBasicArch(qemuCaps);
-/* -chardev spiceport is supported from 1.4.0, but usable through
- * qapi only since 1.5.0, however, it still cannot be queried
- * for as a capability */
-if (qemuCaps->version >= 1005000)
-virQEMUCapsSet(qemuCaps, QEMU_CAPS_CHARDEV_SPICEPORT);
-
if (qemuCaps->version >= 1006000)
virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index fad223eab7..a0134493aa 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -276,7 +276,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_DEVICE_PANIC, /* -device pvpanic */
QEMU_CAPS_ENABLE_FIPS, /* -enable-fips */
QEMU_CAPS_SPICE_FILE_XFER_DISABLE, /* -spice disable-agent-file-xfer */
-QEMU_CAPS_CHARDEV_SPICEPORT, /* -chardev spiceport */
+X_QEMU_CAPS_CHARDEV_SPICEPORT, /* -chardev spiceport */
/* 165 */
QEMU_CAPS_DEVICE_USB_KBD, /* -device usb-kbd */
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 95d79235a4..0a353f87ba 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5224,11 +5224,6 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
break;
case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_SPICEPORT)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("spiceport not supported in this QEMU binary"));
-goto cleanup;
-}
virBufferAsprintf(, "spiceport,id=%s,name=%s", charAlias,
dev->data.spiceport.channel);
break;
diff --git a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
index 34239f1ecc..bff3d7aab5 100644
--- a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
@@ -74,7 +74,6 @@
-
diff --git a/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
index 17b430878e..65982d8d74 100644
--- a/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
@@ -78,7 +78,6 @@
-
diff --git a/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml
index 8d0bad6225..003eafb5e0 100644
--- a/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml
[libvirt] [PATCH 0/3] qemu: Drop some cruft
No longer needed since we bumped our minimum QEMU version to 1.5.0. Andrea Bolognani (3): qemu: Drop QEMU_CAPS_VNC_WEBSOCKET qemu: Drop QEMU_CAPS_CHARDEV_SPICEPORT qemu: Drop redundant version checks src/qemu/qemu_capabilities.c | 27 +-- src/qemu/qemu_capabilities.h | 4 +-- src/qemu/qemu_command.c | 14 +- .../caps_1.5.3.x86_64.xml | 2 -- .../caps_1.6.0.x86_64.xml | 2 -- .../caps_1.7.0.x86_64.xml | 2 -- .../caps_2.1.1.x86_64.xml | 2 -- .../caps_2.10.0.aarch64.xml | 2 -- .../caps_2.10.0.ppc64.xml | 2 -- .../caps_2.10.0.s390x.xml | 2 -- .../caps_2.10.0.x86_64.xml| 2 -- .../caps_2.11.0.s390x.xml | 2 -- .../caps_2.11.0.x86_64.xml| 2 -- .../caps_2.12.0.aarch64.xml | 2 -- .../caps_2.12.0.ppc64.xml | 2 -- .../caps_2.12.0.s390x.xml | 2 -- .../caps_2.12.0.x86_64.xml| 2 -- .../caps_2.4.0.x86_64.xml | 2 -- .../caps_2.5.0.x86_64.xml | 2 -- .../caps_2.6.0.aarch64.xml| 2 -- .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml | 2 -- .../caps_2.6.0.x86_64.xml | 2 -- .../qemucapabilitiesdata/caps_2.7.0.s390x.xml | 2 -- .../caps_2.7.0.x86_64.xml | 2 -- .../qemucapabilitiesdata/caps_2.8.0.s390x.xml | 2 -- .../caps_2.8.0.x86_64.xml | 2 -- .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 2 -- .../qemucapabilitiesdata/caps_2.9.0.s390x.xml | 2 -- .../caps_2.9.0.x86_64.xml | 2 -- .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 -- .../caps_3.0.0.riscv32.xml| 2 -- .../caps_3.0.0.riscv64.xml| 2 -- .../caps_3.0.0.x86_64.xml | 2 -- tests/qemuxml2argvtest.c | 6 ++--- 34 files changed, 12 insertions(+), 99 deletions(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH v2 0/2] Enable libosinfo tests that require network connectivity
On Wed, Sep 12, 2018 at 12:29 PM, Andrea Bolognani
wrote:
> On Tue, 2018-09-11 at 09:36 +0200, Fabiano Fidêncio wrote:
> > Let's enable both media and tree uris tests in order to early catch URL
> > changes that need to be fixed in osinfo-db.
> >
> > Fabiano Fidêncio (2):
> > guests: Enable {media,tree}uris tests for libosinfo
> > projects: Enable {media,tree}uri tests for libosinfo
> >
> > guests/playbooks/build/projects/libosinfo.yml | 4
> > projects/libosinfo.yaml | 3 +++
> > 2 files changed, 7 insertions(+)
>
> So I guess libosinfo has been fixed in the meantime?
>
In the end there was no libosinfo issue but a Fedora mirroring issue in our
infra.
And, yes, it's solved now.
>
> Assuming that's the case and that you squash the two patches
> together[1],
>
As I don't have commit rights, would you mind squashing the patches efore
pushing them?
>
> Reviewed-by: Andrea Bolognani
>
>
> [1] We've been mostly doing that since introducing the build
> playbooks, as changing both Jenkins and Ansible at the same
> time makes it obvious that they're being kept in sync.
> --
> Andrea Bolognani / Red Hat / Virtualization
>
>
Best Regards,
--
Fabiano Fidêncio
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] src: fix incorrect indentation or empty for first line in function body
It would be nice not to mix line removals with re-indentation. Jano On Wed, Sep 12, 2018 at 11:58:21AM +0800, Shi Lei wrote: Signed-off-by: Shi Lei --- src/bhyve/bhyve_command.c | 3 - src/bhyve/bhyve_conf.c | 6 +- src/bhyve/bhyve_driver.c | 33 +-- src/bhyve/bhyve_monitor.c | 1 - src/bhyve/bhyve_process.c | 1 - src/conf/cpu_conf.c| 1 - src/conf/domain_capabilities.c | 1 - src/conf/domain_conf.c | 12 +--- src/conf/network_conf.c| 2 - src/cpu/cpu_s390.c | 82 +- src/cpu/cpu_x86.c | 1 - src/esx/esx_vi_types.c | 2 +- src/locking/lock_driver_nop.c | 1 - src/network/bridge_driver.c| 1 - src/network/bridge_driver_linux.c | 1 - src/nwfilter/nwfilter_learnipaddr.c| 1 - src/phyp/phyp_driver.c | 1 - src/qemu/qemu_alias.c | 1 - src/qemu/qemu_domain_address.c | 1 - src/qemu/qemu_driver.c | 1 - src/qemu/qemu_monitor.c| 1 - src/qemu/qemu_monitor_json.c | 1 - src/qemu/qemu_process.c| 1 - src/storage/storage_backend_sheepdog.c | 3 - src/storage/storage_driver.c | 1 - src/storage/storage_util.c | 1 - src/test/test_driver.c | 2 - src/util/vircommand.c | 6 +- src/util/virdbuspriv.h | 18 +++--- src/util/virdnsmasq.c | 1 - src/util/virfile.c | 1 - src/util/virnetdev.c | 55 - src/util/virnetdevmacvlan.c| 11 ++-- src/util/virnetdevvportprofile.c | 2 +- src/util/virstoragefile.c | 1 - src/util/virutil.c | 3 +- src/vbox/vbox_storage.c| 1 - src/vz/vz_driver.c | 1 - src/vz/vz_sdk.c| 1 - src/xenapi/xenapi_driver.c | 2 - src/xenconfig/xen_common.c | 9 ++- 41 files changed, 111 insertions(+), 164 deletions(-) signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] tools: fix incorrect indentation or empty for first line in function body
This commit only fixes empty lines, why mention incorrect indentation? Jano On Wed, Sep 12, 2018 at 11:58:23AM +0800, Shi Lei wrote: Signed-off-by: Shi Lei --- tools/virsh-volume.c | 1 - tools/virt-admin.c | 1 - 2 files changed, 2 deletions(-) signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] cfg.mk: Introduce syntax-check rule for incorrect indentation or empty for first line in function body
On Wed, Sep 12, 2018 at 11:58:20AM +0800, Shi Lei wrote:
Signed-off-by: Shi Lei
---
cfg.mk | 11 +++
1 file changed, 11 insertions(+)
diff --git a/cfg.mk b/cfg.mk
index 609ae86..a43cb1c 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -702,6 +702,17 @@ sc_preprocessor_indentation:
echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
fi
+# Use 4 spaces rather than TAB for indentation for those top-level
+# function bodies. Doesn't catch all cases, but it helps.
+sc_indentation_for_function_body:
+ @for f in $$($(VC_LIST_EXCEPT) | grep -E '\.[ch](\.in)?$$'); do \
Doing it this way is very inefficient. This spawns 5 greps and one sed
for each .c file. It might be bearable for spec files, when we only have
two, but we have nearly a thousand .c and .h files
I spent quite some time on making syntax-check as fast as possible, to
encourage people to run it more often.
Adding this to build-aux/check-spacing.pl would let you take advantage
of the existing code for opening the files and get rid of all the line
number regexes, making the check more readable.
Also, it would let you split the error message and only output the
relevant one. Grouping indentation and empty lines in one error message
is not very friendly.
+ grep -n -A1 '^{$$' $$f | grep '^[0-9]\{1,\}-' \
+ | grep -v '^[0-9]\{1,\}-[ ]\{4\}\S' |
grep -v '^[0-9]\{1,\}-\([#}/]\|.*:\)' \
There is no need to exempt '/', the following is also wrong:
src/qemu/qemu_domain_address.c:2303:/* Try to find a nice default for busNr for
a new pci-expander-bus.
Jano
+ | $(SED) -ne "s#\(^[0-9]\{1,\}\)-#$$f:\1:#gp" | grep . && \
+ { echo '$(ME): incorrect indentation or empty for first line in function
body' 1>&2; \
+ exit 1; } \
+ done || :
+
# Enforce similar spec file indentation style, by running cppi on a
# (comment-only) C file that mirrors the same layout as the spec file.
sc_spec_indentation:
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
在 2018/9/12 下午6:37, Andrea Bolognani 写道: On Wed, 2018-09-12 at 16:34 +0800, Yi Min Zhao wrote: 在 2018/9/12 下午3:35, Yi Min Zhao 写道: This makes sense and seems to work just fine; however, you are allocating and releasing a bunch of small integers, which seems a bit wasteful. vircgroup is AFAICT avoiding all that extra memory management by stuffing the values straight into the pointers themselves, which you should also be able to do since the biggest legal ID is a 32-bit integer. That said, I haven't been able to get that to actually work, at least with a quick attempt :( Would you mind exploring that route and figuring out whether it's feasible at all? I'm testing this. Actually I wanted to do so like vircgroup. I remembered there's error due to the previous code logic. I will reply to you later. I remebered the reason and test again. FID might be 0. It is treated as an error if we save 0 in void* pointer. Right. Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue... Yes. Just one value makes all things complex. I guess forbidding users from using UINT32_MAX as a fid is not an option, right? Actually as my understanding, it's just a value to identify the pci function. IMO, it's not a big deal to decrease usable FID values. After all, UID set is smaller than FID set. The maximum number of pci devices is limited by UID. Anyway, I have to discuss this with my colleagues internally. I will tell you our discussion result first time. -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 00/23] Introduce metadata locking
Michal Privoznik [2018-09-12, 11:32AM +0200]: > On 09/12/2018 07:19 AM, Bjoern Walk wrote: > > Michal Privoznik [2018-09-10, 11:36AM +0200]: > >> Technically, this is v4 of: > >> > >> https://www.redhat.com/archives/libvir-list/2018-August/msg01627.html > >> > >> However, this is implementing different approach than any of the > >> previous versions. > >> > >> One of the problems with previous version was that it was too > >> complicated. The main reason for that was that we could not close the > >> connection whilst there was a file locked. So we had to invent a > >> mechanism that would prevent that (on the client side). > >> > >> These patches implement different approach. They rely on secdriver's > >> transactions which bring all the paths we want to label into one place > >> so that they can be relabelled within different namespace. > >> I'm extending this idea so that transactions run all the time > >> (regardless of domain namespacing) and only at the very last moment is > >> decided which namespace would the relabeling run in. > >> > >> Metadata locking is then as easy as putting lock/unlock calls around one > >> function. > >> > >> You can find the patches at my github too: > >> > >> https://github.com/zippy2/libvirt/tree/disk_metadata_lock_v4_alt > > > > Hey Michal, > > > > is was running a quick test with this patch series with two domains > > sharing a disk image without and SELinux enabled. When > > starting the second domain, the whole libvirtd daemon hangs for almost a > > minute until giving the error that the image is locked. I haven't > > debugged it yet to figure out what happens. > > Is this on two different hosts or one? On the same host. > I'm unable to reproduce, so can you please attach debugger and share 't > a a bt' output with me? (gdb) thread apply all bt Thread 17 (Thread 0x3ff48dff910 (LWP 193353)): #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x03ff6678c5a8 in udevEventHandleThread (opaque=) at node_device/node_device_udev.c:1603 #3 0x03ff8c6bad16 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 16 (Thread 0x3ff4acfe910 (LWP 193312)): #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x03ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x03ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 15 (Thread 0x3ff4b4ff910 (LWP 193311)): #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x03ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x03ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 14 (Thread 0x3ff64efd910 (LWP 193310)): #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x03ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x03ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 13 (Thread 0x3ff656fe910 (LWP 193309)): #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x03ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x03ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 12 (Thread 0x3ff65eff910 (LWP 193308)): #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x03ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x03ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x03ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x03ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 11 (Thread 0x3ff67fff910 (LWP 193307)): #0 0x03ff8b70d7b8 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x03ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x03ff8c6bbba0 in virThreadPoolWorker () from
Re: [libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC
On Wed, Sep 12, 2018 at 10:57:32AM +0200, Fabiano Fidêncio wrote: > virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup() and > virLXCCgroupSetupCpuTune() and qemuSetupCpuCgroup() are the most similar > functions between QEMU and LXC code. > > Let's move their common code to virCgroup. > > Mind that the first two patches are basically preparing the ground for > the changes introduced in the last two patches. Hi, definitely good idea to remove code duplication! We have similar issue with the virDomainSetBlkioParameters for QEMU and LXC drivers. The code to set cgroup values is the same. Since the common object is domain how about introducing virDomainSetupBlkioTune and virDomainSetupMemTune and move it into domain_conf.c, that way we don't have to extract domain specific data into src/util. Another benefit is that it will not cause me merge conflicts because I'm rewriting cgroup code and adding support for cgroup v2. Pavel signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
On Wed, 2018-09-12 at 16:34 +0800, Yi Min Zhao wrote: > 在 2018/9/12 下午3:35, Yi Min Zhao 写道: > > > This makes sense and seems to work just fine; however, you are > > > allocating and releasing a bunch of small integers, which seems > > > a bit wasteful. > > > > > > vircgroup is AFAICT avoiding all that extra memory management by > > > stuffing the values straight into the pointers themselves, which > > > you should also be able to do since the biggest legal ID is a > > > 32-bit integer. > > > > > > That said, I haven't been able to get that to actually work, at > > > least with a quick attempt :( Would you mind exploring that route > > > and figuring out whether it's feasible at all? > > > > I'm testing this. Actually I wanted to do so like vircgroup. I > > remembered there's > > error due to the previous code logic. I will reply to you later. > > I remebered the reason and test again. FID might be 0. It is treated as > an error > if we save 0 in void* pointer. Right. Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue... I guess forbidding users from using UINT32_MAX as a fid is not an option, right? -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH v2 0/2] Enable libosinfo tests that require network connectivity
On Tue, 2018-09-11 at 09:36 +0200, Fabiano Fidêncio wrote:
> Let's enable both media and tree uris tests in order to early catch URL
> changes that need to be fixed in osinfo-db.
>
> Fabiano Fidêncio (2):
> guests: Enable {media,tree}uris tests for libosinfo
> projects: Enable {media,tree}uri tests for libosinfo
>
> guests/playbooks/build/projects/libosinfo.yml | 4
> projects/libosinfo.yaml | 3 +++
> 2 files changed, 7 insertions(+)
So I guess libosinfo has been fixed in the meantime?
Assuming that's the case and that you squash the two patches
together[1],
Reviewed-by: Andrea Bolognani
[1] We've been mostly doing that since introducing the build
playbooks, as changing both Jenkins and Ansible at the same
time makes it obvious that they're being kept in sync.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 3/5] util: file: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE
Signed-off-by: Shi Lei
---
src/util/virfile.c | 21 +++--
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 01ebdb6..2366c11 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1969,29 +1969,22 @@ int
virFileIsCDROM(const char *path)
{
struct stat st;
-int fd;
-int ret = -1;
+VIR_AUTOCLOSE fd = -1;
if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0)
-goto cleanup;
+return -1;
if (fstat(fd, ) < 0)
-goto cleanup;
+return -1;
-if (!S_ISBLK(st.st_mode)) {
-ret = 0;
-goto cleanup;
-}
+if (!S_ISBLK(st.st_mode))
+return 0;
/* Attempt to detect via a CDROM specific ioctl */
if (ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) >= 0)
-ret = 1;
-else
-ret = 0;
+return 1;
- cleanup:
-VIR_FORCE_CLOSE(fd);
-return ret;
+return 0;
}
#else
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 2/5] cfg.mk: change syntax-check rule for VIR_AUTOCLOSE variable initialization
Signed-off-by: Shi Lei --- cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index 609ae86..eddd110 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1063,7 +1063,7 @@ sc_prohibit_backslash_alignment: # Rule to ensure that varibales declared using a cleanup macro are # always initialized. sc_require_attribute_cleanup_initialization: - @prohibit='VIR_AUTO(FREE|PTR)\(.+\) *[^=]+;' \ + @prohibit='VIR_AUTO((FREE|PTR)\(.+\)|CLOSE) *[^=]+;' \ in_vc_files='\.[chx]$$' \ halt='variable declared with a cleanup macro must be initialized' \ $(_sc_search_regexp) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 4/5] util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE
Signed-off-by: Shi Lei
---
src/util/virnetdevbridge.c | 120 -
1 file changed, 37 insertions(+), 83 deletions(-)
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index ed2db27..e058898 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -81,9 +81,8 @@ static int virNetDevBridgeCmd(const char *brname,
void *arg,
size_t argsize)
{
-int s;
-int ret = -1;
struct ifdrv ifd;
+VIR_AUTOCLOSE s = -1;
memset(, 0, sizeof(ifd));
@@ -97,19 +96,14 @@ static int virNetDevBridgeCmd(const char *brname,
virReportSystemError(ERANGE,
_("Network interface name '%s' is too long"),
brname);
- goto cleanup;
+ return -1;
}
ifd.ifd_cmd = op;
ifd.ifd_len = argsize;
ifd.ifd_data = arg;
-ret = ioctl(s, SIOCSDRVSPEC, );
-
- cleanup:
-VIR_FORCE_CLOSE(s);
-
-return ret;
+return ioctl(s, SIOCSDRVSPEC, );
}
#endif
@@ -167,10 +161,9 @@ static int virNetDevBridgeGet(const char *brname,
const char *paramname, /* sysfs param name */
unsigned long *value) /* current value */
{
-int ret = -1;
-int fd = -1;
struct ifreq ifr;
VIR_AUTOFREE(char *) path = NULL;
+VIR_AUTOCLOSE fd = -1;
if (virAsprintf(, SYSFS_NET_DIR "%s/bridge/%s", brname, paramname) <
0)
return -1;
@@ -180,26 +173,26 @@ static int virNetDevBridgeGet(const char *brname,
if (virFileReadAll(path, INT_BUFSIZE_BOUND(unsigned long),
) < 0)
-goto cleanup;
+return -1;
if (virStrToLong_ul(valuestr, NULL, 10, value) < 0) {
virReportSystemError(EINVAL,
_("Unable to get bridge %s %s"),
brname, paramname);
-goto cleanup;
+return -1;
}
} else {
struct __bridge_info info;
unsigned long args[] = { BRCTL_GET_BRIDGE_INFO, (unsigned long),
0, 0 };
if ((fd = virNetDevSetupControl(brname, )) < 0)
-goto cleanup;
+return -1;
ifr.ifr_data = (char*)
if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
virReportSystemError(errno,
_("Unable to get bridge %s %s"), brname,
paramname);
-goto cleanup;
+return -1;
}
if (STREQ(paramname, "stp_state")) {
@@ -209,14 +202,11 @@ static int virNetDevBridgeGet(const char *brname,
} else {
virReportSystemError(EINVAL,
_("Unable to get bridge %s %s"), brname,
paramname);
-goto cleanup;
+return -1;
}
}
-ret = 0;
- cleanup:
-VIR_FORCE_CLOSE(fd);
-return ret;
+return 0;
}
#endif /* __linux__ */
@@ -391,8 +381,7 @@ virNetDevBridgePortSetUnicastFlood(const char *brname
ATTRIBUTE_UNUSED,
static int
virNetDevBridgeCreateWithIoctl(const char *brname)
{
-int fd = -1;
-int ret = -1;
+VIR_AUTOCLOSE fd = -1;
if ((fd = virNetDevSetupControl(NULL, NULL)) < 0)
return -1;
@@ -400,14 +389,10 @@ virNetDevBridgeCreateWithIoctl(const char *brname)
if (ioctl(fd, SIOCBRADDBR, brname) < 0) {
virReportSystemError(errno,
_("Unable to create bridge %s"), brname);
-goto cleanup;
+return -1;
}
-ret = 0;
-
- cleanup:
-VIR_FORCE_CLOSE(fd);
-return ret;
+return 0;
}
#endif
@@ -448,9 +433,8 @@ virNetDevBridgeCreate(const char *brname)
int
virNetDevBridgeCreate(const char *brname)
{
-int s;
struct ifreq ifr;
-int ret = - 1;
+VIR_AUTOCLOSE s = -1;
if ((s = virNetDevSetupControl("bridge", )) < 0)
return -1;
@@ -458,16 +442,13 @@ virNetDevBridgeCreate(const char *brname)
if (ioctl(s, SIOCIFCREATE2, ) < 0) {
virReportSystemError(errno, "%s",
_("Unable to create bridge device"));
-goto cleanup;
+return -1;
}
if (virNetDevSetName(ifr.ifr_name, brname) == -1)
-goto cleanup;
+return -1;
-ret = 0;
- cleanup:
-VIR_FORCE_CLOSE(s);
-return ret;
+return 0;
}
#else
int virNetDevBridgeCreate(const char *brname)
@@ -490,8 +471,7 @@ int virNetDevBridgeCreate(const char *brname)
static int
virNetDevBridgeDeleteWithIoctl(const char *brname)
{
-int fd = -1;
-int ret = -1;
+VIR_AUTOCLOSE fd = -1;
ignore_value(virNetDevSetOnline(brname, false));
@@ -501,14 +481,10 @@ virNetDevBridgeDeleteWithIoctl(const char *brname)
if (ioctl(fd, SIOCBRDELBR, brname) < 0) {
virReportSystemError(errno,
_("Unable to delete bridge %s"), brname);
-goto cleanup;
+
[libvirt] [PATCHv2 5/5] util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE
Signed-off-by: Shi Lei
---
src/util/virnetdev.c | 251 +++
1 file changed, 85 insertions(+), 166 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5d4ad24..a892251 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -200,27 +200,22 @@ virNetDevSetupControl(const char *ifname ATTRIBUTE_UNUSED,
*/
int virNetDevExists(const char *ifname)
{
-int fd = -1;
-int ret = -1;
struct ifreq ifr;
+VIR_AUTOCLOSE fd = -1;
if ((fd = virNetDevSetupControl(ifname, )) < 0)
return -1;
if (ioctl(fd, SIOCGIFFLAGS, )) {
if (errno == ENODEV || errno == ENXIO)
-ret = 0;
-else
-virReportSystemError(errno,
- _("Unable to check interface flags for %s"),
ifname);
-goto cleanup;
-}
+return 0;
-ret = 1;
+virReportSystemError(errno, _("Unable to check interface flags for
%s"),
+ ifname);
+return -1;
+}
- cleanup:
-VIR_FORCE_CLOSE(fd);
-return ret;
+return 1;
}
#else
int virNetDevExists(const char *ifname)
@@ -251,20 +246,20 @@ virNetDevSetMACInternal(const char *ifname,
const virMacAddr *macaddr,
bool quiet)
{
-int fd = -1;
-int ret = -1;
struct ifreq ifr;
char macstr[VIR_MAC_STRING_BUFLEN];
+VIR_AUTOCLOSE fd = -1;
if ((fd = virNetDevSetupControl(ifname, )) < 0)
return -1;
/* To fill ifr.ifr_hdaddr.sa_family field */
if (ioctl(fd, SIOCGIFHWADDR, ) < 0) {
-virReportSystemError(errno,
- _("Cannot get interface MAC on '%s'"),
+virReportSystemError(errno, _("Cannot get interface MAC on '%s'"),
ifname);
-goto cleanup;
+
+VIR_DEBUG("SIOCSIFHWADDR %s get MAC - Fail", ifname);
+return -1;
}
virMacAddrGetRaw(macaddr, (unsigned char *)ifr.ifr_hwaddr.sa_data);
@@ -272,24 +267,22 @@ virNetDevSetMACInternal(const char *ifname,
if (ioctl(fd, SIOCSIFHWADDR, ) < 0) {
if (quiet &&
-(errno == EADDRNOTAVAIL || errno == EPERM))
-goto cleanup;
+(errno == EADDRNOTAVAIL || errno == EPERM)) {
+VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - Fail",
+ ifname, virMacAddrFormat(macaddr, macstr));
+return -1;
+}
virReportSystemError(errno,
_("Cannot set interface MAC to %s on '%s'"),
virMacAddrFormat(macaddr, macstr), ifname);
-goto cleanup;
+return -1;
}
-ret = 0;
+VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - Success",
+ ifname, virMacAddrFormat(macaddr, macstr));
- cleanup:
-VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - %s",
- ifname, virMacAddrFormat(macaddr, macstr),
- ret < 0 ? "Fail" : "Success");
-
-VIR_FORCE_CLOSE(fd);
-return ret;
+return 0;
}
@@ -305,8 +298,7 @@ virNetDevSetMACInternal(const char *ifname,
struct ifreq ifr;
struct sockaddr_dl sdl;
char mac[VIR_MAC_STRING_BUFLEN + 1] = ":";
-int s;
-int ret = -1;
+VIR_AUTOCLOSE s = -1;
if ((s = virNetDevSetupControl(ifname, )) < 0)
return -1;
@@ -320,23 +312,19 @@ virNetDevSetMACInternal(const char *ifname,
if (ioctl(s, SIOCSIFLLADDR, ) < 0) {
if (quiet &&
-(errno == EADDRNOTAVAIL || errno == EPERM))
-goto cleanup;
+(errno == EADDRNOTAVAIL || errno == EPERM)) {
+VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - Fail", ifname, mac + 1);
+return -1;
+}
virReportSystemError(errno,
_("Cannot set interface MAC to %s on '%s'"),
mac + 1, ifname);
-goto cleanup;
+return -1;
}
-ret = 0;
- cleanup:
-VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - %s", ifname, mac + 1,
- ret < 0 ? "Fail" : "Success");
-
-VIR_FORCE_CLOSE(s);
-
-return ret;
+VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - Success", ifname, mac + 1);
+return 0;
}
@@ -379,9 +367,8 @@ virNetDevSetMAC(const char *ifname,
int virNetDevGetMAC(const char *ifname,
virMacAddrPtr macaddr)
{
-int fd = -1;
-int ret = -1;
struct ifreq ifr;
+VIR_AUTOCLOSE fd = -1;
if ((fd = virNetDevSetupControl(ifname, )) < 0)
return -1;
@@ -390,16 +377,12 @@ int virNetDevGetMAC(const char *ifname,
virReportSystemError(errno,
_("Cannot get interface MAC on '%s'"),
ifname);
-goto cleanup;
+return -1;
}
virMacAddrSetRaw(macaddr, (unsigned char
[libvirt] [PATCHv2 0/5] Introduce VIR_AUTOCLOSE macro for closing fd automatically
v1 here: https://www.redhat.com/archives/libvir-list/2018-September/msg00319.html Diff from v1: - Change VIR_AUTOCLOSE macro (comments from Michal) - Remove others except for patches in util (comments from Erik) - Change sc_require_attribute_cleanup_initialization for this macro Shi Lei (5): util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically cfg.mk: Change syntax-check rule for VIR_AUTOCLOSE variable initialization util: file: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE cfg.mk | 2 +- src/util/virfile.c | 21 ++-- src/util/virfile.h | 18 ++- src/util/virnetdev.c | 251 + src/util/virnetdevbridge.c | 120 ++ 5 files changed, 146 insertions(+), 266 deletions(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 1/5] util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically
Signed-off-by: Shi Lei
---
src/util/virfile.h | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.h b/src/util/virfile.h
index b30a1d3..2bc3cf0 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags)
int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK;
FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
+static inline void virForceCloseHelper(int *fd)
+{
+ignore_value(virFileClose(fd, VIR_FILE_CLOSE_PRESERVE_ERRNO));
+}
+
/* For use on normal paths; caller must check return value,
and failure sets errno per close. */
# define VIR_CLOSE(FD) virFileClose(&(FD), 0)
@@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
ATTRIBUTE_RETURN_CHECK;
/* For use on cleanup paths; errno is unaffected by close,
and no return value to worry about. */
-# define VIR_FORCE_CLOSE(FD) \
-ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO))
+# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD))
# define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true))
/* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected
@@ -80,6 +84,16 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
ATTRIBUTE_RETURN_CHECK;
VIR_FILE_CLOSE_PRESERVE_ERRNO | \
VIR_FILE_CLOSE_DONT_LOG))
+/**
+ * VIR_AUTOCLOSE:
+ *
+ * Macro to automatically force close the fd by calling virForceCloseHelper
+ * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE
+ * in cleanup sections.
+ */
+# define VIR_AUTOCLOSE __attribute__((cleanup(virForceCloseHelper))) int
+
+
/* Opaque type for managing a wrapper around a fd. */
struct _virFileWrapperFd;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 00/23] Introduce metadata locking
On 09/12/2018 07:19 AM, Bjoern Walk wrote: > Michal Privoznik [2018-09-10, 11:36AM +0200]: >> Technically, this is v4 of: >> >> https://www.redhat.com/archives/libvir-list/2018-August/msg01627.html >> >> However, this is implementing different approach than any of the >> previous versions. >> >> One of the problems with previous version was that it was too >> complicated. The main reason for that was that we could not close the >> connection whilst there was a file locked. So we had to invent a >> mechanism that would prevent that (on the client side). >> >> These patches implement different approach. They rely on secdriver's >> transactions which bring all the paths we want to label into one place >> so that they can be relabelled within different namespace. >> I'm extending this idea so that transactions run all the time >> (regardless of domain namespacing) and only at the very last moment is >> decided which namespace would the relabeling run in. >> >> Metadata locking is then as easy as putting lock/unlock calls around one >> function. >> >> You can find the patches at my github too: >> >> https://github.com/zippy2/libvirt/tree/disk_metadata_lock_v4_alt > > Hey Michal, > > is was running a quick test with this patch series with two domains > sharing a disk image without and SELinux enabled. When > starting the second domain, the whole libvirtd daemon hangs for almost a > minute until giving the error that the image is locked. I haven't > debugged it yet to figure out what happens. Is this on two different hosts or one? I'm unable to reproduce, so can you please attach debugger and share 't a a bt' output with me? When I try to reproduce I always get one domain running and the other failing to start because qemu is unable to do its locking. When I put in both, they start successfully. TBH, I don't run SELinux enabled host so I'm testing DAC only, but the changes to the code are merely the same. So I'm wondering if this is really an issue in my SELinux impl or somewhere else. BTW: The one minute timeout comes from patch 16/23: #define LOCK_ACQUIRE_TIMEOUT 60 Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically
On 2018-09-12 at 16:37, Erik Skultety wrote:
>On Wed, Sep 12, 2018 at 10:32:04AM +0200, Michal Privoznik wrote:
>> On 09/12/2018 09:18 AM, Erik Skultety wrote:
>> > On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
>> >> On 2018-09-11 at 20:22, Michal Privoznik wrote:
>> >>> On 09/10/2018 05:47 AM, Shi Lei wrote:
>> By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE
>> macro,
>> many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to
>> getting rid of many of our cleanup sections.
>>
>> Signed-off-by: Shi Lei
>> ---
>> src/util/virfile.h | 20 ++--
>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/virfile.h b/src/util/virfile.h
>> index b30a1d3..70e7203 100644
>> --- a/src/util/virfile.h
>> +++ b/src/util/virfile.h
>> @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags)
>> int virFileFclose(FILE **file, bool preserve_errno)
>> ATTRIBUTE_RETURN_CHECK;
>> FILE *virFileFdopen(int *fdptr, const char *mode)
>> ATTRIBUTE_RETURN_CHECK;
>>
>> +static inline void virForceCloseHelper(int *_fd)
>> >>>
>> >>> No need for this argument to have underscore in its name.
>> >>
>> >> Okay.
>> >>
>> >>>
>> +{
>> + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO));
>> +}
>> +
>> /* For use on normal paths; caller must check return value,
>> and failure sets errno per close. */
>> # define VIR_CLOSE(FD) virFileClose(&(FD), 0)
>> @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
>> ATTRIBUTE_RETURN_CHECK;
>>
>> /* For use on cleanup paths; errno is unaffected by close,
>> and no return value to worry about. */
>> -# define VIR_FORCE_CLOSE(FD) \
>> - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO))
>> +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD))
>> # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE),
>> true))
>>
>> /* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are
>> expected
>> @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
>> ATTRIBUTE_RETURN_CHECK;
>> VIR_FILE_CLOSE_PRESERVE_ERRNO | \
>> VIR_FILE_CLOSE_DONT_LOG))
>>
>> +/**
>> + * VIR_AUTOCLOSE:
>> + * @fd: fd of the file to be closed automatically
>> + *
>> + * Macro to automatically force close the fd by calling
>> virForceCloseHelper
>> + * when the fd goes out of scope. It's used to eliminate
>> VIR_FORCE_CLOSE
>> + * in cleanup sections.
>> + */
>> +# define VIR_AUTOCLOSE(fd) \
>> + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
>> >>>
>> >>> While this may helps us to initialize variables correctly, I think we
>> >>> should do that explicitly. Not only it follows what VIR_AUTOFREE is
>> >>> doing, it also is more visible when used. For instance, in 2/6 when the
>> >>> macro is used for the first time, it's not visible what is @fd
>> >>> initialized to.
>> >>
>> >> Okay. So I think the macro could be like:
>> >>
>> >> # define VIR_AUTOCLOSE \
>> >> __attribute__((cleanup(virForceCloseHelper))) int
>> >
>> > Actually, I'd prefer if we stayed consistent with the existing AUTO_
>> > macros,
>> > IOW, the form of VIR_AUTOCLOSE(fd) = is IMHO desired.
>>
>> I don't think this is correct. Sure, we do have parentheses in
>> VIR_AUTOFREE but only for type, not variable itself:
>
>Oh, right, I missed that fact, sorry for the noise.
>
>>
>> VIR_AUTOFREE(char *) str = NULL;
>>
>> Therefore, in order to be consistent the autoclose macro should look
>> like I'm suggesting actually:
>>
>> VIR_AUTOCLOSE fd = -1;
>>
>> To extend this idea further, we can then have VIR_AUTOFCLOSE and
>> VIR_AUTODIRCLOSE macros to call fclose() and dirclose() respectively (or
>> our wrappers around those functions).
>
>I believe the general consensus is to always use our wrappers. I'm thinking
>whether this could be made more generic, but that would just introduce more
>unnecessary black magic, so I'm fine with your suggestion for the time being,
>we might come up with something better eventually.
>
>Erik
Okay.
So, I would follow the suggestion from both of you and post the v2 series.
Shi Lei
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt PATCH v2 3/4] vircgroup: Add virCgroupSetupBlkioTune()
virCgroupSetupBlkioTune() has been introduced in order to remove the
code duplication present between virLXCCgroupSetupBlkioTune() and
qemuSetupBlkioCgroup().
Signed-off-by: Fabiano Fidêncio
---
src/libvirt_private.syms | 1 +
src/lxc/lxc_cgroup.c | 49 +---
src/qemu/qemu_cgroup.c | 47 +-
src/util/vircgroup.c | 54
src/util/vircgroup.h | 3 +++
5 files changed, 60 insertions(+), 94 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0fc5314b02..9a85575cce 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1576,6 +1576,7 @@ virCgroupSetMemoryHardLimit;
virCgroupSetMemorySoftLimit;
virCgroupSetMemSwapHardLimit;
virCgroupSetOwner;
+virCgroupSetupBlkioTune;
virCgroupSupportsCpuBW;
virCgroupTerminateMachine;
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index d93a19d684..4a95f2a8b0 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -106,54 +106,7 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
static int virLXCCgroupSetupBlkioTune(virDomainDefPtr def,
virCgroupPtr cgroup)
{
-size_t i;
-
-if (def->blkio.weight &&
-virCgroupSetBlkioWeight(cgroup, def->blkio.weight) < 0)
-return -1;
-
-if (def->blkio.ndevices) {
-for (i = 0; i < def->blkio.ndevices; i++) {
-virBlkioDevicePtr dev = >blkio.devices[i];
-
-if (dev->weight &&
-(virCgroupSetBlkioDeviceWeight(cgroup, dev->path,
- dev->weight) < 0 ||
- virCgroupGetBlkioDeviceWeight(cgroup, dev->path,
- >weight) < 0))
-return -1;
-
-if (dev->riops &&
-(virCgroupSetBlkioDeviceReadIops(cgroup, dev->path,
- dev->riops) < 0 ||
- virCgroupGetBlkioDeviceReadIops(cgroup, dev->path,
- >riops) < 0))
-return -1;
-
-if (dev->wiops &&
-(virCgroupSetBlkioDeviceWriteIops(cgroup, dev->path,
- dev->wiops) < 0 ||
- virCgroupGetBlkioDeviceWriteIops(cgroup, dev->path,
- >wiops) < 0))
-return -1;
-
-if (dev->rbps &&
-(virCgroupSetBlkioDeviceReadBps(cgroup, dev->path,
-dev->rbps) < 0 ||
- virCgroupGetBlkioDeviceReadBps(cgroup, dev->path,
->rbps) < 0))
-return -1;
-
-if (dev->wbps &&
-(virCgroupSetBlkioDeviceWriteBps(cgroup, dev->path,
- dev->wbps) < 0 ||
- virCgroupGetBlkioDeviceWriteBps(cgroup, dev->path,
- >wbps) < 0))
-return -1;
-}
-}
-
-return 0;
+return virCgroupSetupBlkioTune(cgroup, >blkio);
}
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 43e17d786e..0e53678faa 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -514,7 +514,6 @@ static int
qemuSetupBlkioCgroup(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
-size_t i;
if (!virCgroupHasController(priv->cgroup,
VIR_CGROUP_CONTROLLER_BLKIO)) {
@@ -527,51 +526,7 @@ qemuSetupBlkioCgroup(virDomainObjPtr vm)
}
}
-if (vm->def->blkio.weight != 0 &&
-virCgroupSetBlkioWeight(priv->cgroup, vm->def->blkio.weight) < 0)
-return -1;
-
-if (vm->def->blkio.ndevices) {
-for (i = 0; i < vm->def->blkio.ndevices; i++) {
-virBlkioDevicePtr dev = >def->blkio.devices[i];
-if (dev->weight &&
-(virCgroupSetBlkioDeviceWeight(priv->cgroup, dev->path,
- dev->weight) < 0 ||
- virCgroupGetBlkioDeviceWeight(priv->cgroup, dev->path,
- >weight) < 0))
-return -1;
-
-if (dev->riops &&
-(virCgroupSetBlkioDeviceReadIops(priv->cgroup, dev->path,
- dev->riops) < 0 ||
- virCgroupGetBlkioDeviceReadIops(priv->cgroup, dev->path,
- >riops) < 0))
-return -1;
-
-if (dev->wiops &&
-(virCgroupSetBlkioDeviceWriteIops(priv->cgroup, dev->path,
- dev->wiops) < 0 ||
- virCgroupGetBlkioDeviceWriteIops(priv->cgroup, dev->path,
-
[libvirt] [libvirt PATCH v2 1/4] domain_conf: split out virBlkioDevice and virDomainBlkiotune definitions
Let's move those to their own newly created files
(src/util/virblkio.{c,h}) as this will help us to easily start sharing
the cgroup code that's duplicated between QEMU and LXC.
Signed-off-by: Fabiano Fidêncio
---
src/Makefile.am | 1 +
src/conf/domain_conf.c | 11 +
src/conf/domain_conf.h | 27 ++---
src/util/Makefile.inc.am | 2 ++
src/util/virblkio.c | 37
src/util/virblkio.h | 52
6 files changed, 95 insertions(+), 35 deletions(-)
create mode 100644 src/util/virblkio.c
create mode 100644 src/util/virblkio.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 2a3ed0d42d..926085ff2d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -671,6 +671,7 @@ libvirt_setuid_rpc_client_la_SOURCES = \
util/viratomic.c \
util/viratomic.h \
util/virbitmap.c \
+ util/virblkio.c \
util/virbuffer.c \
util/vircgroup.c \
util/vircommand.c \
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7e14cea128..6ce50f712a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -59,6 +59,7 @@
#include "virnetdevmacvlan.h"
#include "virhostdev.h"
#include "virmdev.h"
+#include "virblkio.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -1205,16 +1206,6 @@ virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr
xmlopt)
}
-void
-virBlkioDeviceArrayClear(virBlkioDevicePtr devices,
- int ndevices)
-{
-size_t i;
-
-for (i = 0; i < ndevices; i++)
-VIR_FREE(devices[i].path);
-}
-
/**
* virDomainBlkioDeviceParseXML
*
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e30a4b2fe7..e9e6b6d6c4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -57,6 +57,7 @@
# include "virtypedparam.h"
# include "virsavecookie.h"
# include "virresctrl.h"
+# include "virblkio.h"
/* forward declarations of all device types, required by
* virDomainDeviceDef
@@ -2084,17 +2085,6 @@ struct _virDomainClockDef {
};
-typedef struct _virBlkioDevice virBlkioDevice;
-typedef virBlkioDevice *virBlkioDevicePtr;
-struct _virBlkioDevice {
-char *path;
-unsigned int weight;
-unsigned int riops;
-unsigned int wiops;
-unsigned long long rbps;
-unsigned long long wbps;
-};
-
typedef enum {
VIR_DOMAIN_RNG_MODEL_VIRTIO,
@@ -2184,9 +2174,6 @@ struct _virDomainPanicDef {
};
-void virBlkioDeviceArrayClear(virBlkioDevicePtr deviceWeights,
- int ndevices);
-
typedef struct _virDomainResourceDef virDomainResourceDef;
typedef virDomainResourceDef *virDomainResourceDefPtr;
struct _virDomainResourceDef {
@@ -2260,16 +2247,6 @@ struct _virDomainVcpuDef {
virObjectPtr privateData;
};
-typedef struct _virDomainBlkiotune virDomainBlkiotune;
-typedef virDomainBlkiotune *virDomainBlkiotunePtr;
-
-struct _virDomainBlkiotune {
-unsigned int weight;
-
-size_t ndevices;
-virBlkioDevicePtr devices;
-};
-
typedef struct _virDomainMemtune virDomainMemtune;
typedef virDomainMemtune *virDomainMemtunePtr;
@@ -2402,7 +2379,7 @@ struct _virDomainDef {
char *title;
char *description;
-virDomainBlkiotune blkio;
+virBlkioTune blkio;
virDomainMemtune mem;
virDomainVcpuDefPtr *vcpus;
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index a22265606c..13f415b23c 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -17,6 +17,8 @@ UTIL_SOURCES = \
util/virauthconfig.h \
util/virbitmap.c \
util/virbitmap.h \
+ util/virblkio.c \
+ util/virblkio.h \
util/virbuffer.c \
util/virbuffer.h \
util/virperf.c \
diff --git a/src/util/virblkio.c b/src/util/virblkio.c
new file mode 100644
index 00..9711077ee8
--- /dev/null
+++ b/src/util/virblkio.c
@@ -0,0 +1,37 @@
+/*
+ * virblkio.c: Block IO helpers
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * 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
+ *
[libvirt] [libvirt PATCH v2 4/4] vircgroup: Add virCgroupSetupMemTune()
virCgroupSetupMemTune() has been introduced in order to remove the code
duplication present between virLXCCgroupSetupMemTune() and
qemuSetupMemoryCgroup().
Signed-off-by: Fabiano Fidêncio
---
src/libvirt_private.syms | 1 +
src/lxc/lxc_cgroup.c | 20 ++--
src/qemu/qemu_cgroup.c | 14 +-
src/util/vircgroup.c | 20
src/util/vircgroup.h | 4
5 files changed, 28 insertions(+), 31 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9a85575cce..ffee878e34 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1577,6 +1577,7 @@ virCgroupSetMemorySoftLimit;
virCgroupSetMemSwapHardLimit;
virCgroupSetOwner;
virCgroupSetupBlkioTune;
+virCgroupSetupMemTune;
virCgroupSupportsCpuBW;
virCgroupTerminateMachine;
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index 4a95f2a8b0..95311fde9e 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -113,26 +113,10 @@ static int virLXCCgroupSetupBlkioTune(virDomainDefPtr def,
static int virLXCCgroupSetupMemTune(virDomainDefPtr def,
virCgroupPtr cgroup)
{
-int ret = -1;
-
if (virCgroupSetMemory(cgroup, virDomainDefGetMemoryInitial(def)) < 0)
-goto cleanup;
-
-if (virMemoryLimitIsSet(def->mem.hard_limit))
-if (virCgroupSetMemoryHardLimit(cgroup, def->mem.hard_limit) < 0)
-goto cleanup;
-
-if (virMemoryLimitIsSet(def->mem.soft_limit))
-if (virCgroupSetMemorySoftLimit(cgroup, def->mem.soft_limit) < 0)
-goto cleanup;
-
-if (virMemoryLimitIsSet(def->mem.swap_hard_limit))
-if (virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit) < 0)
-goto cleanup;
+return -1;
-ret = 0;
- cleanup:
-return ret;
+return virCgroupSetupMemTune(cgroup, >mem);
}
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 0e53678faa..205098ec30 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -547,19 +547,7 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
}
}
-if (virMemoryLimitIsSet(vm->def->mem.hard_limit))
-if (virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit)
< 0)
-return -1;
-
-if (virMemoryLimitIsSet(vm->def->mem.soft_limit))
-if (virCgroupSetMemorySoftLimit(priv->cgroup, vm->def->mem.soft_limit)
< 0)
-return -1;
-
-if (virMemoryLimitIsSet(vm->def->mem.swap_hard_limit))
-if (virCgroupSetMemSwapHardLimit(priv->cgroup,
vm->def->mem.swap_hard_limit) < 0)
-return -1;
-
-return 0;
+return virCgroupSetupMemTune(priv->cgroup, >def->mem);
}
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index a08b6f4869..3973f6da61 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -4874,3 +4874,23 @@ virCgroupSetupBlkioTune(virCgroupPtr cgroup,
return 0;
}
+
+
+int
+virCgroupSetupMemTune(virCgroupPtr cgroup,
+ virMemTunePtr mem)
+{
+if (virMemoryLimitIsSet(mem->hard_limit))
+if (virCgroupSetMemoryHardLimit(cgroup, mem->hard_limit) < 0)
+return -1;
+
+if (virMemoryLimitIsSet(mem->soft_limit))
+if (virCgroupSetMemorySoftLimit(cgroup, mem->soft_limit) < 0)
+return -1;
+
+if (virMemoryLimitIsSet(mem->swap_hard_limit))
+if (virCgroupSetMemSwapHardLimit(cgroup, mem->swap_hard_limit) < 0)
+return -1;
+
+return 0;
+}
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index 2908f70372..ad88da62d4 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -28,6 +28,7 @@
# include "virutil.h"
# include "virbitmap.h"
# include "virblkio.h"
+# include "virmem.h"
struct _virCgroup;
typedef struct _virCgroup virCgroup;
@@ -289,4 +290,7 @@ int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int
controller);
bool virCgroupControllerAvailable(int controller);
int virCgroupSetupBlkioTune(virCgroupPtr cgroup, virBlkioTunePtr blkio);
+int virCgroupSetupMemTune(virCgroupPtr cgroup, virMemTunePtr mem);
+
+
#endif /* __VIR_CGROUP_H__ */
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt PATCH v2 2/4] domain_conf: split out virDomainMemtune and virDomainHugePage definitions
Let's move those to their own newly created header (src/util/virmem.h)
as this will help us to easily start sharing the cgroup code that's
duplicated between QEMU and LXC.
Signed-off-by: Fabiano Fidêncio
---
src/conf/domain_conf.c | 11 +++
src/conf/domain_conf.h | 43 ++-
src/qemu/qemu_command.c | 4 +--
src/util/virmem.h | 66 +
4 files changed, 76 insertions(+), 48 deletions(-)
create mode 100644 src/util/virmem.h
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6ce50f712a..bd22ce6e7d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -60,6 +60,7 @@
#include "virhostdev.h"
#include "virmdev.h"
#include "virblkio.h"
+#include "virmem.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -6166,7 +6167,7 @@ virDomainDefLifecycleActionValidate(const virDomainDef
*def)
static int
virDomainDefMemtuneValidate(const virDomainDef *def)
{
-const virDomainMemtune *mem = &(def->mem);
+const virMemTune *mem = &(def->mem);
size_t i;
ssize_t pos = virDomainNumaGetNodeCount(def->numa) - 1;
@@ -18197,7 +18198,7 @@ virDomainDefMaybeAddInput(virDomainDefPtr def,
static int
virDomainHugepagesParseXML(xmlNodePtr node,
xmlXPathContextPtr ctxt,
- virDomainHugePagePtr hugepage)
+ virMemHugePagePtr hugepage)
{
int ret = -1;
xmlNodePtr oldnode = ctxt->node;
@@ -26841,7 +26842,7 @@ virDomainResourceDefFormat(virBufferPtr buf,
static int
virDomainHugepagesFormatBuf(virBufferPtr buf,
-virDomainHugePagePtr hugepage)
+virMemHugePagePtr hugepage)
{
int ret = -1;
@@ -26865,7 +26866,7 @@ virDomainHugepagesFormatBuf(virBufferPtr buf,
static void
virDomainHugepagesFormat(virBufferPtr buf,
- virDomainHugePagePtr hugepages,
+ virMemHugePagePtr hugepages,
size_t nhugepages)
{
size_t i;
@@ -27403,7 +27404,7 @@ virDomainIOMMUDefFormat(virBufferPtr buf,
static int
virDomainMemtuneFormat(virBufferPtr buf,
- const virDomainMemtune *mem)
+ const virMemTune *mem)
{
virBuffer childBuf = VIR_BUFFER_INITIALIZER;
int ret = -1;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e9e6b6d6c4..10acc39861 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -58,6 +58,7 @@
# include "virsavecookie.h"
# include "virresctrl.h"
# include "virblkio.h"
+# include "virmem.h"
/* forward declarations of all device types, required by
* virDomainDeviceDef
@@ -2180,14 +2181,6 @@ struct _virDomainResourceDef {
char *partition;
};
-typedef struct _virDomainHugePage virDomainHugePage;
-typedef virDomainHugePage *virDomainHugePagePtr;
-
-struct _virDomainHugePage {
-virBitmapPtr nodemask; /* guest's NUMA node mask */
-unsigned long long size;/* hugepage size in KiB */
-};
-
# define VIR_DOMAIN_CPUMASK_LEN 1024
typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef;
@@ -2247,38 +2240,6 @@ struct _virDomainVcpuDef {
virObjectPtr privateData;
};
-typedef struct _virDomainMemtune virDomainMemtune;
-typedef virDomainMemtune *virDomainMemtunePtr;
-
-struct _virDomainMemtune {
-/* total memory size including memory modules in kibibytes, this field
- * should be accessed only via accessors */
-unsigned long long total_memory;
-unsigned long long cur_balloon; /* in kibibytes, capped at ulong thanks
- to virDomainGetInfo */
-
-virDomainHugePagePtr hugepages;
-size_t nhugepages;
-
-/* maximum supported memory for a guest, for hotplugging */
-unsigned long long max_memory; /* in kibibytes */
-unsigned int memory_slots; /* maximum count of RAM memory slots */
-
-bool nosharepages;
-bool locked;
-int dump_core; /* enum virTristateSwitch */
-unsigned long long hard_limit; /* in kibibytes, limit at off_t bytes */
-unsigned long long soft_limit; /* in kibibytes, limit at off_t bytes */
-unsigned long long min_guarantee; /* in kibibytes, limit at off_t bytes */
-unsigned long long swap_hard_limit; /* in kibibytes, limit at off_t bytes
*/
-
-int source; /* enum virDomainMemorySource */
-int access; /* enum virDomainMemoryAccess */
-int allocation; /* enum virDomainMemoryAllocation */
-
-virTristateBool discard;
-};
-
typedef struct _virDomainPowerManagement virDomainPowerManagement;
typedef virDomainPowerManagement *virDomainPowerManagementPtr;
@@ -2380,7 +2341,7 @@ struct _virDomainDef {
char *description;
virBlkioTune blkio;
-virDomainMemtune mem;
+virMemTune mem;
virDomainVcpuDefPtr *vcpus;
size_t maxvcpus;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ff9589f593..3511505389 100644
---
[libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC
virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup() and virLXCCgroupSetupCpuTune() and qemuSetupCpuCgroup() are the most similar functions between QEMU and LXC code. Let's move their common code to virCgroup. Mind that the first two patches are basically preparing the ground for the changes introduced in the last two patches. changes since v1: - Michal Privoznik pointed out (as did the `make syntax-check` :-)) that we do want to keep src/util independently of the parsing code (thus, including "conf/domain_conf.h" in vircgroup.h is not the way to go). This has been solved now by partially following Michal's suggestion and splitting the structs and functions that would be use in the common code to new different files. Fabiano Fidêncio (4): domain_conf: split out virBlkioDevice and virDomainBlkiotune definitions domain_conf: split out virDomainMemtune and virDomainHugePage definitions vircgroup: Add virCgroupSetupBlkioTune() vircgroup: Add virCgroupSetupMemTune() src/Makefile.am | 1 + src/conf/domain_conf.c | 22 src/conf/domain_conf.h | 70 +++-- src/libvirt_private.syms | 2 ++ src/lxc/lxc_cgroup.c | 69 ++--- src/qemu/qemu_cgroup.c | 61 ++--- src/qemu/qemu_command.c | 4 +-- src/util/Makefile.inc.am | 2 ++ src/util/virblkio.c | 37 src/util/virblkio.h | 52 src/util/vircgroup.c | 74 src/util/vircgroup.h | 7 src/util/virmem.h| 66 +++ 13 files changed, 259 insertions(+), 208 deletions(-) create mode 100644 src/util/virblkio.c create mode 100644 src/util/virblkio.h create mode 100644 src/util/virmem.h -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] storage: Allow inputvol to be encrypted
On 09/11/2018 04:15 PM, John Ferlan wrote:
>
>
> On 09/11/2018 07:16 AM, Michal Privoznik wrote:
>> On 08/21/2018 06:23 PM, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1613737
>>>
>>> When processing the inputvol for encryption, we need to handle
>>> the case where the inputvol is encrypted. This then allows for
>>> the encrypted inputvol to be used either for an output encrypted
>>> volume or an output volume of some XML provided type.
>>>
>>> Add tests to show the various conversion options when either input
>>> or output is encrypted. This includes when both are encrypted.
>>>
>>> Signed-off-by: John Ferlan
>>> ---
>>> src/storage/storage_util.c| 62 ---
>>> src/storage/storage_util.h| 1 +
>>> .../luks-convert-encrypt.argv | 11
>>> .../luks-convert-encrypt2fileqcow2.argv | 7 +++
>>> .../luks-convert-encrypt2fileraw.argv | 7 +++
>>> tests/storagevolxml2argvtest.c| 15 -
>>> tests/storagevolxml2xmlin/vol-encrypt1.xml| 21 +++
>>> tests/storagevolxml2xmlin/vol-encrypt2.xml| 21 +++
>>> 8 files changed, 137 insertions(+), 8 deletions(-)
>>> create mode 100644 tests/storagevolxml2argvdata/luks-convert-encrypt.argv
>>> create mode 100644
>>> tests/storagevolxml2argvdata/luks-convert-encrypt2fileqcow2.argv
>>> create mode 100644
>>> tests/storagevolxml2argvdata/luks-convert-encrypt2fileraw.argv
>>> create mode 100644 tests/storagevolxml2xmlin/vol-encrypt1.xml
>>> create mode 100644 tests/storagevolxml2xmlin/vol-encrypt2.xml
>>>
>>> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
>>> index cc49a5b9f7..3c1e875b27 100644
>>> --- a/src/storage/storage_util.c
>>> +++ b/src/storage/storage_util.c
>>> @@ -1084,6 +1084,7 @@
>>> virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
>>> unsigned int flags,
>>> const char *create_tool,
>>> const char *secretPath,
>>> + const char *inputSecretPath,
>>> virStorageVolEncryptConvertStep
>>> convertStep)
>>> {
>>> virCommandPtr cmd = NULL;
>>> @@ -1101,6 +1102,8 @@
>>> virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
>>> .secretAlias = NULL,
>>> };
>>> virStorageEncryptionPtr enc = vol->target.encryption;
>>> +char *inputSecretAlias = NULL;
>>> +virStorageEncryptionPtr inputenc = inputvol ?
>>> inputvol->target.encryption : NULL;
>>> virStorageEncryptionInfoDefPtr encinfo = NULL;
>>>
>>> virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
>>> @@ -1114,6 +1117,12 @@
>>> virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
>>> goto error;
>>> }
>>>
>>> +if (inputenc && inputenc->format !=
>>> VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("encryption format of inputvol must be LUKS"));
>>> +goto error;
>>> +}
>>> +
>>> if (virStorageBackendCreateQemuImgSetInfo(pool, vol, inputvol,
>>>convertStep, ) < 0)
>>> goto error;
>>> @@ -1153,6 +1162,20 @@
>>> virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
>>> encinfo = >encinfo;
>>> }
>>>
>>> +if (inputenc && convertStep == VIR_STORAGE_VOL_ENCRYPT_CONVERT) {
>>> +if (!inputSecretPath) {
>>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> + _("path to inputvol secret data file is
>>> required"));
>>> +goto error;
>>> +}
>>> +if (virAsprintf(, "%s_encrypt0",
>>> +inputvol->name) < 0)
>>> +goto error;
>>> +if (storageBackendCreateQemuImgSecretObject(cmd, inputSecretPath,
>>> +inputSecretAlias) < 0)
>>> +goto error;
>>> +}
>>> +
>>> if (convertStep != VIR_STORAGE_VOL_ENCRYPT_CONVERT) {
>>> if (storageBackendCreateQemuImgSetOptions(cmd, encinfo, info) < 0)
>>> goto error;
>>> @@ -1163,19 +1186,32 @@
>>> virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
>>> virCommandAddArgFormat(cmd, "%lluK", info.size_arg);
>>> } else {
>>> /* source */
>>> -virCommandAddArgFormat(cmd, "driver=%s,file.filename=%s",
>>> - info.inputType, info.inputPath);
>>> +if (inputenc)
>>> +virCommandAddArgFormat(cmd,
>>> +
>>> "driver=luks,file.filename=%s,key-secret=%s",
>>> + info.inputPath, inputSecretAlias);
>>> +else
>>> +virCommandAddArgFormat(cmd,
[libvirt] Fw: Re: [PATCH 4/6] util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE
On 2018-09-12 at 16:25, Shi Lei wrote:
On 2018-09-12 at 15:21, Erik Skultety wrote:
>On Tue, Sep 11, 2018 at 11:37:45PM +0800, Shi Lei wrote:
>> On 2018-09-11 at 20:44, Erik Skultety wrote:
>> >On Mon, Sep 10, 2018 at 11:47:53AM +0800, Shi Lei wrote:
>> >> By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE
>> >> macro,
>> >> many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to
>> >> getting rid of many of our cleanup sections.
>> >>
>> >> Signed-off-by: Shi Lei
>> >> ---
>> >> static int
>> >> @@ -909,9 +867,9 @@ char *virNetDevGetName(int ifindex)
>> >> #if defined(SIOCGIFINDEX) && defined(HAVE_STRUCT_IFREQ)
>> >> int virNetDevGetIndex(const char *ifname, int *ifindex)
>> >> {
>> >> - int ret = -1;
>> >> struct ifreq ifreq;
>> >> - int fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
>> >> + VIR_AUTOCLOSE(fd);
>> >> + socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
>> >
>> >^this could not potentially work...
>> >
>> >Erik
>>
>> Sorry! I'm too careless.
>>
>> I think that a new syntax-check rule might make sense.
>> This rule checks below:
>>
>> type foo(a0, a1 ...);
>>
>> [type] var = foo(a0, a1, ...); /* It's OK */
>> ignore_value(foo(a0, a1, ...)); /* It's OK */
>>
>> foo(a0, a1, ...); /* Report this usage */
>
>but this would go away with the syntax-check rule you proposed in you response
>to the first patch? If not, then would you be more specific to help me
>understand the problem more?
>
>Thanks,
>Erik
Sorry! I was muddle-headed last night.
It is not the syntax-check rule in my response to the first patch.
This is another rule and I wanted to introduce it to avoid some faults then.
Today I find that it is not easy as I thought.
Please ignore it ... :-)
Shi Lei
>
>>
>> And it takes effect before compilation and even it's in the inactive
>> side of the #if-#else conditons.
>>
>> It would not check macros since their name are upper case and
>> we don't care for the function as condition in if-else statement.
>>
>> How about it?
>> And I can try it if it's a bit helpful ...
>>
>> Shi Lei
>>
>>
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically
On Wed, Sep 12, 2018 at 10:32:04AM +0200, Michal Privoznik wrote:
> On 09/12/2018 09:18 AM, Erik Skultety wrote:
> > On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
> >> On 2018-09-11 at 20:22, Michal Privoznik wrote:
> >>> On 09/10/2018 05:47 AM, Shi Lei wrote:
> By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE
> macro,
> many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to
> getting rid of many of our cleanup sections.
>
> Signed-off-by: Shi Lei
> ---
> src/util/virfile.h | 20 ++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index b30a1d3..70e7203 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags)
> int virFileFclose(FILE **file, bool preserve_errno)
> ATTRIBUTE_RETURN_CHECK;
> FILE *virFileFdopen(int *fdptr, const char *mode)
> ATTRIBUTE_RETURN_CHECK;
>
> +static inline void virForceCloseHelper(int *_fd)
> >>>
> >>> No need for this argument to have underscore in its name.
> >>
> >> Okay.
> >>
> >>>
> +{
> + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO));
> +}
> +
> /* For use on normal paths; caller must check return value,
> and failure sets errno per close. */
> # define VIR_CLOSE(FD) virFileClose(&(FD), 0)
> @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
> ATTRIBUTE_RETURN_CHECK;
>
> /* For use on cleanup paths; errno is unaffected by close,
> and no return value to worry about. */
> -# define VIR_FORCE_CLOSE(FD) \
> - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO))
> +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD))
> # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE),
> true))
>
> /* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are
> expected
> @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
> ATTRIBUTE_RETURN_CHECK;
> VIR_FILE_CLOSE_PRESERVE_ERRNO | \
> VIR_FILE_CLOSE_DONT_LOG))
>
> +/**
> + * VIR_AUTOCLOSE:
> + * @fd: fd of the file to be closed automatically
> + *
> + * Macro to automatically force close the fd by calling
> virForceCloseHelper
> + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE
> + * in cleanup sections.
> + */
> +# define VIR_AUTOCLOSE(fd) \
> + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
> >>>
> >>> While this may helps us to initialize variables correctly, I think we
> >>> should do that explicitly. Not only it follows what VIR_AUTOFREE is
> >>> doing, it also is more visible when used. For instance, in 2/6 when the
> >>> macro is used for the first time, it's not visible what is @fd
> >>> initialized to.
> >>
> >> Okay. So I think the macro could be like:
> >>
> >> # define VIR_AUTOCLOSE \
> >> __attribute__((cleanup(virForceCloseHelper))) int
> >
> > Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros,
> > IOW, the form of VIR_AUTOCLOSE(fd) = is IMHO desired.
>
> I don't think this is correct. Sure, we do have parentheses in
> VIR_AUTOFREE but only for type, not variable itself:
Oh, right, I missed that fact, sorry for the noise.
>
> VIR_AUTOFREE(char *) str = NULL;
>
> Therefore, in order to be consistent the autoclose macro should look
> like I'm suggesting actually:
>
> VIR_AUTOCLOSE fd = -1;
>
> To extend this idea further, we can then have VIR_AUTOFCLOSE and
> VIR_AUTODIRCLOSE macros to call fclose() and dirclose() respectively (or
> our wrappers around those functions).
I believe the general consensus is to always use our wrappers. I'm thinking
whether this could be made more generic, but that would just introduce more
unnecessary black magic, so I'm fine with your suggestion for the time being,
we might come up with something better eventually.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
在 2018/9/12 下午3:35, Yi Min Zhao 写道:
+static void *
+virZPCIAddrCopy(const void *name)
+{
+ unsigned int *copy;
+
+ if (VIR_ALLOC(copy) < 0)
+ return NULL;
+
+ *copy = *((unsigned int *)name);
+ return (void *)copy;
+}
+
+
+static void
+virZPCIAddrKeyFree(void *name)
+{
+ VIR_FREE(name);
+}
This makes sense and seems to work just fine; however, you are
allocating and releasing a bunch of small integers, which seems
a bit wasteful.
vircgroup is AFAICT avoiding all that extra memory management by
stuffing the values straight into the pointers themselves, which
you should also be able to do since the biggest legal ID is a
32-bit integer.
That said, I haven't been able to get that to actually work, at
least with a quick attempt :( Would you mind exploring that route
and figuring out whether it's feasible at all?
I'm testing this. Actually I wanted to do so like vircgroup. I
remembered there's
error due to the previous code logic. I will reply to you later.
I remebered the reason and test again. FID might be 0. It is treated as
an error
if we save 0 in void* pointer.
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically
On 09/12/2018 09:18 AM, Erik Skultety wrote:
> On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
>> On 2018-09-11 at 20:22, Michal Privoznik wrote:
>>> On 09/10/2018 05:47 AM, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE
macro,
many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to
getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei
---
src/util/virfile.h | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.h b/src/util/virfile.h
index b30a1d3..70e7203 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags)
int virFileFclose(FILE **file, bool preserve_errno)
ATTRIBUTE_RETURN_CHECK;
FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
+static inline void virForceCloseHelper(int *_fd)
>>>
>>> No need for this argument to have underscore in its name.
>>
>> Okay.
>>
>>>
+{
+ ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO));
+}
+
/* For use on normal paths; caller must check return value,
and failure sets errno per close. */
# define VIR_CLOSE(FD) virFileClose(&(FD), 0)
@@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
ATTRIBUTE_RETURN_CHECK;
/* For use on cleanup paths; errno is unaffected by close,
and no return value to worry about. */
-# define VIR_FORCE_CLOSE(FD) \
- ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO))
+# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD))
# define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE),
true))
/* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are
expected
@@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
ATTRIBUTE_RETURN_CHECK;
VIR_FILE_CLOSE_PRESERVE_ERRNO | \
VIR_FILE_CLOSE_DONT_LOG))
+/**
+ * VIR_AUTOCLOSE:
+ * @fd: fd of the file to be closed automatically
+ *
+ * Macro to automatically force close the fd by calling
virForceCloseHelper
+ * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE
+ * in cleanup sections.
+ */
+# define VIR_AUTOCLOSE(fd) \
+ __attribute__((cleanup(virForceCloseHelper))) int fd = -1
>>>
>>> While this may helps us to initialize variables correctly, I think we
>>> should do that explicitly. Not only it follows what VIR_AUTOFREE is
>>> doing, it also is more visible when used. For instance, in 2/6 when the
>>> macro is used for the first time, it's not visible what is @fd
>>> initialized to.
>>
>> Okay. So I think the macro could be like:
>>
>> # define VIR_AUTOCLOSE \
>> __attribute__((cleanup(virForceCloseHelper))) int
>
> Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros,
> IOW, the form of VIR_AUTOCLOSE(fd) = is IMHO desired.
I don't think this is correct. Sure, we do have parentheses in
VIR_AUTOFREE but only for type, not variable itself:
VIR_AUTOFREE(char *) str = NULL;
Therefore, in order to be consistent the autoclose macro should look
like I'm suggesting actually:
VIR_AUTOCLOSE fd = -1;
To extend this idea further, we can then have VIR_AUTOFCLOSE and
VIR_AUTODIRCLOSE macros to call fclose() and dirclose() respectively (or
our wrappers around those functions).
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically
On Wed, Sep 12, 2018 at 04:04:04PM +0800, Shi Lei wrote:
> On 2018-09-12 at 15:18, Erik Skultety wrote:
> >On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
> >> On 2018-09-11 at 20:22, Michal Privoznik wrote:
> >> >On 09/10/2018 05:47 AM, Shi Lei wrote:
> >> >> By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE
> >> >> macro,
> >> >> many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to
> >> >> getting rid of many of our cleanup sections.
> >> >>
> >> >> Signed-off-by: Shi Lei
> >> >> ---
> >> >> src/util/virfile.h | 20 ++--
> >> >> 1 file changed, 18 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/src/util/virfile.h b/src/util/virfile.h
> >> >> index b30a1d3..70e7203 100644
> >> >> --- a/src/util/virfile.h
> >> >> +++ b/src/util/virfile.h
> >> >> @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags)
> >> >> int virFileFclose(FILE **file, bool preserve_errno)
> >> >>ATTRIBUTE_RETURN_CHECK;
> >> >> FILE *virFileFdopen(int *fdptr, const char *mode)
> >> >>ATTRIBUTE_RETURN_CHECK;
> >> >>
> >> >> +static inline void virForceCloseHelper(int *_fd)
> >> >
> >> >No need for this argument to have underscore in its name.
> >>
> >> Okay.
> >>
> >> >
> >> >> +{
> >> >> + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO));
> >> >> +}
> >> >> +
> >> >> /* For use on normal paths; caller must check return value,
> >> >> and failure sets errno per close. */
> >> >> # define VIR_CLOSE(FD) virFileClose(&(FD), 0)
> >> >> @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
> >> >> ATTRIBUTE_RETURN_CHECK;
> >> >>
> >> >> /* For use on cleanup paths; errno is unaffected by close,
> >> >> and no return value to worry about. */
> >> >> -# define VIR_FORCE_CLOSE(FD) \
> >> >> - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO))
> >> >> +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD))
> >> >> # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE),
> >> >>true))
> >> >>
> >> >> /* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are
> >> >>expected
> >> >> @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
> >> >> ATTRIBUTE_RETURN_CHECK;
> >> >> VIR_FILE_CLOSE_PRESERVE_ERRNO | \
> >> >> VIR_FILE_CLOSE_DONT_LOG))
> >> >>
> >> >> +/**
> >> >> + * VIR_AUTOCLOSE:
> >> >> + * @fd: fd of the file to be closed automatically
> >> >> + *
> >> >> + * Macro to automatically force close the fd by calling
> >> >> virForceCloseHelper
> >> >> + * when the fd goes out of scope. It's used to eliminate
> >> >> VIR_FORCE_CLOSE
> >> >> + * in cleanup sections.
> >> >> + */
> >> >> +# define VIR_AUTOCLOSE(fd) \
> >> >> + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
> >> >
> >> >While this may helps us to initialize variables correctly, I think we
> >> >should do that explicitly. Not only it follows what VIR_AUTOFREE is
> >> >doing, it also is more visible when used. For instance, in 2/6 when the
> >> >macro is used for the first time, it's not visible what is @fd
> >> >initialized to.
> >>
> >> Okay. So I think the macro could be like:
> >>
> >> # define VIR_AUTOCLOSE \
> >> __attribute__((cleanup(virForceCloseHelper))) int
> >
> >Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros,
> >IOW, the form of VIR_AUTOCLOSE(fd) = is IMHO desired.
>
> Okay. Then the syntax-check rule would be simpler.
>
> >
> >>
> >> And the statement is like:
> >>
> >> VIR_AUTOCLOSE fd = -1;
> >> VIR_AUTOCLOSE sock_fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
> >>
> >> Also, I think I should add a new syntax-check rule to ensure the
> >> initialization
> >> of the variable. Just like sc_require_attribute_cleanup_initialization
> >> for VIR_AUTO(FREE|PTR).
> >
> >Yep, I agree with adding a similar syntax-check rule.
> >
> >Erik
>
> Okay. Just now, I find that we do not need to add a new rule. A minor change
> on
> sc_require_attribute_cleanup_initialization can work.
Even better.
Regards,
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically
On 2018-09-12 at 15:18, Erik Skultety wrote:
>On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
>> On 2018-09-11 at 20:22, Michal Privoznik wrote:
>> >On 09/10/2018 05:47 AM, Shi Lei wrote:
>> >> By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE
>> >> macro,
>> >> many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to
>> >> getting rid of many of our cleanup sections.
>> >>
>> >> Signed-off-by: Shi Lei
>> >> ---
>> >> src/util/virfile.h | 20 ++--
>> >> 1 file changed, 18 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/src/util/virfile.h b/src/util/virfile.h
>> >> index b30a1d3..70e7203 100644
>> >> --- a/src/util/virfile.h
>> >> +++ b/src/util/virfile.h
>> >> @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags)
>> >> int virFileFclose(FILE **file, bool preserve_errno)
>> >>ATTRIBUTE_RETURN_CHECK;
>> >> FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
>> >>
>> >> +static inline void virForceCloseHelper(int *_fd)
>> >
>> >No need for this argument to have underscore in its name.
>>
>> Okay.
>>
>> >
>> >> +{
>> >> + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO));
>> >> +}
>> >> +
>> >> /* For use on normal paths; caller must check return value,
>> >> and failure sets errno per close. */
>> >> # define VIR_CLOSE(FD) virFileClose(&(FD), 0)
>> >> @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
>> >> ATTRIBUTE_RETURN_CHECK;
>> >>
>> >> /* For use on cleanup paths; errno is unaffected by close,
>> >> and no return value to worry about. */
>> >> -# define VIR_FORCE_CLOSE(FD) \
>> >> - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO))
>> >> +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD))
>> >> # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE),
>> >>true))
>> >>
>> >> /* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are
>> >>expected
>> >> @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode)
>> >> ATTRIBUTE_RETURN_CHECK;
>> >> VIR_FILE_CLOSE_PRESERVE_ERRNO | \
>> >> VIR_FILE_CLOSE_DONT_LOG))
>> >>
>> >> +/**
>> >> + * VIR_AUTOCLOSE:
>> >> + * @fd: fd of the file to be closed automatically
>> >> + *
>> >> + * Macro to automatically force close the fd by calling
>> >> virForceCloseHelper
>> >> + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE
>> >> + * in cleanup sections.
>> >> + */
>> >> +# define VIR_AUTOCLOSE(fd) \
>> >> + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
>> >
>> >While this may helps us to initialize variables correctly, I think we
>> >should do that explicitly. Not only it follows what VIR_AUTOFREE is
>> >doing, it also is more visible when used. For instance, in 2/6 when the
>> >macro is used for the first time, it's not visible what is @fd
>> >initialized to.
>>
>> Okay. So I think the macro could be like:
>>
>> # define VIR_AUTOCLOSE \
>> __attribute__((cleanup(virForceCloseHelper))) int
>
>Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros,
>IOW, the form of VIR_AUTOCLOSE(fd) = is IMHO desired.
Okay. Then the syntax-check rule would be simpler.
>
>>
>> And the statement is like:
>>
>> VIR_AUTOCLOSE fd = -1;
>> VIR_AUTOCLOSE sock_fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
>>
>> Also, I think I should add a new syntax-check rule to ensure the
>> initialization
>> of the variable. Just like sc_require_attribute_cleanup_initialization
>> for VIR_AUTO(FREE|PTR).
>
>Yep, I agree with adding a similar syntax-check rule.
>
>Erik
Okay. Just now, I find that we do not need to add a new rule. A minor change on
sc_require_attribute_cleanup_initialization can work.
Shi Lei
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 0/3] Add virNetlinkNewLink helper for simplifying virNetDev*Create
On 2018-09-12 at 15:28, Erik Skultety wrote: >On Fri, Sep 07, 2018 at 03:17:23PM +0800, Shi Lei wrote: >> v2 here: >> https://www.redhat.com/archives/libvir-list/2018-September/msg00131.html >> >> Diff from V2: >> This series remove all unrelated changes. Those changes would be >> standalone patches. >> >> Shi Lei (3): >> util: netlink: Introduce virNetlinkNewLink helper >> util: netlink: Add wrapper macros to make virNetlinkNewLink more >> readable >> util: netlink: Using virNetlinkNewLink to simplify virNetDev*Create >> >> src/libvirt_private.syms | 1 + >> src/util/virnetdevbridge.c | 73 +++- >> src/util/virnetdevmacvlan.c | 110 +--- >> src/util/virnetlink.c | 104 ++ >> src/util/virnetlink.h | 43 ++ >> 5 files changed, 172 insertions(+), 159 deletions(-) >> > >So I applied the changes as discussed and merged. > >Regards, >Erik Okay. I have seen that when I git pull a moment ago. Thanks :-) Shi Lei -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/6] Introduce VIR_AUTOCLOSE macro for closing fd automatically
On 2018-09-12 at 15:24, Erik Skultety wrote:
>On Tue, Sep 11, 2018 at 11:49:11PM +0800, Shi Lei wrote:
>> On 2018-09-11 at 20:50, Erik Skultety wrote:
>> >On Mon, Sep 10, 2018 at 11:47:49AM +0800, Shi Lei wrote:
>> >> This series introduce VIR_AUTOCLOSE macro which can force close fd of the
>> >> file
>> >> automatically when the fd goes out of scope. It's used to eliminate
>> >> VIR_FORCE_CLOSE
>> >> in cleanup sections. The new macro consults VIR_AUTOFREE and VIR_AUTOPTR.
>> >>
>> >> Shi Lei (6):
>> >> util: file: introduce VIR_AUTOCLOSE macro to close fd of the file
>> >> automatically
>> >> util: file: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup
>> >> sections
>> >> util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in
>> >> cleanup sections
>> >> util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup
>> >> sections
>> >> phyp: driver: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup
>> >> sections
>> >> uml: conf: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup
>> >> sections
>> >>
>> >> src/phyp/phyp_driver.c | 29 ++---
>> >> src/uml/uml_conf.c | 13 +-
>> >> src/util/virfile.c | 21 +--
>> >> src/util/virfile.h | 20 ++-
>> >> src/util/virnetdev.c | 253 +
>> >> src/util/virnetdevbridge.c | 120 ++
>> >
>> >This touches only a handful of util modules and random drivers, there
>> >should be
>> >some consistency here, so if we're introducing something like this, then we
>> >should do it in a similar manner as we did for VIR_AUTO{FREE,PTR}, i.e.
>> >util/
>> >first (btw. we still haven't even finished that one yet).
>> >
>> >Erik
>>
>> Okay. But how to do in the v2 series.
>>
>> Whether v2 should only deal with files in the util/ ?
>
>Yeah, I think that's the best approach we can take, since we still haven't
>finished converting util to the other macros, we should start with the those
>modules within util/ that are already completely switched to AUTO{FREE/PTR} and
>similarly do it in batches, as reviewing 30+ patches on one go is always
>demanding...
>
>Thanks,
>Erik
Okay. I see.
Thanks.
Shi Lei
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 03/13] conf: Introduce a new PCI address extension flag
On Wed, 2018-09-12 at 13:27 +0800, Yi Min Zhao wrote:
> 在 2018/9/11 下午4:37, Andrea Bolognani 写道:
> > On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
> > [...]
> > > @@ -164,6 +164,7 @@ struct _virDomainDeviceInfo {
> > >* assignment, never saved and never reported.
> > >*/
> > > int pciConnectFlags; /* enum virDomainPCIConnectFlags */
> > > +int pciAddressExtFlags; /* enum virDomainPCIAddressExtensionFlags */
> >
> > There's a comment right above this that explains how pciConnectFlags
> > is only used during address assignment: you should amend it to
> > mention pciAddressExtFlags too.
>
> As your comment on the 1st patch, if we have virPCIDeviceAddress
> include a extFlag, why not remove this one?
Sure, if you can get away with it that's perfect! :)
However, I'm not entirely convinced you can avoid duplicating the
information, because I believe there will be at least some parts
of the address allocation algorithm where you'll need to access
the flags but haven't figured out you're going to assign a PCI
address to the device yet, which makes accessing the flags in
virPCIDeviceAddress not feasible. I could very well be wrong,
though: I haven't actually looked into it in detail.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
在 2018/9/11 下午7:34, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
This patch provides a caching mechanism for the device address
extensions uid and fid on S390. For efficient sparse address allocation,
we introduce two hash tables for uid/fid which hold the address set
information per domain. Also in order to improve performance of
searching available value, we introduce our own callbacks for the two
hashtables. In this way, uid/fid is saved in hash key and hash value
could be any non-NULL pointer due to no operation on hash value. That is
also the reason why we don't introduce hash value free callback.
Signed-off-by: Yi Min Zhao
Reviewed-by: Boris Fiuczynski
Reviewed-by: Bjoern Walk
Reviewed-by: Ján Tomko
---
src/conf/domain_addr.c | 85 ++
src/conf/domain_addr.h | 9 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain_address.c | 7
4 files changed, 102 insertions(+)
Okay, I've spent some time actually digging into the hash table
implementation this time around :)
[...]
+static uint32_t
+virZPCIAddrCode(const void *name,
+uint32_t seed)
+{
+unsigned int value = *((unsigned int *)name);
+return virHashCodeGen(, sizeof(value), seed);
+}
+
+
+static bool
+virZPCIAddrEqual(const void *namea,
+ const void *nameb)
+{
+return *((unsigned int *)namea) == *((unsigned int *)nameb);
+}
+
+
+static void *
+virZPCIAddrCopy(const void *name)
+{
+unsigned int *copy;
+
+if (VIR_ALLOC(copy) < 0)
+return NULL;
+
+*copy = *((unsigned int *)name);
+return (void *)copy;
+}
+
+
+static void
+virZPCIAddrKeyFree(void *name)
+{
+VIR_FREE(name);
+}
This makes sense and seems to work just fine; however, you are
allocating and releasing a bunch of small integers, which seems
a bit wasteful.
vircgroup is AFAICT avoiding all that extra memory management by
stuffing the values straight into the pointers themselves, which
you should also be able to do since the biggest legal ID is a
32-bit integer.
That said, I haven't been able to get that to actually work, at
least with a quick attempt :( Would you mind exploring that route
and figuring out whether it's feasible at all?
I'm testing this. Actually I wanted to do so like vircgroup. I
remembered there's
error due to the previous code logic. I will reply to you later.
[...]
+int
+virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
+ virDomainPCIAddressExtensionFlags
extFlags)
+{
+if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
+if (addrs->zpciIds)
+return 0;
This check doesn't look necessary.
All right.
[...]
+typedef struct {
+virHashTablePtr uids, fids;
+} virDomainZPCIAddressIds;
One member per line, please.
ok
[...]
+/* create zpci address set for s390 domain */
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) &&
+virDomainPCIAddressSetExtensionAlloc(addrs,
+ VIR_PCI_ADDRESS_EXTENSION_ZPCI)) {
+goto error;
+}
You should check for
virDomainPCIAddressSetExtensionAlloc() < 0
here.
Thanks for your comments.
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 0/3] Add virNetlinkNewLink helper for simplifying virNetDev*Create
On Fri, Sep 07, 2018 at 03:17:23PM +0800, Shi Lei wrote: > v2 here: > https://www.redhat.com/archives/libvir-list/2018-September/msg00131.html > > Diff from V2: > This series remove all unrelated changes. Those changes would be > standalone patches. > > Shi Lei (3): > util: netlink: Introduce virNetlinkNewLink helper > util: netlink: Add wrapper macros to make virNetlinkNewLink more > readable > util: netlink: Using virNetlinkNewLink to simplify virNetDev*Create > > src/libvirt_private.syms| 1 + > src/util/virnetdevbridge.c | 73 +++- > src/util/virnetdevmacvlan.c | 110 +--- > src/util/virnetlink.c | 104 ++ > src/util/virnetlink.h | 43 ++ > 5 files changed, 172 insertions(+), 159 deletions(-) > So I applied the changes as discussed and merged. Regards, Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
