diff mbox

[v3,14/15] tools: don't stop xenstore domain when stopping dom0

Message ID 1452258526-4797-15-git-send-email-jgross@suse.com (mailing list archive)
State New, archived
Headers show

Commit Message

Juergen Gross Jan. 8, 2016, 1:08 p.m. UTC
When restarting or shutting down dom0 the xendomains script tries to
stop all other domains. Don't do this for the xenstore domain, as it
might survive a dom0 reboot in the future.

The same applies to xl shutdown --all. Here the xenstore domain is
flagged as "never stop".

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3: xl shutdown modified to query the "never_stop" flag instead of
    the xenstore domain id as requested by Ian Campbell
    drop usage of xenstore-exists as requested by Ian Campbell
---
 tools/hotplug/Linux/xendomains.in | 15 +++++++++++++++
 tools/libxl/xl_cmdimpl.c          |  8 +++++---
 2 files changed, 20 insertions(+), 3 deletions(-)

Comments

Ian Campbell Jan. 15, 2016, 4:12 p.m. UTC | #1
On Fri, 2016-01-08 at 14:08 +0100, Juergen Gross wrote:
> +# get xenstore domain id (or 0 if no xenstore domain)
> +get_xsdomid()
> +{
> +    XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid 2>/dev/null`
> +    if test $? -ne 0; then
> +        XS_DOMID=0
> +    fi

In principal this could be:
+    XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid 2>/dev/null || echo 0`

But I'm far from convinced that is an improvement.

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index f9933cb..29f9d19 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4856,15 +4856,17 @@ static int main_shutdown_or_reboot(int do_reboot,
> int argc, char **argv)
>          if (wait_for_it)
>              deathws = calloc(nb_domain, sizeof(*deathws));
>  
> +        wait_for_it = 0;

Please can we avoid reusing the boolean flag used for argument handling as
a counter.

I think you will need to introduce a new nrdeathws.

I was thinking if you could invert the loop to count downwards such that
you can decrement nb_domain for uninteresting domains, but then the uses of
deathws[i] don't work, the solving of which would no doubt require another
variable anyway.


>          for (i = 0; i<nb_domain; i++) {
> -            if (dominfo[i].domid == 0)
> +            if (dominfo[i].domid == 0 || dominfo[i].never_stop)
>                  continue;
>              fn(dominfo[i].domid, deathws ? &deathws[i] : NULL, i,
>                 fallback_trigger);
> +            wait_for_it++;
>          }
>  
> -        if (wait_for_it) {
> -            wait_for_domain_deaths(deathws, nb_domain - 1 /* not dom 0
> */);
> +        if (deathws) {
> +            wait_for_domain_deaths(deathws, wait_for_it);
>              free(deathws);
>          }
>
Juergen Gross Jan. 15, 2016, 4:24 p.m. UTC | #2
On 15/01/16 17:12, Ian Campbell wrote:
> On Fri, 2016-01-08 at 14:08 +0100, Juergen Gross wrote:
>> +# get xenstore domain id (or 0 if no xenstore domain)
>> +get_xsdomid()
>> +{
>> +    XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid 2>/dev/null`
>> +    if test $? -ne 0; then
>> +        XS_DOMID=0
>> +    fi
> 
> In principal this could be:
> +    XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid 2>/dev/null || echo 0`
> 
> But I'm far from convinced that is an improvement.

I think I'll leave it as before.

> 
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index f9933cb..29f9d19 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -4856,15 +4856,17 @@ static int main_shutdown_or_reboot(int do_reboot,
>> int argc, char **argv)
>>          if (wait_for_it)
>>              deathws = calloc(nb_domain, sizeof(*deathws));
>>  
>> +        wait_for_it = 0;
> 
> Please can we avoid reusing the boolean flag used for argument handling as
> a counter.
> 
> I think you will need to introduce a new nrdeathws.

Okay, will do.


Thanks,

Juergen
diff mbox

Patch

diff --git a/tools/hotplug/Linux/xendomains.in b/tools/hotplug/Linux/xendomains.in
index dfe0b33..9f88649 100644
--- a/tools/hotplug/Linux/xendomains.in
+++ b/tools/hotplug/Linux/xendomains.in
@@ -196,6 +196,15 @@  rdnames()
     done
 }
 
+# get xenstore domain id (or 0 if no xenstore domain)
+get_xsdomid()
+{
+    XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid 2>/dev/null`
+    if test $? -ne 0; then
+        XS_DOMID=0
+    fi
+}
+
 LIST_GREP='(domain\|(domid\|(name\|^    {$\|"name":\|"domid":'
 parseln()
 {
@@ -216,12 +225,14 @@  parseln()
 
 is_running()
 {
+    get_xsdomid
     rdname $1
     RC=1
     name=;id=
     while read LN; do
 	parseln "$LN" || continue
 	if test $id = 0; then continue; fi
+	if test $id = $XS_DOMID; then continue; fi
 	case $name in
 	    ($NM)
 		RC=0
@@ -302,10 +313,12 @@  start()
 
 all_zombies()
 {
+    get_xsdomid
     name=;id=
     while read LN; do
 	parseln "$LN" || continue
 	if test $id = 0; then continue; fi
+	if test $id = $XS_DOMID; then continue; fi
 	if test "$state" != "-b---d" -a "$state" != "-----d"; then
 	    return 1;
 	fi
@@ -351,11 +364,13 @@  stop()
     if test "$XENDOMAINS_AUTO_ONLY" = "true"; then
 	rdnames
     fi
+    get_xsdomid
     echo -n "Shutting down Xen domains:"
     name=;id=
     while read LN; do
 	parseln "$LN" || continue
 	if test $id = 0; then continue; fi
+	if test $id = $XS_DOMID; then continue; fi
 	echo -n " $name"
 	if test "$XENDOMAINS_AUTO_ONLY" = "true"; then
 	    eval "
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f9933cb..29f9d19 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4856,15 +4856,17 @@  static int main_shutdown_or_reboot(int do_reboot, int argc, char **argv)
         if (wait_for_it)
             deathws = calloc(nb_domain, sizeof(*deathws));
 
+        wait_for_it = 0;
         for (i = 0; i<nb_domain; i++) {
-            if (dominfo[i].domid == 0)
+            if (dominfo[i].domid == 0 || dominfo[i].never_stop)
                 continue;
             fn(dominfo[i].domid, deathws ? &deathws[i] : NULL, i,
                fallback_trigger);
+            wait_for_it++;
         }
 
-        if (wait_for_it) {
-            wait_for_domain_deaths(deathws, nb_domain - 1 /* not dom 0 */);
+        if (deathws) {
+            wait_for_domain_deaths(deathws, wait_for_it);
             free(deathws);
         }