Re: Fixed Mirage

["Vincent B." <vb@xxxxxxxxxxxxxx>]

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:
> https://github.com/mirage/mirage-platform/commit/d22f60048890e0b253788a554d6ce00021f76f4e
>
> 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.
>
> -anil