[PATCH] blktap2: update connection handling to fix build with gcc5

[PATCH] blktap2: update connection handling to fix build with gcc5

[Olaf Hering]

blktap2 fails to build with gcc5 because it fails to recognize that
there can be just one active connection (enforced in ctl_accept).

Rearrange the code to handle just a single connection.
Adjust two strerror calls to use errno instead -1 as input.

[  198s] block-log.c: In function 'ctl_close_sock':
[  198s] block-log.c:363:23: error: array subscript is above array bounds [-Werror=array-bounds]
[  198s]      if (s->connections[i].fd == fd) {
[  198s]                        ^
[  198s] block-log.c: In function 'ctl_request':
[  198s] block-log.c:549:23: error: array subscript is above array bounds [-Werror=array-bounds]
[  198s]      if (s->connections[i].id == id)
[  198s]                        ^
[  198s] cc1: all warnings being treated as errors
[  198s] /home/abuild/rpmbuild/BUILD/xen-4.6.31382/non-dbg/tools/blktap2/drivers/../../../tools/Rules.mk:107: recipe for target 'block-log.o' failed
[  198s] make[5]: *** [block-log.o] Error 1

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---

This is just compile tested.


 tools/blktap2/drivers/block-log.c | 49 +++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/tools/blktap2/drivers/block-log.c b/tools/blktap2/drivers/block-log.c
index 5330cdc..10a6966 100644
--- a/tools/blktap2/drivers/block-log.c
+++ b/tools/blktap2/drivers/block-log.c
@@ -54,8 +54,6 @@
 #include "tapdisk-driver.h"
 #include "tapdisk-interface.h"
 
-#define MAX_CONNECTIONS 1
-
 typedef struct poll_fd {
   int          fd;
   event_id_t   id;
@@ -69,8 +67,7 @@ struct tdlog_state {
   char*        ctlpath;
   poll_fd_t    ctl;
 
-  int          connected;
-  poll_fd_t    connections[MAX_CONNECTIONS];
+  poll_fd_t    connection;
 
   char*        shmpath;
   void*        shm;
@@ -305,7 +302,7 @@ static int ctl_open(struct tdlog_state* s, const char* name)
   s->ctl.id = tapdisk_server_register_event(SCHEDULER_POLL_READ_FD,
 					    s->ctl.fd, 0, ctl_accept, s);
   if (s->ctl.id < 0) {
-    BWPRINTF("error register event handler: %s", strerror(s->ctl.id));
+    BWPRINTF("error register event handler: %s", strerror(errno));
     goto err_sock;
   }
 
@@ -323,12 +320,11 @@ static int ctl_open(struct tdlog_state* s, const char* name)
 
 static int ctl_close(struct tdlog_state* s)
 {
-  while (s->connected) {
-    s->connected--;
-    tapdisk_server_unregister_event(s->connections[s->connected].id);
-    close(s->connections[s->connected].fd);
-    s->connections[s->connected].fd = -1;
-    s->connections[s->connected].id = 0;
+  if (s->connection.fd >= 0) {
+    tapdisk_server_unregister_event(s->connection.id);
+    close(s->connection.fd);
+    s->connection.fd = -1;
+    s->connection.id = 0;
   }
 
   if (s->ctl.fd >= 0) {
@@ -359,15 +355,12 @@ static int ctl_close_sock(struct tdlog_state* s, int fd)
 {
   int i;
 
-  for (i = 0; i < s->connected; i++) {
-    if (s->connections[i].fd == fd) {
-      tapdisk_server_unregister_event(s->connections[i].id);
-      close(s->connections[i].fd);
-      s->connections[i].fd = -1;
-      s->connections[i].id = 0;
-      s->connected--;
-      return 0;
-    }
+  if (fd >= 0 && s->connection.fd == fd) {
+    tapdisk_server_unregister_event(s->connection.id);
+    close(s->connection.fd);
+    s->connection.fd = -1;
+    s->connection.id = 0;
+    return 0;
   }
 
   BWPRINTF("requested to close unknown socket %d", fd);
@@ -385,7 +378,7 @@ static void ctl_accept(event_id_t id, char mode, void *private)
     return;
   }
 
-  if (s->connected) {
+  if (s->connection.fd >= 0) {
     BWPRINTF("control session in progress, closing new connection");
     close(fd);
     return;
@@ -394,14 +387,13 @@ static void ctl_accept(event_id_t id, char mode, void *private)
   cid = tapdisk_server_register_event(SCHEDULER_POLL_READ_FD,
 				      fd, 0, ctl_request, s);
   if (cid < 0) {
-    BWPRINTF("error registering connection event handler: %s", strerror(cid));
+    BWPRINTF("error registering connection event handler: %s", strerror(errno));
     close(fd);
     return;
   }
 
-  s->connections[s->connected].fd = fd;
-  s->connections[s->connected].id = cid;
-  s->connected++;
+  s->connection.fd = fd;
+  s->connection.id = cid;
 }
 
 /* response format: 4 bytes shmsize, 0-terminated path */
@@ -545,9 +537,8 @@ static inline int ctl_find_connection(struct tdlog_state *s, event_id_t id)
 {
   int i;
 
-  for (i = 0; i < s->connected; i++)
-    if (s->connections[i].id == id)
-      return s->connections[i].fd;
+  if (s->connection.fd >= 0 && s->connection.id == id)
+      return s->connection.fd;
 
   BWPRINTF("unrecognized event callback id %d", id);
   return -1;
@@ -593,6 +584,8 @@ static int tdlog_open(td_driver_t* driver, const char* name, td_flag_t flags)
   memset(s, 0, sizeof(*s));
 
   s->size = driver->info.size;
+  s->connection.fd = -1;
+  s->ctl.fd = -1;
 
   if ((rc = writelog_create(s))) {
     tdlog_close(driver);

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[Jan Beulich]

>>> On 19.07.15 at 11:33, <olaf@aepfle.de> wrote:
> blktap2 fails to build with gcc5 because it fails to recognize that
> there can be just one active connection (enforced in ctl_accept).
> 
> Rearrange the code to handle just a single connection.
> Adjust two strerror calls to use errno instead -1 as input.
> 
> [  198s] block-log.c: In function 'ctl_close_sock':
> [  198s] block-log.c:363:23: error: array subscript is above array bounds [-Werror=array-bounds]
> [  198s]      if (s->connections[i].fd == fd) {
> [  198s]                        ^
> [  198s] block-log.c: In function 'ctl_request':
> [  198s] block-log.c:549:23: error: array subscript is above array bounds [-Werror=array-bounds]
> [  198s]      if (s->connections[i].id == id)
> [  198s]                        ^

So what makes the compiler right with that complaint? I.e. how does
it know i > 0 here? After all - afaict - s->connected can only be 0 or
1, and hence either the loop bodies don't get entered at all or the
loops have only a single iteration. Smells like a compiler bug instead,
which I'm not sure we want to work around with a non-trivial patch
like this.

Jan

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[Olaf Hering]

On Mon, Jul 20, Jan Beulich wrote:

> >>> On 19.07.15 at 11:33, <olaf@aepfle.de> wrote:
> > [  198s] block-log.c:549:23: error: array subscript is above array bounds [-Werror=array-bounds]
> > [  198s]      if (s->connections[i].id == id)
> > [  198s]                        ^
> 
> So what makes the compiler right with that complaint? I.e. how does
> it know i > 0 here? After all - afaict - s->connected can only be 0 or

It has to assume that ->connected can get any value because the input
comes from outside the unit.

To reduce the patch size "&& i < MAX_CONNECTIONS" could be added.

Olaf

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[Wei Liu]

On Mon, Jul 20, 2015 at 11:16:34AM +0200, Olaf Hering wrote:
> On Mon, Jul 20, Jan Beulich wrote:
> 
> > >>> On 19.07.15 at 11:33, <olaf@aepfle.de> wrote:
> > > [  198s] block-log.c:549:23: error: array subscript is above array bounds [-Werror=array-bounds]
> > > [  198s]      if (s->connections[i].id == id)
> > > [  198s]                        ^
> > 
> > So what makes the compiler right with that complaint? I.e. how does
> > it know i > 0 here? After all - afaict - s->connected can only be 0 or
> 
> It has to assume that ->connected can get any value because the input
> comes from outside the unit.
> 
> To reduce the patch size "&& i < MAX_CONNECTIONS" could be added.
> 

A smaller patch would be preferable at this stage.

Wei.

> Olaf

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[Olaf Hering]

On Mon, Jul 20, Wei Liu wrote:

> On Mon, Jul 20, 2015 at 11:16:34AM +0200, Olaf Hering wrote:
> > On Mon, Jul 20, Jan Beulich wrote:
> > 
> > > >>> On 19.07.15 at 11:33, <olaf@aepfle.de> wrote:
> > > > [  198s] block-log.c:549:23: error: array subscript is above array bounds [-Werror=array-bounds]
> > > > [  198s]      if (s->connections[i].id == id)
> > > > [  198s]                        ^
> > > 
> > > So what makes the compiler right with that complaint? I.e. how does
> > > it know i > 0 here? After all - afaict - s->connected can only be 0 or
> > 
> > It has to assume that ->connected can get any value because the input
> > comes from outside the unit.
> > 
> > To reduce the patch size "&& i < MAX_CONNECTIONS" could be added.
> > 
> 
> A smaller patch would be preferable at this stage.

I disagree with that.

What is the longterm goal of that binary?
Will it ever be able to handle more than one connection? If so the loops
have to handle holes in ->connections[], which increases patch size.

If it will ever handle only a single conection, please review and spot
possible errors in my patch.

Olaf

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[Jan Beulich]

>>> On 21.07.15 at 08:32, <olaf@aepfle.de> wrote:
> On Mon, Jul 20, Wei Liu wrote:
> 
>> On Mon, Jul 20, 2015 at 11:16:34AM +0200, Olaf Hering wrote:
>> > On Mon, Jul 20, Jan Beulich wrote:
>> > 
>> > > >>> On 19.07.15 at 11:33, <olaf@aepfle.de> wrote:
>> > > > [  198s] block-log.c:549:23: error: array subscript is above array bounds 
> [-Werror=array-bounds]
>> > > > [  198s]      if (s->connections[i].id == id)
>> > > > [  198s]                        ^
>> > > 
>> > > So what makes the compiler right with that complaint? I.e. how does
>> > > it know i > 0 here? After all - afaict - s->connected can only be 0 or
>> > 
>> > It has to assume that ->connected can get any value because the input
>> > comes from outside the unit.
>> > 
>> > To reduce the patch size "&& i < MAX_CONNECTIONS" could be added.
>> > 
>> 
>> A smaller patch would be preferable at this stage.
> 
> I disagree with that.

Please consider that we're in code freeze right now.

> What is the longterm goal of that binary?

It being mostly unmaintained, it's probably a candidate for removal
not too far in the future. Which would be another reason again
extensive changes.

But anyway, the primary question remains - isn't what you're seeing
a compiler bug? If yes, that's imo _yet another_ reason for doing a
minimal workaround (if any at all).

Jan

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[Olaf Hering]

On Tue, Jul 21, Jan Beulich wrote:

> But anyway, the primary question remains - isn't what you're seeing
> a compiler bug? If yes, that's imo _yet another_ reason for doing a
> minimal workaround (if any at all).

I dont think its a compiler bug. How should the compiler know that the
index variable matches the array size? If the code would modify index
and array all inside the same unit it may have a chance to see that it
can never be above array_size. But as it stands the pointer comes from
outside the unit.

One could argue that the struct is defined just inside that unit and it
gets passed back in as a void pointer. So an optimistic view on the
issue could be: noone knows what "void *p" is all about, so nothing
would touch it. But gcc doesnt work that way. Not sure if it should.

Olaf

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[Jan Beulich]

>>> On 21.07.15 at 09:04, <olaf@aepfle.de> wrote:
> On Tue, Jul 21, Jan Beulich wrote:
>> But anyway, the primary question remains - isn't what you're seeing
>> a compiler bug? If yes, that's imo _yet another_ reason for doing a
>> minimal workaround (if any at all).
> 
> I dont think its a compiler bug. How should the compiler know that the
> index variable matches the array size? If the code would modify index
> and array all inside the same unit it may have a chance to see that it
> can never be above array_size. But as it stands the pointer comes from
> outside the unit.

I can only repeat what I said before - that's not the right perspective
to take. Otherwise code like this

void test(char ptr[3], unsigned idx) {
	unsigned i;

	for(i = 0; i < idx; ++i)
		func(ptr[i]);
}

would need to get warned about too, regardless of whether there
is any caller actually passing other than 0, 1, or 2 for "idx". I.e. the
compiler may guess that there could be something wrong here, but
it certainly can't _know_. Replace the 3 above by 1, and you have
the equivalent of what the patch is about.

> One could argue that the struct is defined just inside that unit and it
> gets passed back in as a void pointer. So an optimistic view on the
> issue could be: noone knows what "void *p" is all about, so nothing
> would touch it. But gcc doesnt work that way. Not sure if it should.

Again I don't think the structure definition is the issue, but (wrong)
assumptions made on the array index. Or did you spot a place
where the array index is known to be non-zero?

Jan

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[Ian Campbell]

On Tue, 2015-07-21 at 00:50 -0600, Jan Beulich wrote:

> > What is the longterm goal of that binary?
> 
> It being mostly unmaintained, it's probably a candidate for removal
> not too far in the future.

George already posted a series to remove the in tree copy and treat
blktap2 as an out of tree project[0]. That series isn't going to hit
4.6, obviously, but I'd expect/hope it to go in during 4.7.

Ian.

[0] upstream is the xenserver.org fork IIRC.

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[George Dunlap]

On 07/21/2015 09:38 AM, Ian Campbell wrote:
> On Tue, 2015-07-21 at 00:50 -0600, Jan Beulich wrote:
> 
>>> What is the longterm goal of that binary?
>>
>> It being mostly unmaintained, it's probably a candidate for removal
>> not too far in the future.
> 
> George already posted a series to remove the in tree copy and treat
> blktap2 as an out of tree project[0]. That series isn't going to hit
> 4.6, obviously, but I'd expect/hope it to go in during 4.7.

Yes -- in fact, there have already been checked in block scripts which
allow one to use the out-of-tree blktap.

There are still some changes to the XenServer blktap to allow it to be
built out-of-tree -- let me know if you want those.  (I'll try to
upstream them before I re-submit the blktap2 removal patches.)

 -George

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[Jan Beulich]

>>> On 20.07.15 at 11:16, <olaf@aepfle.de> wrote:
> On Mon, Jul 20, Jan Beulich wrote:
> 
>> >>> On 19.07.15 at 11:33, <olaf@aepfle.de> wrote:
>> > [  198s] block-log.c:549:23: error: array subscript is above array bounds 
> [-Werror=array-bounds]
>> > [  198s]      if (s->connections[i].id == id)
>> > [  198s]                        ^
>> 
>> So what makes the compiler right with that complaint? I.e. how does
>> it know i > 0 here? After all - afaict - s->connected can only be 0 or
> 
> It has to assume that ->connected can get any value because the input
> comes from outside the unit.

But then the warning can't be "is" but only "may be", and a similar
warning would appear for _all_ other array accesses with compile
time known upper bound (and that's clearly not the case). I'm
guessing that the compiler wrongly applies a heuristic that every
loop over an array has at least two iteration (or some such).

Jan

Re: [PATCH] blktap2: update connection handling to fix build with gcc5

[M A Young]

On Mon, 20 Jul 2015, Jan Beulich wrote:

> >>> On 19.07.15 at 11:33, <olaf@aepfle.de> wrote:
> > blktap2 fails to build with gcc5 because it fails to recognize that
> > there can be just one active connection (enforced in ctl_accept).
> > 
> > Rearrange the code to handle just a single connection.
> > Adjust two strerror calls to use errno instead -1 as input.
> > 
> > [  198s] block-log.c: In function 'ctl_close_sock':
> > [  198s] block-log.c:363:23: error: array subscript is above array bounds [-Werror=array-bounds]
> > [  198s]      if (s->connections[i].fd == fd) {
> > [  198s]                        ^
> > [  198s] block-log.c: In function 'ctl_request':
> > [  198s] block-log.c:549:23: error: array subscript is above array bounds [-Werror=array-bounds]
> > [  198s]      if (s->connections[i].id == id)
> > [  198s]                        ^
> 
> So what makes the compiler right with that complaint? I.e. how does
> it know i > 0 here? After all - afaict - s->connected can only be 0 or
> 1, and hence either the loop bodies don't get entered at all or the
> loops have only a single iteration. Smells like a compiler bug instead,
> which I'm not sure we want to work around with a non-trivial patch
> like this.

Does this depend on the gcc 5 version? I think this is something I thought 
I was going to have to fix on Fedora and then found I didn't.

	Michael Young