broken live migration in xen 4.3 (release)

broken live migration in xen 4.3 (release)

[Vasiliy Tolstov]

Hello all. I'm try to test xen 4.3 release (dom0 kernel 3.10)
All works fine, except live migration. On 4.2.1 all works fine, but
now i have errors like:

migration target: Ready to receive domain.
Saving to migration stream new xl format (info 0x0/0x0/730)
Loading new save file <incoming migration stream> (new xl fmt info 0x0/0x0/730)
Savefile contains xl domain config
xc: progress: Reloading memory pages: 13312/262144 5%
xc: progress: Reloading memory pages: 26624/262144 10%
xc: progress: Reloading memory pages: 39936/262144 15%
xc: progress: Reloading memory pages: 53248/262144 20%
xc: progress: Reloading memory pages: 65536/262144 25%
xc: progress: Reloading memory pages: 78848/262144 30%
xc: progress: Reloading memory pages: 92160/262144 35%
xc: progress: Reloading memory pages: 105472/262144 40%
xc: progress: Reloading memory pages: 118784/262144 45%
xc: progress: Reloading memory pages: 131072/262144 50%
xc: progress: Reloading memory pages: 144384/262144 55%
xc: progress: Reloading memory pages: 157696/262144 60%
xc: progress: Reloading memory pages: 171008/262144 65%
xc: progress: Reloading memory pages: 184320/262144 70%
xc: progress: Reloading memory pages: 196608/262144 75%
xc: progress: Reloading memory pages: 209920/262144 80%
xc: progress: Reloading memory pages: 223232/262144 85%
xc: progress: Reloading memory pages: 236544/262144 90%
xc: progress: Reloading memory pages: 249856/262144 95%
migration target: Transfer complete, requesting permission to start domain.
migration receiver stream contained unexpected data instead of ready message
(command run was: exec ssh ib-xen12 xl migrate-receive )
libxl: error: libxl_utils.c:393:libxl_read_exactly: file/stream
truncated reading GO message from migration stream
migration target: Failure, destroying our copy.
migration child [20864] not exiting, no longer waiting (exit status
will be unreported)
Migration failed, resuming at sender.

Guest domain kernel panics (timekeeping errors).
What can i check? Live migration to localhost also fails.
-- 
Vasiliy Tolstov,
e-mail: v.tolstov@selfip.ru
jabber: vase@selfip.ru

Re: broken live migration in xen 4.3 (release)

[Ian Campbell]

On Mon, 2014-03-17 at 16:28 +0400, Vasiliy Tolstov wrote:
> Hello all. I'm try to test xen 4.3 release

Please can you confirm with the latest 4.3.x (which is .2 I think).

Is this migration between two 4.3 hosts or from a 4.2 host to a 4.3 one?

>  (dom0 kernel 3.10)
> All works fine, except live migration. On 4.2.1 all works fine, but
> now i have errors like:
> 
> migration target: Ready to receive domain.
> Saving to migration stream new xl format (info 0x0/0x0/730)
> Loading new save file <incoming migration stream> (new xl fmt info 0x0/0x0/730)
> Savefile contains xl domain config
> xc: progress: Reloading memory pages: 13312/262144 5%
> xc: progress: Reloading memory pages: 26624/262144 10%
> xc: progress: Reloading memory pages: 39936/262144 15%
> xc: progress: Reloading memory pages: 53248/262144 20%
> xc: progress: Reloading memory pages: 65536/262144 25%
> xc: progress: Reloading memory pages: 78848/262144 30%
> xc: progress: Reloading memory pages: 92160/262144 35%
> xc: progress: Reloading memory pages: 105472/262144 40%
> xc: progress: Reloading memory pages: 118784/262144 45%
> xc: progress: Reloading memory pages: 131072/262144 50%
> xc: progress: Reloading memory pages: 144384/262144 55%
> xc: progress: Reloading memory pages: 157696/262144 60%
> xc: progress: Reloading memory pages: 171008/262144 65%
> xc: progress: Reloading memory pages: 184320/262144 70%
> xc: progress: Reloading memory pages: 196608/262144 75%
> xc: progress: Reloading memory pages: 209920/262144 80%
> xc: progress: Reloading memory pages: 223232/262144 85%
> xc: progress: Reloading memory pages: 236544/262144 90%
> xc: progress: Reloading memory pages: 249856/262144 95%
> migration target: Transfer complete, requesting permission to start domain.
> migration receiver stream contained unexpected data instead of ready message
> (command run was: exec ssh ib-xen12 xl migrate-receive )

Since the stream is encrypted it's hard to debug. Perhaps you could add
debugging to migrate_read_fixedmessage in xl_cmdimpl.c to print the
expected and actual messages?

My vague guess is that the unexpected data will say something like
"Segmentation Fault".

> libxl: error: libxl_utils.c:393:libxl_read_exactly: file/stream
> truncated reading GO message from migration stream
> migration target: Failure, destroying our copy.
> migration child [20864] not exiting, no longer waiting (exit status
> will be unreported)
> Migration failed, resuming at sender.
> 
> Guest domain kernel panics (timekeeping errors).
> What can i check? Live migration to localhost also fails.

Re: broken live migration in xen 4.3 (release)

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] broken live migration in xen 4.3 (release)"):
> On Mon, 2014-03-17 at 16:28 +0400, Vasiliy Tolstov wrote:
> > Hello all. I'm try to test xen 4.3 release
> 
> Please can you confirm with the latest 4.3.x (which is .2 I think).
> 
> Is this migration between two 4.3 hosts or from a 4.2 host to a 4.3 one?
...
> > migration target: Transfer complete, requesting permission to start domain.
> > migration receiver stream contained unexpected data instead of ready message
> > (command run was: exec ssh ib-xen12 xl migrate-receive )
> 
> Since the stream is encrypted it's hard to debug. Perhaps you could add
> debugging to migrate_read_fixedmessage in xl_cmdimpl.c to print the
> expected and actual messages?

That is a good idea.

If you say
   ssh ib-xen12 echo hi
does it print anything except just "hi" ?

If so then that is the problem.

> My vague guess is that the unexpected data will say something like
> "Segmentation Fault".

That would go to stderr.

> > libxl: error: libxl_utils.c:393:libxl_read_exactly: file/stream
> > truncated reading GO message from migration stream
> > migration target: Failure, destroying our copy.
> > migration child [20864] not exiting, no longer waiting (exit status
> > will be unreported)
> > Migration failed, resuming at sender.
> > 
> > Guest domain kernel panics (timekeeping errors).
> > What can i check? Live migration to localhost also fails.

That the abortive migration breaks the guest is probably a bug in the
guest.

Ian.

Re: broken live migration in xen 4.3 (release)

[Vasiliy Tolstov]

2014-03-17 19:45 GMT+04:00 Ian Jackson <Ian.Jackson@eu.citrix.com>:
> If you say
>    ssh ib-xen12 echo hi
> does it print anything except just "hi" ?
>

No. hi displayed and nothing more.

> If so then that is the problem.
>
>> My vague guess is that the unexpected data will say something like
>> "Segmentation Fault".
>
> That would go to stderr.
>
>> > libxl: error: libxl_utils.c:393:libxl_read_exactly: file/stream
>> > truncated reading GO message from migration stream
>> > migration target: Failure, destroying our copy.
>> > migration child [20864] not exiting, no longer waiting (exit status
>> > will be unreported)
>> > Migration failed, resuming at sender.
>> >
>> > Guest domain kernel panics (timekeeping errors).
>> > What can i check? Live migration to localhost also fails.
>
> That the abortive migration breaks the guest is probably a bug in the
> guest.


No, this error i'm already see in 4.0.1 (exactly) and it not
reproduced in xm toolstack.
And this domU kernels works fine on 4.1.2.
Guest panics via:
[ 1621.056898] kernel BUG at drivers/xen/events.c:1281!
[ 1621.056898] invalid opcode: 0000 [#1] SMP
[ 1621.056898] last sysfs file: /sys/devices/virtual/vc/vcsa4/uevent
[ 1621.056898] CPU 0
[ 1621.056898] Modules linked in:
[ 1621.056898] Pid: 2601, comm: kstop/0 Not tainted 2.6.32.26 #1
[ 1621.056898] RIP: e030:[<ffffffff812ea748>]  [<ffffffff812ea748>]
xen_irq_resume+0xc8/0x284
[ 1621.056898] RSP: e02b:ffff88001e38fde0  EFLAGS: 00010082
[ 1621.056898] RAX: ffffffffffffffef RBX: 000000000000022f RCX: 0000000000000001
[ 1621.056898] RDX: 0000000000000000 RSI: ffff88001e38fdf0 RDI: 0000000000000001
[ 1621.056898] RBP: ffff88001e38fe30 R08: 0000000000000040 R09: ffffffff81595ab8
[ 1621.056898] R10: 0000000000000000 R11: ffff88001e38fdf0 R12: ffff88001fde9e6c
[ 1621.056898] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 1621.056898] FS:  00007f81c364c700(0000) GS:ffff880003ce6000(0000)
knlGS:0000000000000000
[ 1621.056898] CS:  e033 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1621.056898] CR2: 00007f3ded29efa0 CR3: 000000001cfb3000 CR4: 0000000000002660
[ 1621.056898] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1621.056898] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 1621.056898] Process kstop/0 (pid: 2601, threadinfo
ffff88001e38e000, task ffff88001f74a170)
[ 1621.056898] Stack:
[ 1621.056898]  ffffffff812e89a3 0000000000000000 0000000000000000
ffffffff8100e89f
[ 1621.056898] <0> ffffffff813e3931 0000000000000000 ffff88001fde9e6c
ffff880003d00a80
[ 1621.056898] <0> ffffffff810790bb 0000000000000000 ffff88001e38fe50
ffffffff812eb62d
[ 1621.056898] Call Trace:
[ 1621.056898]  [<ffffffff812e89a3>] ? __max_nr_grant_frames+0x21/0x38
[ 1621.056898]  [<ffffffff8100e89f>] ? xen_restore_fl_direct_end+0x0/0x1
[ 1621.056898]  [<ffffffff813e3931>] ? _spin_unlock_irqrestore+0x19/0x1c
[ 1621.056898]  [<ffffffff810790bb>] ? stop_cpu+0x0/0xcd
[ 1621.056898]  [<ffffffff812eb62d>] xen_suspend+0xb8/0xce
[ 1621.056898]  [<ffffffff81079142>] stop_cpu+0x87/0xcd
[ 1621.056898]  [<ffffffff8105425b>] worker_thread+0x120/0x1c6
[ 1621.056898]  [<ffffffff81056eff>] ? autoremove_wake_function+0x0/0x38
[ 1621.056898]  [<ffffffff8105413b>] ? worker_thread+0x0/0x1c6
[ 1621.056898]  [<ffffffff81056b35>] kthread+0x69/0x71
[ 1621.056898]  [<ffffffff8101228a>] child_rip+0xa/0x20
[ 1621.056898]  [<ffffffff81011461>] ? int_ret_from_sys_call+0x7/0x1b
[ 1621.056898]  [<ffffffff81011c1d>] ? retint_restore_args+0x5/0x6
[ 1621.056898]  [<ffffffff81012280>] ? child_rip+0x0/0x20
[ 1621.056898] Code: e8 73 f0 ff ff 41 89 c6 44 39 f8 74 04 0f 0b eb
fe 44 89 7d c0 44 89 6d c4 48 8d 75 c0 bf 01 00 00 00 e8 f2 f0 ff ff
85 c0 74 04 <0f> 0b eb fe 44 8b
65 c8 44 89 f6 41 0f b7 fc 49 63 c4 48 c1 e0


-- 
Vasiliy Tolstov,
e-mail: v.tolstov@selfip.ru
jabber: vase@selfip.ru

Re: broken live migration in xen 4.3 (release)

[Vasiliy Tolstov]

2014-03-17 16:58 GMT+04:00 Ian Campbell <Ian.Campbell@citrix.com>:
> Please can you confirm with the latest 4.3.x (which is .2 I think).
>
> Is this migration between two 4.3 hosts or from a 4.2 host to a 4.3 one?


I confirm, in 4.3.2 migration also failed. I try to migrate from 4.3.2
to 4.3.2 same cpu, kernels and hardware. I'm try to debug libxl...

-- 
Vasiliy Tolstov,
e-mail: v.tolstov@selfip.ru
jabber: vase@selfip.ru

Re: broken live migration in xen 4.3 (release)

[Vasiliy Tolstov]

2014-03-18 2:07 GMT+04:00 Vasiliy Tolstov <v.tolstov@selfip.ru>:
> 2014-03-17 16:58 GMT+04:00 Ian Campbell <Ian.Campbell@citrix.com>:
>> Please can you confirm with the latest 4.3.x (which is .2 I think).
>>
>> Is this migration between two 4.3 hosts or from a 4.2 host to a 4.3 one?


As i see in dom0 xen logs i get error like:
(XEN) event_channel.c:287:d4 EVTCHNOP failure: error -17

-- 
Vasiliy Tolstov,
e-mail: v.tolstov@selfip.ru
jabber: vase@selfip.ru

Re: broken live migration in xen 4.3 (release)

[Vasiliy Tolstov]

I'm solve my issue. /etc/xen/scripts/vif-ospf (my own network script)
have which ethtool not redirected to /dev/null.
When domain started i don't have errors, but when migration appeared
last message has been /sbin/ethtool. Thanks Ian Campbell and Ian
Jackson!


2014-03-18 3:09 GMT+04:00 Vasiliy Tolstov <v.tolstov@selfip.ru>:
> 2014-03-18 2:07 GMT+04:00 Vasiliy Tolstov <v.tolstov@selfip.ru>:
>> 2014-03-17 16:58 GMT+04:00 Ian Campbell <Ian.Campbell@citrix.com>:
>>> Please can you confirm with the latest 4.3.x (which is .2 I think).
>>>
>>> Is this migration between two 4.3 hosts or from a 4.2 host to a 4.3 one?
>
>
> As i see in dom0 xen logs i get error like:
> (XEN) event_channel.c:287:d4 EVTCHNOP failure: error -17
>
> --
> Vasiliy Tolstov,
> e-mail: v.tolstov@selfip.ru
> jabber: vase@selfip.ru



-- 
Vasiliy Tolstov,
e-mail: v.tolstov@selfip.ru
jabber: vase@selfip.ru

Re: broken live migration in xen 4.3 (release)

[Ian Campbell]

On Tue, 2014-03-18 at 03:25 +0400, Vasiliy Tolstov wrote:
> I'm solve my issue. /etc/xen/scripts/vif-ospf (my own network script)
> have which ethtool not redirected to /dev/null.
> When domain started i don't have errors, but when migration appeared
> last message has been /sbin/ethtool. Thanks Ian Campbell and Ian
> Jackson!

Great.

Did you by any chance produce a debug patch to help you track this down
(e.g. to log the unexpected message)? If so then that might be something
which is useful to others diagnosing the same issue -- would you mind
submitting it? See http://wiki.xen.org/wiki/Submitting_Xen_Patches for
guidance. (Even if it is unsuitable for applying it would perhaps be
helpful to have in the list archives to point at)

Roger, where should the hotplug scripts be logging to? Should we
consider slurping up stdout/stderr of the script to somewhere other than
xl's std*?

Ian.

Re: broken live migration in xen 4.3 (release)

[Roger Pau Monné]

On 18/03/14 10:36, Ian Campbell wrote:
> On Tue, 2014-03-18 at 03:25 +0400, Vasiliy Tolstov wrote:
>> I'm solve my issue. /etc/xen/scripts/vif-ospf (my own network script)
>> have which ethtool not redirected to /dev/null.
>> When domain started i don't have errors, but when migration appeared
>> last message has been /sbin/ethtool. Thanks Ian Campbell and Ian
>> Jackson!
> 
> Great.
> 
> Did you by any chance produce a debug patch to help you track this down
> (e.g. to log the unexpected message)? If so then that might be something
> which is useful to others diagnosing the same issue -- would you mind
> submitting it? See http://wiki.xen.org/wiki/Submitting_Xen_Patches for
> guidance. (Even if it is unsuitable for applying it would perhaps be
> helpful to have in the list archives to point at)
> 
> Roger, where should the hotplug scripts be logging to? Should we
> consider slurping up stdout/stderr of the script to somewhere other than
> xl's std*?

IMHO I like hotplug messages to be printed on screen while using xl
create/destroy... But for migration it should be redirected to /dev/null
or some suitable logfile I guess.

Or maybe a more canonical way to deal with this would be to always
redirect the output of hotplug scripts to a temp file, and in case of
not doing a migration print the contents of the file after executing the
hotplug script.

Roger.

Re: broken live migration in xen 4.3 (release)

[Vasiliy Tolstov]

2014-03-18 13:36 GMT+04:00 Ian Campbell <Ian.Campbell@citrix.com>:
> Did you by any chance produce a debug patch to help you track this down
> (e.g. to log the unexpected message)? If so then that might be something
> which is useful to others diagnosing the same issue -- would you mind
> submitting it? See http://wiki.xen.org/wiki/Submitting_Xen_Patches for
> guidance. (Even if it is unsuitable for applying it would perhaps be
> helpful to have in the list archives to point at)

I create simple patch :

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index a7f888c..6d87afe 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3347,8 +3347,8 @@ static int migrate_read_fixedmessage(int fd,
const void *msg, int msgsz,
     if (rc) return ERROR_FAIL;

     if (memcmp(buf, msg, msgsz)) {
-        fprintf(stderr, "%s contained unexpected data instead of %s\n",
-                stream, what);
+        fprintf(stderr, "%s contained unexpected data instead of %s
buf %s msg %s\n",
+                stream, what, buf, (char*) msg);
         if (rune)
             fprintf(stderr, "(command run was: %s )\n", rune);
         return ERROR_FAIL;

I don't known does it need to be in xen repo?

-- 
Vasiliy Tolstov,
e-mail: v.tolstov@selfip.ru
jabber: vase@selfip.ru

Re: broken live migration in xen 4.3 (release)

[Ian Campbell]

On Tue, 2014-03-18 at 10:49 +0100, Roger Pau Monné wrote:
> On 18/03/14 10:36, Ian Campbell wrote:
> > On Tue, 2014-03-18 at 03:25 +0400, Vasiliy Tolstov wrote:
> >> I'm solve my issue. /etc/xen/scripts/vif-ospf (my own network script)
> >> have which ethtool not redirected to /dev/null.
> >> When domain started i don't have errors, but when migration appeared
> >> last message has been /sbin/ethtool. Thanks Ian Campbell and Ian
> >> Jackson!
> > 
> > Great.
> > 
> > Did you by any chance produce a debug patch to help you track this down
> > (e.g. to log the unexpected message)? If so then that might be something
> > which is useful to others diagnosing the same issue -- would you mind
> > submitting it? See http://wiki.xen.org/wiki/Submitting_Xen_Patches for
> > guidance. (Even if it is unsuitable for applying it would perhaps be
> > helpful to have in the list archives to point at)
> > 
> > Roger, where should the hotplug scripts be logging to? Should we
> > consider slurping up stdout/stderr of the script to somewhere other than
> > xl's std*?
> 
> IMHO I like hotplug messages to be printed on screen while using xl
> create/destroy... But for migration it should be redirected to /dev/null
> or some suitable logfile I guess.

This could perhaps be gated on stdfoo passing isatty?

> Or maybe a more canonical way to deal with this would be to always
> redirect the output of hotplug scripts to a temp file, and in case of
> not doing a migration print the contents of the file after executing the
> hotplug script.

I think it should always be logged regardless of whether it is also
going to std*.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: broken live migration in xen 4.3 (release)

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] broken live migration in xen 4.3 (release)"):
> On Tue, 2014-03-18 at 10:49 +0100, Roger Pau Monné wrote:
> > IMHO I like hotplug messages to be printed on screen while using xl
> > create/destroy... But for migration it should be redirected to /dev/null
> > or some suitable logfile I guess.
> 
> This could perhaps be gated on stdfoo passing isatty?

Surely there is no excuse for these scripts sending output to libxl's
caller's stdout.  stderr is another matter.

So I think libxl should redirect hotplug scripts's stdout to stderr.
If it had done this then Vasiliy would have seen the message on stderr
rather than it being mixed up with the migration stream.

Ian.

Re: broken live migration in xen 4.3 (release)

[Ian Campbell]

On Tue, 2014-03-18 at 16:13 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] broken live migration in xen 4.3 (release)"):
> > On Tue, 2014-03-18 at 10:49 +0100, Roger Pau Monné wrote:
> > > IMHO I like hotplug messages to be printed on screen while using xl
> > > create/destroy... But for migration it should be redirected to /dev/null
> > > or some suitable logfile I guess.
> > 
> > This could perhaps be gated on stdfoo passing isatty?
> 
> Surely there is no excuse for these scripts sending output to libxl's
> caller's stdout.  stderr is another matter.

yes. I was thinking this was an xl level thing but of course it is a
libxl thing.

> So I think libxl should redirect hotplug scripts's stdout to stderr.
> If it had done this then Vasiliy would have seen the message on stderr
> rather than it being mixed up with the migration stream.

This sounds right and also sounds like it should be almost trivial to
arrange.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 1/3] libxl: Make libxl_exec tolerate foofd<=2

[Ian Jackson]

Make passing 0, 1, or 2 as stdinfd, stdoutfd or stderrfd work
properly.

Also, document the meaning of the fd arguments.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
CC: Vasiliy Tolstov <v.tolstov@selfip.ru>
CC: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/libxl/libxl_exec.c     |    6 +++---
 tools/libxl/libxl_internal.h |    4 ++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 4b012dc..478b4c2 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -77,11 +77,11 @@ void libxl__exec(libxl__gc *gc, int stdinfd, int stdoutfd, int stderrfd,
     if (stderrfd != -1)
         dup2(stderrfd, STDERR_FILENO);
 
-    if (stdinfd != -1)
+    if (stdinfd > 2)
         close(stdinfd);
-    if (stdoutfd != -1 && stdoutfd != stdinfd)
+    if (stdoutfd > 2 && stdoutfd != stdinfd)
         close(stdoutfd);
-    if (stderrfd != -1 && stderrfd != stdinfd && stderrfd != stdoutfd)
+    if (stderrfd > 2 && stderrfd != stdinfd && stderrfd != stdoutfd)
         close(stderrfd);
 
     check_open_fds(arg0);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 69d2ba8..6efaab8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1418,6 +1418,10 @@ _hidden int libxl__xenstore_child_wait_deprecated(libxl__gc *gc,
  *
  * The last entry of the array always has to be NULL.
  *
+ * stdinfd, stdoutfd, stderrfd will be dup2'd onto the corresponding
+ * fd in the child, if they are not -1.  The original copy of the
+ * descriptor will be closed in the child (unless it's 0, 1 or 2).
+ *
  * Logs errors, never returns.
  */
 _hidden  void libxl__exec(libxl__gc *gc, int stdinfd, int stdoutfd,
-- 
1.7.10.4

[PATCH 2/3] libxl: hotplug scripts: stdout >& stderr

[Ian Jackson]

Plumb hotplug scripts' stdout to stderr.  That way if they print
anything (which really they shouldn't), it won't get mixed up with
the application's stdout.  (Eg, perhaps with an xl migration
stream...)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Vasiliy Tolstov <v.tolstov@selfip.ru>
CC: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/libxl/libxl_device.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index ba7d100..90f76b7 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1031,7 +1031,7 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
 
     if (!pid) {
         /* child */
-        libxl__exec(gc, -1, -1, -1, args[0], args, env);
+        libxl__exec(gc, -1, 2, -1, args[0], args, env);
         /* notreached */
         abort();
     }
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3/3] libxl: hotplug scripts: stdin < /dev/null

[Ian Jackson]

Give hotplug scripts /dev/null for stdin.  That way if they try read
anything anything (which really they shouldn't), nothing odd will
happen.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
CC: Vasiliy Tolstov <v.tolstov@selfip.ru>
CC: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/libxl/libxl_device.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 90f76b7..fa99f77 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -956,7 +956,7 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
     char *be_path = libxl__device_backend_path(gc, aodev->dev);
     char **args = NULL, **env = NULL;
     int rc = 0;
-    int hotplug;
+    int hotplug, nullfd = -1;
     pid_t pid;
     uint32_t domid;
 
@@ -1021,6 +1021,13 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
     aodev->what = GCSPRINTF("%s %s", args[0], args[1]);
     LOG(DEBUG, "calling hotplug script: %s %s", args[0], args[1]);
 
+    nullfd = open("/dev/null", O_RDONLY);
+    if (nullfd < 0) {
+        LOG(ERROR, "unable to open /dev/null for hotplug script");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     /* fork and execute hotplug script */
     pid = libxl__ev_child_fork(gc, &aodev->child, device_hotplug_child_death_cb);
     if (pid == -1) {
@@ -1031,16 +1038,18 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
 
     if (!pid) {
         /* child */
-        libxl__exec(gc, -1, 2, -1, args[0], args, env);
+        libxl__exec(gc, nullfd, 2, -1, args[0], args, env);
         /* notreached */
         abort();
     }
 
+    close(nullfd);
     assert(libxl__ev_child_inuse(&aodev->child));
 
     return;
 
 out:
+    if (nullfd >= 0) close(nullfd);
     aodev->rc = rc;
     device_hotplug_done(egc, aodev);
     return;
-- 
1.7.10.4

Re: [PATCH 1/3] libxl: Make libxl_exec tolerate foofd<=2

[Ian Campbell]

On Tue, 2014-03-18 at 17:08 +0000, Ian Jackson wrote:
> Make passing 0, 1, or 2 as stdinfd, stdoutfd or stderrfd work
> properly.
> 
> Also, document the meaning of the fd arguments.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> CC: Vasiliy Tolstov <v.tolstov@selfip.ru>

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

(consider s/2/STDERR_FILENO/ perhaps though?)

> + * stdinfd, stdoutfd, stderrfd will be dup2'd onto the corresponding
> + * fd in the child, if they are not -1.  The original copy of the
> + * descriptor will be closed in the child (unless it's 0, 1 or 2).

consider explicit mention of STD{IN,OUT,ERR} for the hard of
understanding?

Ian.

Re: [PATCH 2/3] libxl: hotplug scripts: stdout >& stderr

[Ian Campbell]

On Tue, 2014-03-18 at 17:08 +0000, Ian Jackson wrote:
> Plumb hotplug scripts' stdout to stderr.  That way if they print
> anything (which really they shouldn't), it won't get mixed up with
> the application's stdout.  (Eg, perhaps with an xl migration
> stream...)
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Vasiliy Tolstov <v.tolstov@selfip.ru>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> ---
>  tools/libxl/libxl_device.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index ba7d100..90f76b7 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -1031,7 +1031,7 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
>  
>      if (!pid) {
>          /* child */
> -        libxl__exec(gc, -1, -1, -1, args[0], args, env);
> +        libxl__exec(gc, -1, 2, -1, args[0], args, env);

Do you think we should also put the script's stderr to the application's
stderr?

Acked-by: Ian Campbell <ian.campbell@citrix.com>

as it stands.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] libxl: hotplug scripts: stdin < /dev/null

[Ian Campbell]

On Tue, 2014-03-18 at 17:08 +0000, Ian Jackson wrote:
> Give hotplug scripts /dev/null for stdin.  That way if they try read
> anything anything (which really they shouldn't), nothing odd will
> happen.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> CC: Vasiliy Tolstov <v.tolstov@selfip.ru>

Assuming that the decision to not use the carefd stuff was deliberate
(because we don't care about things inheriting a /dev/null fd)

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

Re: [PATCH 2/3] libxl: hotplug scripts: stdout >& stderr

[Andrew Cooper]

On 18/03/14 17:44, Ian Campbell wrote:
> On Tue, 2014-03-18 at 17:08 +0000, Ian Jackson wrote:
>> Plumb hotplug scripts' stdout to stderr.  That way if they print
>> anything (which really they shouldn't), it won't get mixed up with
>> the application's stdout.  (Eg, perhaps with an xl migration
>> stream...)
>>
>> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Vasiliy Tolstov <v.tolstov@selfip.ru>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> ---
>>  tools/libxl/libxl_device.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index ba7d100..90f76b7 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -1031,7 +1031,7 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
>>  
>>      if (!pid) {
>>          /* child */
>> -        libxl__exec(gc, -1, -1, -1, args[0], args, env);
>> +        libxl__exec(gc, -1, 2, -1, args[0], args, env);
> Do you think we should also put the script's stderr to the application's
> stderr?
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> as it stands.
>
> Ian.

Absolutely - anything on any stderr really needs to get to logs and
probably also to a human.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/3] libxl: hotplug scripts: stdout >& stderr

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 2/3] libxl: hotplug scripts: stdout >& stderr"):
> On Tue, 2014-03-18 at 17:08 +0000, Ian Jackson wrote:
> > Plumb hotplug scripts' stdout to stderr.  That way if they print
> > anything (which really they shouldn't), it won't get mixed up with
> > the application's stdout.  (Eg, perhaps with an xl migration
> > stream...)
...
> >      if (!pid) {
> >          /* child */
> > -        libxl__exec(gc, -1, -1, -1, args[0], args, env);
> > +        libxl__exec(gc, -1, 2, -1, args[0], args, env);
> 
> Do you think we should also put the script's stderr to the application's
> stderr?

I don't understand what you mean.  The scripts already inherit libxl's
stderr.  Passing 2 as stderrfd would have no effect (libxl__exec would
make a no-op call to dup2).

There is a bunch of stuff in the scripts themselves to try to do
logging, which is mad and a swamp I'm choosing not to drain right now.

> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks,
Ian.

Re: [PATCH 3/3] libxl: hotplug scripts: stdin < /dev/null

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 3/3] libxl: hotplug scripts: stdin < /dev/null"):
> On Tue, 2014-03-18 at 17:08 +0000, Ian Jackson wrote:
> > Give hotplug scripts /dev/null for stdin.  That way if they try read
> > anything anything (which really they shouldn't), nothing odd will
> > happen.
...
> Assuming that the decision to not use the carefd stuff was deliberate
> (because we don't care about things inheriting a /dev/null fd)

Indeed.

> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

Thanks,
Ian.

Re: [PATCH 1/3] libxl: Make libxl_exec tolerate foofd<=2

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/3] libxl: Make libxl_exec tolerate foofd<=2"):
> On Tue, 2014-03-18 at 17:08 +0000, Ian Jackson wrote:
> > Make passing 0, 1, or 2 as stdinfd, stdoutfd or stderrfd work
> > properly.
> > 
> > Also, document the meaning of the fd arguments.
...
> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
> 
> (consider s/2/STDERR_FILENO/ perhaps though?)

Urgh.  I detest STDERR_FILENO.  As if it would ever change.

> > + * stdinfd, stdoutfd, stderrfd will be dup2'd onto the corresponding
> > + * fd in the child, if they are not -1.  The original copy of the
> > + * descriptor will be closed in the child (unless it's 0, 1 or 2).
> 
> consider explicit mention of STD{IN,OUT,ERR} for the hard of
> understanding?

I'll do that.

Thanks,
Ian.

Re: [PATCH 2/3] libxl: hotplug scripts: stdout >& stderr

[Ian Campbell]

On Tue, 2014-03-18 at 18:12 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 2/3] libxl: hotplug scripts: stdout >& stderr"):
> > On Tue, 2014-03-18 at 17:08 +0000, Ian Jackson wrote:
> > > Plumb hotplug scripts' stdout to stderr.  That way if they print
> > > anything (which really they shouldn't), it won't get mixed up with
> > > the application's stdout.  (Eg, perhaps with an xl migration
> > > stream...)
> ...
> > >      if (!pid) {
> > >          /* child */
> > > -        libxl__exec(gc, -1, -1, -1, args[0], args, env);
> > > +        libxl__exec(gc, -1, 2, -1, args[0], args, env);
> > 
> > Do you think we should also put the script's stderr to the application's
> > stderr?
> 
> I don't understand what you mean.  The scripts already inherit libxl's
> stderr.  Passing 2 as stderrfd would have no effect (libxl__exec would
> make a no-op call to dup2).

Sorry, I misremembered what -1 meant here.

> There is a bunch of stuff in the scripts themselves to try to do
> logging, which is mad and a swamp I'm choosing not to drain right now.

Right ;-)

> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Thanks,
> Ian.

Re: [PATCH 1/3] libxl: Make libxl_exec tolerate foofd<=2

[Ian Campbell]

On Tue, 2014-03-18 at 18:14 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/3] libxl: Make libxl_exec tolerate foofd<=2"):
> > On Tue, 2014-03-18 at 17:08 +0000, Ian Jackson wrote:
> > > Make passing 0, 1, or 2 as stdinfd, stdoutfd or stderrfd work
> > > properly.
> > > 
> > > Also, document the meaning of the fd arguments.
> ...
> > Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
> > 
> > (consider s/2/STDERR_FILENO/ perhaps though?)
> 
> Urgh.  I detest STDERR_FILENO.  As if it would ever change.

True. I think of it as more a reading comprehension thing, by avoiding
the need to translate {0,1,2} in ones head, which isn't hard but does
introduce a pipeline stall, at least for me...

> > > + * stdinfd, stdoutfd, stderrfd will be dup2'd onto the corresponding
> > > + * fd in the child, if they are not -1.  The original copy of the
> > > + * descriptor will be closed in the child (unless it's 0, 1 or 2).
> > 
> > consider explicit mention of STD{IN,OUT,ERR} for the hard of
> > understanding?
> 
> I'll do that.
> 
> Thanks,
> Ian.

Re: [PATCH 2/3] libxl: hotplug scripts: stdout >& stderr

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 2/3] libxl: hotplug scripts: stdout >& stderr"):
> On Tue, 2014-03-18 at 18:12 +0000, Ian Jackson wrote:
> > I don't understand what you mean.  The scripts already inherit libxl's
> > stderr.  Passing 2 as stderrfd would have no effect (libxl__exec would
> > make a no-op call to dup2).
> 
> Sorry, I misremembered what -1 meant here.

Right.

Thanks for the comments and acks.  I have pushed these three patches.

Ian.

Re: [PATCH 2/3] libxl: hotplug scripts: stdout >& stderr

[Vasiliy Tolstov]

2014-03-19 17:41 GMT+04:00 Ian Jackson <Ian.Jackson@eu.citrix.com>:
>
> Right.
>
> Thanks for the comments and acks.  I have pushed these three patches.


Nice! Thanks

-- 
Vasiliy Tolstov,
e-mail: v.tolstov@selfip.ru
jabber: vase@selfip.ru