Re: [Qemu-devel] [PATCH v2] Basic Illumos support

[Andreas Färber]

Am 24.03.2012 15:51, schrieb Lee Essen:
> On 24 Mar 2012, at 12:56, Blue Swirl wrote:
> 
>> On Sat, Mar 17, 2012 at 08:04, Lee Essen <address@hidden> wrote:
>>> (third email attempt, apologies if you get duplicates)

I did receive all three fine btw.

[...]
> Me and patches just aren't working well! It's fine in my outbox, it must be 
> being stripped by my mail system or mailer somewhere, I'm having trouble 
> getting git-send-mail to work because of auth issues, so I've attached the 
> patch for now.

You may need to install some Perl modules via cpan, assuming you have
ssl / tls and username set correctly. What's the error?


I'm not yet okay with this patch:

First, never put personal comments such as resends above the --- line of
a [PATCH], comments can go under ---, indented by one space. Please
describe in a neutral way what the patch does, how QEMU behaves
with/without (warning, error, new feature?), but not what you've tried,
not done, want, etc. in the course of writing the patch - that would go
into a Change Log (which can go below --- or in the cover letter).

This is still too many changes at once, without proper explanation. A
good patch IMO would be one that, e.g., only adds the libs to configure
and explains in the commit message for what functions those are needed,
so that we don't get the same breakage immediately after elsewhere.
Maybe we should even move them to a more central place as a precaution?

qemu-timer should be one patch ("qemu-timer: Enable dynticks for
Solaris"; why was dynticks limited to Linux?),
cpus (where is sigbus_reraise() being used? again, it being Linux-only
looks odd),
qemu-ga (1. O_ASYNC, 2. mac_addr, 3. libs).

You will find that the smaller a patch is, the easier it is to get it in
since less different maintainers will be involved in acking it and since
they are less likely to postpone review.

Andreas