[stgt] [PATCH] handle SEM Key clash between multiple tgt instances

[FUJITA Tomonori]

On Fri, 18 Mar 2011 10:50:41 +0100
Roland Friedwagner <roland.friedwagner at wu.ac.at> wrote:

> Hello,
> 
> Am Donnerstag 17 März 2011 schrieb FUJITA Tomonori:
> > On Mon, 14 Mar 2011 15:26:45 +0200
> >
> > Or Gerlitz <ogerlitz at mellanox.com> wrote:
> > > From: Roland Friedwagner <roland.friedwagner at wu.ac.at>
> > >
> > > commit e2f95b537 "semaphore key clash with Open-iSCSI initiator"
> > > didn't address a further clash introduced when multiple tgt
> > > instances are running. Address that by creating the IPC semaphore
> > > keyid based on the managment socket file using ftok(3).
> > >
> > > Signed-off-by: Roland Friedwagner <roland.friedwagner at wu.ac.at>
> > > Signed-off-by: Or Gerlitz <ogerlitz at mellanox.com>
> > >
> > > ---
> > >  usr/log.c    |   15 +++++++++++++--
> > >  usr/mgmt.c   |    9 +++++----
> > >  usr/tgtadm.c |    6 +++---
> > >  3 files changed, 21 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/usr/log.c b/usr/log.c
> > > index 06cd8bf..2f0c4d4 100644
> > > --- a/usr/log.c
> > > +++ b/usr/log.c
> > > @@ -33,7 +33,6 @@
> > >
> > >  #include "log.h"
> > >
> > > -#define SEMKEY	0x54475444L /* TGTD */
> > >  #define LOGDBG 0
> > >
> > >  #if LOGDBG
> > > @@ -50,6 +49,8 @@ static pid_t pid;
> > >  static int logarea_init (int size)
> > >  {
> > >  	int shmid;
> > > +	extern char mgmt_path[];
> > > +	key_t semkey;
> > >
> > >  	logdbg(stderr,"enter logarea_init\n");
> > >
> > > @@ -109,13 +110,23 @@ static int logarea_init (int size)
> > >
> > >  	shmctl(shmid, IPC_RMID, NULL);
> > >
> > > -	if ((la->semid = semget(SEMKEY, 1, 0666 | IPC_CREAT)) < 0) {
> > > +	if ((semkey = ftok(mgmt_path, 'a')) < 0) {
> > > +		syslog(LOG_ERR, "semkey failed %d", errno);
> > > +		return 1;
> > > +	}
> > > +
> > > +        if ((semget(semkey, 0, IPC_EXCL)) > 0) {
> > > +		/* Semkey may exists after SIGKILL tgtd */
> > > +		syslog(LOG_WARNING, "semkey 0x%x already exists", semkey);
> > > +	}
> > > +	if ((la->semid = semget(semkey, 1, 0666 | IPC_CREAT)) < 0) {
> > >  		syslog(LOG_ERR, "semget failed %d", errno);
> > >  		shmdt(la->buff);
> > >  		shmdt(la->start);
> > >  		shmdt(la);
> > >  		return 1;
> > >  	}
> > > +	syslog(LOG_INFO, "semkey 0x%x", semkey);
> > >
> > >  	la->semarg.val=1;
> > >  	if (semctl(la->semid, 0, SETVAL, la->semarg) < 0) {
> > > diff --git a/usr/mgmt.c b/usr/mgmt.c
> > > index 5a6a03b..fb703a6 100644
> > > --- a/usr/mgmt.c
> > > +++ b/usr/mgmt.c
> > > @@ -59,6 +59,7 @@ struct mgmt_task {
> > >  };
> > >
> > >  static int ipc_fd;
> > > +char mgmt_path[256];
> > >
> > >  static void set_show_results(struct tgtadm_rsp *rsp, int *err)
> > >  {
> > > @@ -567,7 +568,7 @@ int ipc_init(void)
> > >  	extern short control_port;
> > >  	int fd, err;
> > >  	struct sockaddr_un addr;
> > > -	char path[256];
> > > +	extern char mgmt_path[];
> >
> > No need?
> 
> Yes you are right. It's not absolute needed.

Please resend the fixed patch.

>   I thought calculating the sock path again with sprintf()
>   in log.c is not necessary, if it is a global and will be 

I can't find sprintf in log.c.

>   calculated only when tgtd main() does call ipc_init().
>   I think it whould be a more straight solution.
> 
> >
> > >  	fd = socket(AF_LOCAL, SOCK_STREAM, 0);
> > >  	if (fd < 0) {
> > > @@ -575,11 +576,11 @@ int ipc_init(void)
> > >  		return -1;
> > >  	}
> > >
> > > -	sprintf(path, "%s.%d", TGT_IPC_NAMESPACE, control_port);
> > > -	unlink(path);
> > > +	sprintf(mgmt_path, "%s.%d", TGT_IPC_NAMESPACE, control_port);
> > > +	unlink(mgmt_path);
> > >  	memset(&addr, 0, sizeof(addr));
> > >  	addr.sun_family = AF_LOCAL;
> > > -	strncpy(addr.sun_path, path, sizeof(addr.sun_path));
> > > +	strncpy(addr.sun_path, mgmt_path, sizeof(addr.sun_path));
> > >
> > >  	err = bind(fd, (struct sockaddr *) &addr, sizeof(addr));
> > >  	if (err) {
> > > diff --git a/usr/tgtadm.c b/usr/tgtadm.c
> > > index e81a419..9821917 100644
> > > --- a/usr/tgtadm.c
> > > +++ b/usr/tgtadm.c
> > > @@ -190,7 +190,7 @@ static int ipc_mgmt_connect(int *fd)
> > >  {
> > >  	int err;
> > >  	struct sockaddr_un addr;
> > > -	char path[256];
> > > +	char mgmt_path[256];
> > >
> > >  	*fd = socket(AF_LOCAL, SOCK_STREAM, 0);
> > >  	if (*fd < 0) {
> > > @@ -200,8 +200,8 @@ static int ipc_mgmt_connect(int *fd)
> > >
> > >  	memset(&addr, 0, sizeof(addr));
> > >  	addr.sun_family = AF_LOCAL;
> > > -	sprintf(path, "%s.%d", TGT_IPC_NAMESPACE, control_port);
> > > -	strncpy(addr.sun_path, path, sizeof(addr.sun_path));
> > > +	sprintf(mgmt_path, "%s.%d", TGT_IPC_NAMESPACE, control_port);
> > > +	strncpy(addr.sun_path, mgmt_path, sizeof(addr.sun_path));
> >
> > Why do we need to rename path to mgmt_path in tgtadm.c?
> 
> Only for consistency.
> 
>   So when someone, like I did, searches the source for where the socket
>   path is referenced, does recognize it's the same thing.

I don't think that the change helps in that way but it's fine by me if
you want.
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html