Re: [Xen-devel] [PATCH] [qemu] xen_be_init under stubdom

[Kamala Narasimhan <kamala.narasimhan@xxxxxxxxx>]

On Thu, Jan 20, 2011 at 11:45 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel