Re: [PATCH 04/17] tools: block-remus: fix bug in tdremus_close()

[Wen Congyang <wency@cn.fujitsu.com>]

On 10/20/2014 11:01 AM, Shriram Rajagopalan wrote:
> On Oct 13, 2014 10:15 PM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>>
>> We close ctl_fd.fd, but we don't unregister ctl_fd.id. It will
>> cause select() return fails, and the user cannot talk with
>> tapdisk2.
>>
>> This patch also does some cleanup.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Acked-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
>> ---
>>  tools/blktap2/drivers/block-remus.c | 90
> ++++++++++++++++++++++---------------
>>  1 file changed, 53 insertions(+), 37 deletions(-)
>>
>> diff --git a/tools/blktap2/drivers/block-remus.c
> b/tools/blktap2/drivers/block-remus.c
>> index a2c08d8..fd5f209 100644
>> --- a/tools/blktap2/drivers/block-remus.c
>> +++ b/tools/blktap2/drivers/block-remus.c
>> @@ -151,9 +151,6 @@ typedef struct poll_fd {
>>  } poll_fd_t;
>>
>>  struct tdremus_state {
>> -//  struct tap_disk* driver;
>> -       void* driver_data;
>> -
>>    /* XXX: this is needed so that the server can perform operations on
>>     * the driver from the stream_fd event handler. fix this. */
>>         td_driver_t *tdremus_driver;
>> @@ -731,12 +728,26 @@ static int mwrite(int fd, void* buf, size_t len)
>>
>>  static void inline close_stream_fd(struct tdremus_state *s)
>>  {
>> +       if (s->stream_fd.fd < 0)
>> +               return;
>> +
>>         /* XXX: -2 is magic. replace with macro perhaps? */
>>         tapdisk_server_unregister_event(s->stream_fd.id);
>>         close(s->stream_fd.fd);
>>         s->stream_fd.fd = -2;
>>  }
>>
>> +static void close_server_fd(struct tdremus_state *s)
>> +{
>> +       if (s->server_fd.fd < 0)
>> +               return;
>> +
>> +       tapdisk_server_unregister_event(s->server_fd.id);
>> +       s->server_fd.id = -1;
>> +       close(s->stream_fd.fd);
>> +       s->stream_fd.fd = -1;
>> +}
>> +
>>  /* primary functions */
>>  static void remus_client_event(event_id_t, char mode, void *private);
>>  static void remus_connect_event(event_id_t id, char mode, void *private);
>> @@ -1347,12 +1358,7 @@ static int unprotected_start(td_driver_t *driver)
>>         /* close the server socket */
>>         close_stream_fd(s);
>>
>> -       /* unregister the replication stream */
>> -       tapdisk_server_unregister_event(s->server_fd.id);
>> -
>> -       /* close the replication stream */
>> -       close(s->server_fd.fd);
>> -       s->server_fd.fd = -1;
>> +       close_server_fd(s);
>>
>>         /* install the unprotected read/write handlers */
>>         tapdisk_remus.td_queue_read = unprotected_queue_read;
>> @@ -1553,27 +1559,27 @@ static int ctl_open(td_driver_t *driver, const
> char* name)
>>                         s->ctl_path[i] = '_';
>>         }
>>         if (asprintf(&s->msg_path, "%s.msg", s->ctl_path) < 0)
>> -               goto err_ctlfifo;
>> +               goto err_setmsgfifo;
>>
>>         if (mkfifo(s->ctl_path, S_IRWXU|S_IRWXG|S_IRWXO) && errno !=
> EEXIST) {
>>                 RPRINTF("error creating control FIFO %s: %d\n",
> s->ctl_path, errno);
>> -               goto err_msgfifo;
>> +               goto err_mkctlfifo;
>>         }
>>
>>         if (mkfifo(s->msg_path, S_IRWXU|S_IRWXG|S_IRWXO) && errno !=
> EEXIST) {
>>                 RPRINTF("error creating message FIFO %s: %d\n",
> s->msg_path, errno);
>> -               goto err_msgfifo;
>> +               goto err_mkmsgfifo;
>>         }
>>
>>         /* RDWR so that fd doesn't block select when no writer is present
> */
>>         if ((s->ctl_fd.fd = open(s->ctl_path, O_RDWR)) < 0) {
>>                 RPRINTF("error opening control FIFO %s: %d\n",
> s->ctl_path, errno);
>> -               goto err_msgfifo;
>> +               goto err_openctlfifo;
>>         }
>>
>>         if ((s->msg_fd.fd = open(s->msg_path, O_RDWR)) < 0) {
>>                 RPRINTF("error opening message FIFO %s: %d\n",
> s->msg_path, errno);
>> -               goto err_openctlfifo;
>> +               goto err_openmsgfifo;
>>         }
>>
>>         RPRINTF("control FIFO %s\n", s->ctl_path);
>> @@ -1581,36 +1587,45 @@ static int ctl_open(td_driver_t *driver, const
> char* name)
>>
>>         return 0;
>>
>> - err_openctlfifo:
>> +err_openmsgfifo:
>>         close(s->ctl_fd.fd);
>> - err_msgfifo:
>> +       s->ctl_fd.fd = -1;
>> +err_openctlfifo:
>> +       unlink(s->ctl_path);
>> +err_mkmsgfifo:
>> +       unlink(s->msg_path);
>> +err_mkctlfifo:
>>         free(s->msg_path);
>>         s->msg_path = NULL;
>> - err_ctlfifo:
>> +err_setmsgfifo:
>>         free(s->ctl_path);
>>         s->ctl_path = NULL;
>>         return -1;
>>  }
>>
>> -static void ctl_close(td_driver_t *driver)
>> +static void ctl_close(struct tdremus_state *s)
>>  {
>> -       struct tdremus_state *s = (struct tdremus_state *)driver->data;
>> -
>> -       /* TODO: close *all* connections */
>> -
>> -       if(s->ctl_fd.fd)
>> +       if(s->ctl_fd.fd) {
>>                 close(s->ctl_fd.fd);
>> +               s->ctl_fd.fd = -1;
>> +       }
>>
>>         if (s->ctl_path) {
>>                 unlink(s->ctl_path);
>>                 free(s->ctl_path);
>>                 s->ctl_path = NULL;
>>         }
>> +
>>         if (s->msg_path) {
>>                 unlink(s->msg_path);
>>                 free(s->msg_path);
>>                 s->msg_path = NULL;
>>         }
>> +
>> +       if (s->msg_fd.fd) {
>> +               close(s->msg_fd.fd);
>> +               s->msg_fd.fd = -1;
>> +       }
>>  }
>>
>>  static int ctl_register(struct tdremus_state *s)
>> @@ -1628,6 +1643,16 @@ static int ctl_register(struct tdremus_state *s)
>>         return 0;
>>  }
>>
>> +static void ctl_unregister(struct tdremus_state *s)
>> +{
>> +       RPRINTF("unregistering ctl fifo\n");
>> +
>> +       if (s->ctl_fd.id >= 0) {
>> +               tapdisk_server_unregister_event(s->ctl_fd.id);
>> +               s->ctl_fd.id = -1;
>> +       }
>> +}
>> +
>>  /* interface */
>>
>>  static int tdremus_open(td_driver_t *driver, td_image_t *image,
> td_uuid_t uuid)
>> @@ -1658,13 +1683,12 @@ static int tdremus_open(td_driver_t *driver,
> td_image_t *image, td_uuid_t uuid)
>>
>>         if ((rc = ctl_open(driver, name))) {
>>                 RPRINTF("error setting up control channel\n");
>> -               free(s->driver_data);
>>                 return rc;
>>         }
>>
>>         if ((rc = ctl_register(s))) {
>>                 RPRINTF("error registering control channel\n");
>> -               free(s->driver_data);
>> +               ctl_close(s);
>>                 return rc;
>>         }
>>
>> @@ -1687,19 +1711,11 @@ static int tdremus_close(td_driver_t *driver)
>>         RPRINTF("closing\n");
>>         if (s->ramdisk.inprogress)
>>                 hashtable_destroy(s->ramdisk.inprogress, 0);
>> -
>> -       if (s->driver_data) {
>> -               free(s->driver_data);
>> -               s->driver_data = NULL;
>> -       }
>> -       if (s->server_fd.fd >= 0) {
>> -               close(s->server_fd.fd);
>> -               s->server_fd.fd = -1;
>> -       }
>> -       if (s->stream_fd.fd >= 0)
>> -               close_stream_fd(s);
>>
>> -       ctl_close(driver);
>> +       close_server_fd(s);
>> +       close_stream_fd(s);
>> +       ctl_unregister(s);
>> +       ctl_close(s);
>>
>>         return 0;
>>  }
>> --
>> 1.9.3
>>
>>
>>
> 
> Acked-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>

Hmm, you have acked patch1-4...

Thanks
Wen Congyang

> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>