thr3ads.net - Xen devel - [PATCH] xen: clamp bitmaps to correct number of bits [Sep 2012]

If this information is useful, please help other people find it:
Share via:

Ian Campbell

2012-Sep-06 11:56 UTC

[PATCH] xen: clamp bitmaps to correct number of bits

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1346932601 -3600
# Node ID 22d5e100131d1576d455780b9fdf36eacd453445
# Parent  7db66f2a0242c2dca66dca9165cb58a98d92dba9
xen: clamp bitmaps to correct number of bits

Valgrind running xl create reports:
 ==24777== Invalid read of size 4
 ==24777==    at 0x4072805: libxl__get_numa_candidate (libxl_numa.c:203)
 ==24777==    by 0x40680B6: libxl__build_pre (libxl_dom.c:166)
 ==24777==    by 0x405B82E: libxl__domain_build (libxl_create.c:323)
 ==24777==    by 0x405BB9C: domcreate_bootloader_done (libxl_create.c:747)
 ==24777==    by 0x407AD27: bootloader_local_detached_cb
(libxl_bootloader.c:281)
 ==24777==    by 0x40508D8: local_device_detach_cb (libxl.c:2470)
 ==24777==    by 0x4052B10: libxl__device_disk_local_initiate_detach
(libxl.c:2445)
 ==24777==    by 0x407AE9F: bootloader_callback (libxl_bootloader.c:265)
 ==24777==    by 0x407C69A: libxl__bootloader_run (libxl_bootloader.c:392)
 ==24777==    by 0x405CB24: do_domain_create (libxl_create.c:687)
 ==24777==    by 0x405CC5E: libxl_domain_create_new (libxl_create.c:1177)
 ==24777==    by 0x805BDE2: create_domain (xl_cmdimpl.c:1812)
 ==24777==  Address 0x42dbdd8 is 8 bytes after a block of size 48
alloc''d
 ==24777==    at 0x4023340: calloc (vg_replace_malloc.c:593)
 ==24777==    by 0x406D479: libxl__zalloc (libxl_internal.c:88)
 ==24777==    by 0x404EF38: libxl_get_cpu_topology (libxl.c:3707)
 ==24777==    by 0x4072232: libxl__get_numa_candidate (libxl_numa.c:314)
 ==24777==    by 0x40680B6: libxl__build_pre (libxl_dom.c:166)
 ==24777==    by 0x405B82E: libxl__domain_build (libxl_create.c:323)
 ==24777==    by 0x405BB9C: domcreate_bootloader_done (libxl_create.c:747)
 ==24777==    by 0x407AD27: bootloader_local_detached_cb
(libxl_bootloader.c:281)
 ==24777==    by 0x40508D8: local_device_detach_cb (libxl.c:2470)
 ==24777==    by 0x4052B10: libxl__device_disk_local_initiate_detach
(libxl.c:2445)
 ==24777==    by 0x407AE9F: bootloader_callback (libxl_bootloader.c:265)
 ==24777==    by 0x407C69A: libxl__bootloader_run (libxl_bootloader.c:392)

This is because with nr_cpus=4 the bitmask returned from Xen
contains 0xff rather than 0x0f bit our bitmap walking routines (e.g.
libxl_for_each_set_bit) round up to the next byte (so it iterates
e.g. 8 times not 4). This then causes us to access of the end of
whatever array we are walking through each set bit for.

The principal of least surprise suggests that these bits ought not to
be set and this is not a hot path so fix this at the hypervisor layer
by clamping the bits in the returned bitmap to the correct limit.

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

The impact of this seems to be fairly benign in practice, so I''m not
sure about it for 4.2.0. Could leave to 4.2.1?

Dario,

Any chance you could look at the libxl bitmap functions in 4.3 and
make them use the right limit (i.e. nr_bits not (nr_bits+7)/8?

Thanks,
Ian.

diff -r 7db66f2a0242 -r 22d5e100131d xen/common/bitmap.c
--- a/xen/common/bitmap.c	Thu Sep 06 10:44:46 2012 +0100
+++ b/xen/common/bitmap.c	Thu Sep 06 12:56:41 2012 +0100
@@ -38,6 +38,17 @@
  * for the best explanations of this ordering.
  */
 
+/* Ensure that the last byte is zero from nbits onwards. */
+static void clamp_last_byte(uint8_t *bp, int nbits)
+{
+	int lastbyte = nbits/8;
+	int remainder = nbits % 8;
+	int mask = ((1U<<remainder)-1)&0xff;
+
+	if (remainder)
+		bp[lastbyte] &= mask;
+}
+
 int __bitmap_empty(const unsigned long *bitmap, int bits)
 {
 	int k, lim = bits/BITS_PER_LONG;
@@ -485,6 +496,7 @@ void bitmap_long_to_byte(uint8_t *bp, co
 			nbits -= 8;
 		}
 	}
+	clamp_last_byte(bp, nbits);
 }
 
 void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp, int nbits)
@@ -507,6 +519,7 @@ void bitmap_byte_to_long(unsigned long *
 void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp, int nbits)
 {
 	memcpy(bp, lp, (nbits+7)/8);
+	clamp_last_byte(bp, nbits);
 }
 
 void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp, int nbits)

Jan Beulich

2012-Sep-06 12:23 UTC

head link

Re: [PATCH] xen: clamp bitmaps to correct number of bits

>>> On 06.09.12 at 13:56, Ian Campbell <ian.campbell@citrix.com>
wrote:
> The principal of least surprise suggests that these bits ought not to
> be set and this is not a hot path so fix this at the hypervisor layer
> by clamping the bits in the returned bitmap to the correct limit.
Hmm, I see your point, but without looking at the description
above (i.e. if I just saw the code in its post-patch form) I''d be
immediately tempted to rip this all out again, the more that
the inverse functions don''t do the same. So perhaps extending
the comment before the newly added function would be useful
to prevent such desires from coming up?
> @@ -38,6 +38,17 @@
>   * for the best explanations of this ordering.
>   */
>  
> +/* Ensure that the last byte is zero from nbits onwards. */
> +static void clamp_last_byte(uint8_t *bp, int nbits)
> +{
> +	int lastbyte = nbits/8;
> +	int remainder = nbits % 8;
> +	int mask = ((1U<<remainder)-1)&0xff;
While I realize the callers use plain int, I''d be very much in favor
of not continuing this bad practice (the more that you use 1U
already) - simply make the parameter and all locals (assuming
you really think they''re useful; I would have omitted all but
"remainder", given they''re being used just once) unsigned.

And once at it, please insert spaces consistently and drop the
bogus "&0xff".

Jan
> +
> +	if (remainder)
> +		bp[lastbyte] &= mask;
> +}
> +
>  int __bitmap_empty(const unsigned long *bitmap, int bits)
>  {
>  	int k, lim = bits/BITS_PER_LONG;

Ian Campbell

2012-Sep-06 13:48 UTC

head link

Re: [PATCH] xen: clamp bitmaps to correct number of bits

On Thu, 2012-09-06 at 13:23 +0100, Jan Beulich wrote:> >>> On 06.09.12 at 13:56, Ian Campbell
<ian.campbell@citrix.com> wrote:
> > The principal of least surprise suggests that these bits ought not to
> > be set and this is not a hot path so fix this at the hypervisor layer
> > by clamping the bits in the returned bitmap to the correct limit.
> 
> Hmm, I see your point, but without looking at the description
> above (i.e. if I just saw the code in its post-patch form) I''d be
> immediately tempted to rip this all out again, the more that
> the inverse functions don''t do the same. So perhaps extending
> the comment before the newly added function would be useful
> to prevent such desires from coming up?
Agreed.
> > @@ -38,6 +38,17 @@
> >   * for the best explanations of this ordering.
> >   */
> >  
> > +/* Ensure that the last byte is zero from nbits onwards. */
> > +static void clamp_last_byte(uint8_t *bp, int nbits)
> > +{
> > +	int lastbyte = nbits/8;
> > +	int remainder = nbits % 8;
> > +	int mask = ((1U<<remainder)-1)&0xff;
> 
> While I realize the callers use plain int, I''d be very much in
favor
> of not continuing this bad practice (the more that you use 1U
> already) - simply make the parameter and all locals (assuming
> you really think they''re useful; I would have omitted all but
> "remainder", given they''re being used just once)
unsigned.
I''ll fold them in, it was convenient to have variables while I was
printk''ing what I was doing but not any more.
> And once at it, please insert spaces consistently and drop the
> bogus "&0xff".
Done.

8<----------------------------------------------------

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1346939305 -3600
# Node ID 8652dbb4808f66bc9d85690a5e50fd5b972c6530
# Parent  c0e71dc105f785caccee3e1aec2b18f119e60f04
xen: clamp bitmaps to correct number of bits

Valgrind running xl create reports:
 ==24777== Invalid read of size 4
 ==24777==    at 0x4072805: libxl__get_numa_candidate (libxl_numa.c:203)
 ==24777==    by 0x40680B6: libxl__build_pre (libxl_dom.c:166)
 ==24777==    by 0x405B82E: libxl__domain_build (libxl_create.c:323)
 ==24777==    by 0x405BB9C: domcreate_bootloader_done (libxl_create.c:747)
 ==24777==    by 0x407AD27: bootloader_local_detached_cb
(libxl_bootloader.c:281)
 ==24777==    by 0x40508D8: local_device_detach_cb (libxl.c:2470)
 ==24777==    by 0x4052B10: libxl__device_disk_local_initiate_detach
(libxl.c:2445)
 ==24777==    by 0x407AE9F: bootloader_callback (libxl_bootloader.c:265)
 ==24777==    by 0x407C69A: libxl__bootloader_run (libxl_bootloader.c:392)
 ==24777==    by 0x405CB24: do_domain_create (libxl_create.c:687)
 ==24777==    by 0x405CC5E: libxl_domain_create_new (libxl_create.c:1177)
 ==24777==    by 0x805BDE2: create_domain (xl_cmdimpl.c:1812)
 ==24777==  Address 0x42dbdd8 is 8 bytes after a block of size 48
alloc''d
 ==24777==    at 0x4023340: calloc (vg_replace_malloc.c:593)
 ==24777==    by 0x406D479: libxl__zalloc (libxl_internal.c:88)
 ==24777==    by 0x404EF38: libxl_get_cpu_topology (libxl.c:3707)
 ==24777==    by 0x4072232: libxl__get_numa_candidate (libxl_numa.c:314)
 ==24777==    by 0x40680B6: libxl__build_pre (libxl_dom.c:166)
 ==24777==    by 0x405B82E: libxl__domain_build (libxl_create.c:323)
 ==24777==    by 0x405BB9C: domcreate_bootloader_done (libxl_create.c:747)
 ==24777==    by 0x407AD27: bootloader_local_detached_cb
(libxl_bootloader.c:281)
 ==24777==    by 0x40508D8: local_device_detach_cb (libxl.c:2470)
 ==24777==    by 0x4052B10: libxl__device_disk_local_initiate_detach
(libxl.c:2445)
 ==24777==    by 0x407AE9F: bootloader_callback (libxl_bootloader.c:265)
 ==24777==    by 0x407C69A: libxl__bootloader_run (libxl_bootloader.c:392)

This is because with nr_cpus=4 the bitmask returned from Xen
contains 0xff rather than 0x0f bit our bitmap walking routines (e.g.
libxl_for_each_set_bit) round up to the next byte (so it iterates
e.g. 8 times not 4). This then causes us to access of the end of
whatever array we are walking through each set bit for.

The principal of least surprise suggests that these bits ought not to
be set and this is not a hot path so fix this at the hypervisor layer
by clamping the bits in the returned bitmap to the correct limit.

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

The impact of this seems to be fairly benign in practice, so I''m not
sure about it for 4.2.0. Could leave to 4.2.1?

Dario,

Any chance you could look at the libxl bitmap functions in 4.3 and
make them use the right limit (i.e. nr_bits not (nr_bits+7)/8?

Thanks,
Ian.

diff -r c0e71dc105f7 -r 8652dbb4808f xen/common/bitmap.c
--- a/xen/common/bitmap.c	Thu Sep 06 14:36:09 2012 +0100
+++ b/xen/common/bitmap.c	Thu Sep 06 14:48:25 2012 +0100
@@ -38,6 +38,21 @@
  * for the best explanations of this ordering.
  */
 
+/*
+ * If a bitmap has a number of bits which is not a multiple of 8 then
+ * the last few bits of the last byte of the bitmap can be
+ * unexpectedly set which can confuse consumers (e.g. in the tools)
+ * who also round up their loops to 8 bits. Ensure we clear those left
+ * over bits so as to prevent surprises.
+ */
+static void clamp_last_byte(uint8_t *bp, int nbits)
+{
+	int remainder = nbits % 8;
+
+	if (remainder)
+		bp[nbits/8] &= ((1U<<remainder)-1);
+}
+
 int __bitmap_empty(const unsigned long *bitmap, int bits)
 {
 	int k, lim = bits/BITS_PER_LONG;
@@ -485,6 +500,7 @@ void bitmap_long_to_byte(uint8_t *bp, co
 			nbits -= 8;
 		}
 	}
+	clamp_last_byte(bp, nbits);
 }
 
 void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp, int nbits)
@@ -507,6 +523,7 @@ void bitmap_byte_to_long(unsigned long *
 void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp, int nbits)
 {
 	memcpy(bp, lp, (nbits+7)/8);
+	clamp_last_byte(bp, nbits);
 }
 
 void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp, int nbits)

Jan Beulich

2012-Sep-06 14:47 UTC

head link

Re: [PATCH] xen: clamp bitmaps to correct number of bits

>>> On 06.09.12 at 15:48, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Thu, 2012-09-06 at 13:23 +0100, Jan Beulich wrote:
>> >>> On 06.09.12 at 13:56, Ian Campbell
<ian.campbell@citrix.com> wrote:
>> > +/* Ensure that the last byte is zero from nbits onwards. */
>> > +static void clamp_last_byte(uint8_t *bp, int nbits)
>> > +{
>> > +	int lastbyte = nbits/8;
>> > +	int remainder = nbits % 8;
>> > +	int mask = ((1U<<remainder)-1)&0xff;
>> 
>> While I realize the callers use plain int, I''d be very much in
favor
>> of not continuing this bad practice (the more that you use 1U
>> already) - simply make the parameter and all locals (assuming
>> you really think they''re useful; I would have omitted all but
>> "remainder", given they''re being used just once)
unsigned.
> 
> I''ll fold them in, it was convenient to have variables while I was
> printk''ing what I was doing but not any more.
I won''t ask you for another round because of this, but you
still left the parameter and remaining local variable as plain
int, nor did you insert whitespace into the expressions. If I
were the one to commit this, I would do the adjustment while
committing...

Anyway, as long as there''s no easily visible tools side bug
addressed by this, I would think we should rather leave this
for after branching - Keir?

Jan

Keir Fraser

2012-Sep-06 15:51 UTC

head link

Re: [PATCH] xen: clamp bitmaps to correct number of bits

On 06/09/2012 15:47, "Jan Beulich" <JBeulich@suse.com> wrote:
>> I''ll fold them in, it was convenient to have variables while I
was
>> printk''ing what I was doing but not any more.
> 
> I won''t ask you for another round because of this, but you
> still left the parameter and remaining local variable as plain
> int, nor did you insert whitespace into the expressions. If I
> were the one to commit this, I would do the adjustment while
> committing...
> 
> Anyway, as long as there''s no easily visible tools side bug
> addressed by this, I would think we should rather leave this
> for after branching - Keir?
Yes, we''ll leave it for post 4.2.0.

Ian Campbell

2012-Sep-06 16:20 UTC

head link

Re: [PATCH] xen: clamp bitmaps to correct number of bits

On Thu, 2012-09-06 at 15:47 +0100, Jan Beulich wrote:> >>> On 06.09.12 at 15:48, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> > On Thu, 2012-09-06 at 13:23 +0100, Jan Beulich wrote:
> >> >>> On 06.09.12 at 13:56, Ian Campbell
<ian.campbell@citrix.com> wrote:
> >> > +/* Ensure that the last byte is zero from nbits onwards. */
> >> > +static void clamp_last_byte(uint8_t *bp, int nbits)
> >> > +{
> >> > +	int lastbyte = nbits/8;
> >> > +	int remainder = nbits % 8;
> >> > +	int mask = ((1U<<remainder)-1)&0xff;
> >> 
> >> While I realize the callers use plain int, I''d be very
much in favor
> >> of not continuing this bad practice (the more that you use 1U
> >> already) - simply make the parameter and all locals (assuming
> >> you really think they''re useful; I would have omitted all
but
> >> "remainder", given they''re being used just
once) unsigned.
> > 
> > I''ll fold them in, it was convenient to have variables while
I was
> > printk''ing what I was doing but not any more.
> 
> I won''t ask you for another round because of this, but you
> still left the parameter and remaining local variable as plain
> int,
Sorry, I forgot this bit after I nuked the variables. I''ve respun
(below)
>  nor did you insert whitespace into the expressions. If I
> were the one to commit this, I would do the adjustment while
> committing...
This whole file seems to use Linux coding style which is why I omitted
spaces inside the if. I made the mask "(1U << remainder) - 1"
and
simultaneously drop the superfluous extra brackets in v2 left over from
removing &0xff. 
> 
> Anyway, as long as there''s no easily visible tools side bug
> addressed by this, I would think we should rather leave this
> for after branching - Keir?
I''m fine with that.

Ian

8<-----------------------------------------------

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1346947834 -3600
# Node ID adf93d46186bb9b4d39aa195cf3b2445499c87a1
# Parent  6458749bcd38365bc30ae8adac608619e6eec382
xen: clamp bitmaps to correct number of bits

Valgrind running xl create reports:
 ==24777== Invalid read of size 4
 ==24777==    at 0x4072805: libxl__get_numa_candidate (libxl_numa.c:203)
 ==24777==    by 0x40680B6: libxl__build_pre (libxl_dom.c:166)
 ==24777==    by 0x405B82E: libxl__domain_build (libxl_create.c:323)
 ==24777==    by 0x405BB9C: domcreate_bootloader_done (libxl_create.c:747)
 ==24777==    by 0x407AD27: bootloader_local_detached_cb
(libxl_bootloader.c:281)
 ==24777==    by 0x40508D8: local_device_detach_cb (libxl.c:2470)
 ==24777==    by 0x4052B10: libxl__device_disk_local_initiate_detach
(libxl.c:2445)
 ==24777==    by 0x407AE9F: bootloader_callback (libxl_bootloader.c:265)
 ==24777==    by 0x407C69A: libxl__bootloader_run (libxl_bootloader.c:392)
 ==24777==    by 0x405CB24: do_domain_create (libxl_create.c:687)
 ==24777==    by 0x405CC5E: libxl_domain_create_new (libxl_create.c:1177)
 ==24777==    by 0x805BDE2: create_domain (xl_cmdimpl.c:1812)
 ==24777==  Address 0x42dbdd8 is 8 bytes after a block of size 48
alloc''d
 ==24777==    at 0x4023340: calloc (vg_replace_malloc.c:593)
 ==24777==    by 0x406D479: libxl__zalloc (libxl_internal.c:88)
 ==24777==    by 0x404EF38: libxl_get_cpu_topology (libxl.c:3707)
 ==24777==    by 0x4072232: libxl__get_numa_candidate (libxl_numa.c:314)
 ==24777==    by 0x40680B6: libxl__build_pre (libxl_dom.c:166)
 ==24777==    by 0x405B82E: libxl__domain_build (libxl_create.c:323)
 ==24777==    by 0x405BB9C: domcreate_bootloader_done (libxl_create.c:747)
 ==24777==    by 0x407AD27: bootloader_local_detached_cb
(libxl_bootloader.c:281)
 ==24777==    by 0x40508D8: local_device_detach_cb (libxl.c:2470)
 ==24777==    by 0x4052B10: libxl__device_disk_local_initiate_detach
(libxl.c:2445)
 ==24777==    by 0x407AE9F: bootloader_callback (libxl_bootloader.c:265)
 ==24777==    by 0x407C69A: libxl__bootloader_run (libxl_bootloader.c:392)

This is because with nr_cpus=4 the bitmask returned from Xen
contains 0xff rather than 0x0f bit our bitmap walking routines (e.g.
libxl_for_each_set_bit) round up to the next byte (so it iterates
e.g. 8 times not 4). This then causes us to access of the end of
whatever array we are walking through each set bit for.

The principal of least surprise suggests that these bits ought not to
be set and this is not a hot path so fix this at the hypervisor layer
by clamping the bits in the returned bitmap to the correct limit.

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

The impact of this seems to be fairly benign in practice, so I''m not
sure about it for 4.2.0. Could leave to 4.2.1?

Dario,

Any chance you could look at the libxl bitmap functions in 4.3 and
make them use the right limit (i.e. nr_bits not (nr_bits+7)/8?

Thanks,
Ian.

diff -r 6458749bcd38 -r adf93d46186b xen/common/bitmap.c
--- a/xen/common/bitmap.c	Thu Sep 06 17:04:25 2012 +0100
+++ b/xen/common/bitmap.c	Thu Sep 06 17:10:34 2012 +0100
@@ -38,6 +38,21 @@
  * for the best explanations of this ordering.
  */
 
+/*
+ * If a bitmap has a number of bits which is not a multiple of 8 then
+ * the last few bits of the last byte of the bitmap can be
+ * unexpectedly set which can confuse consumers (e.g. in the tools)
+ * who also round up their loops to 8 bits. Ensure we clear those left
+ * over bits so as to prevent surprises.
+ */
+static void clamp_last_byte(uint8_t *bp, unsigned int nbits)
+{
+	unsigned int remainder = nbits % 8;
+
+	if (remainder)
+		bp[nbits/8] &= (1U << remainder) - 1;
+}
+
 int __bitmap_empty(const unsigned long *bitmap, int bits)
 {
 	int k, lim = bits/BITS_PER_LONG;
@@ -485,6 +500,7 @@ void bitmap_long_to_byte(uint8_t *bp, co
 			nbits -= 8;
 		}
 	}
+	clamp_last_byte(bp, nbits);
 }
 
 void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp, int nbits)
@@ -507,6 +523,7 @@ void bitmap_byte_to_long(unsigned long *
 void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp, int nbits)
 {
 	memcpy(bp, lp, (nbits+7)/8);
+	clamp_last_byte(bp, nbits);
 }
 
 void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp, int nbits)

Jan Beulich

2012-Sep-07 08:17 UTC

head link

Re: [PATCH] xen: clamp bitmaps to correct number of bits

>>> On 06.09.12 at 18:20, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Thu, 2012-09-06 at 15:47 +0100, Jan Beulich wrote:
>>  nor did you insert whitespace into the expressions. If I
>> were the one to commit this, I would do the adjustment while
>> committing...
> 
> This whole file seems to use Linux coding style which is why I omitted
> spaces inside the if. I made the mask "(1U << remainder) -
1" and
> simultaneously drop the superfluous extra brackets in v2 left over from
> removing &0xff. 
Oh yes, that''s what I meant; I didn''t want to you add white
space that the Xen coding style asks for, but Linux''es
doesn''t.
>> Anyway, as long as there''s no easily visible tools side bug
>> addressed by this, I would think we should rather leave this
>> for after branching - Keir?
> 
> I''m fine with that.
Fell free to put my ack on it when committing (but you''ll need
a separate ack from Keir anyway I believe).

Jan

Ian Campbell

2012-Sep-07 08:23 UTC

head link

Re: [PATCH] xen: clamp bitmaps to correct number of bits

On Fri, 2012-09-07 at 09:17 +0100, Jan Beulich wrote:> >>> On 06.09.12 at 18:20, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> > On Thu, 2012-09-06 at 15:47 +0100, Jan Beulich wrote:
> >>  nor did you insert whitespace into the expressions. If I
> >> were the one to commit this, I would do the adjustment while
> >> committing...
> > 
> > This whole file seems to use Linux coding style which is why I omitted
> > spaces inside the if. I made the mask "(1U << remainder) -
1" and
> > simultaneously drop the superfluous extra brackets in v2 left over
from
> > removing &0xff. 
> 
> Oh yes, that''s what I meant; I didn''t want to you add
white
> space that the Xen coding style asks for, but Linux''es
doesn''t.
> 
> >> Anyway, as long as there''s no easily visible tools side
bug
> >> addressed by this, I would think we should rather leave this
> >> for after branching - Keir?
> > 
> > I''m fine with that.
> 
> Fell free to put my ack on it when committing (but you''ll need
> a separate ack from Keir anyway I believe).
Right, I wouldn''t normally commit to the xen subtree even with Acks
from
you both anyway, but an explicit "please commit" would cause me to do
so.

Ian.

Jan Beulich

2012-Sep-07 08:33 UTC

head link

Re: [PATCH] xen: clamp bitmaps to correct number of bits

>>> On 07.09.12 at 10:23, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> Right, I wouldn''t normally commit to the xen subtree even with
Acks from
> you both anyway, but an explicit "please commit" would cause me
to do
> so.
I think both Keir and I imply permission to commit with an ack (in
my case, that permission of course is limited to the portions of
the tree for which I''m listed as maintainer). Keir, please correct
me if that''s a wrong understanding of mine.

Jan

Keir Fraser

2012-Sep-07 09:20 UTC

head link

Re: [PATCH] xen: clamp bitmaps to correct number of bits

On 07/09/2012 09:33, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 07.09.12 at 10:23, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>> Right, I wouldn''t normally commit to the xen subtree even with
Acks from
>> you both anyway, but an explicit "please commit" would cause
me to do
>> so.
> 
> I think both Keir and I imply permission to commit with an ack (in
> my case, that permission of course is limited to the portions of
> the tree for which I''m listed as maintainer). Keir, please correct
> me if that''s a wrong understanding of mine.
I''m not too strict, but I''d say that it takes very little to
add "and please
commit", and avoids confusion where both try to commit at the same time.

For this specific patch, I expect Jan to commit it, as he has a couple of
trivial cleanups he wants to do at the same time. And with that:
Acked-by: Keir Fraser <keir@xen.org>

Jan, you can check it in as soon as 4.3 development opens (hopefully later
today!).

 -- Keir

> Jan
>

Xen devel - Sep 2012 - [PATCH] xen: clamp bitmaps to correct number of bits

[PATCH] xen: clamp bitmaps to correct number of bits

Re: [PATCH] xen: clamp bitmaps to correct number of bits

Re: [PATCH] xen: clamp bitmaps to correct number of bits

Re: [PATCH] xen: clamp bitmaps to correct number of bits

Re: [PATCH] xen: clamp bitmaps to correct number of bits

Re: [PATCH] xen: clamp bitmaps to correct number of bits

Re: [PATCH] xen: clamp bitmaps to correct number of bits

Re: [PATCH] xen: clamp bitmaps to correct number of bits

Re: [PATCH] xen: clamp bitmaps to correct number of bits

Re: [PATCH] xen: clamp bitmaps to correct number of bits