qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Andreas Färber" <andreas.faerber@web.de>
To: Lee Essen <lee.essen@nowonline.co.uk>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] Initial support for Ilumos build and Illumos-kvm
Date: Fri, 16 Mar 2012 11:15:36 +0100	[thread overview]
Message-ID: <4F6312C8.6070009@web.de> (raw)
In-Reply-To: <D757FFAE-B826-4293-BB82-05E8BCF01FB2@nowonline.co.uk>

Am 16.03.2012 10:23, schrieb Lee Essen:
> This fixes a number of issues with the build process (namely ensuring
> the use of bash), adds specific support for the Illumos port of KVM
> and fixes a few general Solaris compatibility issues.
>
> There are still some things outstanding:
>
> - there's a duplicate smb_wmb() definition in qemu-barrier.h and the
> illumos kvm_x86.h which generates some warnings.
> - there's a repeated call to page_size() that should probably be fixed.
> - dtrace support needs to be fixed (-m64/32 option, reserved words and
> linking issues)
> - vnics need to be added
> - the original illumos code added another timer source (multiticks)
> - the issue with Linux needs to be resolved
>
> Other than that, this gets it to the point where it will build and run
> with illumos kvm, and works fine for Windows.
>
> It's my first patch to qemu, and most of the real kvm stuff has come
> from the original illumos-kvm-cmd tree, so be gentle with me!
>
>
> Signed-off-by: Lee Essen <l
> <mailto:david@gibson.dropbear.id.au>ee.essen@nowonline.co.uk
> <mailto:ee.essen@nowonline.co.uk>>

Your patch is HTML-formatted. Please use git-send-email to avoid that.

It is also doing way too many things at once. Properly using existing
$(SHELL) everywhere in Makefiles could be one patch, for instance,
adding $shell in configure another, same for functional
CONFIG_SOLARIS/__sun__ changes, KVM stuff in yet another. Making the
patches smaller and confined to subsystems or aspects will make it more
reviewable, especially since different maintainers are involved in the
files you touch.

"LEE - todo" doesn't sound too assuring. Either write it as a proper
"TODO This and that needs to be done." so that someone can address it or
send it as an [RFC] rather than a [PATCH].
If this patch introduces an issue for Linux (you don't say which?) while
adding support for illumos, it won't be acceptable in the first place. A
[PATCH] is expected to be of regression-free quality for qemu.git.

Is there any SystemTap on illumos? If not, we don't need the .stp file
at all.

Please resubmit with those issues addressed. I just cc'ed you on the C99
fix mentioned yesterday and am rebasing my queue on OpenIndiana (oi_151a).

Regards,
Andreas

  reply	other threads:[~2012-03-16 10:16 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-03-16  9:23 [Qemu-devel] [PATCH] Initial support for Ilumos build and Illumos-kvm Lee Essen
2012-03-16 10:15 ` Andreas Färber [this message]
2012-03-16 11:56 ` Paolo Bonzini
2012-03-16 17:25   ` Lee Essen
2012-03-16 18:15     ` Paolo Bonzini
2012-03-16 13:14 ` Jan Kiszka
2012-03-16 13:46   ` Lee Essen
2012-03-16 16:28     ` Paolo Bonzini
2012-03-16 17:15       ` Lee Essen
2012-03-17  9:00     ` Jan Kiszka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4F6312C8.6070009@web.de \
    --to=andreas.faerber@web.de \
    --cc=lee.essen@nowonline.co.uk \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).