Re: [Xen-devel] [PATCH 05/21] xenpaging: add signal handling

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

Yeah, they certainly seem like reasonable changes.  They just need to
be pointed out, so that people have an idea the actual impact of the
patch (both now when accepting it, and later when going back trying to
figure out where something broke).

FYI, I normally do a bunch of work (involving a large number of
changes) first, then do "hg diff > working.diff ; hg update -C" and
then go through working.diff to see where the individual changes are
best suited, whether in a new patch, or in a modification to an
existing patch.  (emacs diff-mode is really helpful here.)  That helps
me review my own code, and promotes good patch hygiene. :-)

Peace,
 -George

On Thu, Dec 2, 2010 at 9:59 AM, Olaf Hering <olaf@xxxxxxxxx> wrote:
> On Fri, Nov 26, George Dunlap wrote:
>
>> Olaf, I haven't been looking at these patches as we've been going
>> along, but there seem to be two things happening in this patch not
>> mentioned in the description:
>
> This was a "grown" patch.
>
>> * Making xenpaging_teardown() not skip when a tear-down item fails,
>> but continue to try to tear down the rest
>
> If a domain is shutting down, xc_mem_event_disable will always fail
> because d->is_dying is checked. Thats why I removed the bail_out part.
>
>> * Making return values for the program as a whole (1 for initializing
>> the paging, 2 for a failed file open)
>
> return codes are currently not perfect, sometimes -1 is leaked.
> I think xenpaging should either return 0 or 1.
>
>> These kinds of things should at least be mentioned in the description;
>> and I would personally probably pull them out and put them in a
>> separate patch.
>
> Will do better next time.
> Thanks for the comment.
>
>
> Olaf
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
>

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