All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeremy Fitzhardinge <jeremy@goop.org>
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [Xen-devel] [PATCH 2 of 5] early PV on HVM
Date: Fri, 12 Mar 2010 12:22:07 -0800	[thread overview]
Message-ID: <4B9AA26F.4000508@goop.org> (raw)
In-Reply-To: <alpine.DEB.2.00.1003121819500.27222@kaball-desktop>

On 03/12/2010 11:09 AM, Stefano Stabellini wrote:
> My version is not 64-bit specific, should I move it to a new file
> anyway?
>
> I was trying to keep the differences with Sheng's version low, but in
> this case I am not quite sure how to manage it.
> Suggestions are welcome.
>    

My rough rules for code layout (in no particular order):

    * keep similar functions together (even if they're not necessarily
      sharing common code)
    * dislike externs and cross-file references (ie, prefer static
      functions/data) -> keep code sharing functions/data together
    * dislike #ifdef CONFIG_x within files; prefer Makefile rules to
      either completely compile or not compile files
    * dislike dumping-ground files
    * dislike too-small files
    * dislike moving code between files, so its better to get it right
      from the start (it makes merging harder)

So in this case, enlighten.c is the general setup/init/misc file in 
xen/.  I've been trying to move things out of it to reduce its dumping 
ground-ness.  In your case, the changes are clearly init, so they 
probably belong there.  They're compiled unconditionally, so there's no 
#ifdef.  They're fairly small so they would probably be too small in a 
separate file (unless its likely to get bigger?).  So keep in enlighten.c.

(In Sheng's version of this patch, it has more code and is conditionally 
compiled, so I think the balance tips the other way.)

     J


      reply	other threads:[~2010-03-12 20:22 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-03-10 15:47 [PATCH 2 of 5] early PV on HVM Stefano Stabellini
2010-03-11 21:20 ` [Xen-devel] " Jeremy Fitzhardinge
2010-03-12 19:09   ` Stefano Stabellini
2010-03-12 20:22     ` Jeremy Fitzhardinge [this message]

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=4B9AA26F.4000508@goop.org \
    --to=jeremy@goop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stefano.stabellini@eu.citrix.com \
    --cc=xen-devel@lists.xensource.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.