Re: [Xen-devel] [RFC Patch v3 12/18] block-remus: fix bug in tdremus_close()

[Shriram Rajagopalan <rshriram@xxxxxxxxx>]

On Sep 5, 2014 5:16 AM, "Wen Congyang" <wency@xxxxxxxxxxxxxx> 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.
>

I don't recall facing this issue upto Xen 4.1 at least. Clearly tapdisk2 code has changed a lot or the tools talking to tapdisk2 are using different semantics.

Anyway, the code looks fine.
Acked-by: Shriram Rajagopalan <rshriram@xxxxxxxxx>

> This patch also does some cleanup.
>
> Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
> Cc: Shriram Rajagopalan <rshriram@xxxxxxxxx>
> ---
> Â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 23a908a..55363a3 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
>

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