[PATCH] pci-attach: fix assertation

[PATCH] pci-attach: fix assertation

[Chunyan Liu]

Run "xl pci-attach <domain> <pci_device>", the 2nd time fails:
xl: libxl_xshelp.c:209: libxl__xs_transaction_start: Assertion `!*t' failed.
Aborted

To fix that, initialize xs_transaction to avoid libxl__xs_transaction_start
assertation error.

Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
 tools/libxl/libxl_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 1ebdce7..19c597e 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -123,7 +123,7 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
     flexarray_t *back;
     char *num_devs, *be_path;
     int num = 0;
-    xs_transaction_t t;
+    xs_transaction_t t = XBT_NULL;
     libxl__device *device;
     int rc;
     libxl_domain_config d_config;
-- 
2.1.4

Re: [PATCH] pci-attach: fix assertation

[Ian Campbell]

On Wed, 2015-09-16 at 14:16 +0800, Chunyan Liu wrote:

For the subject I prefer to avoid "fix <vague>" style messages. In this
case something like "libxl: ensure xs transaction is initialised in
libxl__device_pci_add_xenstore" would be better.

> Run "xl pci-attach  <pci_device>", the 2nd time fails:
> xl: libxl_xshelp.c:209: libxl__xs_transaction_start: Assertion `!*t' failed.
> Aborted
>
> To fix that, initialize xs_transaction to avoid
> libxl__xs_transaction_start assertation error.

"assertion".

> Signed-off-by: Chunyan Liu <cyliu@suse.com>

With an updated commit message (which I can do as I commit):
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Wei, I think we want this for 4.6.

Ian, tools/libxl/libxl_internal.h says to initialised the transaction to 0
rather than XBT_NULL explicitly -- I don't think there is a specific reason
for that though?

> ---
>  tools/libxl/libxl_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 1ebdce7..19c597e 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -123,7 +123,7 @@ static int libxl__device_pci_add_xenstore(libxl__gc
> *gc, uint32_t domid, libxl_d
>      flexarray_t *back;
>      char *num_devs, *be_path;
>      int num = 0;
> -    xs_transaction_t t;
> +    xs_transaction_t t = XBT_NULL;
>      libxl__device *device;
>      int rc;
>      libxl_domain_config d_config;

Re: [PATCH] pci-attach: fix assertation

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] pci-attach: fix assertation"):
> Ian, tools/libxl/libxl_internal.h says to initialised the transaction to 0
> rather than XBT_NULL explicitly -- I don't think there is a specific reason
> for that though?

It says:

/* Transaction functions, best used together.
 * The caller should initialise *t to 0 (XBT_NULL) before calling start.

I think that amounts to a promise that XBT_NULL == 0.  Therefore the
caller may (from a correctness pov) use either.

It is important that initialisation with 0 is supported because then
FILLZERO is known to DTRT.  (Note that, analogously, we are already
assuming that FILLZERO generates null pointers.)

I know that some people have strong views that writing NULL for null
pointers, rather than 0, is better.  I think it is needless verbiage.
A similar argument applies to XBT_NULL.  I think this is a bikeshed
and am happy to let it be whatever colour it comes from the factory
:-).

Ian.

Re: [PATCH] pci-attach: fix assertation

[Ian Campbell]

On Wed, 2015-09-16 at 14:39 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH] pci-attach: fix assertation"):
> > Ian, tools/libxl/libxl_internal.h says to initialised the transaction
> > to 0
> > rather than XBT_NULL explicitly -- I don't think there is a specific
> > reason
> > for that though?
> 
> It says:
> 
> /* Transaction functions, best used together.
>  * The caller should initialise *t to 0 (XBT_NULL) before calling start.
> 
> I think that amounts to a promise that XBT_NULL == 0.  Therefore the
> caller may (from a correctness pov) use either.

Agreed, I just wanted to be sure there wasn't some magic reason to avoid
XBT_NULL (other than, as you say, needless verbiage).

> It is important that initialisation with 0 is supported because then
> FILLZERO is known to DTRT.  (Note that, analogously, we are already
> assuming that FILLZERO generates null pointers.)

Right.

> I know that some people have strong views that writing NULL for null
> pointers, rather than 0, is better.  I think it is needless verbiage.
> A similar argument applies to XBT_NULL.  I think this is a bikeshed
> and am happy to let it be whatever colour it comes from the factory
> :-).

Ack.

Re: [PATCH] pci-attach: fix assertation

[Wei Liu]

On Wed, Sep 16, 2015 at 09:25:25AM +0100, Ian Campbell wrote:
> On Wed, 2015-09-16 at 14:16 +0800, Chunyan Liu wrote:
> 
> For the subject I prefer to avoid "fix <vague>" style messages. In this
> case something like "libxl: ensure xs transaction is initialised in
> libxl__device_pci_add_xenstore" would be better.
> 
> > Run "xl pci-attach  <pci_device>", the 2nd time fails:
> > xl: libxl_xshelp.c:209: libxl__xs_transaction_start: Assertion `!*t' failed.
> > Aborted
> >
> > To fix that, initialize xs_transaction to avoid
> > libxl__xs_transaction_start assertation error.
> 
> "assertion".
> 
> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
> 
> With an updated commit message (which I can do as I commit):
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Wei, I think we want this for 4.6.
> 

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH] pci-attach: fix assertation

[Ian Campbell]

On Wed, 2015-09-16 at 15:08 +0100, Wei Liu wrote:
> On Wed, Sep 16, 2015 at 09:25:25AM +0100, Ian Campbell wrote:
> > On Wed, 2015-09-16 at 14:16 +0800, Chunyan Liu wrote:
> > 
> > For the subject I prefer to avoid "fix <vague>" style messages. In this
> > case something like "libxl: ensure xs transaction is initialised in
> > libxl__device_pci_add_xenstore" would be better.
> > 
> > > Run "xl pci-attach  <pci_device>", the 2nd time fails:
> > > xl: libxl_xshelp.c:209: libxl__xs_transaction_start: Assertion `!*t'
> > > failed.
> > > Aborted
> > > 
> > > To fix that, initialize xs_transaction to avoid
> > > libxl__xs_transaction_start assertation error.
> > 
> > "assertion".
> > 
> > > Signed-off-by: Chunyan Liu <cyliu@suse.com>
> > 
> > With an updated commit message (which I can do as I commit):
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Wei, I think we want this for 4.6.
> > 
> 
> Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Applied to staging and staging-4.6.