Re: [PATCH] news: document `virsh console --resume` and `virsh (start|create) --console`
On Wed, Oct 25, 2023 at 12:24 PM +0200, Martin Kletzander wrote: > On Wed, Oct 25, 2023 at 11:03:54AM +0200, Marc Hartmayer wrote: >>Document the following changes: >> + added `virsh console --resume` subcommand option >> + improved `virsh start --console` behavior >> + improved `virsh create --console` behavior >> >>Signed-off-by: Marc Hartmayer > > Reviewed-by: Martin Kletzander > > and pushed. Thanks. […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH libvirt v1 0/3] Ensure full early console access with libvirt
On Tue, Oct 24, 2023 at 09:29 AM -0700, Andrea Bolognani wrote: > On Tue, Oct 24, 2023 at 05:14:41PM +0200, Marc Hartmayer wrote: >> On Tue, Oct 24, 2023 at 02:12 PM +0200, Michal Prívozník >> wrote: >> > On 9/28/23 17:37, Marc Hartmayer wrote: >> >> Marc Hartmayer (3): >> >> virsh: add `console --resume` support >> >> Improve `virsh start --console` behavior >> >> Improve `virsh create --console` behavior >> > >> > All 'issues' I've raised are trivial. I've fixed them and pushed. Sorry >> > for leaving this to rot this long on the list. >> > >> > Reviewed-by: Michal Privoznik >> >> Thanks a ton! > > Marc, > > can you please add a couple of lines about this change to the release > notes (NEWS.rst)? It's a really nice improvement and we definitely > want users to learn about it :) > > Thanks in advance! Done. > > -- > Andrea Bolognani / Red Hat / Virtualization > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
[PATCH] news: document `virsh console --resume` and `virsh (start|create) --console`
Document the following changes: + added `virsh console --resume` subcommand option + improved `virsh start --console` behavior + improved `virsh create --console` behavior Signed-off-by: Marc Hartmayer --- NEWS.rst | 15 +++ 1 file changed, 15 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index fbdd9674a329..9d00616cd9b1 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -25,8 +25,23 @@ v9.9.0 (unreleased) guest features to see if the current libvirt supports both deleting and reverting external snapshots. + * virsh: add ``console --resume`` support + +The ``virsh console`` subcommand now accepts a ``--resume`` option. This +will resume a paused guest after connecting to the console. + * **Improvements** + * virsh: Improve ``virsh start --console`` behavior + +The ``virsh start --console`` now tries to connect to the guest console +before starting the vCPUs. + + * virsh: Improve ``virsh create --console`` behavior + +The ``virsh create --console`` now tries to connect to the guest console +before starting the vCPUs. + * **Bug fixes** -- 2.34.1
Re: [PATCH libvirt v1 0/3] Ensure full early console access with libvirt
On Tue, Oct 24, 2023 at 02:12 PM +0200, Michal Prívozník wrote: > On 9/28/23 17:37, Marc Hartmayer wrote: >> Currently, early console output may be lost, e.g. if starting a guest with >> `virsh start --console` guest, which can make debugging of early failures >> very >> difficult >> (like zipl messages or disabled wait conditions happening early). This is >> because QEMU may emit serial console output before the libvirt console client >> starts to consume data from the pts. This can be prevented by starting the >> guest >> in paused state, connect to the console and then resume the guest. >> >> Note: There is still a problem in QEMU itself, see QEMU patch series `[PATCH] >> chardev/char-pty: Avoid losing bytes when the other side just (re-)connected` >> [1] >> >> Changelog: >> RFCv1->v1: >> + rebased on current master >> + worked in comments from Daniel >> >> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg02725.html >> >> Marc Hartmayer (3): >> virsh: add `console --resume` support >> Improve `virsh start --console` behavior >> Improve `virsh create --console` behavior >> >> tools/virsh-console.c | 8 >> tools/virsh-console.h | 1 + >> tools/virsh-domain.c | 94 --- >> 3 files changed, 80 insertions(+), 23 deletions(-) >> >> >> base-commit: dd403f8873cf8de7675b89ed757a4228af7bc05e > > All 'issues' I've raised are trivial. I've fixed them and pushed. Sorry > for leaving this to rot this long on the list. > > Reviewed-by: Michal Privoznik Thanks a ton! > > Michal > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH libvirt v1 0/3] Ensure full early console access with libvirt
On Wed, Oct 11, 2023 at 10:05 AM +0200, "Marc Hartmayer" wrote: > On Thu, Sep 28, 2023 at 05:37 PM +0200, Marc Hartmayer > wrote: >> Currently, early console output may be lost, e.g. if starting a guest with >> `virsh start --console` guest, which can make debugging of early failures >> very >> difficult >> (like zipl messages or disabled wait conditions happening early). This is >> because QEMU may emit serial console output before the libvirt console client >> starts to consume data from the pts. This can be prevented by starting the >> guest >> in paused state, connect to the console and then resume the guest. >> >> Note: There is still a problem in QEMU itself, see QEMU patch series `[PATCH] >> chardev/char-pty: Avoid losing bytes when the other side just (re-)connected` >> [1] This patch is now accepted upstream. >> >> Changelog: >> RFCv1->v1: >> + rebased on current master >> + worked in comments from Daniel >> >> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg02725.html >> >> Marc Hartmayer (3): >> virsh: add `console --resume` support >> Improve `virsh start --console` behavior >> Improve `virsh create --console` behavior >> >> tools/virsh-console.c | 8 >> tools/virsh-console.h | 1 + >> tools/virsh-domain.c | 94 --- >> 3 files changed, 80 insertions(+), 23 deletions(-) >> >> >> base-commit: dd403f8873cf8de7675b89ed757a4228af7bc05e >> -- >> 2.34.1 >> > > Polite ping and adding Daniel to CC - sry I missed that in the > beginning. > > -- > Kind regards / Beste Grüße >Marc Hartmayer > > IBM Deutschland Research & Development GmbH > Vorsitzender des Aufsichtsrats: Gregor Pillen > Geschäftsführung: David Faller > Sitz der Gesellschaft: Böblingen > Registergericht: Amtsgericht Stuttgart, HRB 243294 Very friendly ping. -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH libvirt v1 0/3] Ensure full early console access with libvirt
On Thu, Sep 28, 2023 at 05:37 PM +0200, Marc Hartmayer wrote: > Currently, early console output may be lost, e.g. if starting a guest with > `virsh start --console` guest, which can make debugging of early failures very > difficult > (like zipl messages or disabled wait conditions happening early). This is > because QEMU may emit serial console output before the libvirt console client > starts to consume data from the pts. This can be prevented by starting the > guest > in paused state, connect to the console and then resume the guest. > > Note: There is still a problem in QEMU itself, see QEMU patch series `[PATCH] > chardev/char-pty: Avoid losing bytes when the other side just (re-)connected` > [1] > > Changelog: > RFCv1->v1: > + rebased on current master > + worked in comments from Daniel > > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg02725.html > > Marc Hartmayer (3): > virsh: add `console --resume` support > Improve `virsh start --console` behavior > Improve `virsh create --console` behavior > > tools/virsh-console.c | 8 > tools/virsh-console.h | 1 + > tools/virsh-domain.c | 94 --- > 3 files changed, 80 insertions(+), 23 deletions(-) > > > base-commit: dd403f8873cf8de7675b89ed757a4228af7bc05e > -- > 2.34.1 > Polite ping and adding Daniel to CC - sry I missed that in the beginning. -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
[PATCH libvirt v1 3/3] Improve `virsh create --console` behavior
When starting a guest via libvirt (`virsh create --console`), early
console output was missed because the guest was started first and then
the console was attached. This patch changes this to the following
sequence:
1. create a paused transient guest
2. attach the console
3. resume the guest
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
tools/virsh-domain.c | 34 --
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 36670039444c..2f055df0d97d 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8212,6 +8212,13 @@ static const vshCmdOptDef opts_create[] = {
{.name = NULL}
};
+
+static virshDomain *virDomainCreateXMLHelper(virConnectPtr conn, const char
*xmlDesc, unsigned int nfds, int *fds, unsigned int flags) {
+ if (nfds)
+ return virDomainCreateXMLWithFiles(conn, xmlDesc, nfds, fds, flags);
+ return virDomainCreateXML(conn, xmlDesc, flags);
+}
+
static bool
cmdCreate(vshControl *ctl, const vshCmd *cmd)
{
@@ -8220,6 +8227,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
g_autofree char *buffer = NULL;
#ifndef WIN32
bool console = vshCommandOptBool(cmd, "console");
+bool resume_domain = false;
#endif
unsigned int flags = 0;
size_t nfds = 0;
@@ -8235,8 +8243,14 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
if (virshFetchPassFdsList(ctl, cmd, , ) < 0)
return false;
-if (vshCommandOptBool(cmd, "paused"))
+if (vshCommandOptBool(cmd, "paused")) {
flags |= VIR_DOMAIN_START_PAUSED;
+#ifndef WIN32
+} else if (console) {
+flags |= VIR_DOMAIN_START_PAUSED;
+resume_domain = true;
+#endif
+}
if (vshCommandOptBool(cmd, "autodestroy"))
flags |= VIR_DOMAIN_START_AUTODESTROY;
if (vshCommandOptBool(cmd, "validate"))
@@ -8244,10 +8258,18 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptBool(cmd, "reset-nvram"))
flags |= VIR_DOMAIN_START_RESET_NVRAM;
-if (nfds)
-dom = virDomainCreateXMLWithFiles(priv->conn, buffer, nfds, fds,
flags);
-else
-dom = virDomainCreateXML(priv->conn, buffer, flags);
+dom = virDomainCreateXMLHelper(priv->conn, buffer, nfds, fds, flags);
+#ifndef WIN32
+/* If the driver does not support the paused flag, let's fallback to the
old
+ * behavior without the flag. */
+if (!dom && resume_domain && last_error && last_error->code ==
VIR_ERR_INVALID_ARG) {
+ vshResetLibvirtError();
+
+ flags &= ~VIR_DOMAIN_START_PAUSED;
+ resume_domain = false;
+ dom = virDomainCreateXMLHelper(priv->conn, buffer, nfds, fds, flags);
+}
+#endif
if (!dom) {
vshError(ctl, _("Failed to create domain from %1$s"), from);
@@ -8258,7 +8280,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
virDomainGetName(dom), from);
#ifndef WIN32
if (console)
-cmdRunConsole(ctl, dom, NULL, false, 0);
+cmdRunConsole(ctl, dom, NULL, resume_domain, 0);
#endif
return true;
}
--
2.34.1
[PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
When starting a guest via libvirt (`virsh start --console`), early
console output was missed because the guest was started first and then
the console was attached. This patch changes this to the following
sequence:
1. create a paused guest
2. attach the console
3. resume the guest
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
tools/virsh-domain.c | 50 +++-
1 file changed, 35 insertions(+), 15 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5c3c6d18aebf..36670039444c 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4059,12 +4059,27 @@ static const vshCmdOptDef opts_start[] = {
{.name = NULL}
};
+static int
+virDomainCreateHelper(virDomainPtr dom, unsigned int nfds, int *fds,
+ unsigned int flags)
+{
+/* Prefer older API unless we have to pass a flag. */
+if (nfds > 0) {
+return virDomainCreateWithFiles(dom, nfds, fds, flags);
+} else if (flags != 0) {
+return virDomainCreateWithFlags(dom, flags);
+} else {
+return virDomainCreate(dom);
+}
+}
+
static bool
cmdStart(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
#ifndef WIN32
bool console = vshCommandOptBool(cmd, "console");
+bool resume_domain = false;
#endif
unsigned int flags = VIR_DOMAIN_NONE;
int rc;
@@ -4083,8 +4098,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
if (virshFetchPassFdsList(ctl, cmd, , ) < 0)
return false;
-if (vshCommandOptBool(cmd, "paused"))
+if (vshCommandOptBool(cmd, "paused")) {
flags |= VIR_DOMAIN_START_PAUSED;
+#ifndef WIN32
+} else if (console) {
+flags |= VIR_DOMAIN_START_PAUSED;
+resume_domain = true;
+#endif
+}
if (vshCommandOptBool(cmd, "autodestroy"))
flags |= VIR_DOMAIN_START_AUTODESTROY;
if (vshCommandOptBool(cmd, "bypass-cache"))
@@ -4096,12 +4117,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
/* We can emulate force boot, even for older servers that reject it. */
if (flags & VIR_DOMAIN_START_FORCE_BOOT) {
-if (nfds > 0) {
-rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
-} else {
-rc = virDomainCreateWithFlags(dom, flags);
-}
-
+rc = virDomainCreateHelper(dom, nfds, fds, flags);
if (rc == 0)
goto started;
@@ -4124,14 +4140,18 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
flags &= ~VIR_DOMAIN_START_FORCE_BOOT;
}
-/* Prefer older API unless we have to pass a flag. */
-if (nfds > 0) {
-rc = virDomainCreateWithFiles(dom, nfds, fds, flags);
-} else if (flags != 0) {
-rc = virDomainCreateWithFlags(dom, flags);
-} else {
-rc = virDomainCreate(dom);
+rc = virDomainCreateHelper(dom, nfds, fds, flags);
+#ifndef WIN32
+/* If the driver does not support the paused flag, let's fallback to the
old
+ * behavior without the flag. */
+if (rc < 0 && resume_domain && last_error && last_error->code ==
VIR_ERR_INVALID_ARG) {
+vshResetLibvirtError();
+
+flags &= ~VIR_DOMAIN_START_PAUSED;
+resume_domain = false;
+rc = virDomainCreateHelper(dom, nfds, fds, flags);
}
+#endif
if (rc < 0) {
vshError(ctl, _("Failed to start domain '%1$s'"),
virDomainGetName(dom));
@@ -4142,7 +4162,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
vshPrintExtra(ctl, _("Domain '%1$s' started\n"),
virDomainGetName(dom));
#ifndef WIN32
-if (console && !cmdRunConsole(ctl, dom, NULL, false, 0))
+if (console && !cmdRunConsole(ctl, dom, NULL, resume_domain, 0))
return false;
#endif
--
2.34.1
[PATCH libvirt v1 0/3] Ensure full early console access with libvirt
Currently, early console output may be lost, e.g. if starting a guest with `virsh start --console` guest, which can make debugging of early failures very difficult (like zipl messages or disabled wait conditions happening early). This is because QEMU may emit serial console output before the libvirt console client starts to consume data from the pts. This can be prevented by starting the guest in paused state, connect to the console and then resume the guest. Note: There is still a problem in QEMU itself, see QEMU patch series `[PATCH] chardev/char-pty: Avoid losing bytes when the other side just (re-)connected` [1] Changelog: RFCv1->v1: + rebased on current master + worked in comments from Daniel [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg02725.html Marc Hartmayer (3): virsh: add `console --resume` support Improve `virsh start --console` behavior Improve `virsh create --console` behavior tools/virsh-console.c | 8 tools/virsh-console.h | 1 + tools/virsh-domain.c | 94 --- 3 files changed, 80 insertions(+), 23 deletions(-) base-commit: dd403f8873cf8de7675b89ed757a4228af7bc05e -- 2.34.1
[PATCH libvirt v1 1/3] virsh: add `console --resume` support
This patch adds the command line flag `--resume` to the `virsh console`
command. This resumes a paused guest after connecting to the console.
This might be handy since it's a "common" pattern to start a guest
paused, connect to the console, and then resume it so as not to miss any
console messages.
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
tools/virsh-console.c | 8
tools/virsh-console.h | 1 +
tools/virsh-domain.c | 14 ++
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index 6bfb44a190ec..e44a070e7045 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -401,6 +401,7 @@ int
virshRunConsole(vshControl *ctl,
virDomainPtr dom,
const char *dev_name,
+const bool resume_domain,
unsigned int flags)
{
virConsole *con = NULL;
@@ -476,6 +477,13 @@ virshRunConsole(vshControl *ctl,
goto cleanup;
}
+if (resume_domain) {
+if (virDomainResume(dom) != 0) {
+vshError(ctl, _("Failed to resume domain '%1$s'"),
virDomainGetName(dom));
+goto cleanup;
+}
+}
+
while (!con->quit) {
if (virCondWait(>cond, >parent.lock) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
diff --git a/tools/virsh-console.h b/tools/virsh-console.h
index e89484d24bf4..2d00ed90cf4a 100644
--- a/tools/virsh-console.h
+++ b/tools/virsh-console.h
@@ -27,6 +27,7 @@
int virshRunConsole(vshControl *ctl,
virDomainPtr dom,
const char *dev_name,
+const bool resume_domain,
unsigned int flags);
#endif /* !WIN32 */
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 7abafe2ba30c..5c3c6d18aebf 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3012,6 +3012,10 @@ static const vshCmdOptDef opts_console[] = {
.type = VSH_OT_BOOL,
.help = N_("force console connection (disconnect already connected
sessions)")
},
+{.name = "resume",
+ .type = VSH_OT_BOOL,
+ .help = N_("resume a paused guest after connecting to console")
+},
{.name = "safe",
.type = VSH_OT_BOOL,
.help = N_("only connect if safe console handling is supported")
@@ -3022,6 +3026,7 @@ static const vshCmdOptDef opts_console[] = {
static bool
cmdRunConsole(vshControl *ctl, virDomainPtr dom,
const char *name,
+ const bool resume_domain,
unsigned int flags)
{
int state;
@@ -3048,7 +3053,7 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom,
vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
vshPrintExtra(ctl, "\n");
fflush(stdout);
-if (virshRunConsole(ctl, dom, name, flags) == 0)
+if (virshRunConsole(ctl, dom, name, resume_domain, flags) == 0)
return true;
return false;
@@ -3059,6 +3064,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
bool force = vshCommandOptBool(cmd, "force");
+bool resume = vshCommandOptBool(cmd, "resume");
bool safe = vshCommandOptBool(cmd, "safe");
unsigned int flags = 0;
const char *name = NULL;
@@ -3074,7 +3080,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
if (safe)
flags |= VIR_DOMAIN_CONSOLE_SAFE;
-return cmdRunConsole(ctl, dom, name, flags);
+return cmdRunConsole(ctl, dom, name, resume, flags);
}
#endif /* WIN32 */
@@ -4136,7 +4142,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
vshPrintExtra(ctl, _("Domain '%1$s' started\n"),
virDomainGetName(dom));
#ifndef WIN32
-if (console && !cmdRunConsole(ctl, dom, NULL, 0))
+if (console && !cmdRunConsole(ctl, dom, NULL, false, 0))
return false;
#endif
@@ -8232,7 +8238,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
virDomainGetName(dom), from);
#ifndef WIN32
if (console)
-cmdRunConsole(ctl, dom, NULL, 0);
+cmdRunConsole(ctl, dom, NULL, false, 0);
#endif
return true;
}
--
2.34.1
Re: [RFC PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
On Mon, Sep 25, 2023 at 04:15 PM +0100, Daniel P. Berrangé
wrote:
> On Mon, Sep 25, 2023 at 03:39:09PM +0200, Marc Hartmayer wrote:
>> When starting a guest via libvirt (`virsh start --console`), early
>> console output was missed because the guest was started first and then
>> the console was attached. This patch changes this to the following
>> sequence:
>>
>> 1. create a paused guest
>> 2. attach the console
>> 3. resume the guest
>>
>> Reviewed-by: Boris Fiuczynski
>> Signed-off-by: Marc Hartmayer
>> ---
>> tools/virsh-domain.c | 11 +--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index 5c3c6d18aebf..3581161c6f53 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -4065,6 +4065,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>> g_autoptr(virshDomain) dom = NULL;
>> #ifndef WIN32
>> bool console = vshCommandOptBool(cmd, "console");
>> +bool resume_domain = false;
>> #endif
>> unsigned int flags = VIR_DOMAIN_NONE;
>> int rc;
>> @@ -4083,8 +4084,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>> if (virshFetchPassFdsList(ctl, cmd, , ) < 0)
>> return false;
>>
>> -if (vshCommandOptBool(cmd, "paused"))
>> +if (vshCommandOptBool(cmd, "paused")) {
>> flags |= VIR_DOMAIN_START_PAUSED;
>> +#ifndef WIN32
>> +} else if (console) {
>> +flags |= VIR_DOMAIN_START_PAUSED;
>> +resume_domain = true;
>> +#endif
>
> Hypervisor drivers are not required to support VIR_DOMAIN_START_PAUSED.
>
> So we need to detect the error code, and retry without that flag
> set as a fallback. Same in next patch.
Yep, makes sense - will change. Thanks for the feedback.
[…snip]
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[RFC PATCH libvirt v1 0/3] Ensure full early console access with libvirt
Currently, early console output may be lost, e.g. if starting a guest with `virsh start --console` guest, which can make debugging of early failures very difficult (like zipl messages or disabled wait conditions happening early). This is because QEMU may emit serial console output before the libvirt console client starts to consume data from the pts. This can be prevented by starting the guest in paused state, connect to the console and then resume the guest. Note: There is still a problem in QEMU itself, see QEMU patch series `[PATCH] chardev/char-pty: Avoid losing bytes when the other side just (re-)connected` [1] [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg02725.html Marc Hartmayer (3): virsh: add `console --resume` support Improve `virsh start --console` behavior Improve `virsh create --console` behavior tools/virsh-console.c | 8 tools/virsh-console.h | 1 + tools/virsh-domain.c | 32 ++-- 3 files changed, 35 insertions(+), 6 deletions(-) base-commit: 3fd64fb0e236fc80ffa2cc977c0d471f11fc39bf -- 2.34.1
[RFC PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
When starting a guest via libvirt (`virsh start --console`), early
console output was missed because the guest was started first and then
the console was attached. This patch changes this to the following
sequence:
1. create a paused guest
2. attach the console
3. resume the guest
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
tools/virsh-domain.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5c3c6d18aebf..3581161c6f53 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4065,6 +4065,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
g_autoptr(virshDomain) dom = NULL;
#ifndef WIN32
bool console = vshCommandOptBool(cmd, "console");
+bool resume_domain = false;
#endif
unsigned int flags = VIR_DOMAIN_NONE;
int rc;
@@ -4083,8 +4084,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
if (virshFetchPassFdsList(ctl, cmd, , ) < 0)
return false;
-if (vshCommandOptBool(cmd, "paused"))
+if (vshCommandOptBool(cmd, "paused")) {
flags |= VIR_DOMAIN_START_PAUSED;
+#ifndef WIN32
+} else if (console) {
+flags |= VIR_DOMAIN_START_PAUSED;
+resume_domain = true;
+#endif
+}
if (vshCommandOptBool(cmd, "autodestroy"))
flags |= VIR_DOMAIN_START_AUTODESTROY;
if (vshCommandOptBool(cmd, "bypass-cache"))
@@ -4142,7 +4149,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
vshPrintExtra(ctl, _("Domain '%1$s' started\n"),
virDomainGetName(dom));
#ifndef WIN32
-if (console && !cmdRunConsole(ctl, dom, NULL, false, 0))
+if (console && !cmdRunConsole(ctl, dom, NULL, resume_domain, 0))
return false;
#endif
--
2.34.1
[RFC PATCH libvirt v1 3/3] Improve `virsh create --console` behavior
When starting a guest via libvirt (`virsh create --console`), early
console output was missed because the guest was started first and then
the console was attached. This patch changes this to the following
sequence:
1. create a paused transient guest
2. attach the console
3. resume the guest
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
tools/virsh-domain.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 3581161c6f53..5a97d44190c4 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8207,6 +8207,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
g_autofree char *buffer = NULL;
#ifndef WIN32
bool console = vshCommandOptBool(cmd, "console");
+bool resume_domain = false;
#endif
unsigned int flags = 0;
size_t nfds = 0;
@@ -8222,8 +8223,14 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
if (virshFetchPassFdsList(ctl, cmd, , ) < 0)
return false;
-if (vshCommandOptBool(cmd, "paused"))
+if (vshCommandOptBool(cmd, "paused")) {
flags |= VIR_DOMAIN_START_PAUSED;
+#ifndef WIN32
+} else if (console) {
+flags |= VIR_DOMAIN_START_PAUSED;
+resume_domain = true;
+#endif
+}
if (vshCommandOptBool(cmd, "autodestroy"))
flags |= VIR_DOMAIN_START_AUTODESTROY;
if (vshCommandOptBool(cmd, "validate"))
@@ -8245,7 +8252,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
virDomainGetName(dom), from);
#ifndef WIN32
if (console)
-cmdRunConsole(ctl, dom, NULL, false, 0);
+cmdRunConsole(ctl, dom, NULL, resume_domain, 0);
#endif
return true;
}
--
2.34.1
[RFC PATCH libvirt v1 1/3] virsh: add `console --resume` support
This patch adds the command line flag `--resume` to the `virsh console`
command. This resumes a paused guest after connecting to the console.
This might be handy since it's a "common" pattern to start a guest
paused, connect to the console, and then resume it so as not to miss any
console messages.
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
tools/virsh-console.c | 8
tools/virsh-console.h | 1 +
tools/virsh-domain.c | 14 ++
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index 6bfb44a190ec..e44a070e7045 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -401,6 +401,7 @@ int
virshRunConsole(vshControl *ctl,
virDomainPtr dom,
const char *dev_name,
+const bool resume_domain,
unsigned int flags)
{
virConsole *con = NULL;
@@ -476,6 +477,13 @@ virshRunConsole(vshControl *ctl,
goto cleanup;
}
+if (resume_domain) {
+if (virDomainResume(dom) != 0) {
+vshError(ctl, _("Failed to resume domain '%1$s'"),
virDomainGetName(dom));
+goto cleanup;
+}
+}
+
while (!con->quit) {
if (virCondWait(>cond, >parent.lock) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
diff --git a/tools/virsh-console.h b/tools/virsh-console.h
index e89484d24bf4..2d00ed90cf4a 100644
--- a/tools/virsh-console.h
+++ b/tools/virsh-console.h
@@ -27,6 +27,7 @@
int virshRunConsole(vshControl *ctl,
virDomainPtr dom,
const char *dev_name,
+const bool resume_domain,
unsigned int flags);
#endif /* !WIN32 */
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 7abafe2ba30c..5c3c6d18aebf 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3012,6 +3012,10 @@ static const vshCmdOptDef opts_console[] = {
.type = VSH_OT_BOOL,
.help = N_("force console connection (disconnect already connected
sessions)")
},
+{.name = "resume",
+ .type = VSH_OT_BOOL,
+ .help = N_("resume a paused guest after connecting to console")
+},
{.name = "safe",
.type = VSH_OT_BOOL,
.help = N_("only connect if safe console handling is supported")
@@ -3022,6 +3026,7 @@ static const vshCmdOptDef opts_console[] = {
static bool
cmdRunConsole(vshControl *ctl, virDomainPtr dom,
const char *name,
+ const bool resume_domain,
unsigned int flags)
{
int state;
@@ -3048,7 +3053,7 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom,
vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
vshPrintExtra(ctl, "\n");
fflush(stdout);
-if (virshRunConsole(ctl, dom, name, flags) == 0)
+if (virshRunConsole(ctl, dom, name, resume_domain, flags) == 0)
return true;
return false;
@@ -3059,6 +3064,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
{
g_autoptr(virshDomain) dom = NULL;
bool force = vshCommandOptBool(cmd, "force");
+bool resume = vshCommandOptBool(cmd, "resume");
bool safe = vshCommandOptBool(cmd, "safe");
unsigned int flags = 0;
const char *name = NULL;
@@ -3074,7 +3080,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
if (safe)
flags |= VIR_DOMAIN_CONSOLE_SAFE;
-return cmdRunConsole(ctl, dom, name, flags);
+return cmdRunConsole(ctl, dom, name, resume, flags);
}
#endif /* WIN32 */
@@ -4136,7 +4142,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
vshPrintExtra(ctl, _("Domain '%1$s' started\n"),
virDomainGetName(dom));
#ifndef WIN32
-if (console && !cmdRunConsole(ctl, dom, NULL, 0))
+if (console && !cmdRunConsole(ctl, dom, NULL, false, 0))
return false;
#endif
@@ -8232,7 +8238,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
virDomainGetName(dom), from);
#ifndef WIN32
if (console)
-cmdRunConsole(ctl, dom, NULL, 0);
+cmdRunConsole(ctl, dom, NULL, false, 0);
#endif
return true;
}
--
2.34.1
Re: [PATCH] tests: viracpitest only works on little endian
Boris Fiuczynski writes:
> Commit fc216db4fb789cbd309 introduced a mocked test with binary test data
> which fails on big endian machines.
> Therefore build the viracpitest test only on little endian machines.
>
> Fixes: fc216db4fb789cbd30917be036d0b94d965bdf7f
>
> Signed-off-by: Boris Fiuczynski
> ---
> tests/meson.build | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tests/meson.build b/tests/meson.build
> index 35adbc2d56..0082446029 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -269,7 +269,6 @@ tests += [
>{ 'name': 'storagevolxml2xmltest' },
>{ 'name': 'sysinfotest' },
>{ 'name': 'utiltest' },
> - { 'name': 'viracpitest' },
>{ 'name': 'viralloctest' },
>{ 'name': 'virauthconfigtest' },
>{ 'name': 'virbitmaptest' },
> @@ -308,6 +307,12 @@ tests += [
>{ 'name': 'virmigtest' },
> ]
>
> +if host_machine.endian() == 'little'
> + tests += [
> +{ 'name': 'viracpitest' },
> + ]
> +endif
> +
> if host_machine.system() == 'linux'
>tests += [
> { 'name': 'fchosttest' },
> --
> 2.39.0
LGTM.
Reviewed-by: Marc Hartmayer
Re: [PATCH] qemuValidateDomainDef: Clarify error message when S390 PV launch security is unsupported by the kernel
Peter Krempa writes:
> Split up the condition and report a different error message when the
> host or host config results in S390 PV launch security being
> unavailable.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2122534
> Signed-off-by: Peter Krempa
> ---
> src/qemu/qemu_validate.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 6403266559..63f3459c90 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -1454,11 +1454,14 @@ qemuValidateDomainDef(const virDomainDef *def,
> break;
> case VIR_DOMAIN_LAUNCH_SECURITY_PV:
> if (!virQEMUCapsGet(qemuCaps,
> QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT) ||
> -!virQEMUCapsGet(qemuCaps, QEMU_CAPS_S390_PV_GUEST) ||
> -!virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps)) {
> +!virQEMUCapsGet(qemuCaps, QEMU_CAPS_S390_PV_GUEST)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("S390 PV launch security is not supported
> with "
> - "this QEMU binary"));
> + _("S390 PV launch security is not supported
> with this QEMU binary"));
> +return -1;
> +}
> +if (!virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("S390 PV launch security is not supported
> by this host or kernel"));
Not sure if the error message is clear enough… PV also depends on the
kernel cmdline opt-in - `prot_virt=1` has to be set.
> return -1;
> }
> break;
> --
> 2.37.1
>
Reviewed-by: Marc Hartmayer
Thanks for fixing this.
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH 0/2] virtiofs can be used without NUMA
On Tue, Oct 13, 2020 at 07:10 PM +0200, Michal Privoznik wrote: > On 10/13/20 6:53 PM, Marc Hartmayer wrote: >> Halil Pasic (1): >>Reflect in virtiofs.rst that virtiofs can be used without NUMA >> >> Marc Hartmayer (1): >>qemu: virtiofs can be used without NUMA nodes >> >> docs/kbase/virtiofs.rst | 17 - >> src/qemu/qemu_validate.c | 13 + >> 2 files changed, 21 insertions(+), 9 deletions(-) >> > > Reviewed-by: Michal Privoznik > > and pushed. > > Michal > Thanks. -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
[PATCH 2/2] Reflect in virtiofs.rst that virtiofs can be used without NUMA
From: Halil Pasic Reflect in the virtiofs documentation that virtiofs can now be used even without NUMA. While at it, be more precise where and why shared memory is required. Signed-off-by: Halil Pasic Signed-off-by: Marc Hartmayer --- docs/kbase/virtiofs.rst | 17 - 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/kbase/virtiofs.rst b/docs/kbase/virtiofs.rst index dea8e79f833c..01440420d76d 100644 --- a/docs/kbase/virtiofs.rst +++ b/docs/kbase/virtiofs.rst @@ -16,10 +16,17 @@ See https://virtio-fs.gitlab.io/ Host setup == -The host-side virtiofsd daemon, like other vhost-user backed devices, -requires shared memory between the host and the guest. As of QEMU 4.2, this -requires specifying a NUMA topology for the guest and explicitly specifying -a memory backend. Multiple options are available: +Almost all virtio devices (all that use virtqueues) require access to +at least certain portions of guest RAM (possibly policed by DMA). In +case of virtiofsd, much like in case of other vhost-user (see +https://www.qemu.org/docs/master/interop/vhost-user.html) virtio +devices that are realized by an userspace process, this in practice +means that QEMU needs to allocate the backing memory for all the guest +RAM as shared memory. As of QEMU 4.2, it is possible to explicitly +specify a memory backend when specifying the NUMA topology. This +method is however only viable for machine types that do support +NUMA. As of QEMU 5.0.0, it is possible to specify the memory backend +without NUMA (using the so called memobject interface). Either of the following: @@ -46,7 +53,7 @@ Either of the following: Guest setup === -#. Specify the NUMA topology +#. Specify the NUMA topology (this step is only required for the NUMA case) in the domain XML of the guest. For the simplest one-node topology for a guest with 2GiB of RAM and 8 vCPUs: -- 2.25.4
[PATCH 1/2] qemu: virtiofs can be used without NUMA nodes
...if a machine memory-backend using shared memory is configured for
the guest. This is especially important for QEMU machine types that
don't have NUMA but virtiofs support.
An example snippet:
test
2097152
...
...
and the corresponding QEMU command line:
/usr/bin/qemu-system-s390x \
-machine s390-ccw-virtio-5.2,memory-backend=s390.ram \
-m 2048 \
-object
memory-backend-file,id=s390.ram,mem-path=/var/lib/libvirt/qemu/ram/46-test/s390.ram,share=yes,size=2147483648
\
...
Signed-off-by: Marc Hartmayer
---
src/qemu/qemu_validate.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 27e10d59fd25..bc3043bb3f0d 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3470,14 +3470,19 @@ qemuValidateDomainDeviceDefGraphics(const
virDomainGraphicsDef *graphics,
static int
-qemuValidateDomainDefVirtioFSSharedMemory(const virDomainDef *def)
+qemuValidateDomainDefVirtioFSSharedMemory(const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
{
+const char *defaultRAMId = virQEMUCapsGetMachineDefaultRAMid(qemuCaps,
+ def->virtType,
+
def->os.machine);
size_t numa_nodes = virDomainNumaGetNodeCount(def->numa);
size_t i;
-if (numa_nodes == 0) {
+if (numa_nodes == 0 &&
+!(defaultRAMId && def->mem.access == VIR_DOMAIN_MEMORY_ACCESS_SHARED))
{
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("virtiofs requires one or more NUMA nodes"));
+ _("virtiofs requires shared memory"));
return -1;
}
@@ -3591,7 +3596,7 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
_("virtiofs does not support multidevs"));
return -1;
}
-if (qemuValidateDomainDefVirtioFSSharedMemory(def) < 0)
+if (qemuValidateDomainDefVirtioFSSharedMemory(def, qemuCaps) < 0)
return -1;
break;
--
2.25.4
[PATCH 0/2] virtiofs can be used without NUMA
Halil Pasic (1): Reflect in virtiofs.rst that virtiofs can be used without NUMA Marc Hartmayer (1): qemu: virtiofs can be used without NUMA nodes docs/kbase/virtiofs.rst | 17 - src/qemu/qemu_validate.c | 13 + 2 files changed, 21 insertions(+), 9 deletions(-) -- 2.25.4
Re: [RFC] qemu: virtiofs can be used without NUMA nodes
On Tue, Oct 06, 2020 at 06:20 PM +0200, Marc Hartmayer
wrote:
> ...if a machine memory-backend using shared memory is configured for
> the guest. This is especially important for QEMU machine types that
> don't have NUMA but virtiofs support.
>
> An example snippet:
>
>
> test
> 2097152
>
>
>
>
>
>
>
>
>
> ...
>
> ...
>
>
> and the corresponding QEMU command line:
>
> /usr/bin/qemu-system-s390x \
> -machine s390-ccw-virtio-5.2,memory-backend=s390.ram \
> -m 2048 \
> -object
>
> memory-backend-file,id=s390.ram,mem-path=/var/lib/libvirt/qemu/ram/46-test/s390.ram,share=yes,size=2147483648
> \
> ...
>
> Signed-off-by: Marc Hartmayer
> ---
> Note: There are still some TODOs left... e.g. adapt the virtiofs
> documentation of libvirt.
> ---
> src/qemu/qemu_validate.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index a212605579d2..077a85b30802 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -3470,14 +3470,21 @@ qemuValidateDomainDeviceDefGraphics(const
> virDomainGraphicsDef *graphics,
>
>
> static int
> -qemuValidateDomainDefVirtioFSSharedMemory(const virDomainDef *def)
> +qemuValidateDomainDefVirtioFSSharedMemory(const virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> {
> +const char *defaultRAMId = virQEMUCapsGetMachineDefaultRAMid(qemuCaps,
> +
> def->virtType,
> +
> def->os.machine);
> size_t numa_nodes = virDomainNumaGetNodeCount(def->numa);
> size_t i;
>
> -if (numa_nodes == 0) {
> +if (numa_nodes == 0 &&
> +!(defaultRAMId && def->mem.access ==
> VIR_DOMAIN_MEMORY_ACCESS_SHARED)) {
> +/* TODO do we need further checks here (e.g. check whether
> + * memory backend is supported by the QEMU binary)? */
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("virtiofs requires one or more NUMA nodes"));
> + _("virtiofs requires shared memory"));
> return -1;
> }
>
> @@ -3591,7 +3598,7 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
> _("virtiofs does not support multidevs"));
> return -1;
> }
> -if (qemuValidateDomainDefVirtioFSSharedMemory(def) < 0)
> +if (qemuValidateDomainDefVirtioFSSharedMemory(def, qemuCaps) < 0)
> return -1;
> break;
>
> --
> 2.25.4
>
Gentle ping :)
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
[RFC] qemu: virtiofs can be used without NUMA nodes
...if a machine memory-backend using shared memory is configured for
the guest. This is especially important for QEMU machine types that
don't have NUMA but virtiofs support.
An example snippet:
test
2097152
...
...
and the corresponding QEMU command line:
/usr/bin/qemu-system-s390x \
-machine s390-ccw-virtio-5.2,memory-backend=s390.ram \
-m 2048 \
-object
memory-backend-file,id=s390.ram,mem-path=/var/lib/libvirt/qemu/ram/46-test/s390.ram,share=yes,size=2147483648
\
...
Signed-off-by: Marc Hartmayer
---
Note: There are still some TODOs left... e.g. adapt the virtiofs
documentation of libvirt.
---
src/qemu/qemu_validate.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index a212605579d2..077a85b30802 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3470,14 +3470,21 @@ qemuValidateDomainDeviceDefGraphics(const
virDomainGraphicsDef *graphics,
static int
-qemuValidateDomainDefVirtioFSSharedMemory(const virDomainDef *def)
+qemuValidateDomainDefVirtioFSSharedMemory(const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
{
+const char *defaultRAMId = virQEMUCapsGetMachineDefaultRAMid(qemuCaps,
+ def->virtType,
+
def->os.machine);
size_t numa_nodes = virDomainNumaGetNodeCount(def->numa);
size_t i;
-if (numa_nodes == 0) {
+if (numa_nodes == 0 &&
+!(defaultRAMId && def->mem.access == VIR_DOMAIN_MEMORY_ACCESS_SHARED))
{
+/* TODO do we need further checks here (e.g. check whether
+ * memory backend is supported by the QEMU binary)? */
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("virtiofs requires one or more NUMA nodes"));
+ _("virtiofs requires shared memory"));
return -1;
}
@@ -3591,7 +3598,7 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
_("virtiofs does not support multidevs"));
return -1;
}
-if (qemuValidateDomainDefVirtioFSSharedMemory(def) < 0)
+if (qemuValidateDomainDefVirtioFSSharedMemory(def, qemuCaps) < 0)
return -1;
break;
--
2.25.4
[PATCH] udevProcessCSS: fix segfault
Don't process subchannel devices where `def->driver` is not set. This
fixes the following segfault:
Thread 21 "nodedev-init" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x3ffb08fc910 (LWP 64303)]
(gdb) bt
#0 0x03fffd1272b4 in __strcmp_vx () at /lib64/libc.so.6
#1 0x03ffc260c3a8 in udevProcessCSS (device=0x3ff9018d130,
def=0x3ff90194a90)
#2 0x03ffc260cb78 in udevGetDeviceDetails (device=0x3ff9018d130,
def=0x3ff90194a90)
#3 0x03ffc260d126 in udevAddOneDevice (device=0x3ff9018d130)
#4 0x03ffc260d414 in udevProcessDeviceListEntry (udev=0x3ffa810d800,
list_entry=0x3ff90001990)
#5 0x03ffc260d638 in udevEnumerateDevices (udev=0x3ffa810d800)
#6 0x03ffc260e08e in nodeStateInitializeEnumerate (opaque=0x3ffa810d800)
#7 0x03fffdaa14b6 in virThreadHelper (data=0x3ffa810df00)
#8 0x03fffc309ed6 in start_thread ()
#9 0x03fffd185e66 in thread_start ()
(gdb) p *def
$2 = {
name = 0x0,
sysfs_path = 0x3ff90198e80 "/sys/devices/css0/0.0.ff40",
parent = 0x0,
parent_sysfs_path = 0x0,
parent_wwnn = 0x0,
parent_wwpn = 0x0,
parent_fabric_wwn = 0x0,
driver = 0x0,
devnode = 0x0,
devlinks = 0x3ff90194670,
caps = 0x3ff90194380
}
Fixes: 05e6cdafa6e0 ("node_device: detect CSS devices")
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 5f2841bb7d8e..12e3f30badd1 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1130,8 +1130,9 @@ udevProcessCSS(struct udev_device *device,
virNodeDeviceDefPtr def)
{
/* only process IO subchannel and vfio-ccw devices to keep the list sane */
-if (STRNEQ(def->driver, "io_subchannel") &&
-STRNEQ(def->driver, "vfio_ccw"))
+if (!def->driver ||
+(STRNEQ(def->driver, "io_subchannel") &&
+ STRNEQ(def->driver, "vfio_ccw")))
return -1;
if (udevGetCCWAddress(def->sysfs_path, >caps->data) < 0)
--
2.25.4
Re: [PATCH 0/3] Avoid some GCC 10 warnings
On Mon, Aug 17, 2020 at 08:22 AM +0200, Erik Skultety wrote: > On Thu, Aug 13, 2020 at 04:03:43PM +0200, Boris Fiuczynski wrote: >> Caught these when switching to F32 using GCC v10.2.1 on s390x. >> >> Boris Fiuczynski (3): >> qemu: avoid maybe-uninitialized warning by GCC 10 >> tools: avoid potential null pointer dereference by GCC 10 >> storage: avoid maybe-uninitialized warning by GCC 10 >> >> src/qemu/qemu_migration.c | 2 +- >> src/storage/storage_backend_iscsi_direct.c | 8 >> tools/vsh.c| 2 +- >> 3 files changed, 6 insertions(+), 6 deletions(-) >> >> -- >> 2.25.1 >> > > Reviewed-by: Erik Skultety > Thanks. -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH 3/4] virdevmapper: Don't use libdevmapper to obtain dependencies
On Tue, Aug 04, 2020 at 11:39 PM +0200, Andrea Bolognani
wrote:
> On Mon, 2020-07-27 at 10:31 +0200, Michal Privoznik wrote:
>> CVE-2020-14339
>>
>> When building domain's private /dev in a namespace, libdevmapper
>> is consulted for getting full dependency tree of domain's disks.
>> The reason is that for a multipath devices all dependent devices
>> must be created in the namespace and allowed in CGroups.
>>
>> However, this approach is very fragile as building of namespace
>> happens in the forked off child process, after mass close of FDs
>> and just before dropping privileges and execing QEMU. And it so
>> happens that when calling libdevmapper APIs, one of them opens
>> /dev/mapper/control and saves the FD into a global variable. The
>> FD is kept open until the lib is unlinked or dm_lib_release() is
>> called explicitly. We are doing neither.
>>
>> However, the virDevMapperGetTargets() function is called also
>> from libvirtd (when setting up CGroups) and thus has to be thread
>> safe. Unfortunately, libdevmapper APIs are not thread safe (nor
>> async signal safe) and thus we can't use them. Reimplement what
>> libdevmapper would do using plain C (ioctl()-s, /proc/devices
>> parsing, /dev/mapper dirwalking, and so on).
>>
>> Fixes: a30078cb832646177defd256e77c632905f1e6d0
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1858260
>>
>> Signed-off-by: Michal Privoznik
>> Reviewed-by: Daniel P. Berrangé
>> ---
>> po/POTFILES.in | 1 +
>> src/util/virdevmapper.c | 304
>> 2 files changed, 219 insertions(+), 86 deletions(-)
>
> This patch broke libvirt in Debian for certain setups.
>
> With AppArmor enabled (the default), the error is
>
> $ virsh start cirros
> error: Failed to start domain cirros
> error: internal error: Process exited prior to exec: libvirt:
> error : Cannot delete directory '/run/libvirt/qemu/1-cirros.dev':
> Device or resource busy
>
> If I disable AppArmor by passing security='' on the kernel command
> line, the error message changes to
>
> $ virsh start cirros
> error: Failed to start domain cirros
> error: internal error: Process exited prior to exec: libvirt:
> QEMU Driver error : Unable to get devmapper targets for
> /var/lib/libvirt/images/cirros.qcow2: Success
>
> An effective workaround is to set namespaces=[] in qemu.conf, but
> that's of course not something that we want users doing :)
>
> The underlying issue seems to be caused by the fact that, on a Debian
> installation that uses plain partitions instead of LVM, /proc/devices
> doesn't contain an entry for device-mapper right after boot, which...
>
>> static int
>> virDevMapperOnceInit(void)
>> {
>> -/* Ideally, we would not need this. But libdevmapper prints
>> - * error messages to stderr by default. Sad but true. */
>> -dm_log_with_errno_init(virDevMapperDummyLogger);
>> +g_autofree char *buf = NULL;
>> +VIR_AUTOSTRINGLIST lines = NULL;
>> +size_t i;
>> +
>> +if (virFileReadAll(PROC_DEVICES, BUF_SIZE, ) < 0)
>> +return -1;
>> +
>> +lines = virStringSplit(buf, "\n", 0);
>> +if (!lines)
>> +return -1;
>> +
>> +for (i = 0; lines[i]; i++) {
>> +g_autofree char *dev = NULL;
>> +unsigned int maj;
>> +
>> +if (sscanf(lines[i], "%u %ms\n", , ) == 2 &&
>> +STREQ(dev, DM_NAME)) {
>> +virDMMajor = maj;
>> +break;
>> +}
>> +}
>> +
>> +if (!lines[i]) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Unable to find major for %s"),
>> + DM_NAME);
>> +return -1;
>> +}
>
> ... this code expects.
>
> Running
>
> $ sudo dmsetup info
> No devices found
We see the same problem as mentioned by Andrea. The host kernel
configuration used:
…
CONFIG_BLK_DEV_DM_BUILTIN=y
CONFIG_BLK_DEV_DM=m
…
As soon as we load the kernel module ‘dm-mod‘ everything works because
then ‘device-mapper‘ is listed in /proc/devices.
>
> causes the entry to appear, and from that moment on guest startup
> will work as expected regardless of whether or not AppArmor is
> enabled.
>
> I hope the information above can help someone who's familiar with the
> code figure out a fix. I'll provide more if needed, just ask! I can
> also provide prebuilt .deb files for 6.6.0 that consistently trigger
> the issue when added to a bog standard Debian testing installation.
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH v2 3/4] qemu: command: support for virtio packed option
ons.xml
> index dd9a4f4a..3ca27840 100644
> --- a/tests/qemuxml2argvdata/virtio-options.xml
> +++ b/tests/qemuxml2argvdata/virtio-options.xml
> @@ -15,7 +15,7 @@
>
> /usr/bin/qemu-system-x86_64
>
> -
> +
>
>
> function='0x0'/>
> @@ -27,22 +27,22 @@
> function='0x1'/>
>
>
> -
> +
> function='0x0'/>
>
>
>
> -
> +
> function='0x0'/>
>
>
> -
> +
>
>
> function='0x0'/>
>
>
> -
> + packed='on'/>
>
>
> function='0x0'/>
> @@ -50,30 +50,30 @@
>
>
>
> -
> +
> function='0x0'/>
>
>
> -
> +
> function='0x0'/>
>
>
> -
> +
> function='0x0'/>
>
>
> -
> +
> function='0x0'/>
>
>
> -
> +
>
> function='0x0'/>
>
>
>
>
> -
> +
>
>
>
> @@ -81,11 +81,11 @@
>
>
> function='0x0'/>
> -
> +
>
>
>/dev/random
> -
> +
> function='0x0'/>
>
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index fdeb3c2e..405227fd 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2989,7 +2989,8 @@ mymain(void)
> QEMU_CAPS_OBJECT_RNG_RANDOM,
> QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM,
> -QEMU_CAPS_VIRTIO_PCI_ATS);
> +QEMU_CAPS_VIRTIO_PCI_ATS,
> +QEMU_CAPS_VIRTIO_PACKED_QUEUES);
>
> DO_TEST("fd-memory-numa-topology", QEMU_CAPS_OBJECT_MEMORY_FILE,
> QEMU_CAPS_KVM);
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index b4c83fcc..756d4e2c 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -1291,7 +1291,8 @@ mymain(void)
> QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM,
> QEMU_CAPS_VIRTIO_PCI_ATS,
> -QEMU_CAPS_DEVICE_VHOST_USER_GPU);
> +QEMU_CAPS_DEVICE_VHOST_USER_GPU,
> +QEMU_CAPS_VIRTIO_PACKED_QUEUES);
>
> DO_TEST("fd-memory-numa-topology", QEMU_CAPS_OBJECT_MEMORY_FILE,
> QEMU_CAPS_KVM);
> --
> 2.24.1
Reviewed-by: Marc Hartmayer
Re: [PATCH v2 2/4] conf: domain: support for virtio packed option
On Mon, Apr 06, 2020 at 03:13 PM +0200, Bjoern Walk wrote:
> Expose the virtio parameter for packed virtqueues as an optional libvirt
> XML attribute to virtio-backed devices, e.g.:
>
>
>
>
>
>
>
> If the attribute is omitted, the default value for this attribute is 'off' and
> regular split virtqueues are used.
>
> Reviewed-by: Ján Tomko
> Reviewed-by: Boris Fiuczynski
> Signed-off-by: Bjoern Walk
> ---
> docs/schemas/domaincommon.rng | 5 +
> src/conf/domain_conf.c| 28
> src/conf/domain_conf.h| 1 +
> 3 files changed, 34 insertions(+)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index dcf2e09d..12e842ce 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -5961,6 +5961,11 @@
>
>
>
> +
> +
> +
> +
> +
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 460f8064..51fd5f15 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1513,6 +1513,16 @@ virDomainVirtioOptionsParseXML(xmlNodePtr driver,
> }
> res->ats = val;
> }
> +VIR_FREE(str);
> +
> +if ((str = virXMLPropString(driver, "packed"))) {
> +if ((val = virTristateSwitchTypeFromString(str)) <= 0) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("invalid packed value"));
> +return -1;
> +}
> +res->packed = val;
> +}
>
> return 0;
> }
> @@ -5092,6 +5102,12 @@ virDomainCheckVirtioOptions(virDomainVirtioOptionsPtr
> virtio)
> "for virtio devices"));
> return -1;
> }
> +if (virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("packed driver option is only supported "
> + "for virtio devices"));
> +return -1;
> +}
> return 0;
> }
>
> @@ -7378,6 +7394,10 @@ virDomainVirtioOptionsFormat(virBufferPtr buf,
> virBufferAsprintf(buf, " ats='%s'",
>virTristateSwitchTypeToString(virtio->ats));
> }
> +if (virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
> +virBufferAsprintf(buf, " packed='%s'",
> + virTristateSwitchTypeToString(virtio->packed));
> +}
> }
>
>
> @@ -22416,6 +22436,14 @@
> virDomainVirtioOptionsCheckABIStability(virDomainVirtioOptionsPtr src,
> virTristateSwitchTypeToString(src->ats));
> return false;
> }
> +if (src->packed != dst->packed) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Target device packed option '%s' does not "
> + "match source '%s'"),
> + virTristateSwitchTypeToString(dst->packed),
> + virTristateSwitchTypeToString(src->packed));
> +return false;
> + }
> return true;
> }
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 2038b54c..22f6990e 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2417,6 +2417,7 @@ struct _virDomainVsockDef {
> struct _virDomainVirtioOptions {
> virTristateSwitch iommu;
> virTristateSwitch ats;
> +virTristateSwitch packed;
> };
>
> /*
> --
> 2.24.1
Reviewed-by: Marc Hartmayer
Re: [PATCH v2 1/4] qemu: capabilities: add 'packed' capability
sdata/caps_5.0.0.aarch64.xml > b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml > index 30664c62..3876d08c 100644 > --- a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml > +++ b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml > @@ -183,6 +183,7 @@ > > > > + >4002050 >0 >61700241 > diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml > b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml > index a6800482..b0891613 100644 > --- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml > +++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml > @@ -191,6 +191,7 @@ > > > > + >4002050 >0 >42900241 > diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml > b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml > index df3557d1..b73c4efd 100644 > --- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml > +++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml > @@ -228,6 +228,7 @@ > > > > + >4002091 >0 >43100241 > -- > 2.24.1 Reviewed-by: Marc Hartmayer
Re: [libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Fri, Dec 13, 2019 at 03:32 PM -0500, Cole Robinson wrote: > On 12/12/19 8:46 AM, Marc Hartmayer wrote: >> On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson >> wrote: >>> On 11/14/19 12:44 PM, Marc Hartmayer wrote: >>>> The commit 'close callback: move it to driver' (88f09b75eb99) moved >>>> the responsibility for the close callback to the driver. But if the >>>> driver doesn't support the connectRegisterCloseCallback API this >>>> function does nothing, even no unsupported error report. This behavior >>>> may lead to problems, for example memory leaks, as the caller cannot >>>> differentiate whether the close callback was 'really' registered or >>>> not. >>>> >>> >>> >>> Full context: >>> v1 subtrhead with jferlan and danpb: >>> https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html >>> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html >>> >>> v2 subthread with john: >>> https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html >>> >>> My first thought is, why not just make this API start raising an error >>> if it isn't supported? >>> >>> But you tried that here: >>> https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html >> >> First, thanks for doing all the “history research”. >> >>> >>> I'm not really sure I buy the argument that we can't change the >>> semantics of the API here. This is the only callback API that seems to >>> not raise an explicit error. It's documented to raise an error. And >>> there's possible memory leak due the ambiguity. >> >> If we’re doing this so let’s fix the behavior of >> 'virConnectUnregisterCloseCallback' as well. >> >>> >>> Yeah I see that virsh needs to be updated to match. In practice virsh >>> shouldn't be a problem: this issue will only hit for local drivers, and >>> virsh and client library will be updated together for that case. >>> >>> In theory if a python app is using this API and not expecting an >>> exception, it could cause a regression. But it's also informing them >>> that, hey, that connection callback you requested wasn't working in the >>> first place. So it's arguably a correctness issue. >>> >>> So IMO we should be able to adjust this to return a proper error. >>> >>> >>> BUT, if we stick with this direction, then we need to extend the doc >>> text here to describe all of this: >>> >>> * Returns -1 if the driver can support close callback, but registering >>> one failed. User must free opaque? >>> * Returns 0 if the driver does not support close callback. We will free >>> data for you >>> * Returns 0 if the driver successfully registered a close callback. When >>> that callback is triggered, opaque will be free'd >>> >>> But that sounds pretty nutty IMO :/ >> >> I know… > > I did a bit more digging. Even the virsh case isn't the biggest deal > because CloseCallback failing is non-fatal. But like I mentioned before > it shouldn't realistically be hit in practice because we can expect > virsh and libvirt-client to be updated in lockstep. > > virt-manager, libguestfs, vdsm, kubevirt don't use this API > nova does (nova/virt/libvirt/host.py) but it has code to catch the error > > So IMO this should be changed to have semantics like all the other event > functions. Yes it's a semantic change, but I see it as explicitly > erroring in a case that never actually worked. We've made changes like > that many times, happens with XML validation semi regularly, and the > UNDEFINE flag changes are other notable examples. > > danpb has your thinking changed on this? Previous discussion context is > in this thread: > https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html > > Thanks, > Cole > Polite ping. -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio
wrote:
> Signed-off-by: Fabiano Fidêncio
> ---
> src/util/virutil.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index ed1f696e37..8c255abd7f 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -582,7 +582,7 @@ virGetHostnameQuiet(void)
> char *
> virGetUserDirectory(void)
> {
> -return virGetUserDirectoryByUID(geteuid());
> +return g_strdup_printf("%s/libvirt", g_get_home_dir());
I would suggest to use 'g_build_path' instead of 'g_strdup_printf'.
E.g.
g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
> }
>
>
> --
> 2.23.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
On Fri, Dec 13, 2019 at 03:00 PM -0500, Cole Robinson wrote: > On 12/12/19 9:13 AM, Marc Hartmayer wrote: >> On Wed, Dec 11, 2019 at 07:48 PM -0500, Cole Robinson >> wrote: >>> On 11/14/19 12:44 PM, Marc Hartmayer wrote: >>>> To free the structs and save the error, it is not necessary to hold >>>> @priv->lock, >>>> therefore move these parts after the mutex unlock. >>>> >>>> Signed-off-by: Marc Hartmayer >>>> --- >>>> src/remote/remote_daemon_dispatch.c | 32 ++--- >>>> 1 file changed, 16 insertions(+), 16 deletions(-) >>> >>> Reviewed-by: Cole Robinson >>> >>> Do I understand correctly that 1,3-5 are all independent and can be >>> pushed separately? If so I will do that tomorrow. I'm doing some >>> archaeology on patch #2 >> >> 1, 3, and 5 are all independent. >> >> Patch 4 depends on the second patch as >> remoteDispatchConnectRegisterCloseCallback uses >> virConnectRegisterCloseCallback. Otherwise we would never do the unref >> for @client and @program when conn->driver->connectRegisterCloseCallback >> was NULL. > > Thanks, I pushed 1, 3, 5, 6. I'll reply to your other message > > - Cole > Thanks. -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
On Wed, Dec 11, 2019 at 07:48 PM -0500, Cole Robinson wrote: > On 11/14/19 12:44 PM, Marc Hartmayer wrote: >> To free the structs and save the error, it is not necessary to hold >> @priv->lock, >> therefore move these parts after the mutex unlock. >> >> Signed-off-by: Marc Hartmayer >> --- >> src/remote/remote_daemon_dispatch.c | 32 ++--- >> 1 file changed, 16 insertions(+), 16 deletions(-) > > Reviewed-by: Cole Robinson > > Do I understand correctly that 1,3-5 are all independent and can be > pushed separately? If so I will do that tomorrow. I'm doing some > archaeology on patch #2 1, 3, and 5 are all independent. Patch 4 depends on the second patch as remoteDispatchConnectRegisterCloseCallback uses virConnectRegisterCloseCallback. Otherwise we would never do the unref for @client and @program when conn->driver->connectRegisterCloseCallback was NULL. > > - Cole > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson wrote: > On 11/14/19 12:44 PM, Marc Hartmayer wrote: >> The commit 'close callback: move it to driver' (88f09b75eb99) moved >> the responsibility for the close callback to the driver. But if the >> driver doesn't support the connectRegisterCloseCallback API this >> function does nothing, even no unsupported error report. This behavior >> may lead to problems, for example memory leaks, as the caller cannot >> differentiate whether the close callback was 'really' registered or >> not. >> > > > Full context: > v1 subtrhead with jferlan and danpb: > https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html > https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html > > v2 subthread with john: > https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html > > My first thought is, why not just make this API start raising an error > if it isn't supported? > > But you tried that here: > https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html First, thanks for doing all the “history research”. > > I'm not really sure I buy the argument that we can't change the > semantics of the API here. This is the only callback API that seems to > not raise an explicit error. It's documented to raise an error. And > there's possible memory leak due the ambiguity. If we’re doing this so let’s fix the behavior of 'virConnectUnregisterCloseCallback' as well. > > Yeah I see that virsh needs to be updated to match. In practice virsh > shouldn't be a problem: this issue will only hit for local drivers, and > virsh and client library will be updated together for that case. > > In theory if a python app is using this API and not expecting an > exception, it could cause a regression. But it's also informing them > that, hey, that connection callback you requested wasn't working in the > first place. So it's arguably a correctness issue. > > So IMO we should be able to adjust this to return a proper error. > > > BUT, if we stick with this direction, then we need to extend the doc > text here to describe all of this: > > * Returns -1 if the driver can support close callback, but registering > one failed. User must free opaque? > * Returns 0 if the driver does not support close callback. We will free > data for you > * Returns 0 if the driver successfully registered a close callback. When > that callback is triggered, opaque will be free'd > > But that sounds pretty nutty IMO :/ I know… > >> Therefore call directly @freecb if the connectRegisterCloseCallback >> API is not supported by the driver used by the connection and update >> the documentation. >> >> Signed-off-by: Marc Hartmayer >> --- >> src/libvirt-host.c | 16 +++- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/src/libvirt-host.c b/src/libvirt-host.c >> index 221a1b7a4353..94383ed449a9 100644 >> --- a/src/libvirt-host.c >> +++ b/src/libvirt-host.c >> @@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn) >> * @conn: pointer to connection object >> * @cb: callback to invoke upon close >> * @opaque: user data to pass to @cb >> - * @freecb: callback to free @opaque >> + * @freecb: callback to free @opaque when not used anymore >> * >> * Registers a callback to be invoked when the connection >> * is closed. This callback is invoked when there is any >> @@ -1412,7 +1412,9 @@ virConnectIsAlive(virConnectPtr conn) >> * >> * The @freecb must not invoke any other libvirt public >> * APIs, since it is not called from a re-entrant safe >> - * context. >> + * context. Only if virConnectRegisterCloseCallback is >> + * successful, @freecb will be called, otherwise the >> + * caller is responsible to free @opaque. > > This reads wrong to me. If cb() is successfully registered, then > freecb() is invoked automatically after cb() is called, right? Yep, or if the callback is unregistered. > This > comment makes it sound like freecb() is invoked immediately when > virConnectRegisterCloseCallback returns 0 I can reword it. > > Hopefully I'm not confusing things more :) No, I appreciate that you’re looking for it. > > - Cole > >> * >> * Returns 0 on success, -1 on error >> */ >> @@ -1428,9 +1430,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn, >> virCheckConnectReturn(conn, -1); >> virCheckNonNullArgGoto(cb, error); >> >> -if (conn->driver->connectRegisterCloseCallback && >> -conn->driver->connectRegisterCloseCallback(conn,
Re: [libvirt] [PATCH v4 0/7] Fix virConnectRegisterCloseCallback and get rid of global variables
On Thu, Nov 14, 2019 at 06:44 PM +0100, Marc Hartmayer wrote: > The second patch of this patch series fixes the behavior of > virConnectRegisterCloseCallback. > > The subsequent patches remove the need to have the global variables > 'qemuProgram' and 'remoteProgram' in libvirtd.[ch]. They only work in > combination with the fixed behavior of > virConnectRegisterCloseCallback. > > Changelog: > + v3->v4: >- Rebased to current master >- Added Pavel's r-bs >- Worked in Pavel's comments >- Added patch "remote: shrink the critical sections" in preparation > for patch "remote/rpc: Use virNetServerGetProgram() to determine > the program" > + v2->v3: >- Rebased to current master >- Added Johns r-b to the first patch, all other r-b's I have > dropped as there were to many changes in the meantime >- Removed accepted patches >- Dropped patches 8 and 9 > + v1->v2: >- Removed accepted patches >- Removed NACKed patches >- Added r-b to patch 5 >- Worked in comments >- Rebased >- Added patches 7-9 > > Marc Hartmayer (7): > rpc: use the return value of virObjectRef directly > virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no > connectRegisterCloseCallback > remote: Save reference to program in daemonClientEventCallback > remote: Use domainClientEventCallbacks for > remoteReplayConnectionClosedEvent > rpc: Introduce virNetServerGetProgramLocked helper function > remote: shrink the critical sections > remote/rpc: Use virNetServerGetProgram() to determine the program > > src/libvirt-host.c | 16 +- > src/libvirt_remote.syms | 1 + > src/remote/remote_daemon.c | 4 +- > src/remote/remote_daemon.h | 2 - > src/remote/remote_daemon_dispatch.c | 363 +--- > src/rpc/gendispatch.pl | 6 + > src/rpc/virnetserver.c | 53 +++- > src/rpc/virnetserver.h | 2 + > 8 files changed, 293 insertions(+), 154 deletions(-) > > -- > 2.21.0 > > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > Polite ping :) -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
The commit 'close callback: move it to driver' (88f09b75eb99) moved
the responsibility for the close callback to the driver. But if the
driver doesn't support the connectRegisterCloseCallback API this
function does nothing, even no unsupported error report. This behavior
may lead to problems, for example memory leaks, as the caller cannot
differentiate whether the close callback was 'really' registered or
not.
Therefore call directly @freecb if the connectRegisterCloseCallback
API is not supported by the driver used by the connection and update
the documentation.
Signed-off-by: Marc Hartmayer
---
src/libvirt-host.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 221a1b7a4353..94383ed449a9 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn)
* @conn: pointer to connection object
* @cb: callback to invoke upon close
* @opaque: user data to pass to @cb
- * @freecb: callback to free @opaque
+ * @freecb: callback to free @opaque when not used anymore
*
* Registers a callback to be invoked when the connection
* is closed. This callback is invoked when there is any
@@ -1412,7 +1412,9 @@ virConnectIsAlive(virConnectPtr conn)
*
* The @freecb must not invoke any other libvirt public
* APIs, since it is not called from a re-entrant safe
- * context.
+ * context. Only if virConnectRegisterCloseCallback is
+ * successful, @freecb will be called, otherwise the
+ * caller is responsible to free @opaque.
*
* Returns 0 on success, -1 on error
*/
@@ -1428,9 +1430,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
virCheckNonNullArgGoto(cb, error);
-if (conn->driver->connectRegisterCloseCallback &&
-conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) <
0)
-goto error;
+if (conn->driver->connectRegisterCloseCallback) {
+if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
freecb) < 0)
+goto error;
+} else {
+if (freecb)
+freecb(opaque);
+}
return 0;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 3/7] remote: Save reference to program in daemonClientEventCallback
As a result, you can later determine during the callback which program
was used. This makes it easier to refactor the code in the future and
is less prone to error.
Signed-off-by: Marc Hartmayer
Reviewed-by: Pavel Hrdina
---
src/remote/remote_daemon_dispatch.c | 108 +++-
1 file changed, 59 insertions(+), 49 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index f369ffb02a65..7b53c2241c05 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -78,6 +78,7 @@ VIR_LOG_INIT("daemon.remote");
struct daemonClientEventCallback {
virNetServerClientPtr client;
+virNetServerProgramPtr program;
int eventID;
int callbackID;
bool legacy;
@@ -149,6 +150,7 @@ remoteEventCallbackFree(void *opaque)
daemonClientEventCallbackPtr callback = opaque;
if (!callback)
return;
+virObjectUnref(callback->program);
virObjectUnref(callback->client);
VIR_FREE(callback);
}
@@ -334,7 +336,7 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
data.detail = detail;
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_lifecycle_msg,
);
@@ -342,7 +344,7 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
remote_domain_event_callback_lifecycle_msg msg = {
callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_callback_lifecycle_msg,
);
@@ -371,14 +373,14 @@ remoteRelayDomainEventReboot(virConnectPtr conn,
make_nonnull_domain(, dom);
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_REBOOT,
(xdrproc_t)xdr_remote_domain_event_reboot_msg, );
} else {
remote_domain_event_callback_reboot_msg msg = { callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_REBOOT,
(xdrproc_t)xdr_remote_domain_event_callback_reboot_msg, );
}
@@ -410,14 +412,14 @@ remoteRelayDomainEventRTCChange(virConnectPtr conn,
data.offset = offset;
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_RTC_CHANGE,
(xdrproc_t)xdr_remote_domain_event_rtc_change_msg, );
} else {
remote_domain_event_callback_rtc_change_msg msg = {
callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_RTC_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_rtc_change_msg, );
}
@@ -448,14 +450,14 @@ remoteRelayDomainEventWatchdog(virConnectPtr conn,
data.action = action;
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_WATCHDOG,
(xdrproc_t)xdr_remote_domain_event_watchdog_msg, );
} else {
remote_domain_event_callback_watchdog_msg msg = { callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_WATCHDOG,
[libvirt] [PATCH v4 5/7] rpc: Introduce virNetServerGetProgramLocked helper function
This patch introduces virNetServerGetProgramLocked. It's a function to
determine which program has to be used for a given @msg. This function
will be reused in the next patch.
Signed-off-by: Marc Hartmayer
Reviewed-by: Pavel Hrdina
---
src/rpc/virnetserver.c | 28 +---
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 673bb7c10c86..41226368058f 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -166,6 +166,26 @@ static void virNetServerHandleJob(void *jobOpaque, void
*opaque)
VIR_FREE(job);
}
+/**
+ * virNetServerGetProgramLocked:
+ * @srv: server (must be locked by the caller)
+ * @msg: message
+ *
+ * Searches @srv for the right program for a given message @msg.
+ *
+ * Returns a pointer to the server program or NULL if not found.
+ */
+static virNetServerProgramPtr
+virNetServerGetProgramLocked(virNetServerPtr srv,
+ virNetMessagePtr msg)
+{
+size_t i;
+for (i = 0; i < srv->nprograms; i++) {
+if (virNetServerProgramMatches(srv->programs[i], msg))
+return srv->programs[i];
+}
+return NULL;
+}
static void
virNetServerDispatchNewMessage(virNetServerClientPtr client,
@@ -175,18 +195,12 @@ virNetServerDispatchNewMessage(virNetServerClientPtr
client,
virNetServerPtr srv = opaque;
virNetServerProgramPtr prog = NULL;
unsigned int priority = 0;
-size_t i;
VIR_DEBUG("server=%p client=%p message=%p",
srv, client, msg);
virObjectLock(srv);
-for (i = 0; i < srv->nprograms; i++) {
-if (virNetServerProgramMatches(srv->programs[i], msg)) {
-prog = srv->programs[i];
-break;
-}
-}
+prog = virNetServerGetProgramLocked(srv, msg);
/* we can unlock @srv since @prog can only become invalid in case
* of disposing @srv, but let's grab a ref first to ensure nothing
* disposes of it before we use it. */
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 4/7] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
This allows us later to get rid of another usage of the global
variable `remoteProgram`.
Signed-off-by: Marc Hartmayer
Reviewed-by: Pavel Hrdina
---
src/remote/remote_daemon_dispatch.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 7b53c2241c05..9dc2083d715a 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1620,12 +1620,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
static
void remoteRelayConnectionClosedEvent(virConnectPtr conn G_GNUC_UNUSED, int
reason, void *opaque)
{
-virNetServerClientPtr client = opaque;
+daemonClientEventCallbackPtr callback = opaque;
VIR_DEBUG("Relaying connection closed event, reason %d", reason);
remote_connect_event_connection_closed_msg msg = { reason };
-remoteDispatchObjectEventSend(client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
);
@@ -4170,6 +4170,7 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
virNetMessageErrorPtr rerr)
{
int rv = -1;
+daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
@@ -4179,9 +4180,17 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
if (!conn)
goto cleanup;
+if (VIR_ALLOC(callback) < 0)
+goto cleanup;
+
+callback->client = virObjectRef(client);
+callback->program = virObjectRef(remoteProgram);
+/* eventID, callbackID, and legacy are not used */
+callback->eventID = -1;
+callback->callbackID = -1;
if (virConnectRegisterCloseCallback(conn,
remoteRelayConnectionClosedEvent,
-client, NULL) < 0)
+callback, remoteEventCallbackFree) < 0)
goto cleanup;
priv->closeRegistered = true;
@@ -4189,8 +4198,10 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
cleanup:
virMutexUnlock(>lock);
-if (rv < 0)
+if (rv < 0) {
+remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
+}
return rv;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 0/7] Fix virConnectRegisterCloseCallback and get rid of global variables
The second patch of this patch series fixes the behavior of virConnectRegisterCloseCallback. The subsequent patches remove the need to have the global variables 'qemuProgram' and 'remoteProgram' in libvirtd.[ch]. They only work in combination with the fixed behavior of virConnectRegisterCloseCallback. Changelog: + v3->v4: - Rebased to current master - Added Pavel's r-bs - Worked in Pavel's comments - Added patch "remote: shrink the critical sections" in preparation for patch "remote/rpc: Use virNetServerGetProgram() to determine the program" + v2->v3: - Rebased to current master - Added Johns r-b to the first patch, all other r-b's I have dropped as there were to many changes in the meantime - Removed accepted patches - Dropped patches 8 and 9 + v1->v2: - Removed accepted patches - Removed NACKed patches - Added r-b to patch 5 - Worked in comments - Rebased - Added patches 7-9 Marc Hartmayer (7): rpc: use the return value of virObjectRef directly virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback remote: Save reference to program in daemonClientEventCallback remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent rpc: Introduce virNetServerGetProgramLocked helper function remote: shrink the critical sections remote/rpc: Use virNetServerGetProgram() to determine the program src/libvirt-host.c | 16 +- src/libvirt_remote.syms | 1 + src/remote/remote_daemon.c | 4 +- src/remote/remote_daemon.h | 2 - src/remote/remote_daemon_dispatch.c | 363 +--- src/rpc/gendispatch.pl | 6 + src/rpc/virnetserver.c | 53 +++- src/rpc/virnetserver.h | 2 + 8 files changed, 293 insertions(+), 154 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 6/7] remote: shrink the critical sections
To free the structs and save the error, it is not necessary to hold @priv->lock, therefore move these parts after the mutex unlock. Signed-off-by: Marc Hartmayer --- src/remote/remote_daemon_dispatch.c | 32 ++--- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 9dc2083d715a..6ece51c2889d 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -4293,10 +4293,10 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server G_GNUC_UNUSED, rv = 0; cleanup: +virMutexUnlock(>lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(>lock); return rv; } @@ -4342,9 +4342,9 @@ remoteDispatchConnectDomainEventDeregister(virNetServerPtr server G_GNUC_UNUSED, rv = 0; cleanup: +virMutexUnlock(>lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(>lock); return rv; } @@ -4522,10 +4522,10 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED rv = 0; cleanup: +virMutexUnlock(>lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(>lock); return rv; } @@ -4598,11 +4598,11 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server G_GNU rv = 0; cleanup: +virMutexUnlock(>lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dom); -virMutexUnlock(>lock); return rv; } @@ -4657,9 +4657,9 @@ remoteDispatchConnectDomainEventDeregisterAny(virNetServerPtr server G_GNUC_UNUS rv = 0; cleanup: +virMutexUnlock(>lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(>lock); return rv; } @@ -4702,9 +4702,9 @@ remoteDispatchConnectDomainEventCallbackDeregisterAny(virNetServerPtr server G_G rv = 0; cleanup: +virMutexUnlock(>lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(>lock); return rv; } @@ -6081,11 +6081,11 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server G_GNUC_UNUSE rv = 0; cleanup: +virMutexUnlock(>lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(net); -virMutexUnlock(>lock); return rv; } @@ -6128,9 +6128,9 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr server G_GNUC_UNU rv = 0; cleanup: +virMutexUnlock(>lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(>lock); return rv; } @@ -6202,11 +6202,11 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server G_GNUC_U rv = 0; cleanup: +virMutexUnlock(>lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(pool); -virMutexUnlock(>lock); return rv; } @@ -6248,9 +6248,9 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr server G_GNUC rv = 0; cleanup: +virMutexUnlock(>lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(>lock); return rv; } @@ -6322,11 +6322,11 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server G_GNUC_UN rv = 0; cleanup: +virMutexUnlock(>lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dev); -virMutexUnlock(>lock); return rv; } @@ -6368,9 +6368,9 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr server G_GNUC_ rv = 0; cleanup: +virMutexUnlock(>lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(>lock); return rv; } @@ -6442,11 +6442,11 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED rv = 0; cleanup: +virMutexUnlock(>lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(secret); -virMutexUnlock(>lock); return rv; } @@ -6488,9 +6488,9 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server G_GNUC_UNUS rv = 0; cleanup: +virMutexUnlock(>lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(>lock); return rv; } @@ -6558,11 +6558,11 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server G_GNUC_UNUS rv = 0; cleanup: +virMutexUnlock(>lock); remoteEventCallbackFree(callback); if (rv <
[libvirt] [PATCH v4 1/7] rpc: use the return value of virObjectRef directly
Use the return value of virObjectRef directly. This way, it's easier
for another reader to identify the reason why the additional reference
is required.
Signed-off-by: Marc Hartmayer
Reviewed-by: John Ferlan
Reviewed-by: Pavel Hrdina
---
src/rpc/virnetserver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 590e780b641e..673bb7c10c86 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -199,7 +199,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
if (VIR_ALLOC(job) < 0)
goto error;
-job->client = client;
+job->client = virObjectRef(client);
job->msg = msg;
if (prog) {
@@ -207,7 +207,6 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
priority = virNetServerProgramGetPriority(prog, msg->header.proc);
}
-virObjectRef(client);
if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
virObjectUnref(client);
VIR_FREE(job);
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 7/7] remote/rpc: Use virNetServerGetProgram() to determine the program
Use virNetServerGetProgram() to determine the virNetServerProgram
instead of using hard coded global variables. This allows us to remove
the global variables @remoteProgram and @qemuProgram as they're now no
longer necessary.
Signed-off-by: Marc Hartmayer
---
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +-
src/remote/remote_daemon.h | 2 -
src/remote/remote_daemon_dispatch.c | 238 ++--
src/rpc/gendispatch.pl | 6 +
src/rpc/virnetserver.c | 22 +++
src/rpc/virnetserver.h | 2 +
7 files changed, 187 insertions(+), 88 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0493467f4603..a6883f373608 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -124,6 +124,7 @@ virNetServerGetCurrentUnauthClients;
virNetServerGetMaxClients;
virNetServerGetMaxUnauthClients;
virNetServerGetName;
+virNetServerGetProgram;
virNetServerGetThreadPoolParameters;
virNetServerHasClients;
virNetServerNeedsAuth;
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index b400b1dd1059..a36b5449d5ae 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -73,8 +73,6 @@ VIR_LOG_INIT("daemon." DAEMON_NAME);
#if WITH_SASL
virNetSASLContextPtr saslCtxt = NULL;
#endif
-virNetServerProgramPtr remoteProgram = NULL;
-virNetServerProgramPtr qemuProgram = NULL;
volatile bool driversInitialized = false;
@@ -996,6 +994,8 @@ int main(int argc, char **argv) {
virNetServerPtr srv = NULL;
virNetServerPtr srvAdm = NULL;
virNetServerProgramPtr adminProgram = NULL;
+virNetServerProgramPtr qemuProgram = NULL;
+virNetServerProgramPtr remoteProgram = NULL;
virNetServerProgramPtr lxcProgram = NULL;
char *remote_config_file = NULL;
int statuswrite = -1;
diff --git a/src/remote/remote_daemon.h b/src/remote/remote_daemon.h
index a2d9af403619..a3d6a220f868 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -97,5 +97,3 @@ struct daemonClientPrivate {
#if WITH_SASL
extern virNetSASLContextPtr saslCtxt;
#endif
-extern virNetServerProgramPtr remoteProgram;
-extern virNetServerProgramPtr qemuProgram;
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 6ece51c2889d..7ebb97f49f3b 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4164,9 +4164,9 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server
G_GNUC_UNUSED,
}
static int
-remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
G_GNUC_UNUSED,
+remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr)
{
int rv = -1;
@@ -4174,30 +4174,37 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
+virNetServerProgramPtr program;
+
+if (!(program = virNetServerGetProgram(server, msg))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program
found"));
+goto cleanup;
+}
virMutexLock(>lock);
if (!conn)
-goto cleanup;
+goto cleanup_unlock;
if (VIR_ALLOC(callback) < 0)
-goto cleanup;
+goto cleanup_unlock;
callback->client = virObjectRef(client);
-callback->program = virObjectRef(remoteProgram);
+callback->program = virObjectRef(program);
/* eventID, callbackID, and legacy are not used */
callback->eventID = -1;
callback->callbackID = -1;
if (virConnectRegisterCloseCallback(conn,
remoteRelayConnectionClosedEvent,
callback, remoteEventCallbackFree) < 0)
-goto cleanup;
+goto cleanup_unlock;
priv->closeRegistered = true;
rv = 0;
- cleanup:
+ cleanup_unlock:
virMutexUnlock(>lock);
+ cleanup:
if (rv < 0) {
remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
@@ -4236,9 +4243,9 @@
remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr server G_GNUC_UNUSE
}
static int
-remoteDispatchConnectDomainEventRegister(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectDomainEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
Re: [libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
On Thu, Nov 14, 2019 at 09:20 AM +0100, Pavel Hrdina wrote:
> On Wed, Nov 13, 2019 at 07:12:34PM +0100, Marc Hartmayer wrote:
>> On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina
>> wrote:
>> > On Fri, Nov 01, 2019 at 06:35:48PM +0100, Marc Hartmayer wrote:
>> >> Use virNetServerGetProgram() to determine the virNetServerProgram
>> >> instead of using hard coded global variables. This allows us to remove
>> >> the global variables @remoteProgram and @qemuProgram as they're now no
>> >> longer necessary.
>> >>
>> >> Signed-off-by: Marc Hartmayer
>>
>> […snip…]
>>
>> >> virNetMessageErrorPtr rerr)
>> >> {
>> >> int rv = -1;
>> >> @@ -4180,6 +4180,12 @@
>> >> remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
>> >> G_GNUC_UNUSED,
>> >> struct daemonClientPrivate *priv =
>> >> virNetServerClientGetPrivateData(client);
>> >> virConnectPtr conn = remoteGetHypervisorConn(client);
>> >> +virNetServerProgramPtr program;
>> >> +
>> >> +if (!(program = virNetServerGetProgram(server, msg))) {
>> >> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
>> >> program found"));
>> >> +goto cleanup;
>> >> +}
>> >
>> > This doesn't look right. If the function fails we will jump to cleanup
>> > where we will try to unlock >lock. This has to happen after we
>> > acquire that lock.
>> >
>> > Pavel
>>
>> Yep, will fix that as well. Shall I directly return in the error case or
>> jump to another label (e.g. 'cleanup_unlock')?
>
> Returning directly would not work properly as well, we call
> virNetMessageSaveError() in case of error in the cleanup section.
>
> Another label would work.
>
>> Or do see any reason why we should hold the priv->lock during the
>> virNetServerGetProgram call?
>
> We don't have to hold the lock for virNetServerGetProgram(), it just
> makes the function easier to follow as there will be only one cleanup
> and I don't see any harm of holding the lock for that function call as
> well if the function will later wait for the same lock.
This would enforce the lock order 'server -> priv->lock' (not sure if
this is already the case) + it will become harder to identify what we’re
trying to protect with the lock. So if you have no strong opinion about
that I will introduce a 'cleanup_unlock' label. Fine with you?
Thanks.
>
> Pavel
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina wrote:
> On Fri, Nov 01, 2019 at 06:35:48PM +0100, Marc Hartmayer wrote:
>> Use virNetServerGetProgram() to determine the virNetServerProgram
>> instead of using hard coded global variables. This allows us to remove
>> the global variables @remoteProgram and @qemuProgram as they're now no
>> longer necessary.
>>
>> Signed-off-by: Marc Hartmayer
[…snip…]
>> virNetMessageErrorPtr rerr)
>> {
>> int rv = -1;
>> @@ -4180,6 +4180,12 @@
>> remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
>> G_GNUC_UNUSED,
>> struct daemonClientPrivate *priv =
>> virNetServerClientGetPrivateData(client);
>> virConnectPtr conn = remoteGetHypervisorConn(client);
>> +virNetServerProgramPtr program;
>> +
>> +if (!(program = virNetServerGetProgram(server, msg))) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program
>> found"));
>> +goto cleanup;
>> +}
>
> This doesn't look right. If the function fails we will jump to cleanup
> where we will try to unlock >lock. This has to happen after we
> acquire that lock.
>
> Pavel
Yep, will fix that as well. Shall I directly return in the error case or
jump to another label (e.g. 'cleanup_unlock')?
Or do see any reason why we should hold the priv->lock during the
virNetServerGetProgram call?
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/6] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Fri, Nov 08, 2019 at 04:52 PM +0100, Pavel Hrdina wrote:
> On Fri, Nov 01, 2019 at 06:35:44PM +0100, Marc Hartmayer wrote:
>> The commit 'close callback: move it to driver' (88f09b75eb99) moved
>> the responsibility for the close callback to the driver. But if the
>> driver doesn't support the connectRegisterCloseCallback API this
>> function does nothing, even no unsupported error report. This behavior
>> may lead to problems, for example memory leaks, as the caller cannot
>> differentiate whether the close callback was 'really' registered or
>> not.
>>
>> Therefore call directly @freecb if the connectRegisterCloseCallback
>> API is not supported by the driver used by the connection.
>>
>> Signed-off-by: Marc Hartmayer
>> ---
>> src/libvirt-host.c | 12
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
>> index 221a1b7a4353..9c3a19f33b12 100644
>> --- a/src/libvirt-host.c
>> +++ b/src/libvirt-host.c
>> @@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn)
>> * @conn: pointer to connection object
>> * @cb: callback to invoke upon close
>> * @opaque: user data to pass to @cb
>> - * @freecb: callback to free @opaque
>> + * @freecb: callback to free @opaque when not used anymore
>> *
>> * Registers a callback to be invoked when the connection
>> * is closed. This callback is invoked when there is any
>> @@ -1428,9 +1428,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
>> virCheckConnectReturn(conn, -1);
>> virCheckNonNullArgGoto(cb, error);
>>
>> -if (conn->driver->connectRegisterCloseCallback &&
>> -conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
>> freecb) < 0)
>> -goto error;
>> +if (conn->driver->connectRegisterCloseCallback) {
>> +if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
>> freecb) < 0)
>> +goto error;
>> +} else {
>> +if (freecb)
>> +freecb(opaque);
>> +}
>
> Looks good but I think we should improve the documentation of this API
> to explicitly state that the @freecb is called only on success and if
> the API fails the caller is responsible to free the opaque data.
Will change it in v4.
>
> Pavel
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 5/6] rpc: Introduce virNetServerGetProgramLocked helper function
This patch introduces virNetServerGetProgramLocked. It's a function to
determine which program has to be used for a given @msg. This function
will be reused in the next patch.
Signed-off-by: Marc Hartmayer
---
src/rpc/virnetserver.c | 28 +---
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 54d0e4f31489..154747a1a097 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -171,6 +171,26 @@ static void virNetServerHandleJob(void *jobOpaque, void
*opaque)
VIR_FREE(job);
}
+/**
+ * virNetServerGetProgramLocked:
+ * @srv: server (must be locked by the caller)
+ * @msg: message
+ *
+ * Searches @srv for the right program for a given message @msg.
+ *
+ * Returns a pointer to the server program or NULL if not found.
+ */
+static virNetServerProgramPtr
+virNetServerGetProgramLocked(virNetServerPtr srv,
+ virNetMessagePtr msg)
+{
+size_t i;
+for (i = 0; i < srv->nprograms; i++) {
+if (virNetServerProgramMatches(srv->programs[i], msg))
+return srv->programs[i];
+}
+return NULL;
+}
static void
virNetServerDispatchNewMessage(virNetServerClientPtr client,
@@ -180,18 +200,12 @@ virNetServerDispatchNewMessage(virNetServerClientPtr
client,
virNetServerPtr srv = opaque;
virNetServerProgramPtr prog = NULL;
unsigned int priority = 0;
-size_t i;
VIR_DEBUG("server=%p client=%p message=%p",
srv, client, msg);
virObjectLock(srv);
-for (i = 0; i < srv->nprograms; i++) {
-if (virNetServerProgramMatches(srv->programs[i], msg)) {
-prog = srv->programs[i];
-break;
-}
-}
+prog = virNetServerGetProgramLocked(srv, msg);
/* we can unlock @srv since @prog can only become invalid in case
* of disposing @srv, but let's grab a ref first to ensure nothing
* disposes of it before we use it. */
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
Use virNetServerGetProgram() to determine the virNetServerProgram
instead of using hard coded global variables. This allows us to remove
the global variables @remoteProgram and @qemuProgram as they're now no
longer necessary.
Signed-off-by: Marc Hartmayer
---
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +-
src/remote/remote_daemon.h | 2 -
src/remote/remote_daemon_dispatch.c | 118 +---
src/rpc/gendispatch.pl | 6 ++
src/rpc/virnetserver.c | 22 ++
src/rpc/virnetserver.h | 2 +
7 files changed, 122 insertions(+), 33 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0493467f4603..a6883f373608 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -124,6 +124,7 @@ virNetServerGetCurrentUnauthClients;
virNetServerGetMaxClients;
virNetServerGetMaxUnauthClients;
virNetServerGetName;
+virNetServerGetProgram;
virNetServerGetThreadPoolParameters;
virNetServerHasClients;
virNetServerNeedsAuth;
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 7e63e180344d..c8ac224d52e9 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -73,8 +73,6 @@ VIR_LOG_INIT("daemon." DAEMON_NAME);
#if WITH_SASL
virNetSASLContextPtr saslCtxt = NULL;
#endif
-virNetServerProgramPtr remoteProgram = NULL;
-virNetServerProgramPtr qemuProgram = NULL;
volatile bool driversInitialized = false;
@@ -1007,6 +1005,8 @@ int main(int argc, char **argv) {
virNetServerPtr srv = NULL;
virNetServerPtr srvAdm = NULL;
virNetServerProgramPtr adminProgram = NULL;
+virNetServerProgramPtr qemuProgram = NULL;
+virNetServerProgramPtr remoteProgram = NULL;
virNetServerProgramPtr lxcProgram = NULL;
char *remote_config_file = NULL;
int statuswrite = -1;
diff --git a/src/remote/remote_daemon.h b/src/remote/remote_daemon.h
index a2d9af403619..a3d6a220f868 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -97,5 +97,3 @@ struct daemonClientPrivate {
#if WITH_SASL
extern virNetSASLContextPtr saslCtxt;
#endif
-extern virNetServerProgramPtr remoteProgram;
-extern virNetServerProgramPtr qemuProgram;
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 70f1f7d815e8..8756bd1a222d 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4170,9 +4170,9 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server
G_GNUC_UNUSED,
}
static int
-remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
G_GNUC_UNUSED,
+remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr)
{
int rv = -1;
@@ -4180,6 +4180,12 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
+virNetServerProgramPtr program;
+
+if (!(program = virNetServerGetProgram(server, msg))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program
found"));
+goto cleanup;
+}
virMutexLock(>lock);
@@ -4190,7 +4196,7 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
goto cleanup;
callback->client = virObjectRef(client);
-callback->program = virObjectRef(remoteProgram);
+callback->program = virObjectRef(program);
/* eventID, callbackID, and legacy are not used */
callback->eventID = -1;
callback->callbackID = -1;
@@ -4242,9 +4248,9 @@
remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr server G_GNUC_UNUSE
}
static int
-remoteDispatchConnectDomainEventRegister(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectDomainEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
G_GNUC_UNUSED,
remote_connect_domain_event_register_ret *ret G_GNUC_UNUSED)
{
@@ -4255,6 +4261,12 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr
server G_GNUC_UNUSED,
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
+virNetServerProgramPtr program;
+
+if
[libvirt] [PATCH v3 3/6] remote: Save reference to program in daemonClientEventCallback
As a result, you can later determine during the callback which program
was used. This makes it easier to refactor the code in the future and
is less prone to error.
Signed-off-by: Marc Hartmayer
---
src/remote/remote_daemon_dispatch.c | 108 +++-
1 file changed, 59 insertions(+), 49 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index be20556128ae..e7f6d4c9f138 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -78,6 +78,7 @@ VIR_LOG_INIT("daemon.remote");
struct daemonClientEventCallback {
virNetServerClientPtr client;
+virNetServerProgramPtr program;
int eventID;
int callbackID;
bool legacy;
@@ -149,6 +150,7 @@ remoteEventCallbackFree(void *opaque)
daemonClientEventCallbackPtr callback = opaque;
if (!callback)
return;
+virObjectUnref(callback->program);
virObjectUnref(callback->client);
VIR_FREE(callback);
}
@@ -334,7 +336,7 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
data.detail = detail;
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_lifecycle_msg,
);
@@ -342,7 +344,7 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
remote_domain_event_callback_lifecycle_msg msg = {
callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_callback_lifecycle_msg,
);
@@ -371,14 +373,14 @@ remoteRelayDomainEventReboot(virConnectPtr conn,
make_nonnull_domain(, dom);
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_REBOOT,
(xdrproc_t)xdr_remote_domain_event_reboot_msg, );
} else {
remote_domain_event_callback_reboot_msg msg = { callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_REBOOT,
(xdrproc_t)xdr_remote_domain_event_callback_reboot_msg, );
}
@@ -410,14 +412,14 @@ remoteRelayDomainEventRTCChange(virConnectPtr conn,
data.offset = offset;
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_RTC_CHANGE,
(xdrproc_t)xdr_remote_domain_event_rtc_change_msg, );
} else {
remote_domain_event_callback_rtc_change_msg msg = {
callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_RTC_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_rtc_change_msg, );
}
@@ -448,14 +450,14 @@ remoteRelayDomainEventWatchdog(virConnectPtr conn,
data.action = action;
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_WATCHDOG,
(xdrproc_t)xdr_remote_domain_event_watchdog_msg, );
} else {
remote_domain_event_callback_watchdog_msg msg = { callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_WATCHDOG,
(xdrpro
[libvirt] [PATCH v3 1/6] rpc: use the return value of virObjectRef directly
Use the return value of virObjectRef directly. This way, it's easier
for another reader to identify the reason why the additional reference
is required.
Signed-off-by: Marc Hartmayer
Reviewed-by: John Ferlan
---
src/rpc/virnetserver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 042661ffa5ea..54d0e4f31489 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -204,7 +204,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
if (VIR_ALLOC(job) < 0)
goto error;
-job->client = client;
+job->client = virObjectRef(client);
job->msg = msg;
if (prog) {
@@ -212,7 +212,6 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
priority = virNetServerProgramGetPriority(prog, msg->header.proc);
}
-virObjectRef(client);
if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
virObjectUnref(client);
VIR_FREE(job);
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 4/6] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
This allows us later to get rid of another usage of the global
variable `remoteProgram`.
Signed-off-by: Marc Hartmayer
---
src/remote/remote_daemon_dispatch.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index e7f6d4c9f138..70f1f7d815e8 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1620,12 +1620,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
static
void remoteRelayConnectionClosedEvent(virConnectPtr conn G_GNUC_UNUSED, int
reason, void *opaque)
{
-virNetServerClientPtr client = opaque;
+daemonClientEventCallbackPtr callback = opaque;
VIR_DEBUG("Relaying connection closed event, reason %d", reason);
remote_connect_event_connection_closed_msg msg = { reason };
-remoteDispatchObjectEventSend(client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
);
@@ -4176,6 +4176,7 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
virNetMessageErrorPtr rerr)
{
int rv = -1;
+daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
@@ -4185,9 +4186,17 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
if (!conn)
goto cleanup;
+if (VIR_ALLOC(callback) < 0)
+goto cleanup;
+
+callback->client = virObjectRef(client);
+callback->program = virObjectRef(remoteProgram);
+/* eventID, callbackID, and legacy are not used */
+callback->eventID = -1;
+callback->callbackID = -1;
if (virConnectRegisterCloseCallback(conn,
remoteRelayConnectionClosedEvent,
-client, NULL) < 0)
+callback, remoteEventCallbackFree) < 0)
goto cleanup;
priv->closeRegistered = true;
@@ -4195,8 +4204,10 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
cleanup:
virMutexUnlock(>lock);
-if (rv < 0)
+if (rv < 0) {
+remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
+}
return rv;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 0/6] Fix virConnectRegisterCloseCallback and get rid of global variables
After discussions with Peter and Pavel at the KVM forum, I am now sending a v3 of this series after more than a year... sorry for that long delay! The second patch of this patch series fixes the behavior of virConnectRegisterCloseCallback. The subsequent patches remove the need to have the global variables 'qemuProgram' and 'remoteProgram' in libvirtd.[ch]. They only work in combination with the fixed behavior of virConnectRegisterCloseCallback. Changelog: + v2->v3: - Rebased to current master - Added Johns r-b to the first patch, all other r-b's I have dropped as there were to many changes in the meantime - Removed accepted patches - Dropped patches 8 and 9 + v1->v2: - Removed accepted patches - Removed NACKed patches - Added r-b to patch 5 - Worked in comments - Rebased - Added patches 7-9 Marc Hartmayer (6): rpc: use the return value of virObjectRef directly virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback remote: Save reference to program in daemonClientEventCallback remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent rpc: Introduce virNetServerGetProgramLocked helper function remote/rpc: Use virNetServerGetProgram() to determine the program src/libvirt-host.c | 12 +- src/libvirt_remote.syms | 1 + src/remote/remote_daemon.c | 4 +- src/remote/remote_daemon.h | 2 - src/remote/remote_daemon_dispatch.c | 227 +++- src/rpc/gendispatch.pl | 6 + src/rpc/virnetserver.c | 53 +-- src/rpc/virnetserver.h | 2 + 8 files changed, 217 insertions(+), 90 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 2/6] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
The commit 'close callback: move it to driver' (88f09b75eb99) moved
the responsibility for the close callback to the driver. But if the
driver doesn't support the connectRegisterCloseCallback API this
function does nothing, even no unsupported error report. This behavior
may lead to problems, for example memory leaks, as the caller cannot
differentiate whether the close callback was 'really' registered or
not.
Therefore call directly @freecb if the connectRegisterCloseCallback
API is not supported by the driver used by the connection.
Signed-off-by: Marc Hartmayer
---
src/libvirt-host.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 221a1b7a4353..9c3a19f33b12 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn)
* @conn: pointer to connection object
* @cb: callback to invoke upon close
* @opaque: user data to pass to @cb
- * @freecb: callback to free @opaque
+ * @freecb: callback to free @opaque when not used anymore
*
* Registers a callback to be invoked when the connection
* is closed. This callback is invoked when there is any
@@ -1428,9 +1428,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
virCheckNonNullArgGoto(cb, error);
-if (conn->driver->connectRegisterCloseCallback &&
-conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) <
0)
-goto error;
+if (conn->driver->connectRegisterCloseCallback) {
+if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
freecb) < 0)
+goto error;
+} else {
+if (freecb)
+freecb(opaque);
+}
return 0;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Replace virDomainChrSourceDefFree with virObjectUnref
On Mon, Feb 25, 2019 at 02:38 PM +0100, Ján Tomko wrote: > On Wed, Feb 20, 2019 at 09:51:07AM +0100, Marc Hartmayer wrote: >>Replace virDomainChrSourceDefFree with virObjectUnref. >> >>Signed-off-by: Marc Hartmayer >>Reviewed-by: Boris Fiuczynski >>--- >> cfg.mk| 1 - >> src/conf/domain_conf.c| 17 + >> src/conf/domain_conf.h| 1 - >> src/libvirt_private.syms | 1 - >> src/qemu/qemu_domain.c| 4 ++-- >> src/qemu/qemu_parse_command.c | 4 ++-- >> src/qemu/qemu_process.c | 5 ++--- >> 7 files changed, 11 insertions(+), 22 deletions(-) >> > > Reviewed-by: Ján Tomko Thanks. > > Jano -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] qemu: Use refcounting for priv->monConfig
Use refcounting for priv->monConfig instead of asymmetric freeing.
Signed-off-by: Marc Hartmayer
Reviewed-by: Boris Fiuczynski
---
src/qemu/qemu_driver.c | 5 +
src/qemu/qemu_process.c | 6 +++---
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fe1b7801e9fc..010d7e285384 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17026,21 +17026,18 @@ static virDomainPtr
qemuDomainQemuAttach(virConnectPtr conn,
if (qemuProcessAttach(conn, driver, vm, pid,
pidfile, monConfig, monJSON) < 0) {
-monConfig = NULL;
qemuDomainRemoveInactive(driver, vm);
qemuDomainObjEndJob(driver, vm);
goto cleanup;
}
-monConfig = NULL;
-
dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
qemuDomainObjEndJob(driver, vm);
cleanup:
virDomainDefFree(def);
-virDomainChrSourceDefFree(monConfig);
+virObjectUnref(monConfig);
virDomainObjEndAPI();
VIR_FREE(pidfile);
virObjectUnref(caps);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 85952b997566..f06fe62f9931 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7528,8 +7528,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
goto error;
VIR_DEBUG("Preparing monitor state");
-priv->monConfig = monConfig;
-monConfig = NULL;
+priv->monConfig = virObjectRef(monConfig);
priv->monJSON = monJSON;
/* Attaching to running QEMU so we need to detect whether it was started
@@ -7648,7 +7647,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
VIR_FREE(sec_managers);
if (seclabelgen)
virSecurityLabelDefFree(seclabeldef);
-virDomainChrSourceDefFree(monConfig);
+virObjectUnref(priv->monConfig);
+priv->monConfig = NULL;
virObjectUnref(cfg);
virObjectUnref(caps);
return -1;
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
On Tue, Feb 12, 2019 at 09:46 PM +0100, John Ferlan wrote:
> On 2/7/19 11:08 AM, Marc Hartmayer wrote:
>> Commit "nodedev: Move device enumumeration out of nodeStateInitialize"
>> (9f0ae0b18e3e620) has moved the heavy task of device enumeration into
>> a separate thread. The problem with this commit is that there is a
>> functionality change in the cleanup when udevEnumerateDevices
>> fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up
>> by calling nodeStateCleanup (defined in node_device_udev.c) which
>> resulted in libvirtd stopping with the error message
>> 'daemonRunStateInit:800 : Driver state initialization failed'. With
>> the commit 9f0ae0b18e3e620 there is only a signal to the udev thread
>> that it must stop. This means that for example the watch handle isn't
>> removed from the main loop and this can result in the main loop
>> consuming 100% CPU time as soon as a new udev event occurs.
>>
>> This patch proposes a simple solution to the described problem. In
>> case the udev thread stops the watch handle is removed from the main
>> loop.
>>
>> Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of
>> nodeStateInitialize")
>> Signed-off-by: Marc Hartmayer
>> ---
>>
>> Note: I'm not sure whether we should stop libvirtd (as it would have
>> been done before) or if this patch is already sufficient.
>>
>> ---
>> src/node_device/node_device_udev.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>
[…snip…]
> Furthermore, should nodeStateCleanup be altered to check for priv->watch
> == -1 and thus not worry about the code to set threadQuit, signal, and
> joining the thread.
Hmm, I looked at the code again and it seems like your suggested change
could be a small performance improvement but it makes the code not more
readable. The current code is not wrong/problematic because changing
`priv->threadQuit` to true changes nothing. virCondSignal doesn’t block
and virThreadJoin/pthread_join returns immediately if the passed thread
has already terminated
(http://man7.org/linux/man-pages/man3/pthread_join.3.html).
The join would also still be needed with your proposed change since
otherwise there would be a (possible) race condition between
nodeStateCleanup() and setting `priv->threadQuit`.
>
> John
>
> BTW: I'm subscribed to the list, no need to CC...
>
>> diff --git a/src/node_device/node_device_udev.c
>> b/src/node_device/node_device_udev.c
>> index b1e5f00067e8..299f55260129 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -1628,6 +1628,13 @@ udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
>> }
>>
>> if (priv->threadQuit) {
>> +if (priv->watch != -1) {
>> +/* Since the udev thread getting stopped remove the
>> + * watch handle from the main loop */
>> +virEventRemoveHandle(priv->watch);
>> +priv->watch = -1;
>> +}
>> +
>> virObjectUnlock(priv);
>> return;
>> }
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] udev: Remove udev handle from main loop when udev thread stops
Changelog: + RFC -> v1: - Remove the event handle in the error path of nodeStateInitializeEnumerate - Added patch 2: wake up udev thread in case of an error Marc Hartmayer (2): udev: nodeStateInitializeEnumerate: remove watch handle in case of an error udev: wake up the udev thread for stopping it src/node_device/node_device_udev.c | 3 +++ 1 file changed, 3 insertions(+) -- 2.17.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] udev: nodeStateInitializeEnumerate: remove watch handle in case of an error
If the udev thread is stopped, it must be ensured that the watch handle is also removed from the main loop. Signed-off-by: Marc Hartmayer --- src/node_device/node_device_udev.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 54f372cd4a9f..4dd2e9004fd7 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1802,6 +1802,8 @@ nodeStateInitializeEnumerate(void *opaque) error: virObjectLock(priv); +ignore_value(virEventRemoveHandle(priv->watch)); +priv->watch = -1; priv->threadQuit = true; virObjectUnlock(priv); } -- 2.17.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] udev: wake up the udev thread for stopping it
Signal the udev thread the change of `priv->threadQuit` by using the thread condition. Signed-off-by: Marc Hartmayer --- src/node_device/node_device_udev.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4dd2e9004fd7..32e762009f1c 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1805,6 +1805,7 @@ nodeStateInitializeEnumerate(void *opaque) ignore_value(virEventRemoveHandle(priv->watch)); priv->watch = -1; priv->threadQuit = true; +virCondSignal(>threadCond); virObjectUnlock(priv); } -- 2.17.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
On Tue, Feb 12, 2019 at 09:46 PM +0100, John Ferlan wrote:
> On 2/7/19 11:08 AM, Marc Hartmayer wrote:
>> Commit "nodedev: Move device enumumeration out of nodeStateInitialize"
>> (9f0ae0b18e3e620) has moved the heavy task of device enumeration into
>> a separate thread. The problem with this commit is that there is a
>> functionality change in the cleanup when udevEnumerateDevices
>> fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up
>> by calling nodeStateCleanup (defined in node_device_udev.c) which
>> resulted in libvirtd stopping with the error message
>> 'daemonRunStateInit:800 : Driver state initialization failed'. With
>> the commit 9f0ae0b18e3e620 there is only a signal to the udev thread
>> that it must stop. This means that for example the watch handle isn't
>> removed from the main loop and this can result in the main loop
>> consuming 100% CPU time as soon as a new udev event occurs.
>>
>> This patch proposes a simple solution to the described problem. In
>> case the udev thread stops the watch handle is removed from the main
>> loop.
>>
>> Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of
>> nodeStateInitialize")
>> Signed-off-by: Marc Hartmayer
>> ---
>>
>> Note: I'm not sure whether we should stop libvirtd (as it would have
>> been done before) or if this patch is already sufficient.
>>
>> ---
>> src/node_device/node_device_udev.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>
> What you have seems reasonable - although I wonder if it would be better
> to remove the event handle in the error of nodeStateInitializeEnumerate.
>
> I think the assumption made was that by setting threadQuit that the next
> event have some sort of failure through udevEventMonitorSanityCheck
> which removes the EventHandle too. Although I cannot be sure it's been
> too long to remember for sure ;-)
>
> Furthermore, should nodeStateCleanup be altered to check for priv->watch
> == -1 and thus not worry about the code to set threadQuit, signal, and
> joining the thread.
A simple check for `priv->threadQuit` is probably even more useful since
it’s not safe that the watch callback is set.
I’ll send a v2 with the changes you suggested.
>
> John
>
> BTW: I'm subscribed to the list, no need to CC...
>
>> diff --git a/src/node_device/node_device_udev.c
>> b/src/node_device/node_device_udev.c
>> index b1e5f00067e8..299f55260129 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -1628,6 +1628,13 @@ udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
>> }
>>
>> if (priv->threadQuit) {
>> +if (priv->watch != -1) {
>> +/* Since the udev thread getting stopped remove the
>> + * watch handle from the main loop */
>> +virEventRemoveHandle(priv->watch);
>> +priv->watch = -1;
>> +}
>> +
>> virObjectUnlock(priv);
>> return;
>> }
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] qemu: Replace virDomainChrSourceDefFree with virObjectUnref
Replace virDomainChrSourceDefFree with virObjectUnref.
Signed-off-by: Marc Hartmayer
Reviewed-by: Boris Fiuczynski
---
cfg.mk| 1 -
src/conf/domain_conf.c| 17 +
src/conf/domain_conf.h| 1 -
src/libvirt_private.syms | 1 -
src/qemu/qemu_domain.c| 4 ++--
src/qemu/qemu_parse_command.c | 4 ++--
src/qemu/qemu_process.c | 5 ++---
7 files changed, 11 insertions(+), 22 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index c2524de5fc6b..7762422da764 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -120,7 +120,6 @@ useless_free_options = \
--name=virConfFreeValue \
--name=virDomainActualNetDefFree \
--name=virDomainChrDefFree \
- --name=virDomainChrSourceDefFree \
--name=virDomainControllerDefFree \
--name=virDomainDefFree \
--name=virDomainDeviceDefFree \
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 56c437ca0a34..5b61656d89b9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2194,7 +2194,7 @@ virDomainNetDefClear(virDomainNetDefPtr def)
switch (def->type) {
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
-virDomainChrSourceDefFree(def->data.vhostuser);
+virObjectUnref(def->data.vhostuser);
def->data.vhostuser = NULL;
break;
@@ -2433,13 +2433,6 @@ virDomainChrSourceDefDispose(void *obj)
}
-void
-virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
-{
-virObjectUnref(def);
-}
-
-
/* virDomainChrSourceDefIsEqual:
* @src: Source
* @tgt: Target
@@ -2530,7 +2523,7 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
break;
}
-virDomainChrSourceDefFree(def->source);
+virObjectUnref(def->source);
virDomainDeviceInfoClear(>info);
VIR_FREE(def);
@@ -2553,7 +2546,7 @@ void virDomainSmartcardDefFree(virDomainSmartcardDefPtr
def)
break;
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
-virDomainChrSourceDefFree(def->data.passthru);
+virObjectUnref(def->data.passthru);
break;
default:
@@ -2812,7 +2805,7 @@ void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def)
if (!def)
return;
-virDomainChrSourceDefFree(def->source);
+virObjectUnref(def->source);
virDomainDeviceInfoClear(>info);
VIR_FREE(def);
@@ -26562,7 +26555,7 @@ virDomainRNGDefFree(virDomainRNGDefPtr def)
VIR_FREE(def->source.file);
break;
case VIR_DOMAIN_RNG_BACKEND_EGD:
-virDomainChrSourceDefFree(def->source.chardev);
+virObjectUnref(def->source.chardev);
break;
case VIR_DOMAIN_RNG_BACKEND_LAST:
break;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9bccd8bcd15c..f205fc872328 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2911,7 +2911,6 @@ void virDomainNetDefClear(virDomainNetDefPtr def);
void virDomainNetDefFree(virDomainNetDefPtr def);
void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def);
void virDomainChrDefFree(virDomainChrDefPtr def);
-void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def);
int virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
virDomainChrSourceDefPtr src);
void virDomainSoundCodecDefFree(virDomainSoundCodecDefPtr def);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b720acdc939d..e5c9ccdbe497 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -221,7 +221,6 @@ virDomainChrSerialTargetTypeFromString;
virDomainChrSerialTargetTypeToString;
virDomainChrSourceDefClear;
virDomainChrSourceDefCopy;
-virDomainChrSourceDefFree;
virDomainChrSourceDefGetPath;
virDomainChrSourceDefNew;
virDomainChrSpicevmcTypeFromString;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index bbd4a5efe8d8..9996098494d7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2063,7 +2063,7 @@ qemuDomainObjPrivateFree(void *data)
qemuDomainObjPrivateDataClear(priv);
-virDomainChrSourceDefFree(priv->monConfig);
+virObjectUnref(priv->monConfig);
qemuDomainObjFreeJob(priv);
VIR_FREE(priv->lockState);
VIR_FREE(priv->origname);
@@ -3084,7 +3084,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
VIR_FREE(tmp);
virBitmapFree(priv->namespaces);
priv->namespaces = NULL;
-virDomainChrSourceDefFree(priv->monConfig);
+virObjectUnref(priv->monConfig);
priv->monConfig = NULL;
virStringListFree(priv->qemuDevices);
priv->qemuDevices = NULL;
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index 81691cb85ee1..49b34b1c178e 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -2495,7 +2495,7 @@ qemuParseCommandLine(virFileCachePtr capsCache,
goto error;
if (qemuParseCommandLineChr(chr, val) < 0) {
-
[libvirt] [PATCH 0/2] Get rid of virDomainChrSourceDefFree
Marc Hartmayer (2): qemu: Use refcounting for priv->monConfig qemu: Replace virDomainChrSourceDefFree with virObjectUnref cfg.mk| 1 - src/conf/domain_conf.c| 17 + src/conf/domain_conf.h| 1 - src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c| 4 ++-- src/qemu/qemu_driver.c| 5 + src/qemu/qemu_parse_command.c | 4 ++-- src/qemu/qemu_process.c | 11 +-- 8 files changed, 15 insertions(+), 29 deletions(-) -- 2.17.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] udev: only report a warning if udev_enumerate_scan_devices fails
Even if an error is reported by `udev_enumerate_scan_devices`,
e.g. because a driver of a device has an bug, we can still enumerate
all other devices. Additionally the documentation of
udev_enumerate_scan_devices says that on success an integer >= 0 is
returned (see man udev_enumerate_scan_devices(3)).
Reviewed-by: Bjoern Walk
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 299f55260129..eb4d8b3cfe4a 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1480,13 +1480,8 @@ udevEnumerateDevices(struct udev *udev)
if (udevEnumerateAddMatches(udev_enumerate) < 0)
goto cleanup;
-ret = udev_enumerate_scan_devices(udev_enumerate);
-if (ret != 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("udev scan devices returned %d"),
- ret);
-goto cleanup;
-}
+if (udev_enumerate_scan_devices(udev_enumerate) < 0)
+VIR_WARN("udev scan devices failed");
udev_list_entry_foreach(list_entry,
udev_enumerate_get_list_entry(udev_enumerate)) {
@@ -1494,6 +1489,7 @@ udevEnumerateDevices(struct udev *udev)
udevProcessDeviceListEntry(udev, list_entry);
}
+ret = 0;
cleanup:
udev_enumerate_unref(udev_enumerate);
return ret;
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] udev: only report a warning if udev_enumerate_scan_devices fails
On Wed, Feb 13, 2019 at 03:56 PM +0100, John Ferlan wrote:
> On 2/13/19 7:38 AM, Marc Hartmayer wrote:
>> Even if an error is reported by `udev_enumerate_scan_devices`,
>> e.g. because a driver of a device has an bug, we can still enumerate
>> all other devices. Additionally the documentation of
>> udev_enumerate_scan_devices says that on success an integer >= 0 is
>> returned (see man udev_enumerate_scan_devices(3)).
>>
>> Reviewed-by: Bjoern Walk
>> Signed-off-by: Marc Hartmayer
>> ---
>> src/node_device/node_device_udev.c | 9 ++---
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>
> Interesting - looking at many examples of udev_enumerate_scan_devices
> usage shows a lack of testing the return value and as is done here just
> using @udev_enumerate to add devices after the call.
>
> Eventually found some source code for enumerator_scan_devices_tags which
> I believe is what device_enumerator_scan_devices would call due to what
> our AddMatches does. It seems that code works until it finds an error,
> but still would return a partially enumerated list.
Yep, I’ve also looked at the source code. Unfortunately the behavior is
not documented… I’ve also looked for 'udev_enumerate_get_list_entry' and
it can handle NULL pointers.
>
> Long way of saying I think this is fine... However, now @ret = -1
> doesn't ever get changed, so the caller would still fail. So it's a nice
> way to test your other patch ;-)
Yes… I’ll send a v2.
>
>> diff --git a/src/node_device/node_device_udev.c
>> b/src/node_device/node_device_udev.c
>> index 299f55260129..90168eb8a969 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -1480,13 +1480,8 @@ udevEnumerateDevices(struct udev *udev)
>> if (udevEnumerateAddMatches(udev_enumerate) < 0)
>> goto cleanup;
>>
>> -ret = udev_enumerate_scan_devices(udev_enumerate);
>> -if (ret != 0) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("udev scan devices returned %d"),
>> - ret);
>> -goto cleanup;
>> -}
>> +if (udev_enumerate_scan_devices(udev_enumerate) < 0)
>> +VIR_WARN("udev scan devices failed");
>
> Either before or after this, set ret = 0... or change the default from
> -1 to 0 and only change if the AddMatches fails.
I’ll set 'ret = 0;' at the end.
>
> I think the other patch would still be necessary since if
> udevEnumerateAddMatches fails, then wouldn't the issue of setting
> threadQuit still exist?
>
>>
>> udev_list_entry_foreach(list_entry,
>> udev_enumerate_get_list_entry(udev_enumerate)) {
>>
>
> BTW: Using udevProcessDeviceListEntry as the 'example' of not failing if
> an element of udev_enumerate is problematic, I think logically if we
> don't get a full list we'd be OK to continue as well.
I agree.
>
> John
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
On Wed, Feb 13, 2019 at 03:03 PM +0100, John Ferlan wrote:
> On 2/13/19 4:34 AM, Marc Hartmayer wrote:
>> On Tue, Feb 12, 2019 at 09:46 PM +0100, John Ferlan
>> wrote:
>>> On 2/7/19 11:08 AM, Marc Hartmayer wrote:
>>>> Commit "nodedev: Move device enumumeration out of nodeStateInitialize"
>>>> (9f0ae0b18e3e620) has moved the heavy task of device enumeration into
>>>> a separate thread. The problem with this commit is that there is a
>>>> functionality change in the cleanup when udevEnumerateDevices
>>>> fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up
>>>> by calling nodeStateCleanup (defined in node_device_udev.c) which
>>>> resulted in libvirtd stopping with the error message
>>>> 'daemonRunStateInit:800 : Driver state initialization failed'. With
>>>> the commit 9f0ae0b18e3e620 there is only a signal to the udev thread
>>>> that it must stop. This means that for example the watch handle isn't
>>>> removed from the main loop and this can result in the main loop
>>>> consuming 100% CPU time as soon as a new udev event occurs.
>>>>
>>>> This patch proposes a simple solution to the described problem. In
>>>> case the udev thread stops the watch handle is removed from the main
>>>> loop.
>>>>
>>>> Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of
>>>> nodeStateInitialize")
>>>> Signed-off-by: Marc Hartmayer
>>>> ---
>>>>
>>>> Note: I'm not sure whether we should stop libvirtd (as it would have
>>>> been done before) or if this patch is already sufficient.
>>>>
>>>> ---
>>>> src/node_device/node_device_udev.c | 7 +++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>
>>> What you have seems reasonable - although I wonder if it would be better
>>> to remove the event handle in the error of
>>> nodeStateInitializeEnumerate.
>>
>> Might be better, yes - I’ve no strong opinion about that. I’ve added the
>> removal here because it doesn’t make sense to still have the watch
>> handle registered in the main loop if the udev thread stops - at least I
>> think so (just to be bulletproof).
>>
>> The more important point is before your change, the behavior was that
>> libvirtd stops after this error. Now (with this patch) it only removes
>> the watch handle and stops the udev thread. Is this change of behavior
>> okay?
>>
>
> This was mentioned during review of the patch - final response here:
>
> https://www.redhat.com/archives/libvir-list/2017-December/msg00531.html
>
> with the feeling that at least allowing other aspects of libvirt to
> function even though udev processing is crippled wasn't a bad thing.
Okay.
>
> Without the patch something would need to be done before starting
> libvirtd anyway. The "missing piece" was actually having what you had
> happen occur. Part of me wonders what fails such that enumeration fails
> and is that something we should be chasing instead. IOW: Is there
> something in udevEnumerateDevices failure that could just be ignored and
> we continue to operate logging the failure (like you've done in the
> patch you recently posted). Does that fix what caused this patch?
Yep, it does - if you’re referring to my mail
id:20190213123838.2826-1-mhart...@linux.ibm.com.
>
[…snip]
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] udev: only report a warning if udev_enumerate_scan_devices fails
Even if an error is reported by `udev_enumerate_scan_devices`,
e.g. because a driver of a device has an bug, we can still enumerate
all other devices. Additionally the documentation of
udev_enumerate_scan_devices says that on success an integer >= 0 is
returned (see man udev_enumerate_scan_devices(3)).
Reviewed-by: Bjoern Walk
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 299f55260129..90168eb8a969 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1480,13 +1480,8 @@ udevEnumerateDevices(struct udev *udev)
if (udevEnumerateAddMatches(udev_enumerate) < 0)
goto cleanup;
-ret = udev_enumerate_scan_devices(udev_enumerate);
-if (ret != 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("udev scan devices returned %d"),
- ret);
-goto cleanup;
-}
+if (udev_enumerate_scan_devices(udev_enumerate) < 0)
+VIR_WARN("udev scan devices failed");
udev_list_entry_foreach(list_entry,
udev_enumerate_get_list_entry(udev_enumerate)) {
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
On Tue, Feb 12, 2019 at 09:46 PM +0100, John Ferlan wrote:
> On 2/7/19 11:08 AM, Marc Hartmayer wrote:
>> Commit "nodedev: Move device enumumeration out of nodeStateInitialize"
>> (9f0ae0b18e3e620) has moved the heavy task of device enumeration into
>> a separate thread. The problem with this commit is that there is a
>> functionality change in the cleanup when udevEnumerateDevices
>> fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up
>> by calling nodeStateCleanup (defined in node_device_udev.c) which
>> resulted in libvirtd stopping with the error message
>> 'daemonRunStateInit:800 : Driver state initialization failed'. With
>> the commit 9f0ae0b18e3e620 there is only a signal to the udev thread
>> that it must stop. This means that for example the watch handle isn't
>> removed from the main loop and this can result in the main loop
>> consuming 100% CPU time as soon as a new udev event occurs.
>>
>> This patch proposes a simple solution to the described problem. In
>> case the udev thread stops the watch handle is removed from the main
>> loop.
>>
>> Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of
>> nodeStateInitialize")
>> Signed-off-by: Marc Hartmayer
>> ---
>>
>> Note: I'm not sure whether we should stop libvirtd (as it would have
>> been done before) or if this patch is already sufficient.
>>
>> ---
>> src/node_device/node_device_udev.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>
> What you have seems reasonable - although I wonder if it would be better
> to remove the event handle in the error of
> nodeStateInitializeEnumerate.
Might be better, yes - I’ve no strong opinion about that. I’ve added the
removal here because it doesn’t make sense to still have the watch
handle registered in the main loop if the udev thread stops - at least I
think so (just to be bulletproof).
The more important point is before your change, the behavior was that
libvirtd stops after this error. Now (with this patch) it only removes
the watch handle and stops the udev thread. Is this change of behavior
okay?
>
> I think the assumption made was that by setting threadQuit that the next
> event have some sort of failure through udevEventMonitorSanityCheck
> which removes the EventHandle too. Although I cannot be sure it's been
> too long to remember for sure ;-)
>
> Furthermore, should nodeStateCleanup be altered to check for priv->watch
> == -1 and thus not worry about the code to set threadQuit, signal, and
> joining the thread.
>
> John
>
> BTW: I'm subscribed to the list, no need to CC...
Yep, I know :) But adding you to CC increases the chance that you’re
looking for this patch since it was your commit.
Thanks for the feedback!
>
>> diff --git a/src/node_device/node_device_udev.c
>> b/src/node_device/node_device_udev.c
>> index b1e5f00067e8..299f55260129 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -1628,6 +1628,13 @@ udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
>> }
>>
>> if (priv->threadQuit) {
>> +if (priv->watch != -1) {
>> +/* Since the udev thread getting stopped remove the
>> + * watch handle from the main loop */
>> +virEventRemoveHandle(priv->watch);
>> +priv->watch = -1;
>> +}
>> +
>> virObjectUnlock(priv);
>> return;
>> }
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
On Thu, Feb 07, 2019 at 05:08 PM +0100, Marc Hartmayer
wrote:
> Commit "nodedev: Move device enumumeration out of nodeStateInitialize"
> (9f0ae0b18e3e620) has moved the heavy task of device enumeration into
> a separate thread. The problem with this commit is that there is a
> functionality change in the cleanup when udevEnumerateDevices
> fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up
> by calling nodeStateCleanup (defined in node_device_udev.c) which
> resulted in libvirtd stopping with the error message
> 'daemonRunStateInit:800 : Driver state initialization failed'. With
> the commit 9f0ae0b18e3e620 there is only a signal to the udev thread
> that it must stop. This means that for example the watch handle isn't
> removed from the main loop and this can result in the main loop
> consuming 100% CPU time as soon as a new udev event occurs.
>
> This patch proposes a simple solution to the described problem. In
> case the udev thread stops the watch handle is removed from the main
> loop.
>
> Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of
> nodeStateInitialize")
> Signed-off-by: Marc Hartmayer
> ---
>
> Note: I'm not sure whether we should stop libvirtd (as it would have
> been done before) or if this patch is already sufficient.
>
> ---
> src/node_device/node_device_udev.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/src/node_device/node_device_udev.c
> b/src/node_device/node_device_udev.c
> index b1e5f00067e8..299f55260129 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1628,6 +1628,13 @@ udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
> }
>
> if (priv->threadQuit) {
> +if (priv->watch != -1) {
> +/* Since the udev thread getting stopped remove the
> + * watch handle from the main loop */
> +virEventRemoveHandle(priv->watch);
> +priv->watch = -1;
> +}
> +
> virObjectUnlock(priv);
> return;
> }
> --
> 2.17.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Sorry… I forgot to add John to CC.
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
On Thu, Feb 07, 2019 at 05:08 PM +0100, Marc Hartmayer wrote: > Commit "nodedev: Move device enumumeration out of nodeStateInitialize" > (9f0ae0b18e3e620) has moved the heavy task of device enumeration into > a separate thread. The problem with this commit is that there is a > functionality change in the cleanup when udevEnumerateDevices > fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up > by calling nodeStateCleanup (defined in node_device_udev.c) which > resulted in libvirtd stopping with the error message > 'daemonRunStateInit:800 : Driver state initialization failed'. With > the commit 9f0ae0b18e3e620 there is only a signal to the udev thread > that it must stop. This means that for example the watch handle isn't > removed from the main loop and this can result in the main loop > consuming 100% CPU time as soon as a new udev event occurs. > > This patch proposes a simple solution to the described problem. In > case the udev thread stops the watch handle is removed from the main > loop. Another option would be to stop libvirtd if udevEnumerateDevices fails in nodeStateInitializeEnumerate. […snip] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops
Commit "nodedev: Move device enumumeration out of nodeStateInitialize"
(9f0ae0b18e3e620) has moved the heavy task of device enumeration into
a separate thread. The problem with this commit is that there is a
functionality change in the cleanup when udevEnumerateDevices
fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up
by calling nodeStateCleanup (defined in node_device_udev.c) which
resulted in libvirtd stopping with the error message
'daemonRunStateInit:800 : Driver state initialization failed'. With
the commit 9f0ae0b18e3e620 there is only a signal to the udev thread
that it must stop. This means that for example the watch handle isn't
removed from the main loop and this can result in the main loop
consuming 100% CPU time as soon as a new udev event occurs.
This patch proposes a simple solution to the described problem. In
case the udev thread stops the watch handle is removed from the main
loop.
Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of
nodeStateInitialize")
Signed-off-by: Marc Hartmayer
---
Note: I'm not sure whether we should stop libvirtd (as it would have
been done before) or if this patch is already sufficient.
---
src/node_device/node_device_udev.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index b1e5f00067e8..299f55260129 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1628,6 +1628,13 @@ udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
}
if (priv->threadQuit) {
+if (priv->watch != -1) {
+/* Since the udev thread getting stopped remove the
+ * watch handle from the main loop */
+virEventRemoveHandle(priv->watch);
+priv->watch = -1;
+}
+
virObjectUnlock(priv);
return;
}
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Refresh state before starting the VCPUs
On Mon, Feb 04, 2019 at 02:37 PM +0100, Marc Hartmayer
wrote:
> On Mon, Feb 04, 2019 at 01:41 PM +0100, Peter Krempa
> wrote:
>> On Mon, Feb 04, 2019 at 13:36:24 +0100, Marc Hartmayer wrote:
>>> For normal starts (no incoming migration) the refresh of the QEMU
>>> state must be done before the VCPUs getting started since otherwise
>>> there might be a race condition between a possible shutdown of the
>>> guest OS and the QEMU monitor queries.
>>>
>>> This fixes "qemu: migration: Refresh device information after
>>> transferring state" (93db7eea1b864).
>>>
>>> Signed-off-by: Marc Hartmayer
>>> ---
>>> src/qemu/qemu_process.c | 20 +++-
>>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index dace5aaca102..2a3763f40d49 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
>>> }
>>> relabel = true;
>>>
>>> -if (incoming &&
>>> -incoming->deferredURI &&
>>> -qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) <
>>> 0)
>>> -goto stop;
>>> +if (incoming) {
>>> +if (incoming->deferredURI &&
>>> +qemuMigrationDstRun(driver, vm, incoming->deferredURI,
>>> asyncJob) < 0)
>>> +goto stop;
>>> +} else {
>>
>> This logic does not seem right ...
>
> I’m not familiar with the usage of this function for migration… so
> there's a good chance I'm missing something.
>
>>
>>> +/* Refresh state of devices from QEMU. During migration this
>>> + * needs to happen after the state information is fully
>>> + * transferred. */
>>
>> as this comment clearly states that this should happen after
>> migration.
>
> Is qemuProcessFinishStartup
qemuProcessRefreshState
> not called explicitly in qemuMigrationFinish
> for migration?
>
>>
>> Here it would happen only when migration is not done.
>
> Without this patch qemuProcessRefreshState is called after
> qemuProcessFinishStartup if (!incoming). Now it’s called directly before
> qemuProcessFinishStartup if (!incoming). Why should this be wrong now?
> What happens in qemuProcessFinishStartup?
>
> Before your change in 93db7eea1b86408e the function call
> 'qemuProcessRefreshState' was done in qemuProcessFinishStartup (without
> any condition).
>
> Thanks.
>
>>
>>
>>> +if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
>>> +goto stop;
>>> +}
>>>
>>> if (qemuProcessFinishStartup(driver, vm, asyncJob,
>>> !(flags & VIR_QEMU_PROCESS_START_PAUSED),
>>> @@ -6945,11 +6952,6 @@ qemuProcessStart(virConnectPtr conn,
>>> /* Keep watching qemu log for errors during incoming migration,
>>> otherwise
>>> * unset reporting errors from qemu log. */
>>> qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL);
>>> -
>>> - /* Refresh state of devices from qemu. During migration this needs
>>> to
>>> - * happen after the state information is fully transferred. */
>>> -if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
>>> -goto stop;
>>> }
>>>
>>> ret = 0;
>>> --
>>> 2.17.0
>>>
> --
> Kind regards / Beste Grüße
>Marc Hartmayer
>
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Matthias Hartmann
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Refresh state before starting the VCPUs
On Mon, Feb 04, 2019 at 01:41 PM +0100, Peter Krempa wrote:
> On Mon, Feb 04, 2019 at 13:36:24 +0100, Marc Hartmayer wrote:
>> For normal starts (no incoming migration) the refresh of the QEMU
>> state must be done before the VCPUs getting started since otherwise
>> there might be a race condition between a possible shutdown of the
>> guest OS and the QEMU monitor queries.
>>
>> This fixes "qemu: migration: Refresh device information after
>> transferring state" (93db7eea1b864).
>>
>> Signed-off-by: Marc Hartmayer
>> ---
>> src/qemu/qemu_process.c | 20 +++-
>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index dace5aaca102..2a3763f40d49 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
>> }
>> relabel = true;
>>
>> -if (incoming &&
>> -incoming->deferredURI &&
>> -qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) <
>> 0)
>> -goto stop;
>> +if (incoming) {
>> +if (incoming->deferredURI &&
>> +qemuMigrationDstRun(driver, vm, incoming->deferredURI,
>> asyncJob) < 0)
>> +goto stop;
>> +} else {
>
> This logic does not seem right ...
I’m not familiar with the usage of this function for migration… so
there's a good chance I'm missing something.
>
>> +/* Refresh state of devices from QEMU. During migration this
>> + * needs to happen after the state information is fully
>> + * transferred. */
>
> as this comment clearly states that this should happen after
> migration.
Is qemuProcessFinishStartup not called explicitly in qemuMigrationFinish
for migration?
>
> Here it would happen only when migration is not done.
Without this patch qemuProcessRefreshState is called after
qemuProcessFinishStartup if (!incoming). Now it’s called directly before
qemuProcessFinishStartup if (!incoming). Why should this be wrong now?
What happens in qemuProcessFinishStartup?
Before your change in 93db7eea1b86408e the function call
'qemuProcessRefreshState' was done in qemuProcessFinishStartup (without
any condition).
Thanks.
>
>
>> +if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
>> +goto stop;
>> +}
>>
>> if (qemuProcessFinishStartup(driver, vm, asyncJob,
>> !(flags & VIR_QEMU_PROCESS_START_PAUSED),
>> @@ -6945,11 +6952,6 @@ qemuProcessStart(virConnectPtr conn,
>> /* Keep watching qemu log for errors during incoming migration,
>> otherwise
>> * unset reporting errors from qemu log. */
>> qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL);
>> -
>> -/* Refresh state of devices from qemu. During migration this needs
>> to
>> - * happen after the state information is fully transferred. */
>> -if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
>> -goto stop;
>> }
>>
>> ret = 0;
>> --
>> 2.17.0
>>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Refresh state before starting the VCPUs
For normal starts (no incoming migration) the refresh of the QEMU
state must be done before the VCPUs getting started since otherwise
there might be a race condition between a possible shutdown of the
guest OS and the QEMU monitor queries.
This fixes "qemu: migration: Refresh device information after
transferring state" (93db7eea1b864).
Signed-off-by: Marc Hartmayer
---
src/qemu/qemu_process.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dace5aaca102..2a3763f40d49 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
}
relabel = true;
-if (incoming &&
-incoming->deferredURI &&
-qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
-goto stop;
+if (incoming) {
+if (incoming->deferredURI &&
+qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) <
0)
+goto stop;
+} else {
+/* Refresh state of devices from QEMU. During migration this
+ * needs to happen after the state information is fully
+ * transferred. */
+if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
+goto stop;
+}
if (qemuProcessFinishStartup(driver, vm, asyncJob,
!(flags & VIR_QEMU_PROCESS_START_PAUSED),
@@ -6945,11 +6952,6 @@ qemuProcessStart(virConnectPtr conn,
/* Keep watching qemu log for errors during incoming migration,
otherwise
* unset reporting errors from qemu log. */
qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL);
-
-/* Refresh state of devices from qemu. During migration this needs to
- * happen after the state information is fully transferred. */
-if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
-goto stop;
}
ret = 0;
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: audit: Fix the error code when kernel lacks audit support
On Tue, Jan 15, 2019 at 04:08 PM +0100, Erik Skultety
wrote:
> When commit 4199c2f221c tweaked the error path of virAuditOpen it used
> VIR_FROM_THIS as the error code to virReportError.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1596119
>
> Signed-off-by: Erik Skultety
> ---
> src/util/viraudit.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/util/viraudit.c b/src/util/viraudit.c
> index a02e5b36fd..d04343a542 100644
> --- a/src/util/viraudit.c
> +++ b/src/util/viraudit.c
> @@ -66,7 +66,8 @@ int virAuditOpen(unsigned int audit_level ATTRIBUTE_UNUSED)
> if (audit_level < 2)
> VIR_INFO("Audit is not supported by the kernel");
> else
> -virReportError(VIR_FROM_THIS, "%s", _("Audit is not
> supported by the kernel"));
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Audit is not supported by the kernel"));
> } else {
> virReportSystemError(errno, "%s", _("Unable to initialize audit
> layer"));
> }
> --
> 2.20.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
Thanks for fixing!
Reviewed-by: Marc Hartmayer
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/18] Implement original label remembering
On Thu, Dec 20, 2018 at 09:15 PM +0100, Michal Prívozník wrote: > On 12/20/18 12:48 PM, Marc Hartmayer wrote: >> On Wed, Dec 19, 2018 at 03:37 PM +0100, Michal Privoznik >> wrote: >>> On 12/19/18 2:54 PM, Ján Tomko wrote: >>>> >>>> Reviewed-by: Ján Tomko >>> >>> Thanks to you and Dan. I've pushed these. >> >> I tried out the current master (e05d8e570b) and I got the following >> error message regularly: >> >> 2018-12-20 11:37:37.056+: 30026: error : virProcessWait:274 : internal >> error: Child process (31926) unexpected fatal signal 11 >> 2018-12-20 11:37:37.060+: 30026: warning : >> qemuSecurityRestoreAllLabel:89 : Unable to run security manager transaction > > Looks like there is some crash. Can you try to get stack trace please? Hmm with the newest master (9d42d51eef793d7c) I get no error message. I’ll try to revalidate the behavior/error messages with the previous version. > > Michal > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Thu, Dec 20, 2018 at 09:20 PM +0100, Michal Prívozník wrote: > On 12/18/18 12:18 PM, Michal Privoznik wrote: > >> Otherwise looking good. I agree with Martin that this might not be the >> best solution but it's the best we have (also, the problem is not on our >> side). >> >> ACK (will wait a day or two for others to chime in before pushing) > > No objections were raised, so I've pushed this one. Thanks. > > Michal > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/18] Implement original label remembering
On Wed, Dec 19, 2018 at 03:37 PM +0100, Michal Privoznik wrote: > On 12/19/18 2:54 PM, Ján Tomko wrote: >> >> Reviewed-by: Ján Tomko > > Thanks to you and Dan. I've pushed these. I tried out the current master (e05d8e570b) and I got the following error message regularly: 2018-12-20 11:37:37.056+: 30026: error : virProcessWait:274 : internal error: Child process (31926) unexpected fatal signal 11 2018-12-20 11:37:37.060+: 30026: warning : qemuSecurityRestoreAllLabel:89 : Unable to run security manager transaction Did you try it with SELinux? > > Michal > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Tue, Nov 20, 2018 at 11:00 AM +0100, Marc Hartmayer
wrote:
> On Thu, Nov 01, 2018 at 04:37 PM +0100, Michal Privoznik
> wrote:
>> On 10/30/2018 01:55 PM, Daniel P. Berrangé wrote:
>>> On Tue, Oct 30, 2018 at 10:32:08AM +, Daniel P. Berrangé wrote:
>>>> On Tue, Oct 30, 2018 at 11:08:45AM +0100, Michal Privoznik wrote:
>>>>> On 10/30/2018 10:35 AM, Daniel P. Berrangé wrote:
>>>>>> On Tue, Oct 30, 2018 at 09:13:50AM +0100, Michal Privoznik wrote:
>>>>>>> On 10/29/2018 06:34 PM, Marc Hartmayer wrote:
>>>>>>>> Introduce caching whether /dev/kvm is usable as the QEMU user:QEMU
>>>>>>>> group. This reduces the overhead of the QEMU capabilities cache
>>>>>>>> lookup. Before this patch there were many fork() calls used for
>>>>>>>> checking whether /dev/kvm is accessible. Now we store the result
>>>>>>>> whether /dev/kvm is accessible or not and we only need to re-run the
>>>>>>>> virFileAccessibleAs check if the ctime of /dev/kvm has changed.
>>>>>>>>
>>>>>>>> Suggested-by: Daniel P. Berrangé
>>>>>>>> Signed-off-by: Marc Hartmayer
>>>>>>>> ---
>>>>>>>> src/qemu/qemu_capabilities.c | 54 ++--
>>>>>>>> 1 file changed, 52 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/src/qemu/qemu_capabilities.c
>>>>>>>> b/src/qemu/qemu_capabilities.c
>>>>>>>> index e228f52ec0bb..85516954149b 100644
>>>>>>>> --- a/src/qemu/qemu_capabilities.c
>>>>>>>> +++ b/src/qemu/qemu_capabilities.c
>>>>>>>> @@ -3238,6 +3238,10 @@ struct _virQEMUCapsCachePriv {
>>>>>>>> virArch hostArch;
>>>>>>>> unsigned int microcodeVersion;
>>>>>>>> char *kernelVersion;
>>>>>>>> +
>>>>>>>> +/* cache whether /dev/kvm is usable as runUid:runGuid */
>>>>>>>> +virTristateBool kvmUsable;
>>>>>>>> +time_t kvmCtime;
>>>>>>>> };
>>>>>>>> typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
>>>>>>>> typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
>>>>>>>> @@ -3824,6 +3828,52 @@ virQEMUCapsSaveFile(void *data,
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> +/* Determine whether '/dev/kvm' is usable as QEMU user:QEMU group. */
>>>>>>>> +static bool
>>>>>>>> +virQEMUCapsKVMUsable(virQEMUCapsCachePrivPtr priv)
>>>>>>>> +{
>>>>>>>> +struct stat sb;
>>>>>>>> +static const char *kvm_device = "/dev/kvm";
>>>>>>>> +virTristateBool value;
>>>>>>>> +virTristateBool cached_value = priv->kvmUsable;
>>>>>>>> +time_t kvm_ctime;
>>>>>>>> +time_t cached_kvm_ctime = priv->kvmCtime;
>>>>>>>> +
>>>>>>>> +if (stat(kvm_device, ) < 0) {
>>>>>>>> +virReportSystemError(errno,
>>>>>>>> + _("Failed to stat %s"), kvm_device);
>>>>>>>> +return false;
>>>>>>>> +}
>>>>>>>> +kvm_ctime = sb.st_ctime;
>>>>>>>
>>>>>>> This doesn't feel right. /dev/kvm ctime is changed every time qemu is
>>>>>>> started or powered off (try running stat over it before and after a
>>>>>>> domain is started/shut off). So effectively we will fork more often than
>>>>>>> we would think. Should we cache inode number instead? Because for all
>>>>>>> that we care is simply if the file is there.
>>>>>>
>>>>>> Urgh, that is a bit strange and not the usual semantics for timestamps
>>>>>> :-(
>>>>>
>>>>> Indeed.
>>>>>
>>>>>>
>>>>>> We can't stat the inode - the code was explicitly trying to cope with the
>>>>>> way /dev/kvm can change permissions when udev rules get applied. We would
>>>>>> have to compare the user, group, permissions mask and even ACL, or a hash
>>>>>> of those.
>>>>>
>>>>> Well, we can use ctime as suggested and post a patch for kernel to fix
>>>>> ctime behaviour. Until the patch is merged our behaviour would be
>>>>> suboptimal, but still better than it is now.
>>>>
>>>> I guess lets talk to KVM team for their input on this and then decide
>>>> what todo.
>>>
>>> Hmm, I wonder if it is not actually a kernel problem, but rather something
>>> in userspace genuinely touching the device in a way that caues these
>>> timestamps to be updated.
>>>
>>> eg I vaguely recall a udev rule that resets permissions on device nodes
>>> whenever an FD is closed, which might cause this kind of behaviour
>>
>> After trying hard to find the rule that mangles the ctime I've found the
>> following bug in udev:
>>
>> https://github.com/zippy2/systemd/commit/51915e4e6a64c581da321c25448d80626d8e8408?diff=unified
>>
>> I've send that as a pull request to systemd. So after all, ctime might
>> be usable again.
>
> Are there any other objections against this patch? Because even with the
> ctime bug in udev it’s performs _much_ better than before.
Polite ping :)
[…snip]
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] test: Convert testDriver to virObjectLockable
From: Marc Hartmayer
The test driver state (@testDriver) uses it's own reference counting
and locking implementation. Instead of doing that, convert @testDriver
into a virObjectLockable and use the provided functionalities.
Signed-off-by: Marc Hartmayer
Reviewed-by: Boris Fiuczynski
---
This patch was originally posted with the patch series "[libvirt]
[PATCH libvirt v2 0/9] Fix virConnectRegisterCloseCallback and get rid
of global variables"
(mid:20180412124104.10547-1-mhart...@linux.vnet.ibm.com).
I dropped the r-b from John because so much time has passed.
---
src/test/test_driver.c | 198 ++---
1 file changed, 87 insertions(+), 111 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 31d0da744ce5..b76f0b718ecd 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -92,7 +92,7 @@ typedef struct _testAuth testAuth;
typedef struct _testAuth *testAuthPtr;
struct _testDriver {
-virMutex lock;
+virObjectLockable parent;
virNodeInfo nodeInfo;
virInterfaceObjListPtr ifaces;
@@ -124,9 +124,19 @@ typedef struct _testDriver testDriver;
typedef testDriver *testDriverPtr;
static testDriverPtr defaultPrivconn;
-static int defaultConnections;
static virMutex defaultLock = VIR_MUTEX_INITIALIZER;
+static virClassPtr testDriverClass;
+static void testDriverDispose(void *obj);
+static int testDriverOnceInit(void)
+{
+if (!(VIR_CLASS_NEW(testDriver, virClassForObjectLockable(
+return -1;
+
+return 0;
+}
+VIR_ONCE_GLOBAL_INIT(testDriver)
+
#define TEST_MODEL "i686"
#define TEST_EMULATOR "/usr/bin/test-hv"
@@ -142,10 +152,9 @@ static const virNodeInfo defaultNodeInfo = {
};
static void
-testDriverFree(testDriverPtr driver)
+testDriverDispose(void *obj)
{
-if (!driver)
-return;
+testDriverPtr driver = obj;
virObjectUnref(driver->caps);
virObjectUnref(driver->xmlopt);
@@ -155,21 +164,6 @@ testDriverFree(testDriverPtr driver)
virObjectUnref(driver->ifaces);
virObjectUnref(driver->pools);
virObjectUnref(driver->eventState);
-virMutexUnlock(>lock);
-virMutexDestroy(>lock);
-
-VIR_FREE(driver);
-}
-
-
-static void testDriverLock(testDriverPtr driver)
-{
-virMutexLock(>lock);
-}
-
-static void testDriverUnlock(testDriverPtr driver)
-{
-virMutexUnlock(>lock);
}
#define TEST_NAMESPACE_HREF "http://libvirt.org/schemas/domain/test/1.0;
@@ -401,14 +395,11 @@ testDriverNew(void)
};
testDriverPtr ret;
-if (VIR_ALLOC(ret) < 0)
+if (testDriverInitialize() < 0)
return NULL;
-if (virMutexInit(>lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("cannot initialize mutex"));
-goto error;
-}
+if (!(ret = virObjectLockableNew(testDriverClass)))
+return NULL;
if (!(ret->xmlopt = virDomainXMLOptionNew(NULL, NULL, , NULL, NULL)) ||
!(ret->eventState = virObjectEventStateNew()) ||
@@ -424,7 +415,7 @@ testDriverNew(void)
return ret;
error:
-testDriverFree(ret);
+virObjectUnref(ret);
return NULL;
}
@@ -1256,7 +1247,7 @@ testOpenFromFile(virConnectPtr conn, const char *file)
if (!(privconn = testDriverNew()))
return VIR_DRV_OPEN_ERROR;
-testDriverLock(privconn);
+virObjectLock(privconn);
conn->privateData = privconn;
if (!(privconn->caps = testBuildCapabilities(conn)))
@@ -1273,14 +1264,14 @@ testOpenFromFile(virConnectPtr conn, const char *file)
xmlXPathFreeContext(ctxt);
xmlFreeDoc(doc);
-testDriverUnlock(privconn);
+virObjectUnlock(privconn);
return VIR_DRV_OPEN_SUCCESS;
error:
xmlXPathFreeContext(ctxt);
xmlFreeDoc(doc);
-testDriverFree(privconn);
+virObjectUnref(privconn);
conn->privateData = NULL;
return VIR_DRV_OPEN_ERROR;
}
@@ -1298,8 +1289,8 @@ testOpenDefault(virConnectPtr conn)
size_t i;
virMutexLock();
-if (defaultConnections++) {
-conn->privateData = defaultPrivconn;
+if (defaultPrivconn) {
+conn->privateData = virObjectRef(defaultPrivconn);
virMutexUnlock();
return VIR_DRV_OPEN_SUCCESS;
}
@@ -1348,9 +1339,8 @@ testOpenDefault(virConnectPtr conn)
return ret;
error:
-testDriverFree(privconn);
+virObjectUnref(privconn);
conn->privateData = NULL;
-defaultConnections--;
goto cleanup;
}
@@ -1363,9 +1353,9 @@ testConnectAuthenticate(virConnectPtr conn,
ssize_t i;
char *username = NULL, *password = NULL;
-testDriverLock(privconn);
+virObjectLock(privconn);
if (privconn->numAuths == 0) {
-testDriverUnlock(privconn);
+virObjectUnlock(privconn);
return 0;
}
@@ -1400,7 +1390,7 @@ testConnectAuthenticate(virConnectPtr conn,
ret = 0;
[libvirt] OBFrom 2ca6bb41cd7d86a89ac565ca71ba1f2baffd0c68 Mon Sep 17 00:00:00 2001
The test driver state (@testDriver) uses it's own reference counting
and locking implementation. Instead of doing that, convert @testDriver
into a virObjectLockable and use the provided functionalities.
Signed-off-by: Marc Hartmayer
Reviewed-by: Boris Fiuczynski
---
This patch was originally posted with the patch series "[libvirt]
[PATCH libvirt v2 0/9] Fix virConnectRegisterCloseCallback and get rid
of global variables"
(mid:20180412124104.10547-1-mhart...@linux.vnet.ibm.com).
I dropped the r-b from John because so much time has passed.
---
src/test/test_driver.c | 198 ++---
1 file changed, 87 insertions(+), 111 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 31d0da744ce5..b76f0b718ecd 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -92,7 +92,7 @@ typedef struct _testAuth testAuth;
typedef struct _testAuth *testAuthPtr;
struct _testDriver {
-virMutex lock;
+virObjectLockable parent;
virNodeInfo nodeInfo;
virInterfaceObjListPtr ifaces;
@@ -124,9 +124,19 @@ typedef struct _testDriver testDriver;
typedef testDriver *testDriverPtr;
static testDriverPtr defaultPrivconn;
-static int defaultConnections;
static virMutex defaultLock = VIR_MUTEX_INITIALIZER;
+static virClassPtr testDriverClass;
+static void testDriverDispose(void *obj);
+static int testDriverOnceInit(void)
+{
+if (!(VIR_CLASS_NEW(testDriver, virClassForObjectLockable(
+return -1;
+
+return 0;
+}
+VIR_ONCE_GLOBAL_INIT(testDriver)
+
#define TEST_MODEL "i686"
#define TEST_EMULATOR "/usr/bin/test-hv"
@@ -142,10 +152,9 @@ static const virNodeInfo defaultNodeInfo = {
};
static void
-testDriverFree(testDriverPtr driver)
+testDriverDispose(void *obj)
{
-if (!driver)
-return;
+testDriverPtr driver = obj;
virObjectUnref(driver->caps);
virObjectUnref(driver->xmlopt);
@@ -155,21 +164,6 @@ testDriverFree(testDriverPtr driver)
virObjectUnref(driver->ifaces);
virObjectUnref(driver->pools);
virObjectUnref(driver->eventState);
-virMutexUnlock(>lock);
-virMutexDestroy(>lock);
-
-VIR_FREE(driver);
-}
-
-
-static void testDriverLock(testDriverPtr driver)
-{
-virMutexLock(>lock);
-}
-
-static void testDriverUnlock(testDriverPtr driver)
-{
-virMutexUnlock(>lock);
}
#define TEST_NAMESPACE_HREF "http://libvirt.org/schemas/domain/test/1.0;
@@ -401,14 +395,11 @@ testDriverNew(void)
};
testDriverPtr ret;
-if (VIR_ALLOC(ret) < 0)
+if (testDriverInitialize() < 0)
return NULL;
-if (virMutexInit(>lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("cannot initialize mutex"));
-goto error;
-}
+if (!(ret = virObjectLockableNew(testDriverClass)))
+return NULL;
if (!(ret->xmlopt = virDomainXMLOptionNew(NULL, NULL, , NULL, NULL)) ||
!(ret->eventState = virObjectEventStateNew()) ||
@@ -424,7 +415,7 @@ testDriverNew(void)
return ret;
error:
-testDriverFree(ret);
+virObjectUnref(ret);
return NULL;
}
@@ -1256,7 +1247,7 @@ testOpenFromFile(virConnectPtr conn, const char *file)
if (!(privconn = testDriverNew()))
return VIR_DRV_OPEN_ERROR;
-testDriverLock(privconn);
+virObjectLock(privconn);
conn->privateData = privconn;
if (!(privconn->caps = testBuildCapabilities(conn)))
@@ -1273,14 +1264,14 @@ testOpenFromFile(virConnectPtr conn, const char *file)
xmlXPathFreeContext(ctxt);
xmlFreeDoc(doc);
-testDriverUnlock(privconn);
+virObjectUnlock(privconn);
return VIR_DRV_OPEN_SUCCESS;
error:
xmlXPathFreeContext(ctxt);
xmlFreeDoc(doc);
-testDriverFree(privconn);
+virObjectUnref(privconn);
conn->privateData = NULL;
return VIR_DRV_OPEN_ERROR;
}
@@ -1298,8 +1289,8 @@ testOpenDefault(virConnectPtr conn)
size_t i;
virMutexLock();
-if (defaultConnections++) {
-conn->privateData = defaultPrivconn;
+if (defaultPrivconn) {
+conn->privateData = virObjectRef(defaultPrivconn);
virMutexUnlock();
return VIR_DRV_OPEN_SUCCESS;
}
@@ -1348,9 +1339,8 @@ testOpenDefault(virConnectPtr conn)
return ret;
error:
-testDriverFree(privconn);
+virObjectUnref(privconn);
conn->privateData = NULL;
-defaultConnections--;
goto cleanup;
}
@@ -1363,9 +1353,9 @@ testConnectAuthenticate(virConnectPtr conn,
ssize_t i;
char *username = NULL, *password = NULL;
-testDriverLock(privconn);
+virObjectLock(privconn);
if (privconn->numAuths == 0) {
-testDriverUnlock(privconn);
+virObjectUnlock(privconn);
return 0;
}
@@ -1400,7 +1390,7 @@ testConnectAuthenticate(virConnectPtr conn,
ret = 0;
cleanup:
-test
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Thu, Nov 01, 2018 at 04:37 PM +0100, Michal Privoznik
wrote:
> On 10/30/2018 01:55 PM, Daniel P. Berrangé wrote:
>> On Tue, Oct 30, 2018 at 10:32:08AM +, Daniel P. Berrangé wrote:
>>> On Tue, Oct 30, 2018 at 11:08:45AM +0100, Michal Privoznik wrote:
>>>> On 10/30/2018 10:35 AM, Daniel P. Berrangé wrote:
>>>>> On Tue, Oct 30, 2018 at 09:13:50AM +0100, Michal Privoznik wrote:
>>>>>> On 10/29/2018 06:34 PM, Marc Hartmayer wrote:
>>>>>>> Introduce caching whether /dev/kvm is usable as the QEMU user:QEMU
>>>>>>> group. This reduces the overhead of the QEMU capabilities cache
>>>>>>> lookup. Before this patch there were many fork() calls used for
>>>>>>> checking whether /dev/kvm is accessible. Now we store the result
>>>>>>> whether /dev/kvm is accessible or not and we only need to re-run the
>>>>>>> virFileAccessibleAs check if the ctime of /dev/kvm has changed.
>>>>>>>
>>>>>>> Suggested-by: Daniel P. Berrangé
>>>>>>> Signed-off-by: Marc Hartmayer
>>>>>>> ---
>>>>>>> src/qemu/qemu_capabilities.c | 54 ++--
>>>>>>> 1 file changed, 52 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>>>>>>> index e228f52ec0bb..85516954149b 100644
>>>>>>> --- a/src/qemu/qemu_capabilities.c
>>>>>>> +++ b/src/qemu/qemu_capabilities.c
>>>>>>> @@ -3238,6 +3238,10 @@ struct _virQEMUCapsCachePriv {
>>>>>>> virArch hostArch;
>>>>>>> unsigned int microcodeVersion;
>>>>>>> char *kernelVersion;
>>>>>>> +
>>>>>>> +/* cache whether /dev/kvm is usable as runUid:runGuid */
>>>>>>> +virTristateBool kvmUsable;
>>>>>>> +time_t kvmCtime;
>>>>>>> };
>>>>>>> typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
>>>>>>> typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
>>>>>>> @@ -3824,6 +3828,52 @@ virQEMUCapsSaveFile(void *data,
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> +/* Determine whether '/dev/kvm' is usable as QEMU user:QEMU group. */
>>>>>>> +static bool
>>>>>>> +virQEMUCapsKVMUsable(virQEMUCapsCachePrivPtr priv)
>>>>>>> +{
>>>>>>> +struct stat sb;
>>>>>>> +static const char *kvm_device = "/dev/kvm";
>>>>>>> +virTristateBool value;
>>>>>>> +virTristateBool cached_value = priv->kvmUsable;
>>>>>>> +time_t kvm_ctime;
>>>>>>> +time_t cached_kvm_ctime = priv->kvmCtime;
>>>>>>> +
>>>>>>> +if (stat(kvm_device, ) < 0) {
>>>>>>> +virReportSystemError(errno,
>>>>>>> + _("Failed to stat %s"), kvm_device);
>>>>>>> +return false;
>>>>>>> +}
>>>>>>> +kvm_ctime = sb.st_ctime;
>>>>>>
>>>>>> This doesn't feel right. /dev/kvm ctime is changed every time qemu is
>>>>>> started or powered off (try running stat over it before and after a
>>>>>> domain is started/shut off). So effectively we will fork more often than
>>>>>> we would think. Should we cache inode number instead? Because for all
>>>>>> that we care is simply if the file is there.
>>>>>
>>>>> Urgh, that is a bit strange and not the usual semantics for timestamps :-(
>>>>
>>>> Indeed.
>>>>
>>>>>
>>>>> We can't stat the inode - the code was explicitly trying to cope with the
>>>>> way /dev/kvm can change permissions when udev rules get applied. We would
>>>>> have to compare the user, group, permissions mask and even ACL, or a hash
>>>>> of those.
>>>>
>>>> Well, we can use ctime as suggested and post a patch for kernel to fix
>>>> ctime behaviour. Until the patch is merged our behaviour would be
>>>> suboptimal, but still better than it is now.
>>>
>>> I guess lets talk to KVM team for their input on this and then decide
>>> what todo.
>>
>> Hmm, I wonder if it is not actually a kernel problem, but rather something
>> in userspace genuinely touching the device in a way that caues these
>> timestamps to be updated.
>>
>> eg I vaguely recall a udev rule that resets permissions on device nodes
>> whenever an FD is closed, which might cause this kind of behaviour
>
> After trying hard to find the rule that mangles the ctime I've found the
> following bug in udev:
>
> https://github.com/zippy2/systemd/commit/51915e4e6a64c581da321c25448d80626d8e8408?diff=unified
>
> I've send that as a pull request to systemd. So after all, ctime might
> be usable again.
Are there any other objections against this patch? Because even with the
ctime bug in udev it’s performs _much_ better than before.
>
> Michal
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 0/8] Virtio-crypto device support
On Thu, Oct 26, 2017 at 03:31 AM +0200, "Longpeng (Mike)" wrote: > On 2017/10/25 23:14, Matthew Rosato wrote: > >> On 07/07/2017 04:07 AM, Longpeng(Mike) wrote: >>> As virtio-crypto has been supported in QEMU 2.8 and the frontend >>> driver has been merged in linux 4.10, so it's necessary to support >>> virtio-crypto in libvirt. >>> >>> --- >> >> Hi Mike, >> >> Seems like this topic has gone quiet.. Is there a v5 in the works? >> > > > Hi Matt, > > V5 is always in our plan, but we want to make the virtio-crypto spec (the > latest > version is V20) upstream first. > > I mainly work on an amazing and interesting project these two weeks, so even > the > virtio-crypto spec is delayed. > > I'll take some time to work on the V21 spec these days. > >> Matt >> >> >> > > > -- > Regards, > Longpeng(Mike) Hi Longpeng, any updates so far? Is a v5 still planned? Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 1/3] qemu: Add check for whether nesting was enabled
up;
>
> +qemuCaps->isNested = virQEMUCapsIsNested();
> +
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
> virQEMUCapsInitQMPCommandAbort(cmd);
> if ((rc = virQEMUCapsInitQMPCommandRun(cmd, true)) != 0) {
> diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h
> index 8d1a40fe74..b5d6aae2e5 100644
> --- a/src/qemu/qemu_capspriv.h
> +++ b/src/qemu/qemu_capspriv.h
> @@ -48,6 +48,8 @@ int
> virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>qemuMonitorPtr mon);
>
> +void virQEMUCapsClearIsNested(virQEMUCapsPtr qemuCaps);
> +
> int
> virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps,
> qemuMonitorPtr mon);
> diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
> index 8fe5a55e1d..703fb6a125 100644
> --- a/tests/qemucapabilitiestest.c
> +++ b/tests/qemucapabilitiestest.c
> @@ -63,6 +63,9 @@ testQemuCaps(const void *opaque)
>qemuMonitorTestGetMonitor(mon)) < 0)
> goto cleanup;
>
> +/* Don't apply what the host has... force clear for testing purposes */
> +virQEMUCapsClearIsNested(capsActual);
> +
> if (virQEMUCapsGet(capsActual, QEMU_CAPS_KVM)) {
> qemuMonitorResetCommandID(qemuMonitorTestGetMonitor(mon));
> if (virQEMUCapsInitQMPMonitorTCG(capsActual,
> --
> 2.17.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virSecuritySELinuxTransactionCommit: Don't mask error
On Tue, Nov 13, 2018 at 04:55 PM +0100, Michal Privoznik
wrote:
> In 4674fc6afd6 I've implemented transactions for selinux driver.
> Well, now that I am working in this area I've notice a subtle
> bug: @ret is initialized to 0 instead of -1. Facepalm.
>
> Signed-off-by: Michal Privoznik
> ---
>
> I wonder how this could survive this long (~2y) not being noticed.
>
> src/security/security_selinux.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 467d1e6bfe..c09404f6f8 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1091,7 +1091,7 @@
> virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr
> ATTRIBUTE_UNUSED,
> pid_t pid)
> {
> virSecuritySELinuxContextListPtr list;
> -int ret = 0;
> +int ret = -1;
>
> list = virThreadLocalGet();
> if (!list)
> --
> 2.18.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
Reviewed-by: Marc Hartmayer
Actually, I had the same fix in my pipeline :)
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virSecuritySELinuxTransactionCommit: Return -1 if no transaction is set
Return -1 and report an error message if no transaction is set and
virSecuritySELinuxTransactionCommit is called.
The function description of virSecuritySELinuxTransactionCommit says:
"Also it is considered as error if there's no transaction set and this
function is called."
Signed-off-by: Marc Hartmayer
Reviewed-by: Boris Fiuczynski
---
Please apply this patch after the patch
"virSecuritySELinuxTransactionCommit: Don't mask error" from Michal.
---
src/security/security_selinux.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index c09404f6f833..780d650c69ea 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1094,8 +1094,11 @@
virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
int ret = -1;
list = virThreadLocalGet();
-if (!list)
-return 0;
+if (!list) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("No transaction is set"));
+return -1;
+}
if (virThreadLocalSet(, NULL) < 0) {
virReportSystemError(errno, "%s",
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Mon, Nov 12, 2018 at 03:13 PM +0100, "Daniel P. Berrangé" wrote: > On Mon, Nov 12, 2018 at 02:48:09PM +0100, Marc Hartmayer wrote: >> On Mon, Nov 12, 2018 at 01:30 PM +0100, Pavel Hrdina >> wrote: >> > On Mon, Nov 12, 2018 at 12:50:41PM +0100, Marc Hartmayer wrote: >> >> On Thu, Nov 01, 2018 at 09:31 AM +0100, Martin Kletzander >> >> wrote: >> > >> > [...] >> > >> >> How can you run a machine/QEMU VM under a different user:group other >> >> than changing the user:group in qemu.conf and restart/reload libvirtd? >> >> >> >> As soon as a VM is running we have not to verify /dev/kvm access, no? >> >> (so there should be no problem when libvirtd tries to “reconnect” to >> >> already running VMs). >> > >> > You can add this into your domain XML: >> > >> > >> > phrdina:phrdina >> > >> > >> > And it will run the qemu process under that user. >> >> Interesting :) Actually, if we consider this then the QEMU caps caching >> is broken anyway since 'virQEMUCapsNewData' is calling >> 'virQEMUCapsNewForBinaryInternal(…, priv->runUid, priv->runGid, …)'. >> >> And 'priv->runUid/runGid' is only set once in virQEMUCapsCacheNew. >> >> Maybe I missed something. > > I dont think it is really broken - it merely impacts error reporting. Yep, you’re right. The use of “broken” was clearly exaggerated by me. > > When running 'virsh capabilities' we are trying to figure out if > KVM is usable on the host. This is always using the default uid/gid > so gives a fairly coarse answer, but the answer is correct for most > common usage. > > When building command line ARGV for spawning a specific QEMU > instance, the KVM capability merely affect whether libvirt > reports an error about the guest config. In the case where > the capability works with default uid/gid, but breaks with the > custom per-VM uid/gid, libvirt will mistakenly thing KVM is > usable and so launch the guest. QEMU will then be unable to > access it and quit with some suitable error message. > > So the "brokenness" about not using the per-VM uid/gid merely > delays the error reporting. I don't think this is important > enough to worry about, using the default uid/gid is sufficient. For this patch as well? > > Regards, > Daniel > -- > |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o-https://fstop138.berrange.com :| > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
On Mon, Nov 12, 2018 at 01:14 PM +0100, Erik Skultety
wrote:
> Since we'll need to validate other models apart from VFIO PCI too,
> having a helper for each model should keep the code base cleaner.
>
> Signed-off-by: Erik Skultety
> ---
> src/qemu/qemu_domain.c | 35 +--
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e25afdad6b..17d207513d 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const
> virDomainNetDef *net)
>
>
> static int
> -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
> - const virDomainDef *def,
> - virQEMUCapsPtr qemuCaps)
> +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev
> *dev,
> + const virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> {
> -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT)
> +if (dev->display == VIR_TRISTATE_SWITCH_ABSENT)
> return 0;
>
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
> @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const
> virDomainHostdevSubsysMediatedDev *mdevsrc,
> return -1;
> }
>
> -if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
> +if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _(" attribute 'display' is only supported"
> " with model='vfio-pci'"));
> @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const
> virDomainHostdevSubsysMediatedDev *mdevsrc,
> return -1;
> }
>
> -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) {
> +if (dev->display == VIR_TRISTATE_SWITCH_ON) {
> if (def->ngraphics == 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("graphics device is needed for attribute value "
> @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const
> virDomainHostdevSubsysMediatedDev *mdevsrc,
> }
>
>
> +static int
> +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
> + const virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> +{
> +
> +switch ((virMediatedDeviceModelType) mdevsrc->model) {
> +case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
> +return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps);
> +case VIR_MDEV_MODEL_TYPE_VFIO_AP:
> +break;
> +case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
> +break;
> +case VIR_MDEV_MODEL_TYPE_LAST:
> +virReportEnumRangeError(virMediatedDeviceModelType,
> +mdevsrc->model);
> +return -1;
> +}
default case is missing? Otherwise looking good.
> +
> +return 0;
> +}
> +
> +
> static int
> qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev,
> const virDomainDef *def,
> --
> 2.19.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Mon, Nov 12, 2018 at 01:30 PM +0100, Pavel Hrdina wrote: > On Mon, Nov 12, 2018 at 12:50:41PM +0100, Marc Hartmayer wrote: >> On Thu, Nov 01, 2018 at 09:31 AM +0100, Martin Kletzander >> wrote: > > [...] > >> How can you run a machine/QEMU VM under a different user:group other >> than changing the user:group in qemu.conf and restart/reload libvirtd? >> >> As soon as a VM is running we have not to verify /dev/kvm access, no? >> (so there should be no problem when libvirtd tries to “reconnect” to >> already running VMs). > > You can add this into your domain XML: > > > phrdina:phrdina > > > And it will run the qemu process under that user. Interesting :) Actually, if we consider this then the QEMU caps caching is broken anyway since 'virQEMUCapsNewData' is calling 'virQEMUCapsNewForBinaryInternal(…, priv->runUid, priv->runGid, …)'. And 'priv->runUid/runGid' is only set once in virQEMUCapsCacheNew. Maybe I missed something. > > Pavel > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Thu, Nov 01, 2018 at 09:31 AM +0100, Martin Kletzander
wrote:
> On Tue, Oct 30, 2018 at 03:07:59PM +, Daniel P. Berrangé wrote:
>>On Tue, Oct 30, 2018 at 03:45:36PM +0100, Michal Privoznik wrote:
>>> On 10/30/2018 02:46 PM, Michal Privoznik wrote:
>>> > On 10/30/2018 01:55 PM, Daniel P. Berrangé wrote:
>>> >> On Tue, Oct 30, 2018 at 10:32:08AM +, Daniel P. Berrangé wrote:
>>> >>> On Tue, Oct 30, 2018 at 11:08:45AM +0100, Michal Privoznik wrote:
>>> >>>> On 10/30/2018 10:35 AM, Daniel P. Berrangé wrote:
>>> >>>>> On Tue, Oct 30, 2018 at 09:13:50AM +0100, Michal Privoznik wrote:
>>> >>>>>> On 10/29/2018 06:34 PM, Marc Hartmayer wrote:
>>> >>>>>>> Introduce caching whether /dev/kvm is usable as the QEMU user:QEMU
>>> >>>>>>> group. This reduces the overhead of the QEMU capabilities cache
>>> >>>>>>> lookup. Before this patch there were many fork() calls used for
>>> >>>>>>> checking whether /dev/kvm is accessible. Now we store the result
>>> >>>>>>> whether /dev/kvm is accessible or not and we only need to re-run the
>>> >>>>>>> virFileAccessibleAs check if the ctime of /dev/kvm has changed.
>>> >>>>>>>
>>> >>>>>>> Suggested-by: Daniel P. Berrangé
>>> >>>>>>> Signed-off-by: Marc Hartmayer
>>> >>>>>>> ---
>>> >>>>>>> src/qemu/qemu_capabilities.c | 54
>>> >>>>>>> ++--
>>> >>>>>>> 1 file changed, 52 insertions(+), 2 deletions(-)
>>> >>>>>>>
>>> >>>>>>> diff --git a/src/qemu/qemu_capabilities.c
>>> >>>>>>> b/src/qemu/qemu_capabilities.c
>>> >>>>>>> index e228f52ec0bb..85516954149b 100644
>>> >>>>>>> --- a/src/qemu/qemu_capabilities.c
>>> >>>>>>> +++ b/src/qemu/qemu_capabilities.c
>>> >>>>>>> @@ -3238,6 +3238,10 @@ struct _virQEMUCapsCachePriv {
>>> >>>>>>> virArch hostArch;
>>> >>>>>>> unsigned int microcodeVersion;
>>> >>>>>>> char *kernelVersion;
>>> >>>>>>> +
>>> >>>>>>> +/* cache whether /dev/kvm is usable as runUid:runGuid */
>>> >>>>>>> +virTristateBool kvmUsable;
>>> >>>>>>> +time_t kvmCtime;
>>> >>>>>>> };
>>> >>>>>>> typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
>>> >>>>>>> typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
>>> >>>>>>> @@ -3824,6 +3828,52 @@ virQEMUCapsSaveFile(void *data,
>>> >>>>>>> }
>>> >>>>>>>
>>> >>>>>>>
>>> >>>>>>> +/* Determine whether '/dev/kvm' is usable as QEMU user:QEMU group.
>>> >>>>>>> */
>>> >>>>>>> +static bool
>>> >>>>>>> +virQEMUCapsKVMUsable(virQEMUCapsCachePrivPtr priv)
>>> >>>>>>> +{
>>> >>>>>>> +struct stat sb;
>>> >>>>>>> +static const char *kvm_device = "/dev/kvm";
>>> >>>>>>> +virTristateBool value;
>>> >>>>>>> +virTristateBool cached_value = priv->kvmUsable;
>>> >>>>>>> +time_t kvm_ctime;
>>> >>>>>>> +time_t cached_kvm_ctime = priv->kvmCtime;
>>> >>>>>>> +
>>> >>>>>>> +if (stat(kvm_device, ) < 0) {
>>> >>>>>>> +virReportSystemError(errno,
>>> >>>>>>> + _("Failed to stat %s"), kvm_device);
>>> >>>>>>> +return false;
>>> >>>>>>> +}
>>> >>>>>>> +kvm_ctime = sb.st_ctime;
>>> >>>>>>
>>> >>>>>> This doesn't feel right. /dev/kvm ctime is changed every time qemu is
>>> >>>>>> started or powered off (try running stat ove
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Thu, Nov 01, 2018 at 10:18 AM +0100, "Daniel P. Berrangé" wrote: > On Thu, Nov 01, 2018 at 09:31:50AM +0100, Martin Kletzander wrote: >> On Tue, Oct 30, 2018 at 03:07:59PM +, Daniel P. Berrangé wrote: >> > On Tue, Oct 30, 2018 at 03:45:36PM +0100, Michal Privoznik wrote: >> > > On 10/30/2018 02:46 PM, Michal Privoznik wrote: >> > > > >> > > > It is kernel problem. In my testing, the moment I call: >> > > > >> > > > ioctl(kvm, KVM_CREATE_VM, 0); >> > > >> > > Okay, I have to retract this claim. 'udevadm monitor' shows some events: >> > > >> > > KERNEL[3631.129645] change /devices/virtual/misc/kvm (misc) >> > > UDEV [3631.130816] change /devices/virtual/misc/kvm (misc) >> > > >> > > and stopping udevd leaves all three times untouched. So it is udev after >> > > all. I just don't know how to find the rule that is causing the issue. >> > > Anyway, as for this patch, I think we can merge it in the end, can't we? >> > >> > Not really. Udev is in use everywhere, so this behaviour makes the >> > patch useless in practice, even though it is technically right in >> > theory :-( >> > >> >> Does it? With this behaviour we still do the "expensive" work after any >> machine >> has started. But for one machine starting it still has the effect of >> running it >> only once. And we *need* to run it for each machine unless we also cache the >> result per (at least) user:group of that machine as every machine can run >> under >> different user:group. > > Yes, with the current patch it still rechecks it for each VM startup But that happens only because of udev (see Michal’s answer). > , but > I was going to suggest the caching of this is simply put in a global static > variable as there's no reason to record this device permissions state in > the per VM caps cache. I’m not sure I understand you correctly, but this patch doesn’t use the VM/QEMU capabilities cache (that would be 'struct _virQEMUCaps') for the caching. Instead it uses 'struct _virQEMUCapsCachePriv' and this struct is initialized only once when initializing the QEMU driver. This should be pretty similar to your proposal with the global static variable. > > > Regards, > Daniel > -- > |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o-https://fstop138.berrange.com :| > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
Introduce caching whether /dev/kvm is usable as the QEMU user:QEMU
group. This reduces the overhead of the QEMU capabilities cache
lookup. Before this patch there were many fork() calls used for
checking whether /dev/kvm is accessible. Now we store the result
whether /dev/kvm is accessible or not and we only need to re-run the
virFileAccessibleAs check if the ctime of /dev/kvm has changed.
Suggested-by: Daniel P. Berrangé
Signed-off-by: Marc Hartmayer
---
src/qemu/qemu_capabilities.c | 54 ++--
1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e228f52ec0bb..85516954149b 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3238,6 +3238,10 @@ struct _virQEMUCapsCachePriv {
virArch hostArch;
unsigned int microcodeVersion;
char *kernelVersion;
+
+/* cache whether /dev/kvm is usable as runUid:runGuid */
+virTristateBool kvmUsable;
+time_t kvmCtime;
};
typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
@@ -3824,6 +3828,52 @@ virQEMUCapsSaveFile(void *data,
}
+/* Determine whether '/dev/kvm' is usable as QEMU user:QEMU group. */
+static bool
+virQEMUCapsKVMUsable(virQEMUCapsCachePrivPtr priv)
+{
+struct stat sb;
+static const char *kvm_device = "/dev/kvm";
+virTristateBool value;
+virTristateBool cached_value = priv->kvmUsable;
+time_t kvm_ctime;
+time_t cached_kvm_ctime = priv->kvmCtime;
+
+if (stat(kvm_device, ) < 0) {
+virReportSystemError(errno,
+ _("Failed to stat %s"), kvm_device);
+return false;
+}
+kvm_ctime = sb.st_ctime;
+
+if (kvm_ctime != cached_kvm_ctime) {
+VIR_DEBUG("%s has changed (%lld vs %lld)", kvm_device,
+ (long long)kvm_ctime, (long long)cached_kvm_ctime);
+cached_value = VIR_TRISTATE_BOOL_ABSENT;
+}
+
+if (cached_value != VIR_TRISTATE_BOOL_ABSENT)
+return cached_value == VIR_TRISTATE_BOOL_YES;
+
+if (virFileAccessibleAs(kvm_device, R_OK | W_OK,
+priv->runUid, priv->runGid) == 0) {
+value = VIR_TRISTATE_BOOL_YES;
+} else {
+value = VIR_TRISTATE_BOOL_NO;
+}
+
+/* There is a race window between 'stat' and
+ * 'virFileAccessibleAs'. However, since we're only interested in
+ * detecting changes *after* the virFileAccessibleAs check, we can
+ * neglect this here.
+ */
+priv->kvmCtime = kvm_ctime;
+priv->kvmUsable = value;
+
+return value == VIR_TRISTATE_BOOL_YES;
+}
+
+
static bool
virQEMUCapsIsValid(void *data,
void *privData)
@@ -3872,8 +3922,7 @@ virQEMUCapsIsValid(void *data,
return true;
}
-kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK,
-priv->runUid, priv->runGid) == 0;
+kvmUsable = virQEMUCapsKVMUsable(priv);
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
kvmUsable) {
@@ -4684,6 +4733,7 @@ virQEMUCapsCacheNew(const char *libDir,
priv->runUid = runUid;
priv->runGid = runGid;
priv->microcodeVersion = microcodeVersion;
+priv->kvmUsable = VIR_TRISTATE_BOOL_ABSENT;
if (uname() == 0 &&
virAsprintf(>kernelVersion, "%s %s", uts.release, uts.version) <
0)
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Introduce caching whether /dev/kvm is accessible
Introduce caching whether /dev/kvm is usable as the QEMU user:QEMU
group. This reduces the overhead of the QEMU capabilities cache
lookup. Before this patch there were many fork() calls used for
checking whether /dev/kvm is accessible. Now we store the result
whether /dev/kvm is accessible or not and we only need to re-run the
virFileAccessibleAs check if the ctime of /dev/kvm has changed.
Suggested-by: Daniel P. Berrangé
Signed-off-by: Marc Hartmayer
---
src/qemu/qemu_capabilities.c | 56 ++--
1 file changed, 54 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e228f52ec0bb..ea95915f0f71 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3238,6 +3238,10 @@ struct _virQEMUCapsCachePriv {
virArch hostArch;
unsigned int microcodeVersion;
char *kernelVersion;
+
+/* cache whether /dev/kvm is usable as runUid:runGuid */
+virTristateBool kvmUsable;
+time_t kvmCtime;
};
typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
@@ -3824,6 +3828,54 @@ virQEMUCapsSaveFile(void *data,
}
+/* Determine whether '/dev/kvm' is usable as QEMU user:QEMU group. */
+static bool
+virQEMUCapsKVMUsable(virQEMUCapsCachePrivPtr priv)
+{
+struct stat sb;
+static const char *kvm_device = "/dev/kvm";
+virTristateBool value;
+virTristateBool cached_value = priv->kvmUsable;
+time_t kvm_ctime;
+time_t cached_kvm_ctime = priv->kvmCtime;
+
+if (stat(kvm_device, ) < 0) {
+virReportSystemError(errno,
+ _("Failed to stat %s"), kvm_device);
+return false;
+}
+kvm_ctime = sb.st_ctime;
+
+if (cached_value != VIR_TRISTATE_BOOL_ABSENT) {
+if (kvm_ctime != cached_kvm_ctime) {
+VIR_DEBUG("%s has changed (%lld vs %lld)", kvm_device,
+ (long long)kvm_ctime, (long long)cached_kvm_ctime);
+goto update;
+}
+
+return cached_value == VIR_TRISTATE_BOOL_YES;
+}
+
+ update:
+if (virFileAccessibleAs(kvm_device, R_OK | W_OK,
+priv->runUid, priv->runGid) == 0) {
+value = VIR_TRISTATE_BOOL_YES;
+} else {
+value = VIR_TRISTATE_BOOL_NO;
+}
+
+/* There is a race window between 'stat' and
+ * 'virFileAccessibleAs'. However, since we're only interested in
+ * detecting changes *after* the virFileAccessibleAs check, we can
+ * neglect this here.
+ */
+priv->kvmCtime = kvm_ctime;
+priv->kvmUsable = value;
+
+return value == VIR_TRISTATE_BOOL_YES;
+}
+
+
static bool
virQEMUCapsIsValid(void *data,
void *privData)
@@ -3872,8 +3924,7 @@ virQEMUCapsIsValid(void *data,
return true;
}
-kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK,
-priv->runUid, priv->runGid) == 0;
+kvmUsable = virQEMUCapsKVMUsable(priv);
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
kvmUsable) {
@@ -4684,6 +4735,7 @@ virQEMUCapsCacheNew(const char *libDir,
priv->runUid = runUid;
priv->runGid = runGid;
priv->microcodeVersion = microcodeVersion;
+priv->kvmUsable = VIR_TRISTATE_BOOL_ABSENT;
if (uname() == 0 &&
virAsprintf(>kernelVersion, "%s %s", uts.release, uts.version) <
0)
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/11] Avoid numerous calls of virQEMUCapsCacheLookup
On Wed, Oct 24, 2018 at 11:43 PM +0200, "Daniel P. Berrangé"
wrote:
> On Thu, Sep 20, 2018 at 07:44:46PM +0200, Marc Hartmayer wrote:
>> For a domain definition there are numerous calls of
>> virQEMUCapsCacheLookup (the same applies to the domain start). This
>> slows down the process since virQEMUCapsCacheLookup validates that the
>> QEMU capabilitites are still valid (among other things, a fork is done
>> for this if the user for the QEMU processes is 'qemu'). Therefore
>> let's reduce the number of virQEMUCapsCacheLookup calls whenever
>> possible and reasonable.
>>
>> In addition to the speed up, there is the risk that
>> virQEMUCapsCacheLookup returns different QEMU capabilities during a
>> task if, for example, the QEMU binary has changed during the task.
>>
>> The correct way would be:
>>
>> - get the QEMU capabilities only once per task via virQEMUCapsCacheLookup
>> - do the task with these QEMU capabilities
>>
>> or
>>
>> - abort the task after a cache invalidation
>>
>> Note: With this patch series the behavior is still not (completely)
>> fixed, but the performance has been significantly improved. In a quick
>> test this gave a speed up of factor 4 for a simple define/undefine
>> loop.
>>
>> In general, the more devices a domain has, the more drastic the
>> overhead becomes (because a cache validation is performed for each
>> device).
>
> IIUC from your KVM Forum presentation, the overhead of the
> cache lookup is almost entirely coming from the fork() call
> triggered by the single call
>
> kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK,
> priv->runUid, priv->runGid) == 0;
>
> Rather than the major refactor of the way the parse callbacks
> work, we could instead just optimize this call to avoid the
> repeated forks.
>
> Even if we reduced the number of calls to the cache, it is
> still somewhat overkill to be checking /dev/kvm via fork()
> every time. eg even if you reduced it to just a single
> cache lookup, if you run virDomainDefine for 100 domains
> in parallel it is still going to do 100 forks.
>
> We could optimize this by jcalling virFileAccessibleAs
> once and storing the result in a global. Then just do a
> plain stat() call in process to check the st_ctime field
> for changes. We only need re-run the heavy virFileAccessibleAs
> check if st_ctime has changed (indicating a owner/group/acl
> change).
Okay, if that’s fine I’ll add this as an additional patch to this
series. Because the various virQEMUCapsCacheLookup calls still seem to
be wrong to me.
Marc
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC v2] virfile: fix cast-align error
On s390x the struct member f_type of statsfs is hard coded to
'unsigned int'. So let's try to determine the type by means of the GNU
extension typeof.
This fixes the following error:
../../src/util/virfile.c:3578:38: error: cast increases required alignment of
target type [-Werror=cast-align]
virFileIsSharedFixFUSE(path, (long *) _type);
Signed-off-by: Marc Hartmayer
---
src/util/virfile.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 2a7e87102a25..6cde4ab6c23b 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3464,9 +3464,15 @@ int virFilePrintf(FILE *fp, const char *msg, ...)
# define PROC_MOUNTS "/proc/mounts"
+# ifdef __GNUC__
+typedef typeof(((struct statfs*)0)->f_type) f_type_type;
+# else
+typedef long f_type_type;
+# endif
+
static int
virFileIsSharedFixFUSE(const char *path,
- long *f_type)
+ f_type_type *f_type)
{
char *dirpath = NULL;
const char **mounts = NULL;
@@ -3575,7 +3581,7 @@ virFileIsSharedFSType(const char *path,
if (sb.f_type == FUSE_SUPER_MAGIC) {
VIR_DEBUG("Found FUSE mount for path=%s. Trying to fix it", path);
-virFileIsSharedFixFUSE(path, (long *) _type);
+virFileIsSharedFixFUSE(path, _type);
}
VIR_DEBUG("Check if path %s with FS magic %lld is shared",
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC] virfile: fix cast-align error
Use the correct type in order to fix the following error on s390x:
In function 'virFileIsSharedFSType':
../../src/util/virfile.c:3578:38: error: cast increases required alignment of
target type [-Werror=cast-align]
virFileIsSharedFixFUSE(path, (long *) _type);
Signed-off-by: Marc Hartmayer
---
src/util/virfile.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 2a7e87102a25..832d832696d5 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3466,7 +3466,7 @@ int virFilePrintf(FILE *fp, const char *msg, ...)
static int
virFileIsSharedFixFUSE(const char *path,
- long *f_type)
+ unsigned int *f_type)
{
char *dirpath = NULL;
const char **mounts = NULL;
@@ -3575,7 +3575,7 @@ virFileIsSharedFSType(const char *path,
if (sb.f_type == FUSE_SUPER_MAGIC) {
VIR_DEBUG("Found FUSE mount for path=%s. Trying to fix it", path);
-virFileIsSharedFixFUSE(path, (long *) _type);
+virFileIsSharedFixFUSE(path, _type);
}
VIR_DEBUG("Check if path %s with FS magic %lld is shared",
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
On Mon, Oct 01, 2018 at 02:50 PM +0200, Peter Krempa wrote: > On Mon, Oct 01, 2018 at 14:38:40 +0200, Marc Hartmayer wrote: >> On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa >> wrote: >> > On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote: >> >> On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan >> >> wrote: >> >> > On 9/20/18 1:44 PM, Marc Hartmayer wrote: >> >> >> Move the domainPostParseDataAlloc/Free calls to >> >> >> virDomainDeviceDefParse. As an consequence >> >> >> virDomainDeviceDefPostParseOne is superfluous and can therefore be >> >> >> removed. >> >> >> >> >> >> Signed-off-by: Marc Hartmayer >> >> >> Reviewed-by: Boris Fiuczynski >> >> >> --- >> >> >> src/conf/domain_conf.c | 37 +++-- >> >> >> 1 file changed, 11 insertions(+), 26 deletions(-) >> >> >> >> >> > >> >> > I'm not against this per se; however, I should not that the code was >> >> > specifically extracted in commit e168bc8a. >> >> >> >> There are the following three functions: >> >> >> >> virDomainDeviceDefParse >> >> virDomainDeviceDefPostParse >> >> virDomainDeviceDefPostParseOne >> >> >> >> Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid >> >> the allocation of private data across the callbacks. This is absolutely >> >> fine. >> >> >> >> What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to >> >> virDomainDeviceDefParse instead of having an extra wrapper function >> >> (virDomainDeviceDefPostParse/One) for that. With this change I can reuse >> >> the QEMU caps for the virDomainDeviceDefValidate call in >> >> virDomainDeviceDefParse as well. >> > >> > For the above it's not necessary to open-code what >> > virDomainDeviceDefPostParseOne >> > in a rather massive function. You can pass the opaque in if you want. >> >> I don’t get it. Where can I pass the opaque? > > virDomainDeviceDefParse is a big spaghettified function and > adding other code to it is unwanted. If you need to unify the allocation > of private data, move the validation function call to the helper (with > appropriate rename) rather than blatantly pasting the code back. Ahhh okay. Any ideas for the name? Thanks. Side note: What has always surprised me is that the "virDomainDeviceDefPostParse" function is called right out of the context of the function "virDomainDeviceDefParse". Shouldn’t it be called directly _after_ virDomainDeviceDefParse? -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 12:36 PM +0200, Peter Krempa wrote: […snip…] >> I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t >> valid anymore) as early as possible in the overall “task/job” process. > > Well, technically priv->qemuCaps is invalid and unneeded when the VM is > not running and valid and immutable if it is running. > > If the VM is not running, there is no qemu process associated to it so > it does not make sense to store random qemuCaps along with it, since the > qemu binary might change at the point when we attempt to start it. Yep, that makes sense. > > I was not a fan of needing qemuCaps in the post parse infrastructure of > qemu, but at this point we can't do much about it especially as we want > to keep the config stable after defined. > >> > This patch does not seem to make sense with the justification provided >> > here as the post-parse callbacks fill in the proper caps here and this >> > part clearly uses a new domain configuration, so the default behaviour >> > should be used. >> > >> > Changing this would also need that we change the normal parser path to >> > achieve the same results which is impossible as you don't have access to >> > priv->qemuCaps prior to creating the virDomainObj object. >> >> Yep, that’s why I said in the cover letter “With this patch series the >> behavior is still not (completely) fixed, but the performance has been >> significantly improved.”. > > I don't see what should be fixed here, but mostly as I did not read the > series. The problem is that we currently don’t take into account that the QEMU binary can change during a task (e.g. define a domain). Therefore, it’s possible that two calls of virQEMUCapsCacheLookup return different QEMU capabilities. […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa wrote: > On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote: >> On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan >> wrote: >> > On 9/20/18 1:44 PM, Marc Hartmayer wrote: >> >> Move the domainPostParseDataAlloc/Free calls to >> >> virDomainDeviceDefParse. As an consequence >> >> virDomainDeviceDefPostParseOne is superfluous and can therefore be >> >> removed. >> >> >> >> Signed-off-by: Marc Hartmayer >> >> Reviewed-by: Boris Fiuczynski >> >> --- >> >> src/conf/domain_conf.c | 37 +++-- >> >> 1 file changed, 11 insertions(+), 26 deletions(-) >> >> >> > >> > I'm not against this per se; however, I should not that the code was >> > specifically extracted in commit e168bc8a. >> >> There are the following three functions: >> >> virDomainDeviceDefParse >> virDomainDeviceDefPostParse >> virDomainDeviceDefPostParseOne >> >> Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid >> the allocation of private data across the callbacks. This is absolutely >> fine. >> >> What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to >> virDomainDeviceDefParse instead of having an extra wrapper function >> (virDomainDeviceDefPostParse/One) for that. With this change I can reuse >> the QEMU caps for the virDomainDeviceDefValidate call in >> virDomainDeviceDefParse as well. > > For the above it's not necessary to open-code what > virDomainDeviceDefPostParseOne > in a rather massive function. You can pass the opaque in if you want. I don’t get it. Where can I pass the opaque? At the end, we must use the same qemuCaps in virDomainDeviceDefValidate that we already used for virDomainDeviceDefPostParse(One). > > Please don't remove the wrapper. > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/11] conf: Use domainPostParseData(Alloc|Free) in virDomainDefValidate
On Sat, Sep 29, 2018 at 05:35 PM +0200, John Ferlan wrote:
> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
>> For the usage of the parameter @parseOpqaue within
>> virDomainDefValidate it must be ensured that the parameter
>> @parseOpaque is not NULL. But since there are code paths where
>> virDomainDefValidate is called with @parseOpaque ==
>> NULL (e.g. the function call of virDomainDefParseNode with @parseOpqaue ==
>> NULL leads to this).
>>
>> To prevent this, domainPostParseDataAlloc is called within
>> virDomainDefValidate to ensure that @parseOpaque is always set. The
>> usage of domainPostParseDataAlloc within virDomainDefValidate is
>> allowed since virDomainDefValidate is only called after
>> virDomainDefPostParse.
>>
>> Signed-off-by: Marc Hartmayer
>> Reviewed-by: Boris Fiuczynski
>> ---
>> src/conf/domain_conf.c | 21 +
>> 1 file changed, 21 insertions(+)
>>
>
> Thus is similar to commit e168bc8a did for virDomainDefPostParse...
Yes. That was actually my “inspiration”.
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index ae7f3ed95faf..4f1c569b5733 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6288,6 +6288,9 @@ virDomainDefValidate(virDomainDefPtr def,
>> virDomainXMLOptionPtr xmlopt,
>> void *parseOpaque)
>> {
>> +int ret = -1;
>> +bool localParseOpaque = false;
>> +
>> struct virDomainDefPostParseDeviceIteratorData data = {
>> .caps = caps,
>> .xmlopt = xmlopt,
>> @@ -6299,6 +6302,17 @@ virDomainDefValidate(virDomainDefPtr def,
>> if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)
>> return 0;
>>
>> +if (!data.parseOpaque &&
>> +xmlopt->config.domainPostParseDataAlloc) {
>> +ret = xmlopt->config.domainPostParseDataAlloc(def, caps, parseFlags,
>> + xmlopt->config.priv,
>> + );
>
> So ret = 1 if virQEMUCapsCacheLookup failed in
> qemuDomainPostParseDataAlloc; otherwise, ret = 0; Just looks strange
> below with the ret == 1 check.
Yes, do you have something better in mind? Maybe we can add an error
label. While fixing it we can also adapt virDomainDefPostParse (were
it’s, in my opinion, even more confusing since @ret is used multiple
times).
>
>> +
>> +if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0)
>
> This though has/uses specific @parseFlag options and sets
> def->postParseFailed when it may not be true if for some reason of
> course the post parse processing never got the parseOpaque value.
> Wouldn't it perhaps be problematic in the patch3 scenario where you'd be
> passing the priv->qemuCaps when @postParseFailed?
See my answer for patch 3.
>
> John
>
>> +goto cleanup;
>> +localParseOpaque = true;
>> +}
>> +
>> /* call the domain config callback */
>> if (xmlopt->config.domainValidateCallback &&
>> xmlopt->config.domainValidateCallback(def, caps,
>> xmlopt->config.priv, data.parseOpaque) < 0)
>> @@ -6313,6 +6327,13 @@ virDomainDefValidate(virDomainDefPtr def,
>> if (virDomainDefValidateInternal(def) < 0)
>> return -1;
>>
>> + cleanup:
>> +if (localParseOpaque && xmlopt->config.domainPostParseDataFree)
>> +xmlopt->config.domainPostParseDataFree(data.parseOpaque);
>> +
>> +if (ret == 1)
>> +return -1;
>> +
>> return 0;
>> }
>>
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/11] conf: Extend virDomainDefValidate(Callback) for parseOpaque
On Sat, Sep 29, 2018 at 05:34 PM +0200, John Ferlan wrote:
> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
>> Add @parseOpaque argument to virDomainDefValidate and
>> virDomainDefValidateCallback, but don't use it for now since it's not
>> ensured that it's always a non-NULL value.
>>
>> Signed-off-by: Marc Hartmayer
>> Reviewed-by: Boris Fiuczynski
>> ---
>> src/conf/domain_conf.c | 11 +++
>> src/conf/domain_conf.h | 6 --
>> src/qemu/qemu_domain.c | 3 ++-
>> src/qemu/qemu_process.c | 2 +-
>> src/vz/vz_driver.c | 3 ++-
>> 5 files changed, 16 insertions(+), 9 deletions(-)
>>
>
> I like this idea especially since the Validate paths are the ones where
> qemuCaps seem to be most useful.
>
> Couple of nits below
>
> John
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index e61f04ea2271..ae7f3ed95faf 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6271,6 +6271,7 @@ virDomainDefValidateInternal(const virDomainDef *def)
>> * @caps: driver capabilities object
>> * @parseFlags: virDomainDefParseFlags
>> * @xmlopt: XML parser option object
>> + * @parseOpaque: opaque data and it might be NULL (for QEMU driver it's
>> qemuCaps)
>> *
>> * This validation function is designed to take checks of globally invalid
>> * configurations that the parser needs to accept so that VMs don't vanish
>> upon
>> @@ -6284,12 +6285,14 @@ int
>> virDomainDefValidate(virDomainDefPtr def,
>> virCapsPtr caps,
>> unsigned int parseFlags,
>> - virDomainXMLOptionPtr xmlopt)
>> + virDomainXMLOptionPtr xmlopt,
>> + void *parseOpaque)
>> {
>> struct virDomainDefPostParseDeviceIteratorData data = {
>> .caps = caps,
>> .xmlopt = xmlopt,
>> .parseFlags = parseFlags,
>> +.parseOpaque = parseOpaque,
>> };
>>
>> /* validate configuration only in certain places */
>> @@ -6298,7 +6301,7 @@ virDomainDefValidate(virDomainDefPtr def,
>>
>> /* call the domain config callback */
>> if (xmlopt->config.domainValidateCallback &&
>> -xmlopt->config.domainValidateCallback(def, caps,
>> xmlopt->config.priv) < 0)
>> +xmlopt->config.domainValidateCallback(def, caps,
>> xmlopt->config.priv, data.parseOpaque) < 0)
>> return -1;
>>
>> /* iterate the devices */
>> @@ -21063,7 +21066,7 @@ virDomainObjParseXML(xmlDocPtr xml,
>> goto error;
>>
>> /* valdiate configuration */
>
> May as well fix the typo above *validate
Will change.
>
>> -if (virDomainDefValidate(obj->def, caps, flags, xmlopt) < 0)
>> +if (virDomainDefValidate(obj->def, caps, flags, xmlopt, parseOpaque) <
>> 0)
>> goto error;
>>
>> return obj;
>> @@ -21154,7 +21157,7 @@ virDomainDefParseNode(xmlDocPtr xml,
>> goto cleanup;
>>
>> /* valdiate configuration */
>
> Consistency is the key ;-)
Yep :D
>
>> -if (virDomainDefValidate(def, caps, flags, xmlopt) < 0)
>> +if (virDomainDefValidate(def, caps, flags, xmlopt, parseOpaque) < 0)
>> goto cleanup;
>>
[…snip…]
>> static int
>> vzDomainDefValidate(const virDomainDef *def,
>> virCapsPtr caps ATTRIBUTE_UNUSED,
>> -void *opaque)
>> +void *opaque,
>> +void *parserOpaque ATTRIBUTE_UNUSED)
>
> nit: @parseOpaque
Okay.
>
>> {
>> if (vzCheckUnsupportedControllers(def, opaque) < 0)
>> return -1;
>>
>
Thanks for the review!
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
