Xen devel - Jan 2011 - [PATCH] [qemu] xen_be_init under stubdom

Do nothing in xen_be_init under stubdom plus a minor inconsequential cleanup.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala

diff --git a/hw/xen_backend.c b/hw/xen_backend.c
index d9be513..61e1210 100644
--- a/hw/xen_backend.c
+++ b/hw/xen_backend.c
@@ -613,7 +613,7 @@ static void xenstore_update(void *unused)

     vec = xs_read_watch(xenstore, &count);
     if (vec == NULL)
-       goto cleanup;
+        return;

     if (sscanf(vec[XS_WATCH_TOKEN], "be:%" PRIxPTR ":%d:%"
PRIxPTR,
                &type, &dom, &ops) == 3)
@@ -621,7 +621,6 @@ static void xenstore_update(void *unused)
     if (sscanf(vec[XS_WATCH_TOKEN], "fe:%" PRIxPTR, &ptr) == 1)
        xenstore_update_fe(vec[XS_WATCH_PATH], (void*)ptr);

-cleanup:
     qemu_free(vec);
 }

@@ -646,6 +645,10 @@ static void xen_be_evtchn_event(void *opaque)

 int xen_be_init(void)
 {
+#ifdef CONFIG_STUBDOM
+    return 0;
+#endif
+
     xenstore = xs_daemon_open();
     if (!xenstore) {
        xen_be_printf(NULL, 0, "can''t connect to
xenstored\n");

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

On Wed, 19 Jan 2011, Kamala Narasimhan wrote:
> Do nothing in xen_be_init under stubdom plus a minor inconsequential
cleanup.
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> 
> Kamala
> 
> diff --git a/hw/xen_backend.c b/hw/xen_backend.c
> index d9be513..61e1210 100644
> --- a/hw/xen_backend.c
> +++ b/hw/xen_backend.c
> @@ -613,7 +613,7 @@ static void xenstore_update(void *unused)
> 
>      vec = xs_read_watch(xenstore, &count);
>      if (vec == NULL)
> -       goto cleanup;
> +        return;
> 
>      if (sscanf(vec[XS_WATCH_TOKEN], "be:%" PRIxPTR
":%d:%" PRIxPTR,
>                 &type, &dom, &ops) == 3)
> @@ -621,7 +621,6 @@ static void xenstore_update(void *unused)
>      if (sscanf(vec[XS_WATCH_TOKEN], "fe:%" PRIxPTR, &ptr) ==
1)
>         xenstore_update_fe(vec[XS_WATCH_PATH], (void*)ptr);
> 
> -cleanup:
>      qemu_free(vec);
>  }
> 
> @@ -646,6 +645,10 @@ static void xen_be_evtchn_event(void *opaque)
> 
>  int xen_be_init(void)
>  {
> +#ifdef CONFIG_STUBDOM
> +    return 0;
> +#endif
> +
>      xenstore = xs_daemon_open();
>      if (!xenstore) {
>         xen_be_printf(NULL, 0, "can''t connect to
xenstored\n");
 
I think it would be better if we actually return an error from
xen_be_init and we just print a warning in hw/xen_machine_fv.c if the
backends fail to initialize instead of exit.
It is OK to just call exit in hw/xen_machine_pv.c, because nothing is
running in qemu apart from backends in that case.

Also having another #ifdef CONFIG_STUBDOM might be OK in qemu-xen, but
in qemu upstream we can get away without it implementing a function
like:

int xen_qemu_is_a_stubdom();

that returns 1 in case qemu is running in a stubdom and 0 otherwise.
I would make domid_s a global int variable (see vl.c:5827) so that
xen_qemu_is_a_stubdom can be implemented like this:

return domid_s;

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

> I think it would be better if we actually return an error from
> xen_be_init and we just print a warning in hw/xen_machine_fv.c if the
> backends fail to initialize instead of exit.
> It is OK to just call exit in hw/xen_machine_pv.c, because nothing is
> running in qemu apart from backends in that case.
>
I did give it some thought and my rationale for returning success was
that we shouldn''t be calling it in the first place if that code is not
configured to run in stubdom mode and if we do chose to go with the
call we should do nothing and return success.  But I didn''t feel
strongly either way and I don''t know much about pv/fv code in that
area and you know all about it.  So, I will go with your choice and
send a revised patch momentarily.
> Also having another #ifdef CONFIG_STUBDOM might be OK in qemu-xen, but
> in qemu upstream we can get away without it implementing a function
> like:
>
> int xen_qemu_is_a_stubdom();
>
> that returns 1 in case qemu is running in a stubdom and 0 otherwise.
> I would make domid_s a global int variable (see vl.c:5827) so that
> xen_qemu_is_a_stubdom can be implemented like this:
>
> return domid_s;
>
Definitely.  This patch is specific to qemu unstable and I should have
been specific.  If we didn''t fail the way we do (in an irrelevant
place by corrupting something which was hard to debug) I wouldn''t even
have bothered with this patch.  I will go per your suggestion for
upstream qemu.

Kamala

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

Kamala Narasimhan writes ("[Xen-devel] [PATCH] [qemu] xen_be_init under
stubdom"):
> Do nothing in xen_be_init under stubdom plus a minor inconsequential
cleanup.
...
> -       goto cleanup;
> +        return;
...
> -cleanup:
>      qemu_free(vec);
>  }
I don''t think this is a helpful change.  There is nothing wrong with
calling qemu_free(0) and IMO in general functions that "goto cleanup"
are to be preferred to ones that "return".

Furthermore, even if this patch were good, it''s not a bugfix so is not
acceptable at this stage of the release.
> @@ -646,6 +645,10 @@ static void xen_be_evtchn_event(void *opaque)
> 
>  int xen_be_init(void)
>  {
> +#ifdef CONFIG_STUBDOM
> +    return 0;
> +#endif
I don''t understand this at all.  Why should stubdom not be able to
make pv backends if it wants to ?  I agree that it probably doesn''t
want to but if something iswrongly causing it to then the right fix is
to make it not do so.

Ian.

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

On Thu, 20 Jan 2011, Ian Jackson wrote:
> Kamala Narasimhan writes ("[Xen-devel] [PATCH] [qemu] xen_be_init
under stubdom"):
> > Do nothing in xen_be_init under stubdom plus a minor inconsequential
cleanup.
> ...
> > -       goto cleanup;
> > +        return;
> ...
> > -cleanup:
> >      qemu_free(vec);
> >  }
> 
> I don''t think this is a helpful change.  There is nothing wrong
with
> calling qemu_free(0) and IMO in general functions that "goto
cleanup"
> are to be preferred to ones that "return".
> 
> Furthermore, even if this patch were good, it''s not a bugfix so is
not
> acceptable at this stage of the release.
> 
> > @@ -646,6 +645,10 @@ static void xen_be_evtchn_event(void *opaque)
> > 
> >  int xen_be_init(void)
> >  {
> > +#ifdef CONFIG_STUBDOM
> > +    return 0;
> > +#endif
> 
> I don''t understand this at all.  Why should stubdom not be able to
> make pv backends if it wants to ?  I agree that it probably
doesn''t
> want to but if something iswrongly causing it to then the right fix is
> to make it not do so.
the current xen_backend code in qemu cannot handle being run in a
stubdom, for example:

dom0 = xs_get_domain_path(xenstore, 0);

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

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] [qemu] xen_be_init
under stubdom"):
> the current xen_backend code in qemu cannot handle being run in a
> stubdom, for example:
> 
> dom0 = xs_get_domain_path(xenstore, 0);
Well, fine, then it will fall over.  I don''t see why we need to
special-case anything.

Ian.

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

On Thu, 20 Jan 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] [qemu] xen_be_init
under stubdom"):
> > the current xen_backend code in qemu cannot handle being run in a
> > stubdom, for example:
> > 
> > dom0 = xs_get_domain_path(xenstore, 0);
> 
> Well, fine, then it will fall over.  I don''t see why we need to
> special-case anything.
 
It depends on how it fails.
And in any case, isn''t it better to explicitly avoid running any code
that we know for sure it cannot run? So that the next person that looks
at this code doesn''t assume that the backends are run in the stubdom
too
before realizing that they actually fail to start?

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

>> Do nothing in xen_be_init under stubdom plus a minor inconsequential
cleanup.
> ...
>> -       goto cleanup;
>> +        return;
> ...
>> -cleanup:
>>      qemu_free(vec);
>>  }
>
> I don''t think this is a helpful change.
I did call it inconsequential and wouldn''t have sent it if it
didn''t
happen to be with a one liner that had functional impact that I wanted
changed :)
> There is nothing wrong with
> calling qemu_free(0) and IMO in general functions that "goto
cleanup"
> are to be preferred to ones that "return".
>
You would rather check for null pointer and then go to cleanup code
whose sole purpose in this case is to free that pointer and all this
for free to realize that it has nothing to free and then return back!
This as opposed to simply return when it is null!  I understand how
going to cleanup makes a lot of sense when there is at least some
amount of potential cleanup to do.  In this case, clean up does
nothing more than free that pointer!  Anyway, I am not particularly
worried if this doesn''t make it.
> Furthermore, even if this patch were good, it''s not a bugfix so is
not
> acceptable at this stage of the release.
>
No, it does get rid of an issue I encounter.  I was running into data
corruption as this code path was taken with stubdom in my
configuration and it was a pain to debug as with these kinds of
corruption issues the problem showed up elsewhere.

Kamala
>> @@ -646,6 +645,10 @@ static void xen_be_evtchn_event(void *opaque)
>>
>>  int xen_be_init(void)
>>  {
>> +#ifdef CONFIG_STUBDOM
>> +    return 0;
>> +#endif
>
> I don''t understand this at all.  Why should stubdom not be able to
> make pv backends if it wants to ?  I agree that it probably
doesn''t
> want to but if something iswrongly causing it to then the right fix is
> to make it not do so.
>
> Ian.
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] [qemu] xen_be_init
under stubdom"):
> On Thu, 20 Jan 2011, Ian Jackson wrote:
> > Well, fine, then it will fall over.  I don''t see why we need
to
> > special-case anything.
>  
> It depends on how it fails.
> And in any case, isn''t it better to explicitly avoid running any
code
> that we know for sure it cannot run? So that the next person that looks
> at this code doesn''t assume that the backends are run in the
stubdom too
> before realizing that they actually fail to start?
I agree but I think this should be done in much higher up the stack.

Ian.

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

Kamala Narasimhan writes ("Re: [Xen-devel] [PATCH] [qemu] xen_be_init under
stubdom"):
> I did call it inconsequential and wouldn''t have sent it if it
didn''t
> happen to be with a one liner that had functional impact that I wanted
> changed :)
What functional impact does it have ?
> > There is nothing wrong with
> > calling qemu_free(0) and IMO in general functions that "goto
cleanup"
> > are to be preferred to ones that "return".
> 
> You would rather check for null pointer and then go to cleanup code
> whose sole purpose in this case is to free that pointer and all this
> for free to realize that it has nothing to free and then return back!
Yes.  That avoids having to reason whether there is other stuff that
might need to be freed.  If more code is added later which allocates
something else then the plain "return" may become buggy.
> > Furthermore, even if this patch were good, it''s not a bugfix
so is not
> > acceptable at this stage of the release.
> 
> No, it does get rid of an issue I encounter.  I was running into data
> corruption as this code path was taken with stubdom in my
> configuration and it was a pain to debug as with these kinds of
> corruption issues the problem showed up elsewhere.
Are you saying that in stubdom, this code path causes your code to
crash ?  That is not the fault of the code path.

Ian.

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

On Thu, 20 Jan 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] [qemu] xen_be_init
under stubdom"):
> > On Thu, 20 Jan 2011, Ian Jackson wrote:
> > > Well, fine, then it will fall over.  I don''t see why we
need to
> > > special-case anything.
> >  
> > It depends on how it fails.
> > And in any case, isn''t it better to explicitly avoid running
any code
> > that we know for sure it cannot run? So that the next person that
looks
> > at this code doesn''t assume that the backends are run in the
stubdom too
> > before realizing that they actually fail to start?
> 
> I agree but I think this should be done in much higher up the stack.
 
Considering how the backends are currently setup, there is nothing else
higher up the stack. Keep in mind that this code only enables the
backends in qemu, it doesn''t assume that the backends are going to be
active and linked to any frontends (that is for the toolstack to
decide).
So I think is correct to explicitly fail to register (register !initialize) the
backends in the stubdom case.

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

On Thu, Jan 20, 2011 at 11:45 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Kamala Narasimhan writes ("Re: [Xen-devel] [PATCH] [qemu] xen_be_init
under stubdom"):
>> I did call it inconsequential and wouldn''t have sent it if it
didn''t
>> happen to be with a one liner that had functional impact that I wanted
>> changed :)
>
> What functional impact does it have ?
>
The cleanup code has no functional impact but the change in
xen_be_init in stubdom case does (which is what I meant above) and
here is how -

xen_be_init registers for a notification which it shouldn''t have in
stubdom case and registering for that notification resulted in it
getting called and because it was getting called in an environment
where it wasn''t suppose to, it ended up corrupting data structures and
stubdom-qemu seg faulting which is what I meant by functional impact.

We could fix the handler to gracefully fail but why execute the code
at all when not needed.
>> > There is nothing wrong with
>> > calling qemu_free(0) and IMO in general functions that "goto
cleanup"
>> > are to be preferred to ones that "return".
>>
>> You would rather check for null pointer and then go to cleanup code
>> whose sole purpose in this case is to free that pointer and all this
>> for free to realize that it has nothing to free and then return back!
>
> Yes.  That avoids having to reason whether there is other stuff that
> might need to be freed.  If more code is added later which allocates
> something else then the plain "return" may become buggy.
>
If more cleanup code is expected to be added and it was written with
that in mind, obviously I can''t make a case for the change.
>> > Furthermore, even if this patch were good, it''s not a
bugfix so is not
>> > acceptable at this stage of the release.
>>
>> No, it does get rid of an issue I encounter.  I was running into data
>> corruption as this code path was taken with stubdom in my
>> configuration and it was a pain to debug as with these kinds of
>> corruption issues the problem showed up elsewhere.
>
> Are you saying that in stubdom, this code path causes your code to
> crash ?  That is not the fault of the code path.
>
Yes, the notification that this code path registers for which is not
required in the environment in question is causing stubdom to crash
later.  We could fix the handler to gracefully fail but why register
for a handler that isn''t needed?  And yes the fault is not in the init
code path but in the handler it registers for which it shouldn''t.  The
choice is to fix the handler or simply not register for it and we
chose the latter.

Kamala

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

Kamala Narasimhan writes ("[Xen-devel] [PATCH] [qemu] xen_be_init under
stubdom"):
> Do nothing in xen_be_init under stubdom plus a minor inconsequential
cleanup.
I have applied the consequential half of this patch.

Thanks,
Ian.

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