From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 05/21] xenpaging: add signal handling Date: Thu, 2 Dec 2010 11:48:11 +0000 Message-ID: References: <20101126134901.384130351@aepfle.de> <20101126134903.163026660@aepfle.de> <20101202095914.GB29462@aepfle.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Return-path: In-Reply-To: <20101202095914.GB29462@aepfle.de> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Olaf Hering Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org 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 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@lists.xensource.com > http://lists.xensource.com/xen-devel >