Opened 10 years ago

Closed 9 years ago

#151 closed defect (fixed)

XfroutClient socket location and removal

Reported by: jreed Owned by: jreed
Priority: medium Milestone: 05. 3rd Incremental Release: Serious Secondary
Component: xfrout Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

For the XfroutClient? socket location maybe use $localstatedir/run/auth_xfrout_conn instead?

Also remove the unused socket when b10-xfrout exits?

Subtickets

Attachments (2)

diff (613 bytes) - added by jinmei 10 years ago.
ticket151.diff (5.4 KB) - added by zhanglikun 9 years ago.
the patch for avoid more than one xfrout process starting at same time.

Download all attachments as: .zip

Change History (23)

comment:1 follow-up: Changed 10 years ago by jreed

When owned by someone else, I got this error:

Traceback (most recent call last):
  File "/home/jreed/tmp/bind10-devel-20100421/src/bin/xfrout/b10-xfrout", line 418, in <module>
    xfrout_server = XfroutServer()
  File "/home/jreed/tmp/bind10-devel-20100421/src/bin/xfrout/b10-xfrout", line 331, in __init__
    self._start_xfr_query_listener()
  File "/home/jreed/tmp/bind10-devel-20100421/src/bin/xfrout/b10-xfrout", line 342, in _start_xfr_query_listener
    self._shutdown_event, self._config_data);
  File "/home/jreed/tmp/bind10-devel-20100421/src/bin/xfrout/b10-xfrout", line 279, in __init__
    ThreadingUnixStreamServer.__init__(self, sock_file, handle_class)
  File "/opt/pkg/lib/python3.1/socketserver.py", line 400, in __init__
    self.server_bind()
  File "/opt/pkg/lib/python3.1/socketserver.py", line 411, in server_bind
    self.socket.bind(self.server_address)
socket.error: [Errno 98] Address already in use

Changed 10 years ago by jinmei

comment:2 in reply to: ↑ 1 Changed 10 years ago by jinmei

  • Owner set to jreed
  • Status changed from new to assigned

Replying to jreed:

When owned by someone else, I got this error:

Easy part first: the attached patch removes the unix domain socket on shutdown. Is this okay to commit?

comment:3 follow-up: Changed 10 years ago by zhanglikun

You can commit it temply, But sometime it doesn't work, I am working on a new patch for it.

comment:4 in reply to: ↑ 3 Changed 10 years ago by jinmei

  • Owner changed from jreed to zhanglikun

Replying to zhanglikun:

You can commit it temply, But sometime it doesn't work, I am working on a new patch for it.

Thanks for checking. But now that you've looked at it: I was feeling this might be done within the ThreadingUnixStreamServer class. A ThreadingUnixStreamServer object creats the file for communication on creation of itself, so it seems to me more natural that the file is removed on destroying the object. It just doesn't look intuitive we need to remove the file explicitly outside the class. Wouldn't there be a more natural way to handle this?

comment:5 follow-up: Changed 10 years ago by zhanglikun

  • Owner changed from zhanglikun to jreed

Hi Jeremy, I did some fix for this ticket in revision 1797. Yeah, as your suggestion, unused socket file is removed in class ThreadingUnixStreamServer?(Thanks for you suggestion). But the socket path is set as "$localstatedir/auth_xfrout_conn", instead of "$localstatedir/run/auth_xfrout_conn" suggested by you, since I don't want to create one folder named "run". Does it make sense?

comment:6 follow-up: Changed 10 years ago by zhanglikun

BTW, Jeremy, what's the scenario for the error you got? "When owned by someone else, I got this error". Did you start xfrout twice by hand?

comment:7 in reply to: ↑ 5 ; follow-up: Changed 10 years ago by jreed

It looks like the trunk/src/bin/auth/spec_config.h.in patch is:

+#define UNIX_SOCKET_FILE "@prefix@/var/auth_xfrout_conn"

which doesn't match new trunk/src/bin/xfrout/xfrout.py.in

+UNIX_SOCKET_FILE = "@localstatedir@".replace("${prefix}", PREFIX) + "/auth_xfrout_conn"

Also you can probably use @@LOCALSTATEDIR@@ instead of that hack above.

As for "run" it may not be needed. But in the long run, probably a specific directory to contain this socket will be needed for permissions -- like may not have permission to create it under /usr/local/var/ by default and don't want to open that directory up to be written by auth or xfrout. So something specific like @@LOCALSTATEDIR@@/run/bind10/xfrout/ may be desired.

comment:8 in reply to: ↑ 6 Changed 10 years ago by jreed

Replying to zhanglikun:

BTW, Jeremy, what's the scenario for the error you got? "When owned by someone else, I got this error". Did you start xfrout twice by hand?

This was caused by two different Unix users testing BIND 10 on the same system. (It was started twice using BIND 10 by different people.)

I think the solution is to give a nice warning about it instead of unfriendly traceback.

I probably should have opened a different ticket for this.

comment:9 in reply to: ↑ 7 Changed 10 years ago by zhanglikun

  • Status changed from assigned to reviewing

Replying to jreed:

It looks like the trunk/src/bin/auth/spec_config.h.in patch is:

+#define UNIX_SOCKET_FILE "@prefix@/var/auth_xfrout_conn"

which doesn't match new trunk/src/bin/xfrout/xfrout.py.in

+UNIX_SOCKET_FILE = "@localstatedir@".replace("${prefix}", PREFIX) + "/auth_xfrout_conn"

Also you can probably use @@LOCALSTATEDIR@@ instead of that hack above.

As for "run" it may not be needed. But in the long run, probably a specific directory to contain this socket will be needed for permissions -- like may not have permission to create it under /usr/local/var/ by default and don't want to open that directory up to be written by auth or xfrout. So something specific like @@LOCALSTATEDIR@@/run/bind10/xfrout/ may be desired.

Hi Jeremy, I create one new patch for this ticket as you suggested. please have a look, now xfrout works well both in source code tree and installed folder.

comment:10 Changed 10 years ago by zhanglikun

  • Owner changed from jreed to zhanglikun
  • Status changed from reviewing to accepted

Fixed in revision 1855. Now the path of UNIX_SOCKET_FILE is "@localstatedir@/auth_xfrout_conn". But if xfrout try to bind one UNIX_SOCKET_FILE which is used by another xfrout, there should be a friendly warning(Need to be down).

comment:11 Changed 10 years ago by zhanglikun

  • Component changed from Unclassified to xfrout
  • Milestone set to 04. 2nd Incremental Release
  • Owner changed from zhanglikun to shentingting
  • Status changed from accepted to assigned

comment:12 follow-up: Changed 9 years ago by zhanglikun

One Question: How to make sure there is only one xfrout process running on server? to check whether the socket file /usr/local/var/auth_xfrout_conn used by xfrout exist or not when starting? The problem is: when xfrout crashed last time, and /auth_xfrout_conn isn't deleted properly, you have to let user to delete /auth_xfrout_conn manually. Do this make sense?

comment:13 in reply to: ↑ 12 Changed 9 years ago by shane

Replying to zhanglikun:

One Question: How to make sure there is only one xfrout process running on server? to check whether the socket file /usr/local/var/auth_xfrout_conn used by xfrout exist or not when starting? The problem is: when xfrout crashed last time, and /auth_xfrout_conn isn't deleted properly, you have to let user to delete /auth_xfrout_conn manually. Do this make sense?

Likun and I talked about this. If you try to bind() to an AF_UNIX socket, it will fail if any file exists, even an unused AF_UNIX socket. So, what we will do is try to connect() first. In this case one of 3 things happens:

  1. If there is no file, you get a socket.error with ENOENT
  2. If there is a file, but it is not a socket or nobody is listening, you get socket.err with ECONNREFUSED
  3. If there is a listener, then the connect() will succeed

So if another copy of xfrout is running, the connect() will succeed, and we can exit with appropriate warning/error message at that time.

Otherwise, we get an error and we can safely start.

comment:14 Changed 9 years ago by shane

  • Milestone changed from 04. 2nd Incremental Release: Early Adopters to 05. 3rd Incremental Release

comment:15 Changed 9 years ago by zhanglikun

Hi shane, I create one patch file to implement the solution we had discussed. There isn't big change. so no new branch was created.

comment:16 Changed 9 years ago by shane

  • Owner changed from shentingting to zhanglikun

English correction:

"it becauses one other xfrout" -> "because another xfrout"

Comment typo:

"esle" -> "else"

Comment correction:

"should be removed" -> "will be removed"

It's not very important, but I would move the error message out of the _sock_file_in_use() function and into the __init()__ function before the sys.exit() call. This will make it easier to re-use the code if we want to have a similar technique elsewhere.

comment:17 follow-up: Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to jreed

Hi Jeremy, Could have a review of the patch I just created again, I just worry about whether the os.unlink(sock_file) will throw an exception caused by some privilege problem. that means if unlink the file failed, the xfrout process still need exit.

if err.errno == errno.ECONNREFUSED:

os.unlink(sock_file)

comment:18 in reply to: ↑ 17 Changed 9 years ago by shane

Replying to zhanglikun:

Hi Jeremy, Could have a review of the patch I just created again, I just worry about whether the os.unlink(sock_file) will throw an exception caused by some privilege problem. that means if unlink the file failed, the xfrout process still need exit.

if err.errno == errno.ECONNREFUSED:

os.unlink(sock_file)

Hm... good point. Actually this should probably be removed from the _sock_file_in_use() function. It is an unexpected side-effect, based on the name of the function.

If the unlink() is moved out of the checking function, the original unlink() can be left in the __init__() function.

Changed 9 years ago by zhanglikun

the patch for avoid more than one xfrout process starting at same time.

comment:19 follow-up: Changed 9 years ago by zhanglikun

See the latest patch, Add a new function _remove_unused_sock_file().

comment:20 in reply to: ↑ 19 Changed 9 years ago by shane

Replying to zhanglikun:

See the latest patch, Add a new function _remove_unused_sock_file().

This looks good to me.

One typo: "another frout process" should be "another xfrout process".

comment:21 Changed 9 years ago by zhanglikun

  • Resolution set to fixed
  • Status changed from assigned to closed

The code for this ticket has been committed in r2091. So close this ticket.

Note: See TracTickets for help on using tickets.