[Qemu-devel] [PATCH v7 00/16] xen: xen-domid-restrict improvements

[Qemu-devel] [PATCH v7 00/16] xen: xen-domid-restrict improvements

[Ian Jackson]

This series provides necessary support for running qemu as a Xen
device model without power equivalent to root.  In particular, it
makes -xen-domid-restrict effective.

Compared to v7, it addresses review comments.  As a result, it has
grown a couple of new cleanup patches, including a few that I tacked
on the end which weren't strictly necessary but which I thought people
would appreciate.

  m    01/16 checkpatch: Add xendevicemodel_handle to the list of
  + r  02/16 AccelClass: Introduce accel_setup_post
     a 03/16 xen: link against xentoolcore
     a 04/16 xen: restrict: use xentoolcore_restrict_all
  * -  05/16 xen: defer call to xen_restrict until just before
     a 06/16 xen: destroy_hvm_domain: Move reason into a variable
     a 07/16 xen: move xc_interface compatibility fallback further
    r  08/16 xen: destroy_hvm_domain: Try xendevicemodel_shutdown
  +    09/16 os-posix: cleanup: Replace fprintfs with error_report
  m    10/16 os-posix: Provide new -runas <uid>:<gid> facility
     a 11/16 xen: Use newly added dmops for mapping VGA memory
       12/16 xen: Remove now-obsolete xen_xc_domain_add_to_physmap
     a 13/16 xen: Expect xenstore write to fail when restricted
  +    14/16 os-posix: cleanup: Replace fprintf with error_report in
  +    15/16 os-posix: cleanup: Replace perror with error_report
       16/16 configure: do_compiler: Dump some extra info under bash

 m = commit message (only) changed
 r = reviewed (by someone other than me)
 a = acked
 - = reviewed-by/acked-by dropped due to changes
 + = new patch
 * = amended patch

I'd particularly like to draw attention to what is now patch 12/16 and
16/16 which have been posted before but remained unreviewed.

Thanks for your attention.

Ian.

[PATCH v7 00/16] xen: xen-domid-restrict improvements

[Ian Jackson]

This series provides necessary support for running qemu as a Xen
device model without power equivalent to root.  In particular, it
makes -xen-domid-restrict effective.

Compared to v7, it addresses review comments.  As a result, it has
grown a couple of new cleanup patches, including a few that I tacked
on the end which weren't strictly necessary but which I thought people
would appreciate.

  m    01/16 checkpatch: Add xendevicemodel_handle to the list of
  + r  02/16 AccelClass: Introduce accel_setup_post
     a 03/16 xen: link against xentoolcore
     a 04/16 xen: restrict: use xentoolcore_restrict_all
  * -  05/16 xen: defer call to xen_restrict until just before
     a 06/16 xen: destroy_hvm_domain: Move reason into a variable
     a 07/16 xen: move xc_interface compatibility fallback further
    r  08/16 xen: destroy_hvm_domain: Try xendevicemodel_shutdown
  +    09/16 os-posix: cleanup: Replace fprintfs with error_report
  m    10/16 os-posix: Provide new -runas <uid>:<gid> facility
     a 11/16 xen: Use newly added dmops for mapping VGA memory
       12/16 xen: Remove now-obsolete xen_xc_domain_add_to_physmap
     a 13/16 xen: Expect xenstore write to fail when restricted
  +    14/16 os-posix: cleanup: Replace fprintf with error_report in
  +    15/16 os-posix: cleanup: Replace perror with error_report
       16/16 configure: do_compiler: Dump some extra info under bash

 m = commit message (only) changed
 r = reviewed (by someone other than me)
 a = acked
 - = reviewed-by/acked-by dropped due to changes
 + = new patch
 * = amended patch

I'd particularly like to draw attention to what is now patch 12/16 and
16/16 which have been posted before but remained unreviewed.

Thanks for your attention.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 01/16] checkpatch: Add xendevicemodel_handle to the list of types

[Ian Jackson]

This avoids checkpatch misparsing (as statements) long function
definitions or declarations, which sometimes start with constructs
like this:

  static inline int xendevicemodel_relocate_memory(
      xendevicemodel_handle *dmod, domid_t domid, ...

The type xendevicemodel_handle does not conform to Qemu CODING_STYLE,
which would suggest CamelCase.  However, it is a type defined by the
Xen Project in xen.git.  It would be possible to introduce a typedef
to allow the qemu code to refer to it by a differently-spelled name,
but that would obfuscate more than it would clarify.

CC: Eric Blake <eblake@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v7: Added comment to commit message about why we are not renaming
    this type to CamelCase.
v6.1: New patch
---
 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d52207a..5b8735d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -266,6 +266,7 @@ our @typeList = (
 	qr{target_(?:u)?long},
 	qr{hwaddr},
 	qr{xml${Ident}},
+	qr{xendevicemodel_handle},
 );
 
 # This can be modified by sub possible.  Since it can be empty, be careful
-- 
2.1.4

[PATCH 01/16] checkpatch: Add xendevicemodel_handle to the list of types

[Ian Jackson]

This avoids checkpatch misparsing (as statements) long function
definitions or declarations, which sometimes start with constructs
like this:

  static inline int xendevicemodel_relocate_memory(
      xendevicemodel_handle *dmod, domid_t domid, ...

The type xendevicemodel_handle does not conform to Qemu CODING_STYLE,
which would suggest CamelCase.  However, it is a type defined by the
Xen Project in xen.git.  It would be possible to introduce a typedef
to allow the qemu code to refer to it by a differently-spelled name,
but that would obfuscate more than it would clarify.

CC: Eric Blake <eblake@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v7: Added comment to commit message about why we are not renaming
    this type to CamelCase.
v6.1: New patch
---
 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d52207a..5b8735d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -266,6 +266,7 @@ our @typeList = (
 	qr{target_(?:u)?long},
 	qr{hwaddr},
 	qr{xml${Ident}},
+	qr{xendevicemodel_handle},
 );
 
 # This can be modified by sub possible.  Since it can be empty, be careful
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 02/16] AccelClass: Introduce accel_setup_post

[Ian Jackson]

This is called just before os_setup_post.  Currently none of the
accelerators provide this hook, but the Xen one is going to provide
one in a moment.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v7: New patch in this version of the series
---
 accel/accel.c          | 9 +++++++++
 include/sysemu/accel.h | 3 +++
 vl.c                   | 1 +
 3 files changed, 13 insertions(+)

diff --git a/accel/accel.c b/accel/accel.c
index 93e2434..9cfab11 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -126,6 +126,15 @@ void accel_register_compat_props(AccelState *accel)
     register_compat_props_array(class->global_props);
 }
 
+void accel_setup_post(MachineState *ms)
+{
+    AccelState *accel = ms->accelerator;
+    AccelClass *acc = ACCEL_GET_CLASS(accel);
+    if (acc->setup_post) {
+        acc->setup_post(ms, accel);
+    }
+}
+
 static void register_accel_types(void)
 {
     type_register_static(&accel_type);
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 5a632ce..637358f 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -40,6 +40,7 @@ typedef struct AccelClass {
     const char *name;
     int (*available)(void);
     int (*init_machine)(MachineState *ms);
+    void (*setup_post)(MachineState *ms, AccelState *accel);
     bool *allowed;
     /*
      * Array of global properties that would be applied when specific
@@ -68,5 +69,7 @@ extern unsigned long tcg_tb_size;
 void configure_accelerator(MachineState *ms);
 /* Register accelerator specific global properties */
 void accel_register_compat_props(AccelState *accel);
+/* Called just before os_setup_post (ie just before drop OS privs) */
+void accel_setup_post(MachineState *ms);
 
 #endif
diff --git a/vl.c b/vl.c
index fce1fd1..36c5bd4 100644
--- a/vl.c
+++ b/vl.c
@@ -4729,6 +4729,7 @@ int main(int argc, char **argv, char **envp)
         vm_start();
     }
 
+    accel_setup_post(current_machine);
     os_setup_post();
 
     main_loop();
-- 
2.1.4

[PATCH 02/16] AccelClass: Introduce accel_setup_post

[Ian Jackson]

This is called just before os_setup_post.  Currently none of the
accelerators provide this hook, but the Xen one is going to provide
one in a moment.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v7: New patch in this version of the series
---
 accel/accel.c          | 9 +++++++++
 include/sysemu/accel.h | 3 +++
 vl.c                   | 1 +
 3 files changed, 13 insertions(+)

diff --git a/accel/accel.c b/accel/accel.c
index 93e2434..9cfab11 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -126,6 +126,15 @@ void accel_register_compat_props(AccelState *accel)
     register_compat_props_array(class->global_props);
 }
 
+void accel_setup_post(MachineState *ms)
+{
+    AccelState *accel = ms->accelerator;
+    AccelClass *acc = ACCEL_GET_CLASS(accel);
+    if (acc->setup_post) {
+        acc->setup_post(ms, accel);
+    }
+}
+
 static void register_accel_types(void)
 {
     type_register_static(&accel_type);
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 5a632ce..637358f 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -40,6 +40,7 @@ typedef struct AccelClass {
     const char *name;
     int (*available)(void);
     int (*init_machine)(MachineState *ms);
+    void (*setup_post)(MachineState *ms, AccelState *accel);
     bool *allowed;
     /*
      * Array of global properties that would be applied when specific
@@ -68,5 +69,7 @@ extern unsigned long tcg_tb_size;
 void configure_accelerator(MachineState *ms);
 /* Register accelerator specific global properties */
 void accel_register_compat_props(AccelState *accel);
+/* Called just before os_setup_post (ie just before drop OS privs) */
+void accel_setup_post(MachineState *ms);
 
 #endif
diff --git a/vl.c b/vl.c
index fce1fd1..36c5bd4 100644
--- a/vl.c
+++ b/vl.c
@@ -4729,6 +4729,7 @@ int main(int argc, char **argv, char **envp)
         vm_start();
     }
 
+    accel_setup_post(current_machine);
     os_setup_post();
 
     main_loop();
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 03/16] xen: link against xentoolcore

[Ian Jackson]

From: Anthony PERARD <anthony.perard@citrix.com>

Xen libraries in 4.10 include a new xentoolcore library.  This
contains the xentoolcore_restrict_all function which we are about to
want to use.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
v5: More truthful commit message.
---
 configure | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 0a19b03..f580255 100755
--- a/configure
+++ b/configure
@@ -2188,7 +2188,7 @@ if test "$xen" != "no" ; then
       $($pkg_config --modversion xencontrol | sed 's/\./ /g') )"
     xen=yes
     xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
-    xen_pc="$xen_pc xenevtchn xendevicemodel"
+    xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"
     QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)"
     libs_softmmu="$($pkg_config --libs $xen_pc) $libs_softmmu"
     LDFLAGS="$($pkg_config --libs $xen_pc) $LDFLAGS"
@@ -2220,18 +2220,20 @@ EOF
         cat > $TMPC <<EOF &&
 #undef XC_WANT_COMPAT_MAP_FOREIGN_API
 #include <xenforeignmemory.h>
+#include <xentoolcore.h>
 int main(void) {
   xenforeignmemory_handle *xfmem;
 
   xfmem = xenforeignmemory_open(0, 0);
   xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
+  xentoolcore_restrict_all(0);
 
   return 0;
 }
 EOF
-        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
+        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs -lxentoolcore"
       then
-      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
+      xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
       xen_ctrl_version=41000
       xen=yes
     elif
-- 
2.1.4

[PATCH 03/16] xen: link against xentoolcore

[Ian Jackson]

From: Anthony PERARD <anthony.perard@citrix.com>

Xen libraries in 4.10 include a new xentoolcore library.  This
contains the xentoolcore_restrict_all function which we are about to
want to use.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
v5: More truthful commit message.
---
 configure | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 0a19b03..f580255 100755
--- a/configure
+++ b/configure
@@ -2188,7 +2188,7 @@ if test "$xen" != "no" ; then
       $($pkg_config --modversion xencontrol | sed 's/\./ /g') )"
     xen=yes
     xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
-    xen_pc="$xen_pc xenevtchn xendevicemodel"
+    xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"
     QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)"
     libs_softmmu="$($pkg_config --libs $xen_pc) $libs_softmmu"
     LDFLAGS="$($pkg_config --libs $xen_pc) $LDFLAGS"
@@ -2220,18 +2220,20 @@ EOF
         cat > $TMPC <<EOF &&
 #undef XC_WANT_COMPAT_MAP_FOREIGN_API
 #include <xenforeignmemory.h>
+#include <xentoolcore.h>
 int main(void) {
   xenforeignmemory_handle *xfmem;
 
   xfmem = xenforeignmemory_open(0, 0);
   xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
+  xentoolcore_restrict_all(0);
 
   return 0;
 }
 EOF
-        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
+        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs -lxentoolcore"
       then
-      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
+      xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
       xen_ctrl_version=41000
       xen=yes
     elif
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 04/16] xen: restrict: use xentoolcore_restrict_all

[Ian Jackson]

And insist that it works.

Drop individual use of xendevicemodel_restrict and
xenforeignmemory_restrict.  These are not actually effective in this
version of qemu, because qemu has a large number of fds open onto
various Xen control devices.

The restriction arrangements are still not right, because the
restriction needs to be done very late - after qemu has opened all of
its control fds.

xentoolcore_restrict_all and xentoolcore.h are available in Xen 4.10
and later, only.  Provide a compatibility stub.  And drop the
compatibility stubs for the old functions.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
v2: Modify the compatibility code, too.
    Bump this patch ahead of "defer call to xen_restrict until running"
    Retain call to xentoolcore_restrict_all
---
 include/hw/xen/xen_common.h | 46 +++++++++++----------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 64a978e..1766bb9 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -91,6 +91,16 @@ static inline void *xenforeignmemory_map2(xenforeignmemory_handle *h,
     return xenforeignmemory_map(h, dom, prot, pages, arr, err);
 }
 
+static inline int xentoolcore_restrict_all(domid_t domid)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
+
+#include <xentoolcore.h>
+
 #endif
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
@@ -218,20 +228,6 @@ static inline int xendevicemodel_set_mem_type(
     return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
 }
 
-static inline int xendevicemodel_restrict(
-    xendevicemodel_handle *dmod, domid_t domid)
-{
-    errno = ENOTTY;
-    return -1;
-}
-
-static inline int xenforeignmemory_restrict(
-    xenforeignmemory_handle *fmem, domid_t domid)
-{
-    errno = ENOTTY;
-    return -1;
-}
-
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
 
 #undef XC_WANT_COMPAT_DEVICEMODEL_API
@@ -290,28 +286,8 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn,
 static inline int xen_restrict(domid_t domid)
 {
     int rc;
-
-    /* Attempt to restrict devicemodel operations */
-    rc = xendevicemodel_restrict(xen_dmod, domid);
+    rc = xentoolcore_restrict_all(domid);
     trace_xen_domid_restrict(rc ? errno : 0);
-
-    if (rc < 0) {
-        /*
-         * If errno is ENOTTY then restriction is not implemented so
-         * there's no point in trying to restrict other types of
-         * operation, but it should not be treated as a failure.
-         */
-        if (errno == ENOTTY) {
-            return 0;
-        }
-
-        return rc;
-    }
-
-    /* Restrict foreignmemory operations */
-    rc = xenforeignmemory_restrict(xen_fmem, domid);
-    trace_xen_domid_restrict(rc ? errno : 0);
-
     return rc;
 }
 
-- 
2.1.4

[PATCH 04/16] xen: restrict: use xentoolcore_restrict_all

[Ian Jackson]

And insist that it works.

Drop individual use of xendevicemodel_restrict and
xenforeignmemory_restrict.  These are not actually effective in this
version of qemu, because qemu has a large number of fds open onto
various Xen control devices.

The restriction arrangements are still not right, because the
restriction needs to be done very late - after qemu has opened all of
its control fds.

xentoolcore_restrict_all and xentoolcore.h are available in Xen 4.10
and later, only.  Provide a compatibility stub.  And drop the
compatibility stubs for the old functions.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
v2: Modify the compatibility code, too.
    Bump this patch ahead of "defer call to xen_restrict until running"
    Retain call to xentoolcore_restrict_all
---
 include/hw/xen/xen_common.h | 46 +++++++++++----------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 64a978e..1766bb9 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -91,6 +91,16 @@ static inline void *xenforeignmemory_map2(xenforeignmemory_handle *h,
     return xenforeignmemory_map(h, dom, prot, pages, arr, err);
 }
 
+static inline int xentoolcore_restrict_all(domid_t domid)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
+
+#include <xentoolcore.h>
+
 #endif
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
@@ -218,20 +228,6 @@ static inline int xendevicemodel_set_mem_type(
     return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
 }
 
-static inline int xendevicemodel_restrict(
-    xendevicemodel_handle *dmod, domid_t domid)
-{
-    errno = ENOTTY;
-    return -1;
-}
-
-static inline int xenforeignmemory_restrict(
-    xenforeignmemory_handle *fmem, domid_t domid)
-{
-    errno = ENOTTY;
-    return -1;
-}
-
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
 
 #undef XC_WANT_COMPAT_DEVICEMODEL_API
@@ -290,28 +286,8 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn,
 static inline int xen_restrict(domid_t domid)
 {
     int rc;
-
-    /* Attempt to restrict devicemodel operations */
-    rc = xendevicemodel_restrict(xen_dmod, domid);
+    rc = xentoolcore_restrict_all(domid);
     trace_xen_domid_restrict(rc ? errno : 0);
-
-    if (rc < 0) {
-        /*
-         * If errno is ENOTTY then restriction is not implemented so
-         * there's no point in trying to restrict other types of
-         * operation, but it should not be treated as a failure.
-         */
-        if (errno == ENOTTY) {
-            return 0;
-        }
-
-        return rc;
-    }
-
-    /* Restrict foreignmemory operations */
-    rc = xenforeignmemory_restrict(xen_fmem, domid);
-    trace_xen_domid_restrict(rc ? errno : 0);
-
     return rc;
 }
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post

[Ian Jackson]

We need to restrict *all* the control fds that qemu opens.  Looking in
/proc/PID/fd shows there are many; their allocation seems scattered
throughout Xen support code in qemu.

We must postpone the restrict call until roughly the same time as qemu
changes its uid, chroots (if applicable), and so on.

There doesn't seem to be an appropriate hook already.  The RunState
change hook fires at different times depending on exactly what mode
qemu is operating in.

And it appears that no-one but the Xen code wants a hook at this phase
of execution.  So, introduce a bare call to a new function
xen_setup_post, just before os_setup_post.  Also provide the
appropriate stub for when Xen compilation is disabled.

We do the restriction before rather than after os_setup_post, because
xen_restrict may need to open /dev/null, and os_setup_post might have
called chroot.

Currently this does not work with migration, because when running as
the Xen device model qemu needs to signal to the toolstack that it is
ready.  It currently does this using xenstore, and for incoming
migration (but not for ordinary startup) that happens after
os_setup_post.

It is correct that this happens late: we want the incoming migration
stream to be processed by a restricted qemu.  The fix for this will be
to do the startup notification a different way, without using
xenstore.  (QMP is probably a reasonable choice.)

So for now this restriction feature cannot be used in conjunction with
migration.  (Note that this is not a regression in this patch, because
previously the -xen-restrict-domid call was, in fact, simply
ineffective!)  We will revisit this in the Xen 4.11 release cycle.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
CC: Richard Henderson <rth@twiddle.net> (maintainer:X86)
CC: Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
CC: Michael S. Tsirkin <mst@redhat.com> (supporter:PC)
---
v7: Use new AccelClass setup_post hook, rather than ad-hoc call
    in vl.c.
v5: Discuss problems with migration startup notification
    in the commit message.
v3: Do xen_setup_post just before, not just after, os_setup_post,
    to improve interaction with chroot.  Thanks to Ross Lagerwall.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/i386/xen/xen-hvm.c |  8 --------
 hw/xen/xen-common.c   | 14 ++++++++++++++
 stubs/xen-hvm.c       |  1 +
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index f24b7d4..9c3b6b3 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1254,14 +1254,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
         goto err;
     }
 
-    if (xen_domid_restrict) {
-        rc = xen_restrict(xen_domid);
-        if (rc < 0) {
-            error_report("failed to restrict: error %d", errno);
-            goto err;
-        }
-    }
-
     xen_create_ioreq_server(xen_domid, &state->ioservid);
 
     state->exit.notify = xen_exit_notifier;
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 83099dd..454777c 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -117,6 +117,19 @@ static void xen_change_state_handler(void *opaque, int running,
     }
 }
 
+static void xen_setup_post(MachineState *ms, AccelState *accel)
+{
+    int rc;
+
+    if (xen_domid_restrict) {
+        rc = xen_restrict(xen_domid);
+        if (rc < 0) {
+            perror("xen: failed to restrict");
+            exit(1);
+        }
+    }
+}
+
 static int xen_init(MachineState *ms)
 {
     xen_xc = xc_interface_open(0, 0, 0);
@@ -165,6 +178,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
     AccelClass *ac = ACCEL_CLASS(oc);
     ac->name = "Xen";
     ac->init_machine = xen_init;
+    ac->setup_post = xen_setup_post;
     ac->allowed = &xen_allowed;
     ac->global_props = xen_compat_props;
 }
diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
index 0067bcc..7787ea2 100644
--- a/stubs/xen-hvm.c
+++ b/stubs/xen-hvm.c
@@ -13,6 +13,7 @@
 #include "hw/xen/xen.h"
 #include "exec/memory.h"
 #include "qapi/qapi-commands-misc.h"
+#include "sysemu/sysemu.h"
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
 {
-- 
2.1.4

[PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post

[Ian Jackson]

We need to restrict *all* the control fds that qemu opens.  Looking in
/proc/PID/fd shows there are many; their allocation seems scattered
throughout Xen support code in qemu.

We must postpone the restrict call until roughly the same time as qemu
changes its uid, chroots (if applicable), and so on.

There doesn't seem to be an appropriate hook already.  The RunState
change hook fires at different times depending on exactly what mode
qemu is operating in.

And it appears that no-one but the Xen code wants a hook at this phase
of execution.  So, introduce a bare call to a new function
xen_setup_post, just before os_setup_post.  Also provide the
appropriate stub for when Xen compilation is disabled.

We do the restriction before rather than after os_setup_post, because
xen_restrict may need to open /dev/null, and os_setup_post might have
called chroot.

Currently this does not work with migration, because when running as
the Xen device model qemu needs to signal to the toolstack that it is
ready.  It currently does this using xenstore, and for incoming
migration (but not for ordinary startup) that happens after
os_setup_post.

It is correct that this happens late: we want the incoming migration
stream to be processed by a restricted qemu.  The fix for this will be
to do the startup notification a different way, without using
xenstore.  (QMP is probably a reasonable choice.)

So for now this restriction feature cannot be used in conjunction with
migration.  (Note that this is not a regression in this patch, because
previously the -xen-restrict-domid call was, in fact, simply
ineffective!)  We will revisit this in the Xen 4.11 release cycle.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
CC: Richard Henderson <rth@twiddle.net> (maintainer:X86)
CC: Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
CC: Michael S. Tsirkin <mst@redhat.com> (supporter:PC)
---
v7: Use new AccelClass setup_post hook, rather than ad-hoc call
    in vl.c.
v5: Discuss problems with migration startup notification
    in the commit message.
v3: Do xen_setup_post just before, not just after, os_setup_post,
    to improve interaction with chroot.  Thanks to Ross Lagerwall.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/i386/xen/xen-hvm.c |  8 --------
 hw/xen/xen-common.c   | 14 ++++++++++++++
 stubs/xen-hvm.c       |  1 +
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index f24b7d4..9c3b6b3 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1254,14 +1254,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
         goto err;
     }
 
-    if (xen_domid_restrict) {
-        rc = xen_restrict(xen_domid);
-        if (rc < 0) {
-            error_report("failed to restrict: error %d", errno);
-            goto err;
-        }
-    }
-
     xen_create_ioreq_server(xen_domid, &state->ioservid);
 
     state->exit.notify = xen_exit_notifier;
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 83099dd..454777c 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -117,6 +117,19 @@ static void xen_change_state_handler(void *opaque, int running,
     }
 }
 
+static void xen_setup_post(MachineState *ms, AccelState *accel)
+{
+    int rc;
+
+    if (xen_domid_restrict) {
+        rc = xen_restrict(xen_domid);
+        if (rc < 0) {
+            perror("xen: failed to restrict");
+            exit(1);
+        }
+    }
+}
+
 static int xen_init(MachineState *ms)
 {
     xen_xc = xc_interface_open(0, 0, 0);
@@ -165,6 +178,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
     AccelClass *ac = ACCEL_CLASS(oc);
     ac->name = "Xen";
     ac->init_machine = xen_init;
+    ac->setup_post = xen_setup_post;
     ac->allowed = &xen_allowed;
     ac->global_props = xen_compat_props;
 }
diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
index 0067bcc..7787ea2 100644
--- a/stubs/xen-hvm.c
+++ b/stubs/xen-hvm.c
@@ -13,6 +13,7 @@
 #include "hw/xen/xen.h"
 #include "exec/memory.h"
 #include "qapi/qapi-commands-misc.h"
+#include "sysemu/sysemu.h"
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
 {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 06/16] xen: destroy_hvm_domain: Move reason into a variable

[Ian Jackson]

We are going to want to reuse this.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
 hw/i386/xen/xen-hvm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 9c3b6b3..3590d99 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1387,12 +1387,13 @@ void destroy_hvm_domain(bool reboot)
     xc_interface *xc_handle;
     int sts;
 
+    unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
+
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
         fprintf(stderr, "Cannot acquire xenctrl handle\n");
     } else {
-        sts = xc_domain_shutdown(xc_handle, xen_domid,
-                                 reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff);
+        sts = xc_domain_shutdown(xc_handle, xen_domid, reason);
         if (sts != 0) {
             fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
                     "sts %d, %s\n", reboot ? "reboot" : "poweroff",
-- 
2.1.4

[PATCH 06/16] xen: destroy_hvm_domain: Move reason into a variable

[Ian Jackson]

We are going to want to reuse this.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
 hw/i386/xen/xen-hvm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 9c3b6b3..3590d99 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1387,12 +1387,13 @@ void destroy_hvm_domain(bool reboot)
     xc_interface *xc_handle;
     int sts;
 
+    unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
+
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
         fprintf(stderr, "Cannot acquire xenctrl handle\n");
     } else {
-        sts = xc_domain_shutdown(xc_handle, xen_domid,
-                                 reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff);
+        sts = xc_domain_shutdown(xc_handle, xen_domid, reason);
         if (sts != 0) {
             fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
                     "sts %d, %s\n", reboot ? "reboot" : "poweroff",
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 07/16] xen: move xc_interface compatibility fallback further up the file

[Ian Jackson]

We are going to want to use the dummy xendevicemodel_handle type in
new stub functions in the CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
section.  So we need to provide that definition, or (as applicable)
include the appropriate header, earlier in the file.

(Ideally the newer compatibility layers would be at the bottom of the
file, so that they can naturally benefit from the compatibility layers
for earlier version.  But that's rather too much for this series.)

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
v2: New patch in v2 of the series
---
 include/hw/xen/xen_common.h | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 1766bb9..60c4ebb 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -78,6 +78,17 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
 
 extern xenforeignmemory_handle *xen_fmem;
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
+
+typedef xc_interface xendevicemodel_handle;
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
+
+#undef XC_WANT_COMPAT_DEVICEMODEL_API
+#include <xendevicemodel.h>
+
+#endif
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
 
 #define XEN_COMPAT_PHYSMAP
@@ -105,8 +116,6 @@ static inline int xentoolcore_restrict_all(domid_t domid)
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
 
-typedef xc_interface xendevicemodel_handle;
-
 static inline xendevicemodel_handle *xendevicemodel_open(
     struct xentoollog_logger *logger, unsigned int open_flags)
 {
@@ -228,11 +237,6 @@ static inline int xendevicemodel_set_mem_type(
     return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
 }
 
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
-
-#undef XC_WANT_COMPAT_DEVICEMODEL_API
-#include <xendevicemodel.h>
-
 #endif
 
 extern xendevicemodel_handle *xen_dmod;
-- 
2.1.4

[PATCH 07/16] xen: move xc_interface compatibility fallback further up the file

[Ian Jackson]

We are going to want to use the dummy xendevicemodel_handle type in
new stub functions in the CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
section.  So we need to provide that definition, or (as applicable)
include the appropriate header, earlier in the file.

(Ideally the newer compatibility layers would be at the bottom of the
file, so that they can naturally benefit from the compatibility layers
for earlier version.  But that's rather too much for this series.)

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
v2: New patch in v2 of the series
---
 include/hw/xen/xen_common.h | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 1766bb9..60c4ebb 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -78,6 +78,17 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
 
 extern xenforeignmemory_handle *xen_fmem;
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
+
+typedef xc_interface xendevicemodel_handle;
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
+
+#undef XC_WANT_COMPAT_DEVICEMODEL_API
+#include <xendevicemodel.h>
+
+#endif
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
 
 #define XEN_COMPAT_PHYSMAP
@@ -105,8 +116,6 @@ static inline int xentoolcore_restrict_all(domid_t domid)
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
 
-typedef xc_interface xendevicemodel_handle;
-
 static inline xendevicemodel_handle *xendevicemodel_open(
     struct xentoollog_logger *logger, unsigned int open_flags)
 {
@@ -228,11 +237,6 @@ static inline int xendevicemodel_set_mem_type(
     return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
 }
 
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
-
-#undef XC_WANT_COMPAT_DEVICEMODEL_API
-#include <xendevicemodel.h>
-
 #endif
 
 extern xendevicemodel_handle *xen_dmod;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 08/16] xen: destroy_hvm_domain: Try xendevicemodel_shutdown

[Ian Jackson]

xc_interface_open etc. is not going to work if we have dropped
privilege, but xendevicemodel_shutdown will if everything is new
enough.

xendevicemodel_shutdown is only availabe in Xen 4.10 and later, so
provide a stub for earlier versions.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
v6.1: Fix { } style issue
v6: Do not print message about harmless condition in ENOTTY case.
v2: Add compatibility stub for Xen < 4.10.
    Fix coding style.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/i386/xen/xen-hvm.c       | 12 ++++++++++++
 include/hw/xen/xen_common.h |  7 +++++++
 2 files changed, 19 insertions(+)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 3590d99..fb727bc 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1386,9 +1386,21 @@ void destroy_hvm_domain(bool reboot)
 {
     xc_interface *xc_handle;
     int sts;
+    int rc;
 
     unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
 
+    if (xen_dmod) {
+        rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
+        if (!rc) {
+            return;
+        }
+        if (errno != ENOTTY /* old Xen */) {
+            perror("xendevicemodel_shutdown failed");
+        }
+        /* well, try the old thing then */
+    }
+
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
         fprintf(stderr, "Cannot acquire xenctrl handle\n");
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 60c4ebb..4bd30a3 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -108,6 +108,13 @@ static inline int xentoolcore_restrict_all(domid_t domid)
     return -1;
 }
 
+static inline int xendevicemodel_shutdown(xendevicemodel_handle *dmod,
+                                          domid_t domid, unsigned int reason)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
 
 #include <xentoolcore.h>
-- 
2.1.4

[PATCH 08/16] xen: destroy_hvm_domain: Try xendevicemodel_shutdown

[Ian Jackson]

xc_interface_open etc. is not going to work if we have dropped
privilege, but xendevicemodel_shutdown will if everything is new
enough.

xendevicemodel_shutdown is only availabe in Xen 4.10 and later, so
provide a stub for earlier versions.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
v6.1: Fix { } style issue
v6: Do not print message about harmless condition in ENOTTY case.
v2: Add compatibility stub for Xen < 4.10.
    Fix coding style.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/i386/xen/xen-hvm.c       | 12 ++++++++++++
 include/hw/xen/xen_common.h |  7 +++++++
 2 files changed, 19 insertions(+)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 3590d99..fb727bc 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1386,9 +1386,21 @@ void destroy_hvm_domain(bool reboot)
 {
     xc_interface *xc_handle;
     int sts;
+    int rc;
 
     unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
 
+    if (xen_dmod) {
+        rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
+        if (!rc) {
+            return;
+        }
+        if (errno != ENOTTY /* old Xen */) {
+            perror("xendevicemodel_shutdown failed");
+        }
+        /* well, try the old thing then */
+    }
+
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
         fprintf(stderr, "Cannot acquire xenctrl handle\n");
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 60c4ebb..4bd30a3 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -108,6 +108,13 @@ static inline int xentoolcore_restrict_all(domid_t domid)
     return -1;
 }
 
+static inline int xendevicemodel_shutdown(xendevicemodel_handle *dmod,
+                                          domid_t domid, unsigned int reason)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
 
 #include <xentoolcore.h>
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 09/16] os-posix: cleanup: Replace fprintfs with error_report in change_process_uid

[Ian Jackson]

I'm going to be editing this function and it makes sense to clean up
this style problem in advance.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
CC: Michael Tokarev <mjt@tls.msk.ru>
---
 os-posix.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index b9c2343..560db95 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -167,20 +167,20 @@ static void change_process_uid(void)
 {
     if (user_pwd) {
         if (setgid(user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
+            error_report("Failed to setgid(%d)", user_pwd->pw_gid);
             exit(1);
         }
         if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
-                    user_pwd->pw_name, user_pwd->pw_gid);
+            error_report("Failed to initgroups(\"%s\", %d)",
+                         user_pwd->pw_name, user_pwd->pw_gid);
             exit(1);
         }
         if (setuid(user_pwd->pw_uid) < 0) {
-            fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
+            error_report("Failed to setuid(%d)", user_pwd->pw_uid);
             exit(1);
         }
         if (setuid(0) != -1) {
-            fprintf(stderr, "Dropping privileges failed\n");
+            error_report("Dropping privileges failed");
             exit(1);
         }
     }
-- 
2.1.4

[PATCH 09/16] os-posix: cleanup: Replace fprintfs with error_report in change_process_uid

[Ian Jackson]

I'm going to be editing this function and it makes sense to clean up
this style problem in advance.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
CC: Michael Tokarev <mjt@tls.msk.ru>
---
 os-posix.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index b9c2343..560db95 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -167,20 +167,20 @@ static void change_process_uid(void)
 {
     if (user_pwd) {
         if (setgid(user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
+            error_report("Failed to setgid(%d)", user_pwd->pw_gid);
             exit(1);
         }
         if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
-                    user_pwd->pw_name, user_pwd->pw_gid);
+            error_report("Failed to initgroups(\"%s\", %d)",
+                         user_pwd->pw_name, user_pwd->pw_gid);
             exit(1);
         }
         if (setuid(user_pwd->pw_uid) < 0) {
-            fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
+            error_report("Failed to setuid(%d)", user_pwd->pw_uid);
             exit(1);
         }
         if (setuid(0) != -1) {
-            fprintf(stderr, "Dropping privileges failed\n");
+            error_report("Dropping privileges failed");
             exit(1);
         }
     }
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 10/16] os-posix: Provide new -runas <uid>:<gid> facility

[Ian Jackson]

This allows the caller to specify a uid and gid to use, even if there
is no corresponding password entry.  This will be useful in certain
Xen configurations.

We don't support just -runas <uid> because: (i) deprivileging without
calling setgroups would be ineffective (ii) given only a uid we don't
know what gid we ought to use (since uids may eppear in multiple
passwd file entries with different gids).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
CC: Michael Tokarev <mjt@tls.msk.ru>
---
v7: Be much more explicit about legal combinations of user_{pwd,uid,gid}.
    Rely more on qemu_stroul being sane, dropping pointless
     pre-assignments of errno and lv.
    Retain optarg in error message about -runas argument.
    Rebase over introduction of error_report in change_process_uid.
v6.1: Fix constness of qemu_strtoul end pointer parameter.
v6: Use qemu_strtoul for the first strtoul.
    Use error_report rather than fprintf to print usage error message.
    Fix an error message which still referred to . rather than :
v5: Use : rather than . to separate uid from gid
v4: Changed to reuse option -runas
v3: Error messages fixed.  Thanks to Peter Maydell and Ross Lagerwall.
---
 os-posix.c      | 77 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 qemu-options.hx |  3 ++-
 2 files changed, 67 insertions(+), 13 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index 560db95..0f59566 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -41,7 +41,14 @@
 #include <sys/prctl.h>
 #endif
 
-static struct passwd *user_pwd;
+/*
+ * Must set all three of these at once.
+ * Legal combinations are              unset   by name   by uid
+ */
+static struct passwd *user_pwd;    /*   NULL   non-NULL   NULL   */
+static uid_t user_uid = (uid_t)-1; /*   -1      -1        >=0    */
+static gid_t user_gid = (gid_t)-1; /*   -1      -1        >=0    */
+
 static const char *chroot_dir;
 static int daemonize;
 static int daemon_pipe;
@@ -127,6 +134,33 @@ void os_set_proc_name(const char *s)
 #endif
 }
 
+
+static bool os_parse_runas_uid_gid(const char *optarg)
+{
+    unsigned long lv;
+    const char *ep;
+    uid_t got_uid;
+    gid_t got_gid;
+    int rc;
+
+    rc = qemu_strtoul(optarg, &ep, 0, &lv);
+    got_uid = lv; /* overflow here is ID in C99 */
+    if (rc || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) {
+        return false;
+    }
+
+    rc = qemu_strtoul(ep + 1, 0, 0, &lv);
+    got_gid = lv; /* overflow here is ID in C99 */
+    if (rc || got_gid != lv || got_gid == (gid_t)-1) {
+        return false;
+    }
+
+    user_pwd = NULL;
+    user_uid = got_uid;
+    user_gid = got_gid;
+    return true;
+}
+
 /*
  * Parse OS specific command line options.
  * return 0 if option handled, -1 otherwise
@@ -144,8 +178,13 @@ void os_parse_cmd_args(int index, const char *optarg)
 #endif
     case QEMU_OPTION_runas:
         user_pwd = getpwnam(optarg);
-        if (!user_pwd) {
-            fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
+        if (user_pwd) {
+            user_uid = -1;
+            user_gid = -1;
+        } else if (!os_parse_runas_uid_gid(optarg)) {
+            error_report("User \"%s\" doesn't exist"
+                         " (and is not <uid>:<gid>)",
+                         optarg);
             exit(1);
         }
         break;
@@ -165,18 +204,32 @@ void os_parse_cmd_args(int index, const char *optarg)
 
 static void change_process_uid(void)
 {
-    if (user_pwd) {
-        if (setgid(user_pwd->pw_gid) < 0) {
-            error_report("Failed to setgid(%d)", user_pwd->pw_gid);
+    assert((user_uid == (uid_t)-1) || user_pwd == NULL);
+    assert((user_uid == (uid_t)-1) ==
+           (user_gid == (gid_t)-1));
+
+    if (user_pwd || user_uid != (uid_t)-1) {
+        gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
+        uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
+        if (setgid(intended_gid) < 0) {
+            error_report("Failed to setgid(%d)", intended_gid);
             exit(1);
         }
-        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
-            error_report("Failed to initgroups(\"%s\", %d)",
-                         user_pwd->pw_name, user_pwd->pw_gid);
-            exit(1);
+        if (user_pwd) {
+            if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
+                error_report("Failed to initgroups(\"%s\", %d)",
+                        user_pwd->pw_name, user_pwd->pw_gid);
+                exit(1);
+            }
+        } else {
+            if (setgroups(1, &user_gid) < 0) {
+                error_report("Failed to setgroups(1, [%d])",
+                        user_gid);
+                exit(1);
+            }
         }
-        if (setuid(user_pwd->pw_uid) < 0) {
-            error_report("Failed to setuid(%d)", user_pwd->pw_uid);
+        if (setuid(intended_uid) < 0) {
+            error_report("Failed to setuid(%d)", intended_uid);
             exit(1);
         }
         if (setuid(0) != -1) {
diff --git a/qemu-options.hx b/qemu-options.hx
index ca4e412..5fbf966 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3765,7 +3765,8 @@ ETEXI
 
 #ifndef _WIN32
 DEF("runas", HAS_ARG, QEMU_OPTION_runas, \
-    "-runas user     change to user id user just before starting the VM\n",
+    "-runas user     change to user id user just before starting the VM\n" \
+    "                user can be numeric uid:gid instead\n",
     QEMU_ARCH_ALL)
 #endif
 STEXI
-- 
2.1.4

[PATCH 10/16] os-posix: Provide new -runas <uid>:<gid> facility

[Ian Jackson]

This allows the caller to specify a uid and gid to use, even if there
is no corresponding password entry.  This will be useful in certain
Xen configurations.

We don't support just -runas <uid> because: (i) deprivileging without
calling setgroups would be ineffective (ii) given only a uid we don't
know what gid we ought to use (since uids may eppear in multiple
passwd file entries with different gids).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
CC: Michael Tokarev <mjt@tls.msk.ru>
---
v7: Be much more explicit about legal combinations of user_{pwd,uid,gid}.
    Rely more on qemu_stroul being sane, dropping pointless
     pre-assignments of errno and lv.
    Retain optarg in error message about -runas argument.
    Rebase over introduction of error_report in change_process_uid.
v6.1: Fix constness of qemu_strtoul end pointer parameter.
v6: Use qemu_strtoul for the first strtoul.
    Use error_report rather than fprintf to print usage error message.
    Fix an error message which still referred to . rather than :
v5: Use : rather than . to separate uid from gid
v4: Changed to reuse option -runas
v3: Error messages fixed.  Thanks to Peter Maydell and Ross Lagerwall.
---
 os-posix.c      | 77 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 qemu-options.hx |  3 ++-
 2 files changed, 67 insertions(+), 13 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index 560db95..0f59566 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -41,7 +41,14 @@
 #include <sys/prctl.h>
 #endif
 
-static struct passwd *user_pwd;
+/*
+ * Must set all three of these at once.
+ * Legal combinations are              unset   by name   by uid
+ */
+static struct passwd *user_pwd;    /*   NULL   non-NULL   NULL   */
+static uid_t user_uid = (uid_t)-1; /*   -1      -1        >=0    */
+static gid_t user_gid = (gid_t)-1; /*   -1      -1        >=0    */
+
 static const char *chroot_dir;
 static int daemonize;
 static int daemon_pipe;
@@ -127,6 +134,33 @@ void os_set_proc_name(const char *s)
 #endif
 }
 
+
+static bool os_parse_runas_uid_gid(const char *optarg)
+{
+    unsigned long lv;
+    const char *ep;
+    uid_t got_uid;
+    gid_t got_gid;
+    int rc;
+
+    rc = qemu_strtoul(optarg, &ep, 0, &lv);
+    got_uid = lv; /* overflow here is ID in C99 */
+    if (rc || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) {
+        return false;
+    }
+
+    rc = qemu_strtoul(ep + 1, 0, 0, &lv);
+    got_gid = lv; /* overflow here is ID in C99 */
+    if (rc || got_gid != lv || got_gid == (gid_t)-1) {
+        return false;
+    }
+
+    user_pwd = NULL;
+    user_uid = got_uid;
+    user_gid = got_gid;
+    return true;
+}
+
 /*
  * Parse OS specific command line options.
  * return 0 if option handled, -1 otherwise
@@ -144,8 +178,13 @@ void os_parse_cmd_args(int index, const char *optarg)
 #endif
     case QEMU_OPTION_runas:
         user_pwd = getpwnam(optarg);
-        if (!user_pwd) {
-            fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
+        if (user_pwd) {
+            user_uid = -1;
+            user_gid = -1;
+        } else if (!os_parse_runas_uid_gid(optarg)) {
+            error_report("User \"%s\" doesn't exist"
+                         " (and is not <uid>:<gid>)",
+                         optarg);
             exit(1);
         }
         break;
@@ -165,18 +204,32 @@ void os_parse_cmd_args(int index, const char *optarg)
 
 static void change_process_uid(void)
 {
-    if (user_pwd) {
-        if (setgid(user_pwd->pw_gid) < 0) {
-            error_report("Failed to setgid(%d)", user_pwd->pw_gid);
+    assert((user_uid == (uid_t)-1) || user_pwd == NULL);
+    assert((user_uid == (uid_t)-1) ==
+           (user_gid == (gid_t)-1));
+
+    if (user_pwd || user_uid != (uid_t)-1) {
+        gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
+        uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
+        if (setgid(intended_gid) < 0) {
+            error_report("Failed to setgid(%d)", intended_gid);
             exit(1);
         }
-        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
-            error_report("Failed to initgroups(\"%s\", %d)",
-                         user_pwd->pw_name, user_pwd->pw_gid);
-            exit(1);
+        if (user_pwd) {
+            if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
+                error_report("Failed to initgroups(\"%s\", %d)",
+                        user_pwd->pw_name, user_pwd->pw_gid);
+                exit(1);
+            }
+        } else {
+            if (setgroups(1, &user_gid) < 0) {
+                error_report("Failed to setgroups(1, [%d])",
+                        user_gid);
+                exit(1);
+            }
         }
-        if (setuid(user_pwd->pw_uid) < 0) {
-            error_report("Failed to setuid(%d)", user_pwd->pw_uid);
+        if (setuid(intended_uid) < 0) {
+            error_report("Failed to setuid(%d)", intended_uid);
             exit(1);
         }
         if (setuid(0) != -1) {
diff --git a/qemu-options.hx b/qemu-options.hx
index ca4e412..5fbf966 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3765,7 +3765,8 @@ ETEXI
 
 #ifndef _WIN32
 DEF("runas", HAS_ARG, QEMU_OPTION_runas, \
-    "-runas user     change to user id user just before starting the VM\n",
+    "-runas user     change to user id user just before starting the VM\n" \
+    "                user can be numeric uid:gid instead\n",
     QEMU_ARCH_ALL)
 #endif
 STEXI
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 11/16] xen: Use newly added dmops for mapping VGA memory

[Ian Jackson]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Xen unstable (to be in 4.11) has two new dmops, relocate_memory and
pin_memory_cacheattr. Use these to set up the VGA memory, replacing the
previous calls to libxc. This allows the VGA console to work properly
when QEMU is running restricted (-xen-domid-restrict).

Wrapper functions are provided to allow QEMU to work with older versions
of Xen.

Tweak the error handling while making this change:
* Report pin_memory_cacheattr errors.
* Report errors even when DEBUG_HVM is not set. This is useful for
trying to understand why VGA is not working, since otherwise it just
fails silently.
* Fix the return values when an error occurs. The functions now
consistently return -1 and set errno.

CC: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
v6.1: Fix printf formats to match types in error_report messages
      Fix spurious \n in error_report messages
      Fix { } style issue
v6: New patch in this version of the series
---
 configure                   | 19 +++++++++++++++++
 hw/i386/xen/xen-hvm.c       | 50 ++++++++++++++++++++++++---------------------
 include/hw/xen/xen_common.h | 32 +++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 23 deletions(-)

diff --git a/configure b/configure
index f580255..d5435ff 100755
--- a/configure
+++ b/configure
@@ -2218,6 +2218,25 @@ EOF
     # Xen unstable
     elif
         cat > $TMPC <<EOF &&
+#undef XC_WANT_COMPAT_DEVICEMODEL_API
+#define __XEN_TOOLS__
+#include <xendevicemodel.h>
+int main(void) {
+  xendevicemodel_handle *xd;
+
+  xd = xendevicemodel_open(0, 0);
+  xendevicemodel_pin_memory_cacheattr(xd, 0, 0, 0, 0);
+
+  return 0;
+}
+EOF
+        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs -lxentoolcore"
+      then
+      xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
+      xen_ctrl_version=41100
+      xen=yes
+    elif
+        cat > $TMPC <<EOF &&
 #undef XC_WANT_COMPAT_MAP_FOREIGN_API
 #include <xenforeignmemory.h>
 #include <xentoolcore.h>
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index fb727bc..caa563b 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -347,7 +347,7 @@ static int xen_add_to_physmap(XenIOState *state,
                               MemoryRegion *mr,
                               hwaddr offset_within_region)
 {
-    unsigned long i = 0;
+    unsigned long nr_pages;
     int rc = 0;
     XenPhysmap *physmap = NULL;
     hwaddr pfn, start_gpfn;
@@ -396,22 +396,26 @@ go_physmap:
 
     pfn = phys_offset >> TARGET_PAGE_BITS;
     start_gpfn = start_addr >> TARGET_PAGE_BITS;
-    for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
-        unsigned long idx = pfn + i;
-        xen_pfn_t gpfn = start_gpfn + i;
-
-        rc = xen_xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
-        if (rc) {
-            DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
-                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
-            return -rc;
-        }
+    nr_pages = size >> TARGET_PAGE_BITS;
+    rc = xendevicemodel_relocate_memory(xen_dmod, xen_domid, nr_pages, pfn,
+                                        start_gpfn);
+    if (rc) {
+        int saved_errno = errno;
+
+        error_report("relocate_memory %lu pages from GFN %"HWADDR_PRIx
+                     " to GFN %"HWADDR_PRIx" failed: %s",
+                     nr_pages, pfn, start_gpfn, strerror(saved_errno));
+        errno = saved_errno;
+        return -1;
     }
 
-    xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
+    rc = xendevicemodel_pin_memory_cacheattr(xen_dmod, xen_domid,
                                    start_addr >> TARGET_PAGE_BITS,
                                    (start_addr + size - 1) >> TARGET_PAGE_BITS,
                                    XEN_DOMCTL_MEM_CACHEATTR_WB);
+    if (rc) {
+        error_report("pin_memory_cacheattr failed: %s", strerror(errno));
+    }
     return xen_save_physmap(state, physmap);
 }
 
@@ -419,7 +423,6 @@ static int xen_remove_from_physmap(XenIOState *state,
                                    hwaddr start_addr,
                                    ram_addr_t size)
 {
-    unsigned long i = 0;
     int rc = 0;
     XenPhysmap *physmap = NULL;
     hwaddr phys_offset = 0;
@@ -438,16 +441,17 @@ static int xen_remove_from_physmap(XenIOState *state,
     size >>= TARGET_PAGE_BITS;
     start_addr >>= TARGET_PAGE_BITS;
     phys_offset >>= TARGET_PAGE_BITS;
-    for (i = 0; i < size; i++) {
-        xen_pfn_t idx = start_addr + i;
-        xen_pfn_t gpfn = phys_offset + i;
-
-        rc = xen_xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
-        if (rc) {
-            fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
-                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
-            return -rc;
-        }
+    rc = xendevicemodel_relocate_memory(xen_dmod, xen_domid, size, start_addr,
+                                        phys_offset);
+    if (rc) {
+        int saved_errno = errno;
+
+        error_report("relocate_memory "RAM_ADDR_FMT" pages"
+                     " from GFN %"HWADDR_PRIx
+                     " to GFN %"HWADDR_PRIx" failed: %s",
+                     size, start_addr, phys_offset, strerror(saved_errno));
+        errno = saved_errno;
+        return -1;
     }
 
     QLIST_REMOVE(physmap, list);
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 4bd30a3..2eed6fc 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -89,6 +89,38 @@ typedef xc_interface xendevicemodel_handle;
 
 #endif
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100
+
+static inline int xendevicemodel_relocate_memory(
+    xendevicemodel_handle *dmod, domid_t domid, uint32_t size, uint64_t src_gfn,
+    uint64_t dst_gfn)
+{
+    uint32_t i;
+    int rc;
+
+    for (i = 0; i < size; i++) {
+        unsigned long idx = src_gfn + i;
+        xen_pfn_t gpfn = dst_gfn + i;
+
+        rc = xc_domain_add_to_physmap(xen_xc, domid, XENMAPSPACE_gmfn, idx,
+                                      gpfn);
+        if (rc) {
+            return rc;
+        }
+    }
+
+    return 0;
+}
+
+static inline int xendevicemodel_pin_memory_cacheattr(
+    xendevicemodel_handle *dmod, domid_t domid, uint64_t start, uint64_t end,
+    uint32_t type)
+{
+    return xc_domain_pin_memory_cacheattr(xen_xc, domid, start, end, type);
+}
+
+#endif /* CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100 */
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
 
 #define XEN_COMPAT_PHYSMAP
-- 
2.1.4

[PATCH 11/16] xen: Use newly added dmops for mapping VGA memory

[Ian Jackson]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Xen unstable (to be in 4.11) has two new dmops, relocate_memory and
pin_memory_cacheattr. Use these to set up the VGA memory, replacing the
previous calls to libxc. This allows the VGA console to work properly
when QEMU is running restricted (-xen-domid-restrict).

Wrapper functions are provided to allow QEMU to work with older versions
of Xen.

Tweak the error handling while making this change:
* Report pin_memory_cacheattr errors.
* Report errors even when DEBUG_HVM is not set. This is useful for
trying to understand why VGA is not working, since otherwise it just
fails silently.
* Fix the return values when an error occurs. The functions now
consistently return -1 and set errno.

CC: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
v6.1: Fix printf formats to match types in error_report messages
      Fix spurious \n in error_report messages
      Fix { } style issue
v6: New patch in this version of the series
---
 configure                   | 19 +++++++++++++++++
 hw/i386/xen/xen-hvm.c       | 50 ++++++++++++++++++++++++---------------------
 include/hw/xen/xen_common.h | 32 +++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 23 deletions(-)

diff --git a/configure b/configure
index f580255..d5435ff 100755
--- a/configure
+++ b/configure
@@ -2218,6 +2218,25 @@ EOF
     # Xen unstable
     elif
         cat > $TMPC <<EOF &&
+#undef XC_WANT_COMPAT_DEVICEMODEL_API
+#define __XEN_TOOLS__
+#include <xendevicemodel.h>
+int main(void) {
+  xendevicemodel_handle *xd;
+
+  xd = xendevicemodel_open(0, 0);
+  xendevicemodel_pin_memory_cacheattr(xd, 0, 0, 0, 0);
+
+  return 0;
+}
+EOF
+        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs -lxentoolcore"
+      then
+      xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
+      xen_ctrl_version=41100
+      xen=yes
+    elif
+        cat > $TMPC <<EOF &&
 #undef XC_WANT_COMPAT_MAP_FOREIGN_API
 #include <xenforeignmemory.h>
 #include <xentoolcore.h>
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index fb727bc..caa563b 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -347,7 +347,7 @@ static int xen_add_to_physmap(XenIOState *state,
                               MemoryRegion *mr,
                               hwaddr offset_within_region)
 {
-    unsigned long i = 0;
+    unsigned long nr_pages;
     int rc = 0;
     XenPhysmap *physmap = NULL;
     hwaddr pfn, start_gpfn;
@@ -396,22 +396,26 @@ go_physmap:
 
     pfn = phys_offset >> TARGET_PAGE_BITS;
     start_gpfn = start_addr >> TARGET_PAGE_BITS;
-    for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
-        unsigned long idx = pfn + i;
-        xen_pfn_t gpfn = start_gpfn + i;
-
-        rc = xen_xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
-        if (rc) {
-            DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
-                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
-            return -rc;
-        }
+    nr_pages = size >> TARGET_PAGE_BITS;
+    rc = xendevicemodel_relocate_memory(xen_dmod, xen_domid, nr_pages, pfn,
+                                        start_gpfn);
+    if (rc) {
+        int saved_errno = errno;
+
+        error_report("relocate_memory %lu pages from GFN %"HWADDR_PRIx
+                     " to GFN %"HWADDR_PRIx" failed: %s",
+                     nr_pages, pfn, start_gpfn, strerror(saved_errno));
+        errno = saved_errno;
+        return -1;
     }
 
-    xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
+    rc = xendevicemodel_pin_memory_cacheattr(xen_dmod, xen_domid,
                                    start_addr >> TARGET_PAGE_BITS,
                                    (start_addr + size - 1) >> TARGET_PAGE_BITS,
                                    XEN_DOMCTL_MEM_CACHEATTR_WB);
+    if (rc) {
+        error_report("pin_memory_cacheattr failed: %s", strerror(errno));
+    }
     return xen_save_physmap(state, physmap);
 }
 
@@ -419,7 +423,6 @@ static int xen_remove_from_physmap(XenIOState *state,
                                    hwaddr start_addr,
                                    ram_addr_t size)
 {
-    unsigned long i = 0;
     int rc = 0;
     XenPhysmap *physmap = NULL;
     hwaddr phys_offset = 0;
@@ -438,16 +441,17 @@ static int xen_remove_from_physmap(XenIOState *state,
     size >>= TARGET_PAGE_BITS;
     start_addr >>= TARGET_PAGE_BITS;
     phys_offset >>= TARGET_PAGE_BITS;
-    for (i = 0; i < size; i++) {
-        xen_pfn_t idx = start_addr + i;
-        xen_pfn_t gpfn = phys_offset + i;
-
-        rc = xen_xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
-        if (rc) {
-            fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
-                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
-            return -rc;
-        }
+    rc = xendevicemodel_relocate_memory(xen_dmod, xen_domid, size, start_addr,
+                                        phys_offset);
+    if (rc) {
+        int saved_errno = errno;
+
+        error_report("relocate_memory "RAM_ADDR_FMT" pages"
+                     " from GFN %"HWADDR_PRIx
+                     " to GFN %"HWADDR_PRIx" failed: %s",
+                     size, start_addr, phys_offset, strerror(saved_errno));
+        errno = saved_errno;
+        return -1;
     }
 
     QLIST_REMOVE(physmap, list);
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 4bd30a3..2eed6fc 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -89,6 +89,38 @@ typedef xc_interface xendevicemodel_handle;
 
 #endif
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100
+
+static inline int xendevicemodel_relocate_memory(
+    xendevicemodel_handle *dmod, domid_t domid, uint32_t size, uint64_t src_gfn,
+    uint64_t dst_gfn)
+{
+    uint32_t i;
+    int rc;
+
+    for (i = 0; i < size; i++) {
+        unsigned long idx = src_gfn + i;
+        xen_pfn_t gpfn = dst_gfn + i;
+
+        rc = xc_domain_add_to_physmap(xen_xc, domid, XENMAPSPACE_gmfn, idx,
+                                      gpfn);
+        if (rc) {
+            return rc;
+        }
+    }
+
+    return 0;
+}
+
+static inline int xendevicemodel_pin_memory_cacheattr(
+    xendevicemodel_handle *dmod, domid_t domid, uint64_t start, uint64_t end,
+    uint32_t type)
+{
+    return xc_domain_pin_memory_cacheattr(xen_xc, domid, start, end, type);
+}
+
+#endif /* CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100 */
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
 
 #define XEN_COMPAT_PHYSMAP
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 12/16] xen: Remove now-obsolete xen_xc_domain_add_to_physmap

[Ian Jackson]

The last user was just removed; remove this function, accordingly.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/hw/xen/xen_common.h | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 2eed6fc..5f1402b 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -645,28 +645,6 @@ static inline int xen_set_ioreq_server_state(domid_t dom,
 
 #endif
 
-#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40600
-static inline int xen_xc_domain_add_to_physmap(xc_interface *xch, uint32_t domid,
-                                               unsigned int space,
-                                               unsigned long idx,
-                                               xen_pfn_t gpfn)
-{
-    return xc_domain_add_to_physmap(xch, domid, space, idx, gpfn);
-}
-#else
-static inline int xen_xc_domain_add_to_physmap(xc_interface *xch, uint32_t domid,
-                                               unsigned int space,
-                                               unsigned long idx,
-                                               xen_pfn_t gpfn)
-{
-    /* In Xen 4.6 rc is -1 and errno contains the error value. */
-    int rc = xc_domain_add_to_physmap(xch, domid, space, idx, gpfn);
-    if (rc == -1)
-        return errno;
-    return rc;
-}
-#endif
-
 #ifdef CONFIG_XEN_PV_DOMAIN_BUILD
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40700
 static inline int xen_domain_create(xc_interface *xc, uint32_t ssidref,
-- 
2.1.4

[PATCH 12/16] xen: Remove now-obsolete xen_xc_domain_add_to_physmap

[Ian Jackson]

The last user was just removed; remove this function, accordingly.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/hw/xen/xen_common.h | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 2eed6fc..5f1402b 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -645,28 +645,6 @@ static inline int xen_set_ioreq_server_state(domid_t dom,
 
 #endif
 
-#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40600
-static inline int xen_xc_domain_add_to_physmap(xc_interface *xch, uint32_t domid,
-                                               unsigned int space,
-                                               unsigned long idx,
-                                               xen_pfn_t gpfn)
-{
-    return xc_domain_add_to_physmap(xch, domid, space, idx, gpfn);
-}
-#else
-static inline int xen_xc_domain_add_to_physmap(xc_interface *xch, uint32_t domid,
-                                               unsigned int space,
-                                               unsigned long idx,
-                                               xen_pfn_t gpfn)
-{
-    /* In Xen 4.6 rc is -1 and errno contains the error value. */
-    int rc = xc_domain_add_to_physmap(xch, domid, space, idx, gpfn);
-    if (rc == -1)
-        return errno;
-    return rc;
-}
-#endif
-
 #ifdef CONFIG_XEN_PV_DOMAIN_BUILD
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40700
 static inline int xen_domain_create(xc_interface *xc, uint32_t ssidref,
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 13/16] xen: Expect xenstore write to fail when restricted

[Ian Jackson]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Saving the current state to xenstore may fail when running restricted
(in particular, after a migration). Therefore, don't report the error or
exit when running restricted.  Toolstacks that want to allow running
QEMU restricted should instead make use of QMP events to listen for
state changes.

CC: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
v6: New patch in this version of the series
---
 hw/xen/xen-common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 454777c..6ec14c7 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -101,7 +101,12 @@ static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
     }
 
     snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
-    if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
+    /*
+     * This call may fail when running restricted so don't make it fatal in
+     * that case. Toolstacks should instead use QMP to listen for state changes.
+     */
+    if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) &&
+            !xen_domid_restrict) {
         error_report("error recording dm state");
         exit(1);
     }
-- 
2.1.4

[PATCH 13/16] xen: Expect xenstore write to fail when restricted

[Ian Jackson]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Saving the current state to xenstore may fail when running restricted
(in particular, after a migration). Therefore, don't report the error or
exit when running restricted.  Toolstacks that want to allow running
QEMU restricted should instead make use of QMP events to listen for
state changes.

CC: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
v6: New patch in this version of the series
---
 hw/xen/xen-common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 454777c..6ec14c7 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -101,7 +101,12 @@ static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
     }
 
     snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
-    if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
+    /*
+     * This call may fail when running restricted so don't make it fatal in
+     * that case. Toolstacks should instead use QMP to listen for state changes.
+     */
+    if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) &&
+            !xen_domid_restrict) {
         error_report("error recording dm state");
         exit(1);
     }
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 14/16] os-posix: cleanup: Replace fprintf with error_report in remaining call sites

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
CC: Michael Tokarev <mjt@tls.msk.ru>
---
v7: New patch
---
 os-posix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index 0f59566..d4cf466 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -129,7 +129,7 @@ void os_set_proc_name(const char *s)
         exit(1);
     }
 #else
-    fprintf(stderr, "Change of process name not supported by your OS\n");
+    error_report("Change of process name not supported by your OS\n");
     exit(1);
 #endif
 }
@@ -243,7 +243,7 @@ static void change_root(void)
 {
     if (chroot_dir) {
         if (chroot(chroot_dir) < 0) {
-            fprintf(stderr, "chroot failed\n");
+            error_report("chroot failed");
             exit(1);
         }
         if (chdir("/")) {
-- 
2.1.4

[PATCH 14/16] os-posix: cleanup: Replace fprintf with error_report in remaining call sites

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
CC: Michael Tokarev <mjt@tls.msk.ru>
---
v7: New patch
---
 os-posix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index 0f59566..d4cf466 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -129,7 +129,7 @@ void os_set_proc_name(const char *s)
         exit(1);
     }
 #else
-    fprintf(stderr, "Change of process name not supported by your OS\n");
+    error_report("Change of process name not supported by your OS\n");
     exit(1);
 #endif
 }
@@ -243,7 +243,7 @@ static void change_root(void)
 {
     if (chroot_dir) {
         if (chroot(chroot_dir) < 0) {
-            fprintf(stderr, "chroot failed\n");
+            error_report("chroot failed");
             exit(1);
         }
         if (chdir("/")) {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Ian Jackson]

perror() is defined to fprintf(stderr,...).  HACKING says
fprintf(stderr,...) is wrong.  So perror() is too.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
CC: Michael Tokarev <mjt@tls.msk.ru>
CC: Alistair Francis <alistair.francis@xilinx.com>
---
v7: New patch
---
 os-posix.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index d4cf466..0108028 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -125,7 +125,7 @@ void os_set_proc_name(const char *s)
     /* Could rewrite argv[0] too, but that's a bit more complicated.
        This simple way is enough for `top'. */
     if (prctl(PR_SET_NAME, name)) {
-        perror("unable to change process name");
+        error_report("unable to change process name: %s", strerror(errno));
         exit(1);
     }
 #else
@@ -247,7 +247,7 @@ static void change_root(void)
             exit(1);
         }
         if (chdir("/")) {
-            perror("not able to chdir to /");
+            error_report("not able to chdir to /: %s", strerror(errno));
             exit(1);
         }
     }
@@ -309,7 +309,7 @@ void os_setup_post(void)
 
     if (daemonize) {
         if (chdir("/")) {
-            perror("not able to chdir to /");
+            error_report("not able to chdir to /: %s", strerror(errno));
             exit(1);
         }
         TFR(fd = qemu_open("/dev/null", O_RDWR));
@@ -383,7 +383,7 @@ int os_mlock(void)
 
     ret = mlockall(MCL_CURRENT | MCL_FUTURE);
     if (ret < 0) {
-        perror("mlockall");
+        error_report("mlockall: %s", strerror(errno));
     }
 
     return ret;
-- 
2.1.4

[PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Ian Jackson]

perror() is defined to fprintf(stderr,...).  HACKING says
fprintf(stderr,...) is wrong.  So perror() is too.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
CC: Michael Tokarev <mjt@tls.msk.ru>
CC: Alistair Francis <alistair.francis@xilinx.com>
---
v7: New patch
---
 os-posix.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index d4cf466..0108028 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -125,7 +125,7 @@ void os_set_proc_name(const char *s)
     /* Could rewrite argv[0] too, but that's a bit more complicated.
        This simple way is enough for `top'. */
     if (prctl(PR_SET_NAME, name)) {
-        perror("unable to change process name");
+        error_report("unable to change process name: %s", strerror(errno));
         exit(1);
     }
 #else
@@ -247,7 +247,7 @@ static void change_root(void)
             exit(1);
         }
         if (chdir("/")) {
-            perror("not able to chdir to /");
+            error_report("not able to chdir to /: %s", strerror(errno));
             exit(1);
         }
     }
@@ -309,7 +309,7 @@ void os_setup_post(void)
 
     if (daemonize) {
         if (chdir("/")) {
-            perror("not able to chdir to /");
+            error_report("not able to chdir to /: %s", strerror(errno));
             exit(1);
         }
         TFR(fd = qemu_open("/dev/null", O_RDWR));
@@ -383,7 +383,7 @@ int os_mlock(void)
 
     ret = mlockall(MCL_CURRENT | MCL_FUTURE);
     if (ret < 0) {
-        perror("mlockall");
+        error_report("mlockall: %s", strerror(errno));
     }
 
     return ret;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

This makes it much easier to find a particular thing in config.log.

The information may be lacking in other shells, resulting in harmless
empty output.  (This is why we don't use the proper ${FUNCNAME[*]}
array syntax - other shells will choke on that.)

The extra output is only printed if configure is run with bash.  On
systems where /bin/sh is not bash, it is necessary to say bash
./configure to get the extra debug info in the log.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Kent R. Spillner <kspillner@acm.org>
CC: Janosch Frank <frankja@linux.vnet.ibm.com>
CC: Thomas Huth <thuth@redhat.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
v6: Fix commit message wording.
v4: No longer tag this patch RFC.
---
 configure | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure b/configure
index d5435ff..a4c5292 100755
--- a/configure
+++ b/configure
@@ -60,6 +60,10 @@ do_compiler() {
     # is compiler binary to execute.
     local compiler="$1"
     shift
+    echo >>config.log "
+funcs: ${FUNCNAME}
+lines: ${BASH_LINENO}
+files: ${BASH_SOURCE}"
     echo $compiler "$@" >> config.log
     $compiler "$@" >> config.log 2>&1 || return $?
     # Test passed. If this is an --enable-werror build, rerun
-- 
2.1.4

[PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

This makes it much easier to find a particular thing in config.log.

The information may be lacking in other shells, resulting in harmless
empty output.  (This is why we don't use the proper ${FUNCNAME[*]}
array syntax - other shells will choke on that.)

The extra output is only printed if configure is run with bash.  On
systems where /bin/sh is not bash, it is necessary to say bash
./configure to get the extra debug info in the log.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Kent R. Spillner <kspillner@acm.org>
CC: Janosch Frank <frankja@linux.vnet.ibm.com>
CC: Thomas Huth <thuth@redhat.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
v6: Fix commit message wording.
v4: No longer tag this patch RFC.
---
 configure | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure b/configure
index d5435ff..a4c5292 100755
--- a/configure
+++ b/configure
@@ -60,6 +60,10 @@ do_compiler() {
     # is compiler binary to execute.
     local compiler="$1"
     shift
+    echo >>config.log "
+funcs: ${FUNCNAME}
+lines: ${BASH_LINENO}
+files: ${BASH_SOURCE}"
     echo $compiler "$@" >> config.log
     $compiler "$@" >> config.log 2>&1 || return $?
     # Test passed. If this is an --enable-werror build, rerun
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH v7 00/16] xen: xen-domid-restrict improvements

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1524156319-11465-1-git-send-email-ian.jackson@eu.citrix.com
Subject: [Qemu-devel] [PATCH v7 00/16] xen: xen-domid-restrict improvements

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1524156319-11465-1-git-send-email-ian.jackson@eu.citrix.com -> patchew/1524156319-11465-1-git-send-email-ian.jackson@eu.citrix.com
Switched to a new branch 'test'
791deba00b configure: do_compiler: Dump some extra info under bash
b10a749616 os-posix: cleanup: Replace perror with error_report
f6ee193e03 os-posix: cleanup: Replace fprintf with error_report in remaining call sites
38a44ba872 xen: Expect xenstore write to fail when restricted
4862292c1d xen: Remove now-obsolete xen_xc_domain_add_to_physmap
9a713e3565 xen: Use newly added dmops for mapping VGA memory
75ebbfeb30 os-posix: Provide new -runas <uid>:<gid> facility
013cb70cd4 os-posix: cleanup: Replace fprintfs with error_report in change_process_uid
581e476008 xen: destroy_hvm_domain: Try xendevicemodel_shutdown
e13e890b93 xen: move xc_interface compatibility fallback further up the file
daeeb16831 xen: destroy_hvm_domain: Move reason into a variable
9b037ca2ab xen: defer call to xen_restrict until just before os_setup_post
59764a2887 xen: restrict: use xentoolcore_restrict_all
11c13929a6 xen: link against xentoolcore
4fbbd8e7d6 AccelClass: Introduce accel_setup_post
e572031cba checkpatch: Add xendevicemodel_handle to the list of types

=== OUTPUT BEGIN ===
Checking PATCH 1/16: checkpatch: Add xendevicemodel_handle to the list of types...
Checking PATCH 2/16: AccelClass: Introduce accel_setup_post...
Checking PATCH 3/16: xen: link against xentoolcore...
Checking PATCH 4/16: xen: restrict: use xentoolcore_restrict_all...
Checking PATCH 5/16: xen: defer call to xen_restrict until just before os_setup_post...
Checking PATCH 6/16: xen: destroy_hvm_domain: Move reason into a variable...
Checking PATCH 7/16: xen: move xc_interface compatibility fallback further up the file...
Checking PATCH 8/16: xen: destroy_hvm_domain: Try xendevicemodel_shutdown...
Checking PATCH 9/16: os-posix: cleanup: Replace fprintfs with error_report in change_process_uid...
Checking PATCH 10/16: os-posix: Provide new -runas <uid>:<gid> facility...
Checking PATCH 11/16: xen: Use newly added dmops for mapping VGA memory...
Checking PATCH 12/16: xen: Remove now-obsolete xen_xc_domain_add_to_physmap...
Checking PATCH 13/16: xen: Expect xenstore write to fail when restricted...
Checking PATCH 14/16: os-posix: cleanup: Replace fprintf with error_report in remaining call sites...
ERROR: Error messages should not contain newlines
#23: FILE: os-posix.c:132:
+    error_report("Change of process name not supported by your OS\n");

total: 1 errors, 0 warnings, 16 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 15/16: os-posix: cleanup: Replace perror with error_report...
Checking PATCH 16/16: configure: do_compiler: Dump some extra info under bash...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v7 00/16] xen: xen-domid-restrict improvements

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1524156319-11465-1-git-send-email-ian.jackson@eu.citrix.com
Subject: [Qemu-devel] [PATCH v7 00/16] xen: xen-domid-restrict improvements

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1524156319-11465-1-git-send-email-ian.jackson@eu.citrix.com -> patchew/1524156319-11465-1-git-send-email-ian.jackson@eu.citrix.com
Switched to a new branch 'test'
791deba00b configure: do_compiler: Dump some extra info under bash
b10a749616 os-posix: cleanup: Replace perror with error_report
f6ee193e03 os-posix: cleanup: Replace fprintf with error_report in remaining call sites
38a44ba872 xen: Expect xenstore write to fail when restricted
4862292c1d xen: Remove now-obsolete xen_xc_domain_add_to_physmap
9a713e3565 xen: Use newly added dmops for mapping VGA memory
75ebbfeb30 os-posix: Provide new -runas <uid>:<gid> facility
013cb70cd4 os-posix: cleanup: Replace fprintfs with error_report in change_process_uid
581e476008 xen: destroy_hvm_domain: Try xendevicemodel_shutdown
e13e890b93 xen: move xc_interface compatibility fallback further up the file
daeeb16831 xen: destroy_hvm_domain: Move reason into a variable
9b037ca2ab xen: defer call to xen_restrict until just before os_setup_post
59764a2887 xen: restrict: use xentoolcore_restrict_all
11c13929a6 xen: link against xentoolcore
4fbbd8e7d6 AccelClass: Introduce accel_setup_post
e572031cba checkpatch: Add xendevicemodel_handle to the list of types

=== OUTPUT BEGIN ===
Checking PATCH 1/16: checkpatch: Add xendevicemodel_handle to the list of types...
Checking PATCH 2/16: AccelClass: Introduce accel_setup_post...
Checking PATCH 3/16: xen: link against xentoolcore...
Checking PATCH 4/16: xen: restrict: use xentoolcore_restrict_all...
Checking PATCH 5/16: xen: defer call to xen_restrict until just before os_setup_post...
Checking PATCH 6/16: xen: destroy_hvm_domain: Move reason into a variable...
Checking PATCH 7/16: xen: move xc_interface compatibility fallback further up the file...
Checking PATCH 8/16: xen: destroy_hvm_domain: Try xendevicemodel_shutdown...
Checking PATCH 9/16: os-posix: cleanup: Replace fprintfs with error_report in change_process_uid...
Checking PATCH 10/16: os-posix: Provide new -runas <uid>:<gid> facility...
Checking PATCH 11/16: xen: Use newly added dmops for mapping VGA memory...
Checking PATCH 12/16: xen: Remove now-obsolete xen_xc_domain_add_to_physmap...
Checking PATCH 13/16: xen: Expect xenstore write to fail when restricted...
Checking PATCH 14/16: os-posix: cleanup: Replace fprintf with error_report in remaining call sites...
ERROR: Error messages should not contain newlines
#23: FILE: os-posix.c:132:
+    error_report("Change of process name not supported by your OS\n");

total: 1 errors, 0 warnings, 16 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 15/16: os-posix: cleanup: Replace perror with error_report...
Checking PATCH 16/16: configure: do_compiler: Dump some extra info under bash...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 14/16] os-posix: cleanup: Replace fprintf with error_report in remaining call sites

[Philippe Mathieu-Daudé]

Hi Ian,

On 04/19/2018 01:45 PM, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Daniel P. Berrange <berrange@redhat.com>
> CC: Michael Tokarev <mjt@tls.msk.ru>
> ---
> v7: New patch
> ---
>  os-posix.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/os-posix.c b/os-posix.c
> index 0f59566..d4cf466 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -129,7 +129,7 @@ void os_set_proc_name(const char *s)
>          exit(1);
>      }
>  #else
> -    fprintf(stderr, "Change of process name not supported by your OS\n");
> +    error_report("Change of process name not supported by your OS\n");

removing the trailing "\n":
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>      exit(1);
>  #endif
>  }
> @@ -243,7 +243,7 @@ static void change_root(void)
>  {
>      if (chroot_dir) {
>          if (chroot(chroot_dir) < 0) {
> -            fprintf(stderr, "chroot failed\n");
> +            error_report("chroot failed");
>              exit(1);
>          }
>          if (chdir("/")) {
> 

Re: [Qemu-devel] [PATCH 14/16] os-posix: cleanup: Replace fprintf with error_report in remaining call sites

[Philippe Mathieu-Daudé]

Hi Ian,

On 04/19/2018 01:45 PM, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Daniel P. Berrange <berrange@redhat.com>
> CC: Michael Tokarev <mjt@tls.msk.ru>
> ---
> v7: New patch
> ---
>  os-posix.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/os-posix.c b/os-posix.c
> index 0f59566..d4cf466 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -129,7 +129,7 @@ void os_set_proc_name(const char *s)
>          exit(1);
>      }
>  #else
> -    fprintf(stderr, "Change of process name not supported by your OS\n");
> +    error_report("Change of process name not supported by your OS\n");

removing the trailing "\n":
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>      exit(1);
>  #endif
>  }
> @@ -243,7 +243,7 @@ static void change_root(void)
>  {
>      if (chroot_dir) {
>          if (chroot(chroot_dir) < 0) {
> -            fprintf(stderr, "chroot failed\n");
> +            error_report("chroot failed");
>              exit(1);
>          }
>          if (chdir("/")) {
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Philippe Mathieu-Daudé]

On 04/19/2018 01:45 PM, Ian Jackson wrote:
> perror() is defined to fprintf(stderr,...).  HACKING says
> fprintf(stderr,...) is wrong.  So perror() is too.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Daniel P. Berrange <berrange@redhat.com>
> CC: Michael Tokarev <mjt@tls.msk.ru>
> CC: Alistair Francis <alistair.francis@xilinx.com>
> ---
> v7: New patch
> ---
>  os-posix.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/os-posix.c b/os-posix.c
> index d4cf466..0108028 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -125,7 +125,7 @@ void os_set_proc_name(const char *s)
>      /* Could rewrite argv[0] too, but that's a bit more complicated.
>         This simple way is enough for `top'. */
>      if (prctl(PR_SET_NAME, name)) {
> -        perror("unable to change process name");
> +        error_report("unable to change process name: %s", strerror(errno));
>          exit(1);
>      }
>  #else
> @@ -247,7 +247,7 @@ static void change_root(void)
>              exit(1);
>          }
>          if (chdir("/")) {
> -            perror("not able to chdir to /");
> +            error_report("not able to chdir to /: %s", strerror(errno));
>              exit(1);
>          }
>      }
> @@ -309,7 +309,7 @@ void os_setup_post(void)
>  
>      if (daemonize) {
>          if (chdir("/")) {
> -            perror("not able to chdir to /");
> +            error_report("not able to chdir to /: %s", strerror(errno));
>              exit(1);
>          }
>          TFR(fd = qemu_open("/dev/null", O_RDWR));
> @@ -383,7 +383,7 @@ int os_mlock(void)
>  
>      ret = mlockall(MCL_CURRENT | MCL_FUTURE);
>      if (ret < 0) {
> -        perror("mlockall");
> +        error_report("mlockall: %s", strerror(errno));
>      }
>  
>      return ret;

Thinking loudly, maybe we can refactor as error_report_errno(const char
*desc)...

Anyway:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Philippe Mathieu-Daudé]

On 04/19/2018 01:45 PM, Ian Jackson wrote:
> perror() is defined to fprintf(stderr,...).  HACKING says
> fprintf(stderr,...) is wrong.  So perror() is too.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Daniel P. Berrange <berrange@redhat.com>
> CC: Michael Tokarev <mjt@tls.msk.ru>
> CC: Alistair Francis <alistair.francis@xilinx.com>
> ---
> v7: New patch
> ---
>  os-posix.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/os-posix.c b/os-posix.c
> index d4cf466..0108028 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -125,7 +125,7 @@ void os_set_proc_name(const char *s)
>      /* Could rewrite argv[0] too, but that's a bit more complicated.
>         This simple way is enough for `top'. */
>      if (prctl(PR_SET_NAME, name)) {
> -        perror("unable to change process name");
> +        error_report("unable to change process name: %s", strerror(errno));
>          exit(1);
>      }
>  #else
> @@ -247,7 +247,7 @@ static void change_root(void)
>              exit(1);
>          }
>          if (chdir("/")) {
> -            perror("not able to chdir to /");
> +            error_report("not able to chdir to /: %s", strerror(errno));
>              exit(1);
>          }
>      }
> @@ -309,7 +309,7 @@ void os_setup_post(void)
>  
>      if (daemonize) {
>          if (chdir("/")) {
> -            perror("not able to chdir to /");
> +            error_report("not able to chdir to /: %s", strerror(errno));
>              exit(1);
>          }
>          TFR(fd = qemu_open("/dev/null", O_RDWR));
> @@ -383,7 +383,7 @@ int os_mlock(void)
>  
>      ret = mlockall(MCL_CURRENT | MCL_FUTURE);
>      if (ret < 0) {
> -        perror("mlockall");
> +        error_report("mlockall: %s", strerror(errno));
>      }
>  
>      return ret;

Thinking loudly, maybe we can refactor as error_report_errno(const char
*desc)...

Anyway:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 14/16] os-posix: cleanup: Replace fprintf with error_report in remaining call sites

[Ian Jackson]

Philippe Mathieu-Daudé writes ("Re: [Qemu-devel] [PATCH 14/16] os-posix: cleanup: Replace fprintf with error_report in remaining call sites"):
> On 04/19/2018 01:45 PM, Ian Jackson wrote:
> > -    fprintf(stderr, "Change of process name not supported by your OS\n");
> > +    error_report("Change of process name not supported by your OS\n");
> 
> removing the trailing "\n":
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Damn, missed one!  Thanks :-).

Ian.

Re: [Qemu-devel] [PATCH 14/16] os-posix: cleanup: Replace fprintf with error_report in remaining call sites

[Ian Jackson]

Philippe Mathieu-Daudé writes ("Re: [Qemu-devel] [PATCH 14/16] os-posix: cleanup: Replace fprintf with error_report in remaining call sites"):
> On 04/19/2018 01:45 PM, Ian Jackson wrote:
> > -    fprintf(stderr, "Change of process name not supported by your OS\n");
> > +    error_report("Change of process name not supported by your OS\n");
> 
> removing the trailing "\n":
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Damn, missed one!  Thanks :-).

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 10/16] os-posix: Provide new -runas <uid>:<gid> facility

[Markus Armbruster]

Ian Jackson <ian.jackson@eu.citrix.com> writes:

> This allows the caller to specify a uid and gid to use, even if there
> is no corresponding password entry.  This will be useful in certain
> Xen configurations.
>
> We don't support just -runas <uid> because: (i) deprivileging without
> calling setgroups would be ineffective (ii) given only a uid we don't
> know what gid we ought to use (since uids may eppear in multiple
> passwd file entries with different gids).
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Daniel P. Berrange <berrange@redhat.com>
> CC: Michael Tokarev <mjt@tls.msk.ru>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 10/16] os-posix: Provide new -runas <uid>:<gid> facility

[Markus Armbruster]

Ian Jackson <ian.jackson@eu.citrix.com> writes:

> This allows the caller to specify a uid and gid to use, even if there
> is no corresponding password entry.  This will be useful in certain
> Xen configurations.
>
> We don't support just -runas <uid> because: (i) deprivileging without
> calling setgroups would be ineffective (ii) given only a uid we don't
> know what gid we ought to use (since uids may eppear in multiple
> passwd file entries with different gids).
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Daniel P. Berrange <berrange@redhat.com>
> CC: Michael Tokarev <mjt@tls.msk.ru>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post

[Anthony PERARD]

On Thu, Apr 19, 2018 at 05:45:08PM +0100, Ian Jackson wrote:
> We need to restrict *all* the control fds that qemu opens.  Looking in
> /proc/PID/fd shows there are many; their allocation seems scattered
> throughout Xen support code in qemu.
> 
> We must postpone the restrict call until roughly the same time as qemu
> changes its uid, chroots (if applicable), and so on.
> 
> There doesn't seem to be an appropriate hook already.  The RunState
> change hook fires at different times depending on exactly what mode
> qemu is operating in.
> 
> And it appears that no-one but the Xen code wants a hook at this phase
> of execution.  So, introduce a bare call to a new function
> xen_setup_post, just before os_setup_post.  Also provide the
> appropriate stub for when Xen compilation is disabled.
> 
> We do the restriction before rather than after os_setup_post, because
> xen_restrict may need to open /dev/null, and os_setup_post might have
> called chroot.
> 
> Currently this does not work with migration, because when running as
> the Xen device model qemu needs to signal to the toolstack that it is
> ready.  It currently does this using xenstore, and for incoming
> migration (but not for ordinary startup) that happens after
> os_setup_post.
> 
> It is correct that this happens late: we want the incoming migration
> stream to be processed by a restricted qemu.  The fix for this will be
> to do the startup notification a different way, without using
> xenstore.  (QMP is probably a reasonable choice.)
> 
> So for now this restriction feature cannot be used in conjunction with
> migration.  (Note that this is not a regression in this patch, because
> previously the -xen-restrict-domid call was, in fact, simply
> ineffective!)  We will revisit this in the Xen 4.11 release cycle.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

Re: [PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post

[Anthony PERARD]

On Thu, Apr 19, 2018 at 05:45:08PM +0100, Ian Jackson wrote:
> We need to restrict *all* the control fds that qemu opens.  Looking in
> /proc/PID/fd shows there are many; their allocation seems scattered
> throughout Xen support code in qemu.
> 
> We must postpone the restrict call until roughly the same time as qemu
> changes its uid, chroots (if applicable), and so on.
> 
> There doesn't seem to be an appropriate hook already.  The RunState
> change hook fires at different times depending on exactly what mode
> qemu is operating in.
> 
> And it appears that no-one but the Xen code wants a hook at this phase
> of execution.  So, introduce a bare call to a new function
> xen_setup_post, just before os_setup_post.  Also provide the
> appropriate stub for when Xen compilation is disabled.
> 
> We do the restriction before rather than after os_setup_post, because
> xen_restrict may need to open /dev/null, and os_setup_post might have
> called chroot.
> 
> Currently this does not work with migration, because when running as
> the Xen device model qemu needs to signal to the toolstack that it is
> ready.  It currently does this using xenstore, and for incoming
> migration (but not for ordinary startup) that happens after
> os_setup_post.
> 
> It is correct that this happens late: we want the incoming migration
> stream to be processed by a restricted qemu.  The fix for this will be
> to do the startup notification a different way, without using
> xenstore.  (QMP is probably a reasonable choice.)
> 
> So for now this restriction feature cannot be used in conjunction with
> migration.  (Note that this is not a regression in this patch, because
> previously the -xen-restrict-domid call was, in fact, simply
> ineffective!)  We will revisit this in the Xen 4.11 release cycle.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 03/16] xen: link against xentoolcore

[Anthony PERARD]

On Thu, Apr 19, 2018 at 05:45:06PM +0100, Ian Jackson wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> Xen libraries in 4.10 include a new xentoolcore library.  This
> contains the xentoolcore_restrict_all function which we are about to
> want to use.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v5: More truthful commit message.
> ---
>  configure | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 0a19b03..f580255 100755
> --- a/configure
> +++ b/configure
> @@ -2188,7 +2188,7 @@ if test "$xen" != "no" ; then
>        $($pkg_config --modversion xencontrol | sed 's/\./ /g') )"
>      xen=yes
>      xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
> -    xen_pc="$xen_pc xenevtchn xendevicemodel"
> +    xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"

I think we want to check if "xentoolcore" pkg_config exist before trying
to use it. Otherwith, that is not going to work with Xen older than
4.10.

`pkg-config --libs` will throw an error without printing anything if
one of the library doesn't exist.

Instead, we could do this:
if $pkg_config --exists xentoolcore; then
  xen_pc+=" xentoolcore"
fi

>      QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)"
>      libs_softmmu="$($pkg_config --libs $xen_pc) $libs_softmmu"
>      LDFLAGS="$($pkg_config --libs $xen_pc) $LDFLAGS"


-- 
Anthony PERARD

Re: [PATCH 03/16] xen: link against xentoolcore

[Anthony PERARD]

On Thu, Apr 19, 2018 at 05:45:06PM +0100, Ian Jackson wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> Xen libraries in 4.10 include a new xentoolcore library.  This
> contains the xentoolcore_restrict_all function which we are about to
> want to use.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v5: More truthful commit message.
> ---
>  configure | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 0a19b03..f580255 100755
> --- a/configure
> +++ b/configure
> @@ -2188,7 +2188,7 @@ if test "$xen" != "no" ; then
>        $($pkg_config --modversion xencontrol | sed 's/\./ /g') )"
>      xen=yes
>      xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
> -    xen_pc="$xen_pc xenevtchn xendevicemodel"
> +    xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"

I think we want to check if "xentoolcore" pkg_config exist before trying
to use it. Otherwith, that is not going to work with Xen older than
4.10.

`pkg-config --libs` will throw an error without printing anything if
one of the library doesn't exist.

Instead, we could do this:
if $pkg_config --exists xentoolcore; then
  xen_pc+=" xentoolcore"
fi

>      QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)"
>      libs_softmmu="$($pkg_config --libs $xen_pc) $libs_softmmu"
>      LDFLAGS="$($pkg_config --libs $xen_pc) $LDFLAGS"


-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 12/16] xen: Remove now-obsolete xen_xc_domain_add_to_physmap

[Anthony PERARD]

On Thu, Apr 19, 2018 at 05:45:15PM +0100, Ian Jackson wrote:
> The last user was just removed; remove this function, accordingly.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

Re: [PATCH 12/16] xen: Remove now-obsolete xen_xc_domain_add_to_physmap

[Anthony PERARD]

On Thu, Apr 19, 2018 at 05:45:15PM +0100, Ian Jackson wrote:
> The last user was just removed; remove this function, accordingly.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post

[Anthony PERARD]

On Thu, Apr 19, 2018 at 05:45:08PM +0100, Ian Jackson wrote:
> diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
> index 0067bcc..7787ea2 100644
> --- a/stubs/xen-hvm.c
> +++ b/stubs/xen-hvm.c
> @@ -13,6 +13,7 @@
>  #include "hw/xen/xen.h"
>  #include "exec/memory.h"
>  #include "qapi/qapi-commands-misc.h"
> +#include "sysemu/sysemu.h"

I think this include is not needed anymore, and can go away from the
patch series.

>  
>  int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
>  {

-- 
Anthony PERARD

Re: [PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post

[Anthony PERARD]

On Thu, Apr 19, 2018 at 05:45:08PM +0100, Ian Jackson wrote:
> diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
> index 0067bcc..7787ea2 100644
> --- a/stubs/xen-hvm.c
> +++ b/stubs/xen-hvm.c
> @@ -13,6 +13,7 @@
>  #include "hw/xen/xen.h"
>  #include "exec/memory.h"
>  #include "qapi/qapi-commands-misc.h"
> +#include "sysemu/sysemu.h"

I think this include is not needed anymore, and can go away from the
patch series.

>  
>  int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
>  {

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Anthony PERARD]

On Thu, Apr 19, 2018 at 05:45:19PM +0100, Ian Jackson wrote:
> This makes it much easier to find a particular thing in config.log.
> 
> The information may be lacking in other shells, resulting in harmless
> empty output.  (This is why we don't use the proper ${FUNCNAME[*]}
> array syntax - other shells will choke on that.)
> 
> The extra output is only printed if configure is run with bash.  On
> systems where /bin/sh is not bash, it is necessary to say bash
> ./configure to get the extra debug info in the log.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  configure | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configure b/configure
> index d5435ff..a4c5292 100755
> --- a/configure
> +++ b/configure
> @@ -60,6 +60,10 @@ do_compiler() {
>      # is compiler binary to execute.
>      local compiler="$1"
>      shift
> +    echo >>config.log "
> +funcs: ${FUNCNAME}
> +lines: ${BASH_LINENO}
> +files: ${BASH_SOURCE}"
>      echo $compiler "$@" >> config.log
>      $compiler "$@" >> config.log 2>&1 || return $?
>      # Test passed. If this is an --enable-werror build, rerun

How is this usefull? All I have in my config.log is a lot of:
  funcs: do_compiler
  lines: 91
  files: ./configure

And one:
  funcs: do_compiler
  lines: 95
  files: ./configure

It still don't tell me which test had runned.

Regards,

-- 
Anthony PERARD

Re: [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Anthony PERARD]

On Thu, Apr 19, 2018 at 05:45:19PM +0100, Ian Jackson wrote:
> This makes it much easier to find a particular thing in config.log.
> 
> The information may be lacking in other shells, resulting in harmless
> empty output.  (This is why we don't use the proper ${FUNCNAME[*]}
> array syntax - other shells will choke on that.)
> 
> The extra output is only printed if configure is run with bash.  On
> systems where /bin/sh is not bash, it is necessary to say bash
> ./configure to get the extra debug info in the log.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  configure | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configure b/configure
> index d5435ff..a4c5292 100755
> --- a/configure
> +++ b/configure
> @@ -60,6 +60,10 @@ do_compiler() {
>      # is compiler binary to execute.
>      local compiler="$1"
>      shift
> +    echo >>config.log "
> +funcs: ${FUNCNAME}
> +lines: ${BASH_LINENO}
> +files: ${BASH_SOURCE}"
>      echo $compiler "$@" >> config.log
>      $compiler "$@" >> config.log 2>&1 || return $?
>      # Test passed. If this is an --enable-werror build, rerun

How is this usefull? All I have in my config.log is a lot of:
  funcs: do_compiler
  lines: 91
  files: ./configure

And one:
  funcs: do_compiler
  lines: 95
  files: ./configure

It still don't tell me which test had runned.

Regards,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Daniel P. Berrangé]

On Mon, Apr 23, 2018 at 05:21:42PM +0100, Anthony PERARD wrote:
> On Thu, Apr 19, 2018 at 05:45:19PM +0100, Ian Jackson wrote:
> > This makes it much easier to find a particular thing in config.log.
> > 
> > The information may be lacking in other shells, resulting in harmless
> > empty output.  (This is why we don't use the proper ${FUNCNAME[*]}
> > array syntax - other shells will choke on that.)
> > 
> > The extra output is only printed if configure is run with bash.  On
> > systems where /bin/sh is not bash, it is necessary to say bash
> > ./configure to get the extra debug info in the log.
> > 
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > ---
> >  configure | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/configure b/configure
> > index d5435ff..a4c5292 100755
> > --- a/configure
> > +++ b/configure
> > @@ -60,6 +60,10 @@ do_compiler() {
> >      # is compiler binary to execute.
> >      local compiler="$1"
> >      shift
> > +    echo >>config.log "
> > +funcs: ${FUNCNAME}
> > +lines: ${BASH_LINENO}
> > +files: ${BASH_SOURCE}"
> >      echo $compiler "$@" >> config.log
> >      $compiler "$@" >> config.log 2>&1 || return $?
> >      # Test passed. If this is an --enable-werror build, rerun
> 
> How is this usefull? All I have in my config.log is a lot of:
>   funcs: do_compiler
>   lines: 91
>   files: ./configure
> 
> And one:
>   funcs: do_compiler
>   lines: 95
>   files: ./configure
> 
> It still don't tell me which test had runned.

In autoconf, you would generally have a line output to stdout for every
test being run, as it is done so you can see immediately which test it
stopped on. QEMU's configure by comparison is completely silent, except
for the fnal summary at the end which is fine if everything works perfectly,
but not great when it doesn't.

Personally I'd suggest we add informative messages throughout the
configure script for each check being run. If people really hate the
idea of a verbose output from configure, we could leave it silent by
default and add a '--verbose' option to turn it on.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Daniel P. Berrangé]

On Mon, Apr 23, 2018 at 05:21:42PM +0100, Anthony PERARD wrote:
> On Thu, Apr 19, 2018 at 05:45:19PM +0100, Ian Jackson wrote:
> > This makes it much easier to find a particular thing in config.log.
> > 
> > The information may be lacking in other shells, resulting in harmless
> > empty output.  (This is why we don't use the proper ${FUNCNAME[*]}
> > array syntax - other shells will choke on that.)
> > 
> > The extra output is only printed if configure is run with bash.  On
> > systems where /bin/sh is not bash, it is necessary to say bash
> > ./configure to get the extra debug info in the log.
> > 
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > ---
> >  configure | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/configure b/configure
> > index d5435ff..a4c5292 100755
> > --- a/configure
> > +++ b/configure
> > @@ -60,6 +60,10 @@ do_compiler() {
> >      # is compiler binary to execute.
> >      local compiler="$1"
> >      shift
> > +    echo >>config.log "
> > +funcs: ${FUNCNAME}
> > +lines: ${BASH_LINENO}
> > +files: ${BASH_SOURCE}"
> >      echo $compiler "$@" >> config.log
> >      $compiler "$@" >> config.log 2>&1 || return $?
> >      # Test passed. If this is an --enable-werror build, rerun
> 
> How is this usefull? All I have in my config.log is a lot of:
>   funcs: do_compiler
>   lines: 91
>   files: ./configure
> 
> And one:
>   funcs: do_compiler
>   lines: 95
>   files: ./configure
> 
> It still don't tell me which test had runned.

In autoconf, you would generally have a line output to stdout for every
test being run, as it is done so you can see immediately which test it
stopped on. QEMU's configure by comparison is completely silent, except
for the fnal summary at the end which is fine if everything works perfectly,
but not great when it doesn't.

Personally I'd suggest we add informative messages throughout the
configure script for each check being run. If people really hate the
idea of a verbose output from configure, we could leave it silent by
default and add a '--verbose' option to turn it on.

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 :|

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 16/16] configure: do_compiler: Dump some extra info under bash"):
> On Thu, Apr 19, 2018 at 05:45:19PM +0100, Ian Jackson wrote:
> > +funcs: ${FUNCNAME}
> > +lines: ${BASH_LINENO}
> > +files: ${BASH_SOURCE}"
> >      echo $compiler "$@" >> config.log
> >      $compiler "$@" >> config.log 2>&1 || return $?
> >      # Test passed. If this is an --enable-werror build, rerun
> 
> How is this usefull? All I have in my config.log is a lot of:
>   funcs: do_compiler
>   lines: 91
>   files: ./configure
> 
> And one:
>   funcs: do_compiler
>   lines: 95
>   files: ./configure
> 
> It still don't tell me which test had runned.

You are right.  Perhaps my testing was inadequate.  I wrote this a
long while ago, and if there was a syntax along these lines that DTRT
in both bash and dash in my tests it is long gone.  Starting de novo,
the following code works for me:

    (echo >>config.log "
 funcs: ${FUNCNAME[*]}
 lines: ${BASH_LINENO[*]}
 files: ${BASH_SOURCE[*]}") 2>/dev/null ||:

With bash I get the expected information in config.log, which looks
like this:

 funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
 lines: 91 124 1720 1724 0
 files: ./configure ./configure ./configure ./configure ./configure

With dash the script runs but there is nothing from this segment in
the log.  Without the 2>/dev/null, it prints
  ./configure: 63: ./configure: Bad substitution
so the syntax error is indeed being suprresed and ignored.

The ( ) is necessary because syntax errors are not like set -e errors:
they cause the shell process to exit.

I will update the patch and put some of this info in the commit
message.

Ian.

Re: [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 16/16] configure: do_compiler: Dump some extra info under bash"):
> On Thu, Apr 19, 2018 at 05:45:19PM +0100, Ian Jackson wrote:
> > +funcs: ${FUNCNAME}
> > +lines: ${BASH_LINENO}
> > +files: ${BASH_SOURCE}"
> >      echo $compiler "$@" >> config.log
> >      $compiler "$@" >> config.log 2>&1 || return $?
> >      # Test passed. If this is an --enable-werror build, rerun
> 
> How is this usefull? All I have in my config.log is a lot of:
>   funcs: do_compiler
>   lines: 91
>   files: ./configure
> 
> And one:
>   funcs: do_compiler
>   lines: 95
>   files: ./configure
> 
> It still don't tell me which test had runned.

You are right.  Perhaps my testing was inadequate.  I wrote this a
long while ago, and if there was a syntax along these lines that DTRT
in both bash and dash in my tests it is long gone.  Starting de novo,
the following code works for me:

    (echo >>config.log "
 funcs: ${FUNCNAME[*]}
 lines: ${BASH_LINENO[*]}
 files: ${BASH_SOURCE[*]}") 2>/dev/null ||:

With bash I get the expected information in config.log, which looks
like this:

 funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
 lines: 91 124 1720 1724 0
 files: ./configure ./configure ./configure ./configure ./configure

With dash the script runs but there is nothing from this segment in
the log.  Without the 2>/dev/null, it prints
  ./configure: 63: ./configure: Bad substitution
so the syntax error is indeed being suprresed and ignored.

The ( ) is necessary because syntax errors are not like set -e errors:
they cause the shell process to exit.

I will update the patch and put some of this info in the commit
message.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

Daniel P. Berrangé writes ("Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash"):
> Personally I'd suggest we add informative messages throughout the
> configure script for each check being run. If people really hate the
> idea of a verbose output from configure, we could leave it silent by
> default and add a '--verbose' option to turn it on.

Doing that would be a lot of work.  I am not opposed to it, but I am
not going to do it.

My suggestion is a very small change which instantly makes configure
much easier to debug, at least if you have bash available (and the bug
you are fixing doesn't go away with bash).

Please don't let the best be the enemy of an improvement.  If
configure is ever improved (without being simply replaced), the
logging code can easily be removed.

Thanks,
Ian.

Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

Daniel P. Berrangé writes ("Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash"):
> Personally I'd suggest we add informative messages throughout the
> configure script for each check being run. If people really hate the
> idea of a verbose output from configure, we could leave it silent by
> default and add a '--verbose' option to turn it on.

Doing that would be a lot of work.  I am not opposed to it, but I am
not going to do it.

My suggestion is a very small change which instantly makes configure
much easier to debug, at least if you have bash available (and the bug
you are fixing doesn't go away with bash).

Please don't let the best be the enemy of an improvement.  If
configure is ever improved (without being simply replaced), the
logging code can easily be removed.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

On 04/23/2018 11:38 AM, Ian Jackson wrote:

> You are right.  Perhaps my testing was inadequate.  I wrote this a
> long while ago, and if there was a syntax along these lines that DTRT
> in both bash and dash in my tests it is long gone.  Starting de novo,
> the following code works for me:
> 
>     (echo >>config.log "
>  funcs: ${FUNCNAME[*]}
>  lines: ${BASH_LINENO[*]}
>  files: ${BASH_SOURCE[*]}") 2>/dev/null ||:

That's still fork-heavy.  You could do:

test -n "$BASH_VERSION" && eval '
echo >>config.log "
funcs: ${FUNCNAME[*]}
lines: ${BASH_LINENO[*]}
files: ${BASH_SOURCE[*]}"'

which avoids the fork, but remains silent on dash.

> 
> With bash I get the expected information in config.log, which looks
> like this:
> 
>  funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
>  lines: 91 124 1720 1724 0
>  files: ./configure ./configure ./configure ./configure ./configure

Is files: really useful information?  The other two are (as it gives a
full stack trace), but if we aren't actively sourcing lots of other
files, seeing a bunch of ./configure doesn't add much.

> 
> With dash the script runs but there is nothing from this segment in
> the log.  Without the 2>/dev/null, it prints
>   ./configure: 63: ./configure: Bad substitution
> so the syntax error is indeed being suprresed and ignored.
> 
> The ( ) is necessary because syntax errors are not like set -e errors:
> they cause the shell process to exit.

See above - a well-quoted eval is sufficient to avoid a subshell.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Eric Blake]

[-- Attachment #1.1.1: Type: text/plain, Size: 1685 bytes --]

On 04/23/2018 11:38 AM, Ian Jackson wrote:

> You are right.  Perhaps my testing was inadequate.  I wrote this a
> long while ago, and if there was a syntax along these lines that DTRT
> in both bash and dash in my tests it is long gone.  Starting de novo,
> the following code works for me:
> 
>     (echo >>config.log "
>  funcs: ${FUNCNAME[*]}
>  lines: ${BASH_LINENO[*]}
>  files: ${BASH_SOURCE[*]}") 2>/dev/null ||:

That's still fork-heavy.  You could do:

test -n "$BASH_VERSION" && eval '
echo >>config.log "
funcs: ${FUNCNAME[*]}
lines: ${BASH_LINENO[*]}
files: ${BASH_SOURCE[*]}"'

which avoids the fork, but remains silent on dash.

> 
> With bash I get the expected information in config.log, which looks
> like this:
> 
>  funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
>  lines: 91 124 1720 1724 0
>  files: ./configure ./configure ./configure ./configure ./configure

Is files: really useful information?  The other two are (as it gives a
full stack trace), but if we aren't actively sourcing lots of other
files, seeing a bunch of ./configure doesn't add much.

> 
> With dash the script runs but there is nothing from this segment in
> the log.  Without the 2>/dev/null, it prints
>   ./configure: 63: ./configure: Bad substitution
> so the syntax error is indeed being suprresed and ignored.
> 
> The ( ) is necessary because syntax errors are not like set -e errors:
> they cause the shell process to exit.

See above - a well-quoted eval is sufficient to avoid a subshell.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 03/16] xen: link against xentoolcore

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 03/16] xen: link against xentoolcore"):
> On Thu, Apr 19, 2018 at 05:45:06PM +0100, Ian Jackson wrote:
> >      xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
> > -    xen_pc="$xen_pc xenevtchn xendevicemodel"
> > +    xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"
> 
> I think we want to check if "xentoolcore" pkg_config exist before trying
> to use it. Otherwith, that is not going to work with Xen older than
> 4.10.

Yes.  Thanks for spotting this.  My tests were all with builds from in
the Xen tree and I hadn't spotted that that is handled as a special
case.

> Instead, we could do this:
> if $pkg_config --exists xentoolcore; then
>   xen_pc+=" xentoolcore"
> fi

I will test that.

Ian.

Re: [PATCH 03/16] xen: link against xentoolcore

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 03/16] xen: link against xentoolcore"):
> On Thu, Apr 19, 2018 at 05:45:06PM +0100, Ian Jackson wrote:
> >      xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
> > -    xen_pc="$xen_pc xenevtchn xendevicemodel"
> > +    xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"
> 
> I think we want to check if "xentoolcore" pkg_config exist before trying
> to use it. Otherwith, that is not going to work with Xen older than
> 4.10.

Yes.  Thanks for spotting this.  My tests were all with builds from in
the Xen tree and I hadn't spotted that that is handled as a special
case.

> Instead, we could do this:
> if $pkg_config --exists xentoolcore; then
>   xen_pc+=" xentoolcore"
> fi

I will test that.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Ian Jackson]

Philippe Mathieu-Daudé writes ("Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report"):
> On 04/19/2018 01:45 PM, Ian Jackson wrote:
> > -        perror("mlockall");
> > +        error_report("mlockall: %s", strerror(errno));
> >      }
> >  
> >      return ret;
> 
> Thinking loudly, maybe we can refactor as error_report_errno(const char
> *desc)...

git-grep 'error_report.*errno' shows a lot of call sites that do
something more exciting than const char *desc would support.

I think the right approach would be

 - static void vreport(report_type type, const char *fmt, va_list ap)
 + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
...
 +     if (errnoval >= 0) {
 +         error_printf(": %s", strerror(errnoval);
 +     }

and then add both
  error_report_errno
  error_vreport_errno
with the obvious semantics.

Ian.

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Ian Jackson]

Philippe Mathieu-Daudé writes ("Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report"):
> On 04/19/2018 01:45 PM, Ian Jackson wrote:
> > -        perror("mlockall");
> > +        error_report("mlockall: %s", strerror(errno));
> >      }
> >  
> >      return ret;
> 
> Thinking loudly, maybe we can refactor as error_report_errno(const char
> *desc)...

git-grep 'error_report.*errno' shows a lot of call sites that do
something more exciting than const char *desc would support.

I think the right approach would be

 - static void vreport(report_type type, const char *fmt, va_list ap)
 + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
...
 +     if (errnoval >= 0) {
 +         error_printf(": %s", strerror(errnoval);
 +     }

and then add both
  error_report_errno
  error_vreport_errno
with the obvious semantics.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

Eric Blake writes ("Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash"):
> That's still fork-heavy.  You could do:
> 
> test -n "$BASH_VERSION" && eval '
> echo >>config.log "
> funcs: ${FUNCNAME[*]}
> lines: ${BASH_LINENO[*]}
> files: ${BASH_SOURCE[*]}"'
> 
> which avoids the fork, but remains silent on dash.

Thanks.  I will adopt this.  Although I will use if ... then as that
seems to be the usual style.  (&& would break with set -e which
configure does not use...)

Ian.

Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

Eric Blake writes ("Re: [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash"):
> That's still fork-heavy.  You could do:
> 
> test -n "$BASH_VERSION" && eval '
> echo >>config.log "
> funcs: ${FUNCNAME[*]}
> lines: ${BASH_LINENO[*]}
> files: ${BASH_SOURCE[*]}"'
> 
> which avoids the fork, but remains silent on dash.

Thanks.  I will adopt this.  Although I will use if ... then as that
seems to be the usual style.  (&& would break with set -e which
configure does not use...)

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post"):
> I think this include is not needed anymore, and can go away from the
> patch series.

Yes.

Thanks,
Ian.

Re: [PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 05/16] xen: defer call to xen_restrict until just before os_setup_post"):
> I think this include is not needed anymore, and can go away from the
> patch series.

Yes.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Daniel P. Berrangé]

On Tue, Apr 24, 2018 at 03:53:48PM +0100, Ian Jackson wrote:
> Philippe Mathieu-Daudé writes ("Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report"):
> > On 04/19/2018 01:45 PM, Ian Jackson wrote:
> > > -        perror("mlockall");
> > > +        error_report("mlockall: %s", strerror(errno));
> > >      }
> > >  
> > >      return ret;
> > 
> > Thinking loudly, maybe we can refactor as error_report_errno(const char
> > *desc)...
> 
> git-grep 'error_report.*errno' shows a lot of call sites that do
> something more exciting than const char *desc would support.
> 
> I think the right approach would be
> 
>  - static void vreport(report_type type, const char *fmt, va_list ap)
>  + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
> ...
>  +     if (errnoval >= 0) {
>  +         error_printf(": %s", strerror(errnoval);
>  +     }
> 
> and then add both
>   error_report_errno
>   error_vreport_errno
> with the obvious semantics.

That would be nice, because then we can make these two functions actually
use strerror_r() instead of strerror(), for thread safety on all platforms.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Daniel P. Berrangé]

On Tue, Apr 24, 2018 at 03:53:48PM +0100, Ian Jackson wrote:
> Philippe Mathieu-Daudé writes ("Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report"):
> > On 04/19/2018 01:45 PM, Ian Jackson wrote:
> > > -        perror("mlockall");
> > > +        error_report("mlockall: %s", strerror(errno));
> > >      }
> > >  
> > >      return ret;
> > 
> > Thinking loudly, maybe we can refactor as error_report_errno(const char
> > *desc)...
> 
> git-grep 'error_report.*errno' shows a lot of call sites that do
> something more exciting than const char *desc would support.
> 
> I think the right approach would be
> 
>  - static void vreport(report_type type, const char *fmt, va_list ap)
>  + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
> ...
>  +     if (errnoval >= 0) {
>  +         error_printf(": %s", strerror(errnoval);
>  +     }
> 
> and then add both
>   error_report_errno
>   error_vreport_errno
> with the obvious semantics.

That would be nice, because then we can make these two functions actually
use strerror_r() instead of strerror(), for thread safety on all platforms.

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 :|

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote:

>>  - static void vreport(report_type type, const char *fmt, va_list ap)
>>  + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
>> ...
>>  +     if (errnoval >= 0) {
>>  +         error_printf(": %s", strerror(errnoval);
>>  +     }
>>
>> and then add both
>>   error_report_errno
>>   error_vreport_errno
>> with the obvious semantics.
> 
> That would be nice, because then we can make these two functions actually
> use strerror_r() instead of strerror(), for thread safety on all platforms.

Except that strerror_r() is a bear to use portably, given that glibc's
default declaration differs from the POSIX requirement (you can force
glibc to give you the POSIX version, but doing so causes you to lose
access to many other useful extensions).  It's rather telling that 'git
grep strerror_r' currently comes up empty.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Eric Blake]

[-- Attachment #1.1.1: Type: text/plain, Size: 1058 bytes --]

On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote:

>>  - static void vreport(report_type type, const char *fmt, va_list ap)
>>  + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
>> ...
>>  +     if (errnoval >= 0) {
>>  +         error_printf(": %s", strerror(errnoval);
>>  +     }
>>
>> and then add both
>>   error_report_errno
>>   error_vreport_errno
>> with the obvious semantics.
> 
> That would be nice, because then we can make these two functions actually
> use strerror_r() instead of strerror(), for thread safety on all platforms.

Except that strerror_r() is a bear to use portably, given that glibc's
default declaration differs from the POSIX requirement (you can force
glibc to give you the POSIX version, but doing so causes you to lose
access to many other useful extensions).  It's rather telling that 'git
grep strerror_r' currently comes up empty.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On 04/24/2018 10:40 AM, Eric Blake wrote:
> On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote:
> 
>>>  - static void vreport(report_type type, const char *fmt, va_list ap)
>>>  + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
>>> ...
>>>  +     if (errnoval >= 0) {
>>>  +         error_printf(": %s", strerror(errnoval);
>>>  +     }
>>>
>>> and then add both
>>>   error_report_errno
>>>   error_vreport_errno
>>> with the obvious semantics.
>>
>> That would be nice, because then we can make these two functions actually
>> use strerror_r() instead of strerror(), for thread safety on all platforms.
> 
> Except that strerror_r() is a bear to use portably, given that glibc's
> default declaration differs from the POSIX requirement (you can force
> glibc to give you the POSIX version, but doing so causes you to lose
> access to many other useful extensions).  It's rather telling that 'git
> grep strerror_r' currently comes up empty.

That said, glib's g_strerror() may be suitable for this purpose,
although we are currently using it only in tests/ivshmem-test.c.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Eric Blake]

[-- Attachment #1.1.1: Type: text/plain, Size: 1264 bytes --]

On 04/24/2018 10:40 AM, Eric Blake wrote:
> On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote:
> 
>>>  - static void vreport(report_type type, const char *fmt, va_list ap)
>>>  + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
>>> ...
>>>  +     if (errnoval >= 0) {
>>>  +         error_printf(": %s", strerror(errnoval);
>>>  +     }
>>>
>>> and then add both
>>>   error_report_errno
>>>   error_vreport_errno
>>> with the obvious semantics.
>>
>> That would be nice, because then we can make these two functions actually
>> use strerror_r() instead of strerror(), for thread safety on all platforms.
> 
> Except that strerror_r() is a bear to use portably, given that glibc's
> default declaration differs from the POSIX requirement (you can force
> glibc to give you the POSIX version, but doing so causes you to lose
> access to many other useful extensions).  It's rather telling that 'git
> grep strerror_r' currently comes up empty.

That said, glib's g_strerror() may be suitable for this purpose,
although we are currently using it only in tests/ivshmem-test.c.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Daniel P. Berrangé]

On Tue, Apr 24, 2018 at 10:43:09AM -0500, Eric Blake wrote:
> On 04/24/2018 10:40 AM, Eric Blake wrote:
> > On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote:
> > 
> >>>  - static void vreport(report_type type, const char *fmt, va_list ap)
> >>>  + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
> >>> ...
> >>>  +     if (errnoval >= 0) {
> >>>  +         error_printf(": %s", strerror(errnoval);
> >>>  +     }
> >>>
> >>> and then add both
> >>>   error_report_errno
> >>>   error_vreport_errno
> >>> with the obvious semantics.
> >>
> >> That would be nice, because then we can make these two functions actually
> >> use strerror_r() instead of strerror(), for thread safety on all platforms.
> > 
> > Except that strerror_r() is a bear to use portably, given that glibc's
> > default declaration differs from the POSIX requirement (you can force
> > glibc to give you the POSIX version, but doing so causes you to lose
> > access to many other useful extensions).  It's rather telling that 'git
> > grep strerror_r' currently comes up empty.
> 
> That said, glib's g_strerror() may be suitable for this purpose,
> although we are currently using it only in tests/ivshmem-test.c.

Yes, that uses strerror_r internally, or strerror_s on Windows:

https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gstrfuncs.c#L1236


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Daniel P. Berrangé]

On Tue, Apr 24, 2018 at 10:43:09AM -0500, Eric Blake wrote:
> On 04/24/2018 10:40 AM, Eric Blake wrote:
> > On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote:
> > 
> >>>  - static void vreport(report_type type, const char *fmt, va_list ap)
> >>>  + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
> >>> ...
> >>>  +     if (errnoval >= 0) {
> >>>  +         error_printf(": %s", strerror(errnoval);
> >>>  +     }
> >>>
> >>> and then add both
> >>>   error_report_errno
> >>>   error_vreport_errno
> >>> with the obvious semantics.
> >>
> >> That would be nice, because then we can make these two functions actually
> >> use strerror_r() instead of strerror(), for thread safety on all platforms.
> > 
> > Except that strerror_r() is a bear to use portably, given that glibc's
> > default declaration differs from the POSIX requirement (you can force
> > glibc to give you the POSIX version, but doing so causes you to lose
> > access to many other useful extensions).  It's rather telling that 'git
> > grep strerror_r' currently comes up empty.
> 
> That said, glib's g_strerror() may be suitable for this purpose,
> although we are currently using it only in tests/ivshmem-test.c.

Yes, that uses strerror_r internally, or strerror_s on Windows:

https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gstrfuncs.c#L1236


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 :|

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 04/19/2018 01:45 PM, Ian Jackson wrote:
>> perror() is defined to fprintf(stderr,...).  HACKING says
>> fprintf(stderr,...) is wrong.  So perror() is too.
>> 
>> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Daniel P. Berrange <berrange@redhat.com>
>> CC: Michael Tokarev <mjt@tls.msk.ru>
>> CC: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> v7: New patch
>> ---
>>  os-posix.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/os-posix.c b/os-posix.c
>> index d4cf466..0108028 100644
>> --- a/os-posix.c
>> +++ b/os-posix.c
>> @@ -125,7 +125,7 @@ void os_set_proc_name(const char *s)
>>      /* Could rewrite argv[0] too, but that's a bit more complicated.
>>         This simple way is enough for `top'. */
>>      if (prctl(PR_SET_NAME, name)) {
>> -        perror("unable to change process name");
>> +        error_report("unable to change process name: %s", strerror(errno));
>>          exit(1);
>>      }
>>  #else
>> @@ -247,7 +247,7 @@ static void change_root(void)
>>              exit(1);
>>          }
>>          if (chdir("/")) {
>> -            perror("not able to chdir to /");
>> +            error_report("not able to chdir to /: %s", strerror(errno));
>>              exit(1);
>>          }
>>      }
>> @@ -309,7 +309,7 @@ void os_setup_post(void)
>>  
>>      if (daemonize) {
>>          if (chdir("/")) {
>> -            perror("not able to chdir to /");
>> +            error_report("not able to chdir to /: %s", strerror(errno));
>>              exit(1);
>>          }
>>          TFR(fd = qemu_open("/dev/null", O_RDWR));
>> @@ -383,7 +383,7 @@ int os_mlock(void)
>>  
>>      ret = mlockall(MCL_CURRENT | MCL_FUTURE);
>>      if (ret < 0) {
>> -        perror("mlockall");
>> +        error_report("mlockall: %s", strerror(errno));
>>      }
>>  
>>      return ret;
>
> Thinking loudly, maybe we can refactor as error_report_errno(const char
> *desc)...

Maybe.  If you want to try, make it a separate series not blocking this
one.

> Anyway:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 04/19/2018 01:45 PM, Ian Jackson wrote:
>> perror() is defined to fprintf(stderr,...).  HACKING says
>> fprintf(stderr,...) is wrong.  So perror() is too.
>> 
>> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Daniel P. Berrange <berrange@redhat.com>
>> CC: Michael Tokarev <mjt@tls.msk.ru>
>> CC: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> v7: New patch
>> ---
>>  os-posix.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/os-posix.c b/os-posix.c
>> index d4cf466..0108028 100644
>> --- a/os-posix.c
>> +++ b/os-posix.c
>> @@ -125,7 +125,7 @@ void os_set_proc_name(const char *s)
>>      /* Could rewrite argv[0] too, but that's a bit more complicated.
>>         This simple way is enough for `top'. */
>>      if (prctl(PR_SET_NAME, name)) {
>> -        perror("unable to change process name");
>> +        error_report("unable to change process name: %s", strerror(errno));
>>          exit(1);
>>      }
>>  #else
>> @@ -247,7 +247,7 @@ static void change_root(void)
>>              exit(1);
>>          }
>>          if (chdir("/")) {
>> -            perror("not able to chdir to /");
>> +            error_report("not able to chdir to /: %s", strerror(errno));
>>              exit(1);
>>          }
>>      }
>> @@ -309,7 +309,7 @@ void os_setup_post(void)
>>  
>>      if (daemonize) {
>>          if (chdir("/")) {
>> -            perror("not able to chdir to /");
>> +            error_report("not able to chdir to /: %s", strerror(errno));
>>              exit(1);
>>          }
>>          TFR(fd = qemu_open("/dev/null", O_RDWR));
>> @@ -383,7 +383,7 @@ int os_mlock(void)
>>  
>>      ret = mlockall(MCL_CURRENT | MCL_FUTURE);
>>      if (ret < 0) {
>> -        perror("mlockall");
>> +        error_report("mlockall: %s", strerror(errno));
>>      }
>>  
>>      return ret;
>
> Thinking loudly, maybe we can refactor as error_report_errno(const char
> *desc)...

Maybe.  If you want to try, make it a separate series not blocking this
one.

> Anyway:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel