[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Fixed Mirage

On 11/07/2013 23:15, Anil Madhavapeddy wrote:
Awesome! I'm pulling to trunk now and trying this out, as I could reproduce the 
hanging bug before on a static IP.

I noticed this:

Doesn't this totally nullify the point of Console.log_s blocking?  With the 
switch to Lwt.async, multiple log_s calls can now interleave and corrupt the 
console ring (whereas previously they would be synchronous).  This would 
require a mutex around the console ring to be totally safe (and Console.log 
exists for when you want to be non-blocking).

Agreed, I did not think about that, you're right. This should be reverted.

And just a minor thing in the same commit, but the change to "if num > 0" in 
refill_requests also causes one less event notification, and so isn't quite a noop.  That's 
worth making a separate commit from the interface adaptation also.

Yeah, I agree but at the same time, if num > 0 is currently a hack to cope with the fact that event channels are currently activated all the time. So it is fine for now to not notify anything when actually nothing happened.

A good general workflow for making changes is:

- make many commits and one pull request, and then merge it if you don't want 
code review (e.g. an urgent fix).  This gives everyone watching the repo an 
explicit mail about the series.

I did not think about it. I'll do it next time.

- even if you make a lot of changes in one go, split up the commits by using "git 
add -p" to break up a patch into smaller patches interactively.

Yeah, I'm doing it most of the time, I've been messy this time, was stressed about this bug :)

- you will love the smaller commits later on when you are tracking down a bug, since you 
can use "git bisect" and some shell scripts to automatically narrow it down to 
one commit in many cases.  This requires slightly more automated unit tests for us.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.