Respect environment variable for MFL socket path #214

NsCDE · 2020-09-08T21:17:21Z

Hi Thomas,

While reading man page for FvwmMFL, I saw reference to FVWMMFL_SOCKET, but this doesn't work because it was still in plan in modules/FvwmMFL/FvwmMFL.c.

I played a bit this evening and got this to work for me. Please see if this is ok, and if it can be merged. Thanks. :-)

ThomasAdam

Hi @NsCDE,

Thanks for this. I clearly got ahead of myself in documenting this feature, despite never having written the code. Ooops!

I've left some comments in-line for you to ponder.

I think the biggest suggestion I've got is to make const char *sock_path a global by declaring it as: static const char *sock_path, putting it someone near the top of the file. This isn't going to change/be read again, once FvwmMFL has been started. So we only need to work it out once.

I've also suggested a minor change in logic for set_sock_path() -- perhaps this now needs to be renamed since it's not just setting the path, it's also the path to the socket as well? I'll leave that up to you to decide if it's worth while.

Thanks for this though!

ThomasAdam · 2020-09-08T21:32:15Z

modules/FvwmMFL/FvwmMFL.c

@@ -113,6 +114,7 @@ static struct fvwm_msg *fvwm_msg_new(void);
 static void fvwm_msg_free(struct fvwm_msg *);
 static void register_interest(void);
 static void send_version_info(struct client *);
+static char *set_socket_path();


Suggested change

static char *set_socket_path();

static char *set_socket_path(void);

This at least makes it >C89 compliant. ;)

ThomasAdam · 2020-09-08T21:33:31Z

modules/FvwmMFL/FvwmMFL.c

 	fprintf(stderr, "%s: dying...\n", __func__);
-	unlink(SOCK);
+	unlink(sock_path);


We should only be setting sock_path once. More on this later...

ThomasAdam · 2020-09-08T21:34:18Z

modules/FvwmMFL/FvwmMFL.c

 		exit(0);
 	}

 	broadcast_to_client(packet);
 }

+static char *set_socket_path()


Suggested change

static char *set_socket_path()

static const char *

set_socket_path(void)

Hi Thomas,

As you pointed, it is a full filesystem address, so I have renamed sock_path into sock_pathname, made freeing of variable where you pointed it out, *sock_pathname is now global and at the top with other similar variables, but I have one problem:

I have changed everything you pointed out, but if I make "const" socket_pathname and put const function set_socket_pathname prototype and body, code compiles but xasprintf complains (see below). I don't know how to resolve this except of not making this "const". Maybe cast it into what xasprintf expects? I'm not sure how to do it safely.

CC FvwmMFL.o
FvwmMFL.c: In function ‘set_socket_pathname’:
FvwmMFL.c:662:16: warning: passing argument 1 of ‘xasprintf’ from incompatible pointer type [-Wincompatible-pointer-types]
662 | xasprintf(&sock_pathname, "%s/%s", getenv("FVWM_USERDIR"), unrolled_path);
| ^~~~~~~~~~~~~~
| |
| const char **
In file included from ../../libs/fvwmlib.h:13,
from ../../libs/Flocale.h:14,
from ../../fvwm/fvwm.h:43,
from FvwmMFL.c:11:
../../libs/safemalloc.h:7:16: note: expected ‘char **’ but argument is of type ‘const char **’
7 | int xasprintf(char **, const char *, ...);
| ^~~~~~~
FvwmMFL.c: In function ‘main’:
FvwmMFL.c:694:7: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
694 | free(sock_pathname);
| ^~~~~~~~~~~~~
In file included from ../../config.h:474,
from FvwmMFL.c:9:
/usr/include/stdlib.h:565:25: note: expected ‘void *’ but argument is of type ‘const char *’
565 | extern void free (void *__ptr) __THROW;
| ~~~~~~^~~~~
CCLD FvwmMFL

ThomasAdam · 2020-09-08T21:39:49Z

modules/FvwmMFL/FvwmMFL.c

+	xasprintf(&sock_path, "%s", MFL_SOCKET_DEFAULT);
+
+	mflsock_env = getenv("FVWMMFL_SOCKET");
+	if (mflsock_env != NULL) {


I think it would be easier to check for NULL first of all, as in:

mflsock_env = getenv("FVWMMFL_SOCKET"); if (mflsock_env == NULL) { return (xstrdup(MFL_SOCKET_DEFAULT)); } unrolled_path = expand_path(mflsock_env); if (unrolled_path[0] == '/') sock_path = fxstrdup(unrolled_path); else xasprintf(&sock_path, "%s/%s", getenv("FVWM_USERDIR"), unrolled_path);

ThomasAdam · 2020-09-08T21:40:46Z

modules/FvwmMFL/FvwmMFL.c

+		else
+			xasprintf(&sock_path, "%s/%s", getenv("FVWM_USERDIR"), unrolled_path);
+
+		free((char *)unrolled_path);


Suggested change

free((char *)unrolled_path);

free((void *)unrolled_path);

ThomasAdam · 2020-09-08T21:41:12Z

modules/FvwmMFL/FvwmMFL.c

 int main(int argc, char **argv)
 {
 	struct event_base     *base;
 	struct evconnlistener *fmd_cfd;
 	struct sockaddr_un    sin;

+	char *sock_path;


Suggest making this a static variable at the top of this file, outside of main()

ThomasAdam · 2020-09-08T21:42:01Z

modules/FvwmMFL/FvwmMFL.c

 	memset(&sin, 0, sizeof(sin));
 	sin.sun_family = AF_LOCAL;
-	strcpy(sin.sun_path, SOCK);
+	strcpy(sin.sun_path, sock_path);


Memory leak...

free(sock_path)

NsCDE · 2020-09-09T06:20:09Z

Hi. I just made proposed corrections, and I have problems with const + xasprintf - see comment above. Thanks ...

P. S.
This is during my effort to make NsCDE fully FVWM3 enabled. I have some notes about FvwmPager inconsistent behaviour that I saw and can reproduce, I will make issue reports during the day ...

ThomasAdam · 2020-09-09T08:43:37Z

Hi @NsCDE,

Thanks! I've attached with this message, a diff which is on top of your latest change, which fixes the const vs char confusion, as well as makes sock_pathname assigned once in main() and used where needed. Before, you were declaring sock_pathname as a static global and reassigning it in different functions. This would have been a memory leak, and also not necessary as we only need to declare it once in main() anyway.

Please take a look at the patch. You can apply it on top of your work by saving the attached file and running:

git apply /path/to/patch

I've not tested this, so if you could please check it's doing the right thing, and then incorporate those changes in to your work, I'll take one last look and we'll get this merged in.

Thanks again!

ontop.diff.txt

ThomasAdam · 2020-09-09T12:14:10Z

Hi @NsCDE,

Thanks for the change. Are you happy that it's working as it should now?

NsCDE · 2020-09-09T12:21:31Z

Hi @NsCDE,

Thanks! I've attached with this message, a diff which is on top of your latest change, which fixes the const vs char confusion, as well as makes sock_pathname assigned once in main() and used where needed. Before, you were declaring sock_pathname as a static global and reassigning it in different functions. This would have been a memory leak, and also not necessary as we only need to declare it once in main() anyway.

Please take a look at the patch. You can apply it on top of your work by saving the attached file and running:

git apply /path/to/patch

I've not tested this, so if you could please check it's doing the right thing, and then incorporate those changes in to your work, I'll take one last look and we'll get this merged in.

Thanks again!

ontop.diff.txt

Hi Thomas,

I have applied patch and tested how things are working. It compiles fine, but after testing and reading FvwmMFL process with lsof and strace, I concluded that I have to put back sock_pathname = set_socket_pathname(); in functions HandleTerminate and fvwm_read, because unlink gets empty string or garbage without it:
Strace:
/tmp/bla:85338 unlink("") = -1 ENOENT (No such file or directory)
/tmp/bla2:86157 unlink("\300\302,\1") = -1 ENOENT (No such file or directory)

Here is the strace with set_socket_pathname call to fill sock_pathname put back:

/tmp/bla3:86457 unlink("/tmp/fvwm_mfl.sock") = 0
/tmp/bla4:87264 unlink("/home/tmangrow/.NsCDE/tmp/fvwm_mfl.sock") = 0

Everything now here seems to be ok, EXCEPT one issue: You are handling the start of FvwmMFL module internally, not from the user configuration (Init or Start conf functions). If FVWM3 configuration is used ($FVWM_USERDIR/config) to put:

Test (EnvMatch FVWM_IS_FVWM3 1) SetEnv FVWMMFL_SOCKET $[FVWM_USERDIR]/tmp/fvwm_mfl.sock

This will not work, because FvwmMFL is started prior to that. Obvious workaround is to KillModule FvwmMFL and Module FvwmMFL from the StartFunction, but this leaves unnecessary creation and closing of socket in /tmp and recreation of the process. It looks like this is in fvwm/fvwm3.c, but I'm not sure how to solve this.

NsCDE · 2020-09-09T12:23:09Z

Hi @NsCDE,

Thanks for the change. Are you happy that it's working as it should now?

You are really fast to reply. :-) Please see my answer above, which I wrote a minute ago.

ThomasAdam · 2020-09-09T12:33:21Z

Hi Thomas,

Hello,

I have applied patch and tested how things are working. It compiles fine, but after testing and reading FvwmMFL process with lsof and strace, I concluded that I have to put back sock_pathname = set_socket_pathname(); in functions HandleTerminate and fvwm_read, because unlink gets empty string or garbage without it:

Look at the patch embedded in this message. The HandleTerminate() problem is because we're declaring sock_pathname too late, so I've moved this up a bit. If this is still failing for fvwm_read that's a different bug and one that needs looking at -- rather than just reassigning sock_pathname = set_socket_pathname() each time, which in itself is a bug, since you need to free() the assignment first of all.

Everything now here seems to be ok, EXCEPT one issue: You are handling the start of FvwmMFL module internally, not from the user configuration (Init or Start conf functions). If FVWM3 configuration is used ($FVWM_USERDIR/config) to put:

Test (EnvMatch FVWM_IS_FVWM3 1) SetEnv FVWMMFL_SOCKET $[FVWM_USERDIR]/tmp/fvwm_mfl.sock

This will not work, because FvwmMFL is started prior to that. Obvious workaround is to KillModule FvwmMFL and Module FvwmMFL from the StartFunction, but this leaves unnecessary creation and closing of socket in /tmp and recreation of the process. It looks like this is in fvwm/fvwm3.c, but I'm not sure how to solve this.

Well, I was going for always having that module running so that users didn't need to. I've commented this out for now. Have a look at this, and let me know how you get on. Again, if you feel this is working correctly, commit the changes and resubmit. If you feel that NOT starting FvwmMFL is the right thing to do, and making that something the user needs to do, then I'm OK with that. Please make sure your PR includes a git revert 4a30c4fbdfd258012dbf612f005485b2e2f2bd9f.

Here's the patch on top of your latest changes:

diff --git a/fvwm/fvwm3.c b/fvwm/fvwm3.c
index 68702e0a3..b6dc82f9c 100644
--- a/fvwm/fvwm3.c
+++ b/fvwm/fvwm3.c
@@ -1396,7 +1396,6 @@ static void SetRCDefaults(void)
                { "Key Up M A MenuMoveCursor -1", "", "" },
                { "Key Down M A MenuMoveCursor 1", "", "" },
                { "Mouse 1 MI A MenuSelectItem", "", "" },
-               { "Module FvwmMFL", "", "" },
                /* don't add anything below */
                { RC_DEFAULTS_COMPLETE, "", "" },
                { "Read "FVWM_DATADIR"/ConfigFvwmDefaults", "", "" },
diff --git a/modules/FvwmMFL/FvwmMFL.c b/modules/FvwmMFL/FvwmMFL.c
index 722421af7..52fd210c9 100644
--- a/modules/FvwmMFL/FvwmMFL.c
+++ b/modules/FvwmMFL/FvwmMFL.c
@@ -682,6 +682,8 @@ int main(int argc, char **argv)
                return (1);
        }
 
+       sock_pathname = set_socket_pathname();
+
        /* Create new event base */
        if ((base = event_base_new()) == NULL) {
                fprintf(stderr, "Couldn't start libevent\n");
@@ -689,8 +691,6 @@ int main(int argc, char **argv)
        }
        setup_signal_handlers(base);
 
-       sock_pathname = set_socket_pathname();
-
        memset(&sin, 0, sizeof(sin));
        sin.sun_family = AF_LOCAL;
        strcpy(sin.sun_path, sock_pathname);

This reverts commit 4a30c4f.

NsCDE · 2020-09-09T12:55:56Z

Thomas:

Look at the patch embedded in this message. The HandleTerminate() problem is because we're declaring sock_pathname too late, so I've moved this up a bit. If this is still failing for fvwm_read that's a different bug and one that needs looking at -- rather than just reassigning sock_pathname = set_socket_pathname() each time, which in itself is a bug, since you need to free() the assignment first of all.

I have reverted this and removed calls from HandleTerminate() and fvwm_read(). Removed call to FvwmMFL in fvwm/fvwm3.c and moved last remaining sock_pathname = set_socket_pathname() up where you pointed it in this simple patch.

Outcome:

This unlink() problem still persists. Socket remains in directory and on the second call to "Module FvwmMFL" this fails with "Couldn't create listener: Address already in use".

P. S.
As expected, FvwmMFL is not started if not called from the configuration, this part is ok, and environment is picked up when StartFunction with modules is executing from the config.

NsCDE · 2020-09-09T13:15:52Z

Replying myself:

Outcome:

This unlink() problem still persists. Socket remains in directory and on the second call to "Module FvwmMFL" this fails with "Couldn't create listener: Address already in use".

Turns out this is not the problem of HandleTerminate, but unlink from the end of main(). The cause is: free(sock_pathname); right next after strcpy(sin.sun_path, sock_pathname); on line 692. I'm not sure where this
free should go. Probably before return in main and AFTER unlink. :-) Sorry for my poor C ...

EDIT ... if anywhere this free() should go. Program terminates here, so it is freed if I'm correct.

ThomasAdam · 2020-09-09T13:28:54Z

EDIT ... if anywhere this free() should go. Program terminates here, so it is freed if I'm correct.

I'm tired. Forgive me. No need at all to free() here -- the variable needs to persist! Please remove that.

NsCDE · 2020-09-09T13:35:32Z

Hi.

Here is then final PR proposal. And have some rest. Tomorrow I will try to make some torture with WindowName patch for FvwmButtons. :-)

ThomasAdam · 2020-09-09T13:58:24Z

Hi @NsCDE,

So. I've squashed and rebased your commits together, and came up with:

#217

I've retained your original authorship, so things are rightly accredited to yourself. Please have a look at that. If you're happy, Please close this PR, mention me on the one above and I'll merge that, and everything will be hunky-dory as they say.

Kindly,
Thomas

NsCDE · 2020-09-09T14:06:46Z

Thanks Thomas, and there is no necessity accreditation if something is rebased technically, I'm glad to help with FVWM.

Closing this, and going to mention you on the #217 whatever this means. :-)

Respect environment variable for MFL socket path

0062e6b

ThomasAdam reviewed Sep 8, 2020

View reviewed changes

MFL socket, code corrections from Thomas

65e7dc5

MFL socket: const and xasprintf, unlink issue

6f02ccb

NsCDE added 2 commits September 9, 2020 14:41

Revert "FvwmMFL: start by default"

1ce2d7f

This reverts commit 4a30c4f.

MFL socket: still unlink issue

2477a7c

MFL socket: unlink solved

ef56cb0

NsCDE closed this Sep 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect environment variable for MFL socket path #214

Respect environment variable for MFL socket path #214

NsCDE commented Sep 8, 2020

ThomasAdam left a comment

ThomasAdam Sep 8, 2020

ThomasAdam Sep 8, 2020

ThomasAdam Sep 8, 2020

NsCDE Sep 9, 2020 •

edited

Loading

ThomasAdam Sep 8, 2020

ThomasAdam Sep 8, 2020

ThomasAdam Sep 8, 2020

ThomasAdam Sep 8, 2020

NsCDE commented Sep 9, 2020

ThomasAdam commented Sep 9, 2020

ThomasAdam commented Sep 9, 2020

NsCDE commented Sep 9, 2020

NsCDE commented Sep 9, 2020

ThomasAdam commented Sep 9, 2020 •

edited

Loading

NsCDE commented Sep 9, 2020

NsCDE commented Sep 9, 2020 •

edited

Loading

ThomasAdam commented Sep 9, 2020

NsCDE commented Sep 9, 2020

ThomasAdam commented Sep 9, 2020

NsCDE commented Sep 9, 2020

	static char *set_socket_path();
	static char *set_socket_path(void);

	static char *set_socket_path()
	static const char *
	set_socket_path(void)

Respect environment variable for MFL socket path #214

Respect environment variable for MFL socket path #214

Conversation

NsCDE commented Sep 8, 2020

ThomasAdam left a comment

Choose a reason for hiding this comment

ThomasAdam Sep 8, 2020

Choose a reason for hiding this comment

ThomasAdam Sep 8, 2020

Choose a reason for hiding this comment

ThomasAdam Sep 8, 2020

Choose a reason for hiding this comment

NsCDE Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

ThomasAdam Sep 8, 2020

Choose a reason for hiding this comment

ThomasAdam Sep 8, 2020

Choose a reason for hiding this comment

ThomasAdam Sep 8, 2020

Choose a reason for hiding this comment

ThomasAdam Sep 8, 2020

Choose a reason for hiding this comment

NsCDE commented Sep 9, 2020

ThomasAdam commented Sep 9, 2020

ThomasAdam commented Sep 9, 2020

NsCDE commented Sep 9, 2020

NsCDE commented Sep 9, 2020

ThomasAdam commented Sep 9, 2020 • edited Loading

NsCDE commented Sep 9, 2020

NsCDE commented Sep 9, 2020 • edited Loading

ThomasAdam commented Sep 9, 2020

NsCDE commented Sep 9, 2020

ThomasAdam commented Sep 9, 2020

NsCDE commented Sep 9, 2020

NsCDE Sep 9, 2020 •

edited

Loading

ThomasAdam commented Sep 9, 2020 •

edited

Loading

NsCDE commented Sep 9, 2020 •

edited

Loading