[libvirt] [PATCH v2 0/3] Better syntax-check on BSD
Since v1: fix the gnulib bug I accidentally introduced, then copy the gist of Roman's gnulib changes to also apply to our cfg.mk syntax checks. Eric Blake (3): maint: update gnulib for syntax-check on BSD maint: prefer $(GREP) in cfg.mk maint: split long lines for BSD syntax-check .gnulib | 2 +- cfg.mk | 95 + 2 files changed, 49 insertions(+), 48 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/3] maint: split long lines for BSD syntax-check
Similar to the gnulib changes we just incorporated into maint.mk,
it's time to use '$(VC_LIST) | xargs program' instead of
'program $$($(VC_LIST))', in order to bypass the problem of hitting
argv limits due to our large set of files.
Drop several uses of $$files as a temporary variable when we can
instead directly use xargs. While at it, fix a typo in the
prohibit_windows_special_chars error message.
Signed-off-by: Eric Blake
---
cfg.mk | 75 +-
1 file changed, 38 insertions(+), 37 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 9956d20034..e2702d50c9 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -639,8 +639,10 @@ sc_libvirt_unmarked_diagnostics:
exclude='_\(' \
halt='found unmarked diagnostic(s)' \
$(_sc_search_regexp)
- @{ $(GREP) -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \
- $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \
+ @{ $(VC_LIST_EXCEPT) | xargs \
+ $(GREP) -nE '\<$(func_re) *\(.*;$$' /dev/null; \
+ $(VC_LIST_EXCEPT) | xargs \
+ $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' /dev/null; } \
| $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[]"%s"//' \
| $(GREP) '[ ]"' && \
{ echo '$(ME): found unmarked diagnostic(s)' 1>&2; \
@@ -654,8 +656,8 @@ sc_libvirt_unmarked_diagnostics:
# there are functions to which this one applies but that do not get marked
# diagnostics.
sc_prohibit_newline_at_end_of_diagnostic:
- @$(GREP) -A2 -nE \
- '\<$(func_re) *\(' $$($(VC_LIST_EXCEPT)) \
+ @$(VC_LIST_EXCEPT) | xargs $(GREP) -A2 -nE \
+ '\<$(func_re) *\(' /dev/null \
| $(GREP) '\\n"' \
&& { echo '$(ME): newline at end of message(s)' 1>&2; \
exit 1; } || :
@@ -664,8 +666,10 @@ sc_prohibit_newline_at_end_of_diagnostic:
# allow VIR_ERROR to do this, and ignore functions that take a single
# string rather than a format argument.
sc_prohibit_diagnostic_without_format:
- @{ $(GREP) -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \
- $(GREP) -A2 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \
+ @{ $(VC_LIST_EXCEPT) | xargs \
+ $(GREP) -nE '\<$(func_re) *\(.*;$$' /dev/null; \
+ $(VC_LIST_EXCEPT) | xargs \
+ $(GREP) -A2 -nE '\<$(func_re) *\(.*,$$' /dev/null; } \
| $(SED) -rn -e ':l; /[,"]$$/ {N;b l;}' \
-e '/(xenapiSessionErrorHandler|vah_(error|warning))/d' \
-e '/\<$(func_re) *\([^"]*"([^%"]|"\n[^"]*")*"[,)]/p' \
@@ -687,7 +691,7 @@ sc_prohibit_useless_translation:
# When splitting a diagnostic across lines, ensure that there is a space
# or \n on one side of the split.
sc_require_whitespace_in_translation:
- @$(GREP) -n -A1 '"$$' $$($(VC_LIST_EXCEPT)) \
+ @$(VC_LIST_EXCEPT) | xargs $(GREP) -n -A1 '"$$' /dev/null \
| $(SED) -ne ':l; /"$$/ {N;b l;}; s/"\n[^"]*"/""/g; s/\\n/ /g' \
-e '/_(.*[^\ ]""[^\ ]/p' | $(GREP) . && \
{ echo '$(ME): missing whitespace at line split' 1>&2; \
@@ -803,7 +807,8 @@ sc_prohibit_cross_inclusion:
# When converting an enum to a string, make sure that we track any new
# elements added to the enum by using a _LAST marker.
sc_require_enum_last_marker:
- @$(GREP) -A1 -nE '^[^#]*VIR_ENUM_IMPL *\(' $$($(VC_LIST_EXCEPT)) \
+ @$(VC_LIST_EXCEPT) | xargs \
+ $(GREP) -A1 -nE '^[^#]*VIR_ENUM_IMPL *\(' /dev/null \
| $(SED) -ne '/VIR_ENUM_IMPL[^,]*,$$/N' \
-e '/VIR_ENUM_IMPL[^,]*,[^,]*[^_,][^L,][^A,][^S,][^T,],/p' \
-e '/VIR_ENUM_IMPL[^,]*,[^,]\{0,4\},/p' \
@@ -866,8 +871,7 @@ sc_prohibit_atoi:
$(_sc_search_regexp)
sc_prohibit_wrong_filename_in_comment:
- @fail=0; \
- awk 'BEGIN { \
+ @$(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$' | xargs awk 'BEGIN { \
fail=0; \
} FNR < 3 { \
n=match($$0, /[[:space:]][^[:space:]]*[.][ch][[:space:]:]/); \
@@ -883,11 +887,8 @@ sc_prohibit_wrong_filename_in_comment:
if (fail == 1) { \
exit 1; \
} \
- }' $$($(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$') || fail=1; \
- if test $$fail -eq 1; then \
- { echo '$(ME): The file name in comments must match the' \
- 'actual file name' 1>&2; exit 1; } \
- fi;
+ }' || { echo '$(ME): The file name in comments must match the' \
+ 'actual file name' 1>&2; exit 1; }
sc_prohibit_virConnectOpen_in_virsh:
@prohibit='\bvirConnectOpen[a-zA-Z]* *\(' \
@@ -918,22 +919,21 @@ sc_require_if_else_matching_braces:
$(_sc_search_regexp)
sc_curly_braces_style:
- @files=$$($(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$'); \
- if $(GREP) -nHP \
+ @if $(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$' | xargs $(GREP) -nHP \
'^\s*(?!([a-zA-Z_]*for_?each[a-zA-Z_]*) ?\()([_a-zA-Z0-9]+( [_a-zA-Z0-9]+)*
[libvirt] [PATCH v2 2/3] maint: prefer $(GREP) in cfg.mk
We already used $(GREP) in some places, but might as well use it
everywhere during syntax check, in line with similar recent gnulib
changes.
---
cfg.mk | 54 +++---
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 90ec8dfc60..9956d20034 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1,5 +1,5 @@
# Customize Makefile.maint. -*- makefile -*-
-# Copyright (C) 2008-2015 Red Hat, Inc.
+# Copyright (C) 2008-2019 Red Hat, Inc.
# Copyright (C) 2003-2008 Free Software Foundation, Inc.
# This program is free software: you can redistribute it and/or modify
@@ -305,7 +305,7 @@ sc_flags_usage:
$(srcdir)/include/libvirt/libvirt-qemu.h \
$(srcdir)/include/libvirt/libvirt-lxc.h \
$(srcdir)/include/libvirt/libvirt-admin.h \
- | grep -c '\(long\|unsigned\) flags')" != 4 && \
+ | $(GREP) -c '\(long\|unsigned\) flags')" != 4 && \
{ echo '$(ME): new API should use "unsigned int flags"' 1>&2; \
exit 1; } || :
@prohibit=' flags ATTRIBUTE_UNUSED' \
@@ -639,10 +639,10 @@ sc_libvirt_unmarked_diagnostics:
exclude='_\(' \
halt='found unmarked diagnostic(s)' \
$(_sc_search_regexp)
- @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \
- grep -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \
+ @{ $(GREP) -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \
+ $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \
| $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[]"%s"//' \
- | grep '[ ]"' && \
+ | $(GREP) '[ ]"' && \
{ echo '$(ME): found unmarked diagnostic(s)' 1>&2; \
exit 1; } || :
@@ -654,9 +654,9 @@ sc_libvirt_unmarked_diagnostics:
# there are functions to which this one applies but that do not get marked
# diagnostics.
sc_prohibit_newline_at_end_of_diagnostic:
- @grep -A2 -nE \
+ @$(GREP) -A2 -nE \
'\<$(func_re) *\(' $$($(VC_LIST_EXCEPT)) \
- | grep '\\n"' \
+ | $(GREP) '\\n"' \
&& { echo '$(ME): newline at end of message(s)' 1>&2; \
exit 1; } || :
@@ -664,12 +664,12 @@ sc_prohibit_newline_at_end_of_diagnostic:
# allow VIR_ERROR to do this, and ignore functions that take a single
# string rather than a format argument.
sc_prohibit_diagnostic_without_format:
- @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \
- grep -A2 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \
+ @{ $(GREP) -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \
+ $(GREP) -A2 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \
| $(SED) -rn -e ':l; /[,"]$$/ {N;b l;}' \
-e '/(xenapiSessionErrorHandler|vah_(error|warning))/d' \
-e '/\<$(func_re) *\([^"]*"([^%"]|"\n[^"]*")*"[,)]/p' \
- | grep -vE 'VIR_ERROR' && \
+ | $(GREP) -vE 'VIR_ERROR' && \
{ echo '$(ME): found diagnostic without %' 1>&2; \
exit 1; } || :
@@ -687,16 +687,16 @@ sc_prohibit_useless_translation:
# When splitting a diagnostic across lines, ensure that there is a space
# or \n on one side of the split.
sc_require_whitespace_in_translation:
- @grep -n -A1 '"$$' $$($(VC_LIST_EXCEPT)) \
+ @$(GREP) -n -A1 '"$$' $$($(VC_LIST_EXCEPT)) \
| $(SED) -ne ':l; /"$$/ {N;b l;}; s/"\n[^"]*"/""/g; s/\\n/ /g' \
- -e '/_(.*[^\ ]""[^\ ]/p' | grep . && \
+ -e '/_(.*[^\ ]""[^\ ]/p' | $(GREP) . && \
{ echo '$(ME): missing whitespace at line split' 1>&2; \
exit 1; } || :
# Enforce recommended preprocessor indentation style.
sc_preprocessor_indentation:
@if cppi --version >/dev/null 2>&1; then \
- $(VC_LIST_EXCEPT) | grep -E '\.[ch](\.in)?$$' | xargs cppi -a -c \
+ $(VC_LIST_EXCEPT) | $(GREP) -E '\.[ch](\.in)?$$' | xargs cppi -a -c \
|| { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
exit 1; }; \
else \
@@ -707,13 +707,13 @@ sc_preprocessor_indentation:
# (comment-only) C file that mirrors the same layout as the spec file.
sc_spec_indentation:
@if cppi --version >/dev/null 2>&1; then \
- for f in $$($(VC_LIST_EXCEPT) | grep '\.spec\.in$$'); do \
+ for f in $$($(VC_LIST_EXCEPT) | $(GREP) '\.spec\.in$$'); do \
$(SED) -e 's|#|// #|; s|%ifn*\(arch\)* |#if a // |' \
-e 's/%\(else\|endif\|define\)/#\1/' \
-e 's/^\( *\)\1\1\1#/#\1/' \
-e 's|^\( *[^#/ ]\)|// \1|; s|^\( */[^/]\)|// \1|' $$f \
| cppi -a -c 2>&1 | $(SED) "s|standard input|$$f|"; \
- done | { if grep . >&2; then false; else :; fi; } \
+ done | { if $(GREP) . >&2; then false; else :; fi; } \
|| { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
[libvirt] [PATCH v2 1/3] maint: update gnulib for syntax-check on BSD
In particular, this incorporates Roman's patches to allow 'make syntax-check' to work on BSD with its exec argv limitations that previously failed when trying to grep the large number of files present in libvirt. cfg.mk needs similar changes, but that will be tackled separately. Signed-off-by: Eric Blake --- .gnulib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gnulib b/.gnulib index 4652c7bafa..70eb4687b1 16 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 4652c7bafa60332145f1e05a7de5f48e1bc56226 +Subproject commit 70eb4687b1d84ae780bbf2b8141bee07c85e6d3b -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 21/36] qemu_process: Use unique directories for QMP processes
On Sun, Dec 02, 2018 at 23:10:15 -0600, Chris Venteicher wrote:
> Multiple QEMU processes for QMP commands can operate concurrently.
>
> Use a unique directory under libDir for each QEMU processes
> to avoid pidfile and unix socket collision between processes.
>
> The pid file name is changed from "capabilities.pidfile" to "qmp.pid"
> because we no longer need to avoid a possible clash with a qemu domain
> called "capabilities" now that the processes artifacts are stored in
> their own unique temporary directories.
>
> "Capabilities" was changed to "qmp" in the pid file name because these
> processes are no longer specific to the capabilities usecase and are
> more generic in terms of being used for any general purpose QMP message
> exchanges with a QEMU process that is not associated with a domain.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_process.c | 24 ++--
> src/qemu/qemu_process.h | 1 +
> 2 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 80b938cc0b..26ba59143d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8119,6 +8119,7 @@ qemuProcessQmpFree(qemuProcessQmpPtr proc)
>
> VIR_FREE(proc->binary);
> VIR_FREE(proc->libDir);
> +VIR_FREE(proc->uniqDir);
> VIR_FREE(proc->monpath);
> VIR_FREE(proc->monarg);
> VIR_FREE(proc->pidfile);
> @@ -8181,33 +8182,33 @@ qemuProcessQmpNew(const char *binary,
> static int
> qemuProcessQmpInit(qemuProcessQmpPtr proc)
> {
> +char *template = NULL;
> int ret = -1;
>
> VIR_DEBUG("Beginning VM startup process"
>" proc=%p, emulator=%s",
>proc, proc->binary);
>
> -/* the ".sock" sufix is important to avoid a possible clash with a qemu
> - * domain called "capabilities"
> - */
> -if (virAsprintf(>monpath, "%s/%s", proc->libDir,
> -"capabilities.monitor.sock") < 0)
> +if (virAsprintf(, "%s/qemu.XX", proc->libDir) < 0)
Directories for domains are called "domain-%s" so maybe we could call
directories for QMP processes as "qmp-%s" rather than "qemu.%s".
> +goto cleanup;
> +
> +proc->uniqDir = mkdtemp(template);
mkdtemp returns NULL on error and you need to handle it here.
> +
> +if (virAsprintf(>monpath, "%s/%s", proc->uniqDir,
> +"qmp.monitor") < 0)
> goto cleanup;
...
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 22/36] qemu_process: Stop locking QMP process monitor immediately
On Sun, Dec 02, 2018 at 23:10:16 -0600, Chris Venteicher wrote:
> Locking the monitor object immediately after call to qemuMonitorOpen
> doesn't make sense now that we have expanded the QEMU process code to
> cover more than the original capabilities usecase.
The expanded use case has nothing to do with locking. You just call QMP
commands and the use case only changes what commands are called.
> Removing the monitor lock makes the qemuProcessQmpConnectMonitor code
> consistent with the qemuConnectMonitor code used to establish the
> monitor when QEMU process is started for domains.
qemuConnectMonitor does not lock the monitor code because we have
EnterMonitor and ExitMonitor calls which lock/unlock the monitor around
every QMP command we call.
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_process.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 26ba59143d..b491f9f91a 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8331,8 +8331,6 @@ qemuProcessQmpConnectMonitor(qemuProcessQmpPtr proc)
>
> VIR_STEAL_PTR(proc->mon, mon);
>
> -virObjectLock(proc->mon);
> -
> /* Exit capabilities negotiation mode and enter QEMU command mode
> * by issuing qmp_capabilities command to QEMU */
> if (qemuMonitorSetCapabilities(proc->mon) < 0) {
All monitor code expects the monitor to be locked. Not to mention that
there is a corresponding unlock in qemuProcessQmpStop().
In other words, NACK to this patch.
Did you hit any issues which inspired you to make this patch? Or was it
just a way to make the code seemingly consistent with
qemuConnectMonitor?
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 20/36] qemu_process: Enter QMP command mode when starting QEMU Process
On Sun, Dec 02, 2018 at 23:10:14 -0600, Chris Venteicher wrote:
> qemuProcessQmpStart starts a QEMU process and monitor connection that
> can be used by multiple functions possibly for multiple QMP commands.
>
> The QMP exchange to exit capabilities negotiation mode and enter command mode
> can only be performed once after the monitor connection is established.
>
> Move responsibility for entering QMP command mode into the qemuProcessQmp
> code so multiple functions can issue QMP commands in arbitrary orders.
>
> This also simplifies the functions using the connection provided by
> qemuProcessQmpStart to issue QMP commands.
>
> Test code now needs to call qemuMonitorSetCapabilities to send the
> message to switch to command mode because the test code does not use the
> qemuProcessQmp command that internally calls qemuMonitorSetCapabilities.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 12
> src/qemu/qemu_process.c | 8
> tests/qemucapabilitiestest.c | 7 +++
> 3 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index ce60648897..038e3ecf7a 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4035,12 +4035,6 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>
> /* @mon is supposed to be locked by callee */
>
> -if (qemuMonitorSetCapabilities(mon) < 0) {
> -VIR_DEBUG("Failed to set monitor capabilities %s",
> - virGetLastErrorMessage());
> -goto cleanup;
> -}
> -
> if (qemuMonitorGetVersion(mon,
>, , ,
>) < 0) {
> @@ -4213,12 +4207,6 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps
> ATTRIBUTE_UNUSED,
> {
> int ret = -1;
>
> -if (qemuMonitorSetCapabilities(mon) < 0) {
> -VIR_DEBUG("Failed to set monitor capabilities %s",
> - virGetLastErrorMessage());
> -goto cleanup;
> -}
> -
> if (virQEMUCapsProbeQMPCPUDefinitions(qemuCaps, mon, true) < 0)
> goto cleanup;
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 24945b1d17..80b938cc0b 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8332,6 +8332,14 @@ qemuProcessQmpConnectMonitor(qemuProcessQmpPtr proc)
>
> virObjectLock(proc->mon);
>
> +/* Exit capabilities negotiation mode and enter QEMU command mode
> + * by issuing qmp_capabilities command to QEMU */
> +if (qemuMonitorSetCapabilities(proc->mon) < 0) {
> +VIR_DEBUG("Failed to set monitor capabilities %s",
> + virGetLastErrorMessage());
> +goto cleanup;
> +}
> +
It's quite unlikely, but we may eventually issue more commands to
initialize the monitor, which would require us to update the code in
qemucapabilitiestest.c again. Moreover, the test is supposed to test the
QEMU capabilities probing code rather than individual monitor commands.
Thus it should avoid direct qemuMonitor* calls.
For this reason, I think it would be better to separate this code into a
new function called qemuProcessQMPInitMonitor. And you can remove the
comment or transform it into a description of the new function. The new
function will be called from here and from qemucapabilitiestest.c. Thus
it cannot be static and its prototype should be placed in
src/qemu/qemu_processpriv.h since the function will be externally called
only from the test suite.
As a bonus, the code will be even more consistent with the VM startup
code, where qemuProcessInitMonitor is doing similar thing.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 19/36] qemu_monitor: Make monitor callbacks optional
On Sun, Dec 02, 2018 at 23:10:13 -0600, Chris Venteicher wrote:
> Qemu process code for capababilities doesn't use monitor callbacks and
> defines empty callback functions.
>
> Allow NULL to be passed to qemuMonitorOpen for callbacks and remove the
> empty functions from the QMP process code.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_monitor.c | 4 ++--
> src/qemu/qemu_process.c | 14 +-
> 2 files changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index a65d638ab8..84065c59dc 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -813,12 +813,12 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
> {
> qemuMonitorPtr mon;
>
> -if (!cb->eofNotify) {
> +if (cb && !cb->eofNotify) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("EOF notify callback must be supplied"));
> return NULL;
> }
> -if (!cb->errorNotify) {
> +if (cb && !cb->errorNotify) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Error notify callback must be supplied"));
> return NULL;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1b6adf1b64..24945b1d17 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8095,18 +8095,6 @@ qemuProcessReconnectAll(virQEMUDriverPtr driver)
> }
>
>
> -static void virQEMUCapsMonitorNotify(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> - virDomainObjPtr vm ATTRIBUTE_UNUSED,
> - void *opaque ATTRIBUTE_UNUSED)
> -{
> -}
> -
> -static qemuMonitorCallbacks callbacks = {
> -.eofNotify = virQEMUCapsMonitorNotify,
> -.errorNotify = virQEMUCapsMonitorNotify,
> -};
> -
> -
> /**
> * qemuProcessQmpFree:
> * @proc: Stores Process and Connection State
> @@ -8302,7 +8290,7 @@ qemuProcessQmpConnectMonitor(qemuProcessQmpPtr proc)
> bool retry = true;
> bool enableJson = true;
> virQEMUDriverPtr driver = NULL;
> -qemuMonitorCallbacksPtr monCallbacks =
> +qemuMonitorCallbacksPtr monCallbacks = NULL;
> virDomainXMLOptionPtr xmlopt = NULL;
> virDomainChrSourceDef monConfig;
So qemuMonitorOpenInternal would not report an error anymore, but
qemuMonitorIO would just crash on error or eof. This doesn't sound like
a change in the right direction, does it?
Sure, we could make the code tolerant to NULL callbacks, but I think
it's actually better to require the callbacks than covering possible
programmer's errors.
Drop this patch, please.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 18/36] qemu_process: Catch process free before process stop
On Sun, Dec 02, 2018 at 23:10:12 -0600, Chris Venteicher wrote:
> Catch execution paths where qemuProcessQmpFree is called before
> qemuProcessQmpStop then report error and force stop before proceeding.
>
> Also added public function header and debug message.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_process.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index e9b50745d3..1b6adf1b64 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8107,13 +8107,28 @@ static qemuMonitorCallbacks callbacks = {
> };
>
>
> +/**
> + * qemuProcessQmpFree:
> + * @proc: Stores Process and Connection State
Strange capitalization again.
> + *
> + * Free process data structure.
> + */
> void
> qemuProcessQmpFree(qemuProcessQmpPtr proc)
> {
> +VIR_DEBUG("proc=%p, proc->mon=%p", proc, (proc ? proc->mon : NULL));
> +
I don't think we need to log qemuProcessQmpFree(NULL) calls. I'd just
push this debug message after the check for proc == NULL.
> if (!proc)
> return;
>
> -qemuProcessQmpStop(proc);
> +/* This should never be non-NULL if we get here, but just in case... */
> +if (proc->mon || proc->pid) {
> +VIR_ERROR(_("Unexpected QEMU still active during process free"
> +" emulator: %s, pid: %lld, mon: %p"),
> + NULLSTR(proc->binary), (long long)proc->pid, proc->mon);
> +qemuProcessQmpStop(proc);
> +}
> +
Instead of doing this, I think it would make more sense to merge Stop
and Free functions into a single one.
> VIR_FREE(proc->binary);
> VIR_FREE(proc->libDir);
> VIR_FREE(proc->monpath);
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 17/36] qemu_process: Cleanup qemuProcessQmpStop function
On Sun, Dec 02, 2018 at 23:10:11 -0600, Chris Venteicher wrote:
> qemuProcessQmpStop is one of the 4 public functions used to create and
> manage a Qemu process for QMP command exchanges.
>
> Add comment header and debug message.
>
> Other minor code formatting cleanup.
This formatting "cleanup" is actually wrong.
> No change in functionality is intended.
And this statement can be dropped from the commit message too.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_process.c | 16 +---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index faf86dac5d..e9b50745d3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8390,10 +8390,20 @@ qemuProcessQmpStart(qemuProcessQmpPtr proc)
> goto cleanup;
> }
>
> -
> -void
> -qemuProcessQmpStop(qemuProcessQmpPtr proc)
> +/**
> + * qemuProcessStop:
> + * @proc: Stores Process and Connection State
> + *
> + * Stop Monitor Connection and QEMU Process
> + */
OK, But I Don't Understand Why All Words Need To Be Capitalized :-)
> +void qemuProcessQmpStop(qemuProcessQmpPtr proc)
This is against our coding style.
> {
> +if (!proc)
> +return;
This change belongs to "qemu_capabilities: Stop QEMU process before
freeing".
> +
> +VIR_DEBUG("Shutting down proc=%p emulator=%s mon=%p pid=%lld",
> + proc, proc->binary, proc->mon, (long long)proc->pid);
> +
> if (proc->mon) {
> virObjectUnlock(proc->mon);
> qemuMonitorClose(proc->mon);
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 16/36] qemu_process: Cleanup qemuProcessQmp alloc function
On Sun, Dec 02, 2018 at 23:10:10 -0600, Chris Venteicher wrote:
> qemuProcessQmpNew is one of the 4 public functions used to create and
> manage a qemu process for QMP command exchanges outside of domain
> operations.
>
> Add descriptive comment block, Debug statement and make source
> consistent with the cleanup / VIR_STEAL_PTR format used elsewhere.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_process.c | 32 ++--
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 31d41688fe..faf86dac5d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8124,6 +8124,18 @@ qemuProcessQmpFree(qemuProcessQmpPtr proc)
> }
>
>
> +/**
> + * qemuProcessQmpNew:
> + * @binary: Qemu binary
> + * @libDir: Directory for process and connection artifacts
> + * @runUid: UserId for Qemu Process
> + * @runGid: GroupId for Qemu Process
> + * @forceTCG: Force TCG mode if true
s/Qemu/QEMU/
> + *
> + * Allocate and initialize domain structure encapsulating
> + * QEMU Process state and monitor connection to QEMU
> + * for completing QMP Queries.
> + */
> qemuProcessQmpPtr
> qemuProcessQmpNew(const char *binary,
>const char *libDir,
> @@ -8131,25 +8143,33 @@ qemuProcessQmpNew(const char *binary,
>gid_t runGid,
>bool forceTCG)
> {
> +qemuProcessQmpPtr ret = NULL;
> qemuProcessQmpPtr proc = NULL;
>
> +VIR_DEBUG("exec=%s, libDir=%s, runUid=%u, runGid=%u, forceTCG=%d",
> + NULLSTR(binary), NULLSTR(libDir), runUid, runGid, forceTCG);
The code is not designed to work with binary or libDir being NULL. Thus
we don't need to guard them here with NULLSTR() macro.
> +
> if (VIR_ALLOC(proc) < 0)
> -goto error;
> +goto cleanup;
>
> if (VIR_STRDUP(proc->binary, binary) < 0 ||
> VIR_STRDUP(proc->libDir, libDir) < 0)
> -goto error;
> +goto cleanup;
>
>
> proc->runUid = runUid;
> proc->runGid = runGid;
> proc->forceTCG = forceTCG;
>
> -return proc;
> +VIR_STEAL_PTR(ret, proc);
>
> - error:
> -qemuProcessQmpFree(proc);
> -return NULL;
> + cleanup:
> +if (proc)
> +qemuProcessQmpFree(proc);
Just
qemuProcessQmpFree(proc);
all *Free functions are designed to handle NULL pointers.
> +
> +VIR_DEBUG("ret=%p", ret);
> +
> +return ret;
This is the appropriate usage of VIR_STEAL_PTR() macro.
With the obvious s/Qmp/QMP/
Reviewed-by: Jiri Denemark
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 15/36] qemu_process: Don't open monitor if process failed
On Sun, Dec 02, 2018 at 23:10:09 -0600, Chris Venteicher wrote:
> Gracefully handle case when proc activation failed prior to calling.
>
> Consistent with the existing code for qemuConnectMonitor (for domains)
> in qemu_process.c...
>
> - Handle qemMonitorOpen failure with INFO message and NULL ptr
> - Identify parameters passed to qemuMonitorOpen
>
> Monitor callbacks will be removed in future patch so we prep for passing
> NULL for the callback pointer.
>
> Set proc->mon to NULL then use VIR_STEAL_PTR if successful to be
> consistent with other functions.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_process.c | 29 +++--
> 1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 8906a22e3c..31d41688fe 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8262,25 +8262,50 @@ static int
> qemuProcessQmpConnectMonitor(qemuProcessQmpPtr proc)
> {
> int ret = -1;
> +qemuMonitorPtr mon = NULL;
> +unsigned long long timeout = 0;
> +bool retry = true;
> +bool enableJson = true;
> +virQEMUDriverPtr driver = NULL;
> +qemuMonitorCallbacksPtr monCallbacks =
> virDomainXMLOptionPtr xmlopt = NULL;
> virDomainChrSourceDef monConfig;
>
> VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld",
>proc, NULLSTR(proc->binary), (long long)proc->pid);
>
> +if (!proc || !proc->pid) {
> +ret = 0;
> +goto cleanup;
> +}
This is a dead code. We'd just see a segfault much earlier than at this
point if proc == NULL. For example, two lines above in the debug
message. But anyway, this function will just never be called if proc ==
NULL. Similarly proc->pid == 0 only when starting the process failed in
which case this function will not (and should not) be called.
> +
> +proc->mon = NULL;
> +
> monConfig.type = VIR_DOMAIN_CHR_TYPE_UNIX;
> monConfig.data.nix.path = proc->monpath;
> monConfig.data.nix.listen = false;
>
> +/* Create a NULL Domain object for qemuMonitor */
> if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) ||
> !(proc->vm = virDomainObjNew(xmlopt)))
> goto cleanup;
>
> proc->vm->pid = proc->pid;
>
> -if (!(proc->mon = qemuMonitorOpen(proc->vm, , true, true,
> - 0, , NULL)))
> +mon = qemuMonitorOpen(proc->vm,
> + ,
> + enableJson,
> + retry,
> + timeout,
> + monCallbacks,
> + driver);
This just hides the values we pass to qemuMonitorOpen. I agree that the
previous state is not ideal, but checking the prototype of
qemuMonitorOpen is easy. On the other hand one would have to scroll up
and down several times to see what values we pass here.
> +
> +if (!mon) {
> +VIR_INFO("Failed to connect monitor to emulator %s", proc->binary);
qemuMonitorOpen already reported an error if it failed for any reason so
logging any further message with info priority makes no sense.
> goto cleanup;
> +}
> +
> +VIR_STEAL_PTR(proc->mon, mon);
This is useful when we want to share success and error paths and just
free, cleanup, or do something similar with mon only in case of error.
But there's no such thing here and even none of the later patches
introduces anything like that.
>
> virObjectLock(proc->mon);
That said I don't see any reason for keeping this patch. Please, drop
it.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 14/36] qemu_process: Stop retaining Monitor config in qemuProcessQmp
On Sun, Dec 02, 2018 at 23:10:08 -0600, Chris Venteicher wrote: > The monitor config data is removed from the qemuProcessQmp struct. > > The monitor config data can be initialized immediately before call to > qemuMonitorOpen and does not need to be maintained after the call > because qemuMonitorOpen copies any strings it needs. Yeah, I was thinking we should do this when I saw "qemu_process: Collect monitor code in single function" patch. Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: Fix the default log output to 'journald' when running under systemd
Essentially, bring back the old behaviour as of commit eba36a38 which
was later changed by commit ae06048bf5d. Even though all the stderr
messages will eventually end up in the journal, we're not making use of
the fields journald provides.
https://bugzilla.redhat.com/show_bug.cgi?id=1592644
Signed-off-by: Erik Skultety
---
src/util/virlog.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/util/virlog.c b/src/util/virlog.c
index cb6901d9bf..3ee58c5db6 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -223,11 +223,17 @@ virLogSetDefaultOutputToFile(const char *filename, bool
privileged)
int
virLogSetDefaultOutput(const char *filename, bool godaemon, bool privileged)
{
-if (!godaemon)
+bool have_journald = access("/run/systemd/journal/socket", W_OK) >= 0;
+
+if (godaemon) {
+if (have_journald)
+return virLogSetDefaultOutputToJournald();
+} else {
+if (!isatty(STDIN_FILENO) && have_journald)
+return virLogSetDefaultOutputToJournald();
+
return virLogSetDefaultOutputToStderr();
-
-if (access("/run/systemd/journal/socket", W_OK) >= 0)
-return virLogSetDefaultOutputToJournald();
+}
return virLogSetDefaultOutputToFile(filename, privileged);
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 13/36] qemu_process: Setup paths within qemuProcessQmpInit
On Sun, Dec 02, 2018 at 23:10:07 -0600, Chris Venteicher wrote: > Move code for setting paths and prepping file system from > qemuProcessQmpNew to qemuProcessQmpInit. > > This keeps qemuProcessQmpNew limited to data structures > and path initialization is done in qemuProcessQmpInit. > > The patch is a non-functional, cut / paste change, > however goto is now "cleanup" rather than "error". > > Signed-off-by: Chris Venteicher > --- > src/qemu/qemu_process.c | 46 + > 1 file changed, 24 insertions(+), 22 deletions(-) With s/Qmp/QMP/g Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 12/36] qemu_process: Store libDir in qemuProcessQmp struct
On Sun, Dec 02, 2018 at 23:10:06 -0600, Chris Venteicher wrote: > Store libDir path in the qemuProcessQmp struct in anticipation of moving > path construction code into qemuProcessQmpInit function. > > Signed-off-by: Chris Venteicher > --- > src/qemu/qemu_process.c | 8 +--- > src/qemu/qemu_process.h | 1 + > 2 files changed, 6 insertions(+), 3 deletions(-) Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 11/36] qemu_process: Collect monitor code in single function
On Sun, Dec 02, 2018 at 23:10:05 -0600, Chris Venteicher wrote:
> qemuMonitor code lives in qemuProcessQmpConnectMonitor rather than in
> qemuProcessQmpNew and qemuProcessQmpLaunch.
>
> This is consistent with existing structure in qemu_process.c where
> qemuConnectMonitor function contains monitor code for domain process
> activation.
>
> Simple code moves in this patch. Improvements in later patch.
>
> Only intended functional change in this patch is we don't
> move (include) code to initiate process stop on failure to create monitor.
>
> As comments in qemuProcessQmpStart say... Client must always call
> qemuProcessStop and qemuProcessQmpFree, even in error cases.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_process.c | 50 -
> 1 file changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 938d328235..a688be7f2c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8163,10 +8163,6 @@ qemuProcessQmpNew(const char *binary,
>
> virPidFileForceCleanupPath(proc->pidfile);
>
> -proc->config.type = VIR_DOMAIN_CHR_TYPE_UNIX;
> -proc->config.data.nix.path = proc->monpath;
> -proc->config.data.nix.listen = false;
> -
> return proc;
>
> error:
> @@ -8198,7 +8194,6 @@ qemuProcessQmpInit(qemuProcessQmpPtr proc)
> static int
> qemuProcessQmpLaunch(qemuProcessQmpPtr proc)
> {
> -virDomainXMLOptionPtr xmlopt = NULL;
> const char *machine;
> int ret = -1;
>
> @@ -8250,6 +8245,28 @@ qemuProcessQmpLaunch(qemuProcessQmpPtr proc)
> goto cleanup;
> }
>
> +ret = 0;
> +
> + cleanup:
> +return ret;
> +}
> +
> +
> +/* Connect Monitor to QEMU Process
> + */
> +static int
> +qemuProcessQmpConnectMonitor(qemuProcessQmpPtr proc)
> +{
> +int ret = -1;
> +virDomainXMLOptionPtr xmlopt = NULL;
> +
> +VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld",
> + proc, NULLSTR(proc->binary), (long long)proc->pid);
> +
> +proc->config.type = VIR_DOMAIN_CHR_TYPE_UNIX;
> +proc->config.data.nix.path = proc->monpath;
> +proc->config.data.nix.listen = false;
> +
> if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) ||
> !(proc->vm = virDomainObjNew(xmlopt)))
> goto cleanup;
> @@ -8257,7 +8274,7 @@ qemuProcessQmpLaunch(qemuProcessQmpPtr proc)
> proc->vm->pid = proc->pid;
>
> if (!(proc->mon = qemuMonitorOpen(proc->vm, >config, true, true,
> - 0, , NULL)))
> + 0, , NULL)))
> goto cleanup;
>
> virObjectLock(proc->mon);
This hunk belongs to the patch which renamed cmd as proc.
> @@ -8265,27 +8282,8 @@ qemuProcessQmpLaunch(qemuProcessQmpPtr proc)
> ret = 0;
>
> cleanup:
> -if (!proc->mon)
> -qemuProcessQmpStop(proc);
> +VIR_DEBUG("ret=%i", ret);
> virObjectUnref(xmlopt);
> -
> -return ret;
> -}
> -
> -
> -/* Connect Monitor to QEMU Process
> - */
> -static int
> -qemuProcessQmpConnectMonitor(qemuProcessQmpPtr proc)
> -{
> -int ret = -1;
> -
> -VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld",
> - proc, NULLSTR(proc->binary), (long long)proc->pid);
> -
> -ret = 0;
> -
> -VIR_DEBUG("ret=%i", ret);
> return ret;
> }
Looks OK otherwise.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 10/36] qemu_process: Introduce qemuProcessQmpStart
On Sun, Dec 02, 2018 at 23:10:04 -0600, Chris Venteicher wrote:
> Move a step closer to the function structure used elsewhere in
> qemu_process where qemuProcessStart and qemuProcessStop are the exposed
> functions.
>
> qemuProcessQmpStart mirrors qemuProcessStart in calling sub functions to
> initialize, launch the process and connect the monitor to the QEMU
> process.
>
> static functions qemuProcessQmpInit, qemuProcessQmpLaunch and
> qemuProcessQmpConnectMonitor are introduced.
>
> qemuProcessQmpLaunch is just renamed from qemuProcessQmpRun and
> encapsulates all of the original code.
>
> qemuProcessQmpInit and qemuProcessQmpMonitor are nearly empty functions
> acting as placeholders for later patches where blocks of semi-complicated code
> are cut/pasted into these functions without modification
> (hopefully making review easier.)
>
> Looking forward, the patch series ultimately moves the code into this
> partitioning:
>
> - qemuProcessQmpInit
> Becomes the location of ~25 lines of code to create storage
> directory, in thread safe way, and initialize paths
> for monpath, monarg and pidfile.
>
> - qemuProcessQmpLaunch
> Becomes the location of ~48 lines of code used to create and run the
> QEMU command.
>
> - qemuProcessQmpConnectMonitor
> Becomes the final location of ~58 lines of code used to open and
> initialize the monitor connection between libvirt and qemu.
>
> Three smaller, purpose-identifying, functions of ~60 lines or less seem
> better than a single large process "start" function of > 130 lines.
>
> Being able to compare and contrast between the domain and non-domain
> versions of process code is useful too. There is some significant
> overlap between what the non-domain and domain functions do. There is
> also significant additional functionality in the domain functions that
> might be useful in the non-domain functions in the future.
> Possibly there could be sharing between non-domain and
> domain process code in the future but common code would have
> to be carefully extracted from the domain process code (not trivial.)
>
> Mirroring the domain process code has some value, but
> partitioning the code into logical chunks of < 60 lines
> is the main reason for the static functions.
Since the changes are all hidden inside qemuProcessQmpStart (formerly
called qemuProcessQmpRun) and nothing is calling the new internal
functions, all this refactoring could have been separated from this
series. But since it's already wired in the series, there's no reason to
separate it. Unless serious issues are found in which case it could be
easier to separate the refactoring (and deal with the result) than
fixing the issues. I don't expect such issues, though.
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 4 +-
> src/qemu/qemu_process.c | 94 +++-
> src/qemu/qemu_process.h | 2 +-
> 3 files changed, 95 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 997f8c19d5..ce60648897 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4265,7 +4265,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> runUid, runGid, false)))
> goto cleanup;
>
> -if (qemuProcessQmpRun(proc) < 0) {
> +if (qemuProcessQmpStart(proc) < 0) {
> if (proc->status != 0)
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to probe QEMU binary with QMP: %s"),
> @@ -4288,7 +4288,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> procTCG = qemuProcessQmpNew(qemuCaps->binary, libDir,
> runUid, runGid, true);
>
> -if (qemuProcessQmpRun(procTCG) < 0)
> +if (qemuProcessQmpStart(procTCG) < 0)
> goto cleanup;
>
> if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, procTCG->mon) < 0)
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 8ad685f3ea..938d328235 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8175,8 +8175,28 @@ qemuProcessQmpNew(const char *binary,
> }
>
>
> -int
> -qemuProcessQmpRun(qemuProcessQmpPtr proc)
> +/* Initialize configuration and paths prior to starting QEMU
> + */
> +static int
> +qemuProcessQmpInit(qemuProcessQmpPtr proc)
> +{
> +int ret = -1;
> +
> +VIR_DEBUG("Beginning VM startup process"
Too much copy, this function is not about VM startup process.
> + " proc=%p, emulator=%s",
We usually log function parameters separately in the first debug message
coming from a function.
> + proc, proc->binary);
> +
> +ret = 0;
> +
> +VIR_DEBUG("ret=%i", ret);
> +return ret;
> +}
> +
> +
> +/* Launch QEMU Process
> + */
> +static int
> +qemuProcessQmpLaunch(qemuProcessQmpPtr proc)
> {
> virDomainXMLOptionPtr xmlopt = NULL;
> const
Re: [libvirt] [PATCH] maint: update gnulib
Hi, This series was run against 'syntax-check' test by patchew.org, which failed, please find the details below: Subject: [libvirt] [PATCH] maint: update gnulib Message-id: 20190103140002.10947-1-ebl...@redhat.com Type: series === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch time bash -c './autogen.sh && make syntax-check' === TEST SCRIPT END === Updating bcb55ab053bc79561b55d0394490f4b64e0f2d01 >From https://github.com/patchew-project/libvirt 3af2c5d..c99e954 master -> master * [new tag] patchew/20190103140002.10947-1-ebl...@redhat.com -> patchew/20190103140002.10947-1-ebl...@redhat.com Switched to a new branch 'test' 59e1b6c maint: update gnulib === OUTPUT BEGIN === Updating submodules... Submodule 'gnulib' (https://git.savannah.gnu.org/git/gnulib.git/) registered for path '.gnulib' Submodule 'keycodemapdb' (https://gitlab.com/keycodemap/keycodemapdb.git) registered for path 'src/keycodemapdb' Cloning into '.gnulib'... Submodule path '.gnulib': checked out '7216e3e8d9f914d216fb10abf7dbeafa6570eb87' Cloning into 'src/keycodemapdb'... Submodule path 'src/keycodemapdb': checked out '16e5b0787687d8904dad2c026107409eb9bfcb95' Running bootstrap... ./bootstrap: Bootstrapping from checked-out libvirt sources... ./bootstrap: consider installing git-merge-changelog from gnulib ./bootstrap: getting gnulib files... running: libtoolize --install --copy libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, `build-aux'. libtoolize: copying file `build-aux/config.guess' libtoolize: copying file `build-aux/config.sub' libtoolize: copying file `build-aux/install-sh' libtoolize: copying file `build-aux/ltmain.sh' libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'. libtoolize: copying file `m4/libtool.m4' libtoolize: copying file `m4/ltoptions.m4' libtoolize: copying file `m4/ltsugar.m4' libtoolize: copying file `m4/ltversion.m4' libtoolize: copying file `m4/lt~obsolete.m4' ./bootstrap: .gnulib/gnulib-tool--no-changelog --aux-dir=build-aux --doc-base=doc --lib=libgnu --m4-base=m4/ --source-base=gnulib/lib/ --tests-base=gnulib/tests --local-dir=gnulib/local--lgpl=2 --with-tests --makefile-name=gnulib.mk --avoid=pt_chown --avoid=lock-tests --libtool --import ... Module list with included dependencies (indented): absolute-header accept accept-tests alloca alloca-opt alloca-opt-tests allocator areadlink areadlink-tests arpa_inet arpa_inet-tests assure autobuild base64 base64-tests binary-io binary-io-tests bind bind-tests bitrotate bitrotate-tests btowc btowc-tests builtin-expect byteswap byteswap-tests c-ctype c-ctype-tests c-strcase c-strcase-tests c-strcasestr c-strcasestr-tests calloc-posix canonicalize-lgpl canonicalize-lgpl-tests careadlinkat chown chown-tests clock-time cloexec cloexec-tests close close-tests configmake connect connect-tests count-leading-zeros count-leading-zeros-tests count-one-bits count-one-bits-tests ctype ctype-tests dirname-lgpl dosname double-slash-root dup dup-tests dup2 dup2-tests environ environ-tests errno errno-tests error execinfo exitfail extensions extern-inline fatal-signal fclose fclose-tests fcntl fcntl-h fcntl-h-tests fcntl-tests fd-hook fdatasync fdatasync-tests fdopen fdopen-tests fflush fflush-tests ffs ffs-tests ffsl ffsl-tests fgetc-tests filename flexmember float float-tests fnmatch fnmatch-h fnmatch-h-tests fnmatch-tests fpieee fpucw fpurge fpurge-tests fputc-tests fread-tests freading freading-tests fseek fseek-tests fseeko fseeko-tests fstat fstat-tests fsync fsync-tests ftell ftell-tests ftello ftello-tests ftruncate ftruncate-tests func func-tests fwrite-tests getaddrinfo getaddrinfo-tests getcwd-lgpl getcwd-lgpl-tests getdelim getdelim-tests getdtablesize getdtablesize-tests getgroups getgroups-tests gethostname gethostname-tests getline getline-tests getopt-posix getopt-posix-tests getpagesize getpass getpeername getpeername-tests getprogname getprogname-tests getsockname getsockname-tests getsockopt getsockopt-tests gettext-h gettimeofday gettimeofday-tests getugroups gitlog-to-changelog gnumakefile grantpt grantpt-tests hard-locale havelib hostent ignore-value ignore-value-tests include_next inet_ntop inet_ntop-tests inet_pton inet_pton-tests intprops intprops-tests inttypes inttypes-incomplete
Re: [libvirt] [PATCH v5 08/36] qemu_process: All ProcessQMP errors are fatal
On Sun, Dec 02, 2018 at 23:10:02 -0600, Chris Venteicher wrote:
> In the past capabilities could be determined in other ways if QMP
> messaging didn't succeed so a non-fatal error case was included in the
> capabilities and QMP Process code.
>
> For a while now, QMP capabilities failure has been a fatal case.
>
> This patch makes QMP process failures return as a fatal error in all
> cases consistent with 1) all failures actually being fatal in
> QMP capabilities code and 2) the QMP process code being made generic.
>
> The process changes impact the capabilities code because non-fatal
> return codes are no longer returned.
>
> The rest of the QMP associated capabilities code is updated to make all
> errors fatal for consistency.
>
> The process changes also force a change in QMP process stderr reporting
> because the previous triggers for stderr reporting are no longer passed
> through the function interfaces.
>
> As best I can tell the only case where QMP process stderr should be
> explicitly reported is when the QMP process exits with a non-zero
> status.
>
> Error reporting is moved to virQEMUCapsInitQMP and the QMP process
> stderr is reported only when the QMP process status code is non-zero
> (and is only reported for the primary QMP query not the secondary TCG
> specific QMP query.)
Looks like the patch is unnecessarily doing more things at once...
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 83
> src/qemu/qemu_process.c | 24 ---
> src/qemu/qemu_process.h | 1 +
> 3 files changed, 45 insertions(+), 63 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index bed9ca26a1..1efec85b57 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
...
> @@ -4234,25 +4231,47 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps
> ATTRIBUTE_UNUSED,
> }
>
>
> +#define MESSAGE_ID_CAPS_PROBE_FAILURE "8ae2f3fb-2dbe-498e-8fbd-012d40afa361"
> +
> +static void
> +virQEMUCapsLogProbeFailure(const char *binary)
> +{
> +virLogMetadata meta[] = {
> +{ .key = "MESSAGE_ID", .s = MESSAGE_ID_CAPS_PROBE_FAILURE, .iv = 0 },
> +{ .key = "LIBVIRT_QEMU_BINARY", .s = binary, .iv = 0 },
> +{ .key = NULL },
> +};
> +
> +virLogMessage(,
> + VIR_LOG_WARN,
> + __FILE__, __LINE__, __func__,
> + meta,
> + _("Failed to probe capabilities for %s: %s"),
> + binary, virGetLastErrorMessage());
> +}
> +
> +
I agree calling virQEMUCapsLogProbeFailure from inside
virQEMUCapsInitQMP makes a bit more sense then doing it in the caller,
but it should be done separately. The change has nothing to do with
the purpose of this patch.
> static int
> virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> const char *libDir,
> uid_t runUid,
> - gid_t runGid,
> - char **qmperr)
> + gid_t runGid)
This could be moved to the "qemu_process: Persist stderr in
qemuProcessQmp struct" patch. There should be no reason for splitting
a single logical change in two patches.
> {
> qemuProcessQmpPtr proc = NULL;
> qemuProcessQmpPtr procTCG = NULL;
> +char *qmperr = NULL;
> int ret = -1;
> -int rc;
>
> if (!(proc = qemuProcessQmpNew(qemuCaps->binary, libDir,
> - runUid, runGid, qmperr, false)))
> + runUid, runGid, , false)))
> goto cleanup;
>
> -if ((rc = qemuProcessQmpRun(proc)) != 0) {
> -if (rc == 1)
> -ret = 0;
> +if (qemuProcessQmpRun(proc) < 0) {
> +if (proc->status != 0)
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to probe QEMU binary with QMP: %s"),
> + qmperr ? qmperr : _("uknown error"));
[*]
> +
> goto cleanup;
> }
>
> @@ -4270,11 +4289,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> procTCG = qemuProcessQmpNew(qemuCaps->binary, libDir,
> runUid, runGid, NULL, true);
>
> -if ((rc = qemuProcessQmpRun(procTCG)) != 0) {
> -if (rc == 1)
> -ret = 0;
> +if (qemuProcessQmpRun(procTCG) < 0)
> goto cleanup;
This would return -1 without setting any error. The virReportError [*]
from the previous hunk would need to be repeated here. And doing so
would indicate this is a wrong place for reporting the error. The
qemuProcessQmpRun function returned -1 and thus it should have called
virReportError itself.
And as I suggested in my review for the previous version, there's no
need to have two almost identical copies of the same code in
virQEMUCapsInitQMP. Just separate the code into a new function which
will then be called twice in a new patch put before this
Re: [libvirt] [PATCH v5 09/36] qemu_process: Persist stderr in qemuProcessQmp struct
On Sun, Dec 02, 2018 at 23:10:03 -0600, Chris Venteicher wrote:
> A qemuProcessQmp struct tracks the entire lifespan of a single QEMU Process
> including storing error output when the process terminates or activation
> fails.
>
> Error output remains available until qemuProcessQmpFree is called.
>
> The qmperr variable is renamed stderr (captures stderr from process.)
>
> The stderr buffer no longer needs to be maintained outside of the
> qemuProcessQmp structure because the structure is used for a single QEMU
> process and the structures can be maintained as long as required
> to retrieve the process error info.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 8 +++-
> src/qemu/qemu_process.c | 9 +++--
> src/qemu/qemu_process.h | 3 +--
> 3 files changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 1efec85b57..997f8c19d5 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4259,18 +4259,17 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> {
> qemuProcessQmpPtr proc = NULL;
> qemuProcessQmpPtr procTCG = NULL;
> -char *qmperr = NULL;
> int ret = -1;
>
> if (!(proc = qemuProcessQmpNew(qemuCaps->binary, libDir,
> - runUid, runGid, , false)))
> + runUid, runGid, false)))
> goto cleanup;
>
> if (qemuProcessQmpRun(proc) < 0) {
> if (proc->status != 0)
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to probe QEMU binary with QMP: %s"),
> - qmperr ? qmperr : _("uknown error"));
> + proc->stderr ? proc->stderr : _("uknown error"));
>
> goto cleanup;
> }
This patch will obviously change a bit according to the comments to
previous patches.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 07/36] qemu_process: Use qemuProcessQmp struct for a single process
On Sun, Dec 02, 2018 at 23:10:01 -0600, Chris Venteicher wrote:
> In new process code, move from model where qemuProcessQmp struct can be
> used to activate a series of Qemu processes to model where one
> qemuProcessQmp struct is used for one and only one Qemu process.
>
> By allowing only one process activation per qemuProcessQmp struct, the
> struct can safely store process outputs like status and stderr, without
> being overwritten, until qemuProcessQmpFree is called.
>
> By doing this, process outputs like status and stderr can remain stored
> in the qemuProcessQmp struct without being overwritten by subsequent
> process activations.
>
> The forceTCG parameter (use / don't use KVM) will be passed when the
> qemuProcessQmp struct is initialized since the qemuProcessQmp struct
> won't be reused.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 20
> src/qemu/qemu_process.c | 10 ++
> src/qemu/qemu_process.h | 7 ---
> 3 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index a79329a134..bed9ca26a1 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
...
> @@ -4259,14 +4260,23 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> goto cleanup;
>
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
> +
I don't think there's any need for this empty line.
> +/* The second QEMU process probes for TCG capabilities
> + * in case the first process reported KVM as enabled
> + * (otherwise the first one already reported TCG capabilities). */
> +
> qemuProcessQmpStop(proc);
...
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index fa050a1a27..324151a7c3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
...
> @@ -8137,9 +8138,11 @@ qemuProcessQmpNew(const char *binary,
> if (VIR_STRDUP(proc->binary, binary) < 0)
> goto error;
>
> +
Drop this extra empty line.
> proc->runUid = runUid;
> proc->runGid = runGid;
> proc->qmperr = qmperr;
> +proc->forceTCG = forceTCG;
>
> /* the ".sock" sufix is important to avoid a possible clash with a qemu
> * domain called "capabilities"
With the empty lines removed and s/Qmp/QMP/g
Reviewed-by: Jiri Denemark
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/36] qemu_capabilities: Stop QEMU process before freeing
On Sun, Dec 02, 2018 at 23:10:00 -0600, Chris Venteicher wrote:
> virQEMUCapsInitQMP now stops QEMU process in all execution paths,
> before freeing the process structure.
>
> The qemuProcessQmpStop function can be called multiple times without
> problems... Won't attempt to stop processes and free resources multiple
> times.
>
> Follow the convention established in qemu_process of
> 1) alloc process structure
> 2) start process
> 3) use process
> 4) stop process
> 5) free process data structure
>
> The process data structure persists after the process activation fails
> or the process dies or is killed so stderr strings can be retrieved
> until the process data structure is freed.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 1 +
> src/qemu/qemu_process.c | 19 ---
> 2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index d903fbddf8..a79329a134 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4273,6 +4273,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> ret = 0;
>
> cleanup:
> +qemuProcessQmpStop(proc);
This would still crash libvirt if qemuProcessQmpNew() failed.
> qemuProcessQmpFree(proc);
> return ret;
> }
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 8465448a49..fa050a1a27 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8263,14 +8263,17 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc,
> void
> qemuProcessQmpStop(qemuProcessQmpPtr proc)
> {
Here you'd need to add
if (!proc)
return;
> -if (proc->mon)
> +if (proc->mon) {
> virObjectUnlock(proc->mon);
...
And obviously s/Qmp/QMP/g.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 05/36] qemu_process: Use consistent name for stop process function
On Sun, Dec 02, 2018 at 23:09:59 -0600, Chris Venteicher wrote: > s/qemuProcessQmpAbort/qemuProcessQmpStop/ applied to change function name > used to stop QEMU processes in process code moved from qemu_capabilities. > > No functionality change. > > The new name, qemuProcessQmpStop, is consistent with the existing > function qemuProcessStop used to stop Domain processes. > > Signed-off-by: Chris Venteicher > --- > src/qemu/qemu_capabilities.c | 2 +- > src/qemu/qemu_process.c | 6 +++--- > src/qemu/qemu_process.h | 2 +- > 3 files changed, 5 insertions(+), 5 deletions(-) After s/Qmp/QMP/g Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 04/36] qemu_process: Refer to proc not cmd in process code
On Sun, Dec 02, 2018 at 23:09:58 -0600, Chris Venteicher wrote: > s/cmd/proc/ in process code imported from qemu_capabilities. > > No functionality is changed. Just variable renaming. > > Process code imported from qemu_capabilities was oriented around > starting a process to issue a single QMP command. > > Future usecases (ex. baseline, compare) expect to use a single process > to issue multiple different QMP commands. > > This patch changes the variable naming from cmd to proc to put focus > on the process being maintained to issue commands. > > Signed-off-by: Chris Venteicher > --- > src/qemu/qemu_capabilities.c | 18 ++--- > src/qemu/qemu_process.c | 138 +-- > src/qemu/qemu_process.h | 4 +- > 3 files changed, 80 insertions(+), 80 deletions(-) ... > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 46aed4fc9c..5f4853e0c4 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c ... > @@ -8201,55 +8201,55 @@ qemuProcessQmpRun(qemuProcessQmpPtr cmd, ... > if (!(xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL, NULL, NULL)) || > -!(cmd->vm = virDomainObjNew(xmlopt))) > +!(proc->vm = virDomainObjNew(xmlopt))) > goto cleanup; > > -cmd->vm->pid = cmd->pid; > +proc->vm->pid = proc->pid; > > -if (!(cmd->mon = qemuMonitorOpen(cmd->vm, >config, true, true, > +if (!(proc->mon = qemuMonitorOpen(proc->vm, >config, true, true, > 0, , NULL))) You forgot to update the indentation of the line above. > goto ignore; > > -virObjectLock(cmd->mon); > +virObjectLock(proc->mon); > > ret = 0; > > cleanup: > -if (!cmd->mon) > -qemuProcessQmpAbort(cmd); > +if (!proc->mon) > +qemuProcessQmpAbort(proc); > virObjectUnref(xmlopt); > > return ret; ... After fixing that and doing s/Qmp/QMP/g Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 03/36] qemu_process: Limit qemuProcessQmpNew to const input strings
On Sun, Dec 02, 2018 at 23:09:57 -0600, Chris Venteicher wrote:
> Add the const qualifier on non modified strings
> (string only copied inside qemuProcessQmpNew)
> so that const strings can be used directly in calls to
> qemuProcessQmpNew in future patches.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_process.c | 2 +-
> src/qemu/qemu_process.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 35e0c6172a..46aed4fc9c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8123,7 +8123,7 @@ qemuProcessQmpFree(qemuProcessQmpPtr cmd)
>
>
> qemuProcessQmpPtr
> -qemuProcessQmpNew(char *binary,
> +qemuProcessQmpNew(const char *binary,
>const char *libDir,
>uid_t runUid,
>gid_t runGid,
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 67ed90ad4d..6d4fbda5fc 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -231,7 +231,7 @@ struct _qemuProcessQmp {
> virDomainObjPtr vm;
> };
>
> -qemuProcessQmpPtr qemuProcessQmpNew(char *binary,
> +qemuProcessQmpPtr qemuProcessQmpNew(const char *binary,
> const char *libDir,
> uid_t runUid,
> gid_t runGid,
After s/Qmp/QMP/g
Reviewed-by: Jiri Denemark
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 02/36] qemu_process: Use qemuProcessQmp prefix
On Sun, Dec 02, 2018 at 23:09:56 -0600, Chris Venteicher wrote: > s/virQEMUCapsInitQMPCommand/qemuProcessQmp/ Please, use qemuProcessQMP prefix as suggested in my previous review. There are several reasons for this: - QMP is not a word, it's the abbreviation for QEMU Monitor Protocol - we use QMP consistently in our code, we don't spell it as Qmp anywhere - Qmp is very ugly (might be subjective, though) With s/Qmp/QMP/g Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 01/36] qemu_process: Move process code from qemu_capabilities to qemu_process
On Sun, Dec 02, 2018 at 23:09:55 -0600, Chris Venteicher wrote: > Qemu process code in qemu_capabilities.c is moved to qemu_process.c in > order to make the code usable outside the original capabilities > usecases. > > The moved code activates and manages Qemu processes without > establishing a guest domain. > > ** This patch is a straight cut/paste move between files. ** I don't think there's any need for the stars here. > (Exception: I did change a function prototype in qemu_process.h to be one > parameter per line.) There's really no exception. There were no prototypes at all in the original code since the functions were used in the same file where they were defined. While moving the code, you naturally had to add prototypes. Please drop this part of the commit message. > Then, subsequent patches modify the process code > to make function prefixes and variable names match qemu_process, > and make the code usable for more than the capabilities usecase. > > Signed-off-by: Chris Venteicher Reviewed-by: Jiri Denemark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] maint: update gnulib
In particular, this incorporates Roman's patches to allow 'make syntax-check' to work on BSD with its exec argv limitations that previously failed when trying to grep the large number of files present in libvirt. Signed-off-by: Eric Blake --- Really just two changes since the new year copyright update, but posting for review to make sure this works on BSD boxes. * .gnulib 4652c7b...7216e3e (2): > maint.mk: Replace grep with $(GREP) > maint.mk: Split long argument lists .gnulib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gnulib b/.gnulib index 4652c7bafa..7216e3e8d9 16 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 4652c7bafa60332145f1e05a7de5f48e1bc56226 +Subproject commit 7216e3e8d9f914d216fb10abf7dbeafa6570eb87 -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Remove even more Author(s): lines from source files
On Wed, Jan 02, 2019 at 07:11:44PM +0100, Michal Privoznik wrote: > In 600462834f4ec1955a9a4 we've tried to remove Author(s): lines > from comments at the beginning of our source files. Well, in some > files while we removed the "Author" line we did not remove the > actual list of authors. > > Signed-off-by: Michal Privoznik Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
