xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Ian Campbell <ian.campbell@citrix.com>
To: George Dunlap <George.Dunlap@eu.citrix.com>,
	Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: Re: [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling
Date: Tue, 15 Sep 2015 10:31:36 +0100	[thread overview]
Message-ID: <1442309496.3549.351.camel@citrix.com> (raw)
In-Reply-To: <CAFLBxZYYHVUazguzNonScvVEAKVqX+AxPgMNTLRBEe==2R7AKw@mail.gmail.com>

On Mon, 2015-09-14 at 10:50 +0100, George Dunlap wrote:
> On Thu, Sep 10, 2015 at 1:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > While it appears to be intentional for "xl pci-assignable-remove" to
> > not re-bind the original driver by default (requires the -r option),
> > permanently losing the information which driver was originally used
> > seems bad. Make "add; remove; add; remove -r" re-bind the original
> > driver by allowing "remove" to delete the information only upon
> > successful re-bind.
> 
> I would be open to the argument that I was being overly paranoid in
> making "xl pci-assignable-remove" not re-bind by default.  But either
> way:
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

The use of "rc" to hold a non-libxl error code (0 or -1 in this case) in
_add is not allowed by libxl coding style, but is consistent with the same
thing existing in _remove, also this code is mostly in hypervisor coding
style so it seems tolerable for this new code to be so too.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> > In the course of this I also noticed that binding information is lost
> > when upon first "add" pciback isn't loaded yet, due to its presence not
> > being checked for early enough. Adjust pciback_dev_is_assigned()
> > accordingly, and properly distinguish "yes" and "error" returns in the
> > "add" case (removing a redundant error message from the "remove" path
> > for consistency).
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > As to 4.6 I'm not overly fussed: It'd be nice, but it could easily be
> > backported later on.
> 
> I wouldn't really consider this a bug fix, but an improvement.  As
> such, I don't think it should be given a freeze exception, and my
> inclination would be to say that it shouldn't be backported.  But the
> strength of my opinion isn't very strong.

I wouldn't argue either way.

Ian.

  reply	other threads:[~2015-09-15  9:31 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-09-10 12:36 [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling Jan Beulich
2015-09-14  9:50 ` George Dunlap
2015-09-15  9:31   ` Ian Campbell [this message]
2015-09-15 11:16     ` Ian Campbell

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=1442309496.3549.351.camel@citrix.com \
    --to=ian.campbell@citrix.com \
    --cc=George.Dunlap@eu.citrix.com \
    --cc=Ian.Jackson@eu.citrix.com \
    --cc=JBeulich@suse.com \
    --cc=stefano.stabellini@eu.citrix.com \
    --cc=wei.liu2@citrix.com \
    --cc=xen-devel@lists.xenproject.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).