Pr/improve option handling #557

uli42 · 2017-11-19T01:35:52Z

some improvements for Args.c

sunweaver

@uil42: I left some comments on PR #557. Please amend. Thanks!

sunweaver · 2017-11-20T09:35:01Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

        {
-          fprintf(stderr, "ddxProcessArgument: WARNING! failed string allocation.\n");
+          FatalError("malloc failed");


Wouldn't it be good to mention what we tried to malloc here?

Well, maybe. But if you happen to suffer from malloc failing you have other problems... ;-) Will extend these messages anyway.

sunweaver · 2017-11-20T09:35:08Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

      {
-        envOptions = strcpy(envOptions, envDisplay);
+        FatalError("malloc failed");


sunweaver · 2017-11-20T09:35:17Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

        {
-          fprintf(stderr, "ddxProcessArgument: WARNING! failed string allocation.\n");
+          FatalError("malloc failed");


sunweaver · 2017-11-20T09:35:26Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

-        return 2;
-      } else {
-	FatalError("malloc failed");
+        FatalError("malloc failed");


sunweaver · 2017-11-20T09:36:16Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

@@ -758,16 +723,13 @@ int ddxProcessArgument(int argc, char *argv[], int i)
        nxagentKeyboard = NULL;
      }

+      /* FIXME: why limit to 256? */


Probably some comand line length limitation adopted here?

sunweaver · 2017-11-20T09:38:15Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

@@ -1351,7 +1351,7 @@ static void nxagentParseOptions(char *name, char *value)
    errno = 0;
    sleep_parse = strtol(value, NULL, 10);

-    if ((errno) && (0 == sleep_parse))
+    if ((errno) && (sleep_parse == 0))


@Ionic: I noticed these comparisons, too, when you worked on the sleep and tolerance feature. Any reason for having the order == ?

I think this is a coding habit that prevents accidently writing = instead of ==. The compiler will complain if you try to assign a value to a constant.

Yes, exactly for this reason. It's a pretty common pattern to avoid mistakes (and I'd like to keep that part.)

sunweaver · 2017-11-20T09:39:29Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

@@ -355,7 +355,7 @@ int ddxProcessArgument(int argc, char *argv[], int i)
      /* FIXME: why limit 10 1024 bytes? */
      if ((size = strlen(argv[i])) < 1024)
      {
-        if ((nxagentOptionsFilename = strndup(argv[i], size)) == NULL;
+        if ((nxagentOptionsFilename = strndup(argv[i], size)) == NULL);


the semicolon at the line end feels wrong. Also: please rebase with the previous commit that you try to fix with this commit.

uli42 · 2017-11-20T10:15:43Z

I also noticed I still have some fixup commits in there. New version to come soon ;-)

uli42 · 2017-11-21T00:24:09Z

Ready for another round. I have added some more changes, so please start over again.

uli42 · 2017-11-21T23:03:31Z

rebased

Ionic · 2017-11-21T02:38:52Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

+/*
+ * This is the entry point, called from os/utils.c
+ */
+


I'd drop the newline here and in the next hunk.

Ionic · 2017-11-21T02:48:16Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

-
-        if (nxagentOptionsFilenameOrString != NULL)
+        free(nxagentOptionsFilenameOrString);
+        if ((nxagentOptionsFilenameOrString = strdup(argv[j + 1])) == NULL)


That's more compact, but potentially also slower. We probably don't really care since this code will be only called "once". (Actually multiple times, but only once during the program's lifetime.) Just mentioning it.

Speed was not an issue here. I just wanted to simplify the code to make it more readable.

Ionic · 2017-11-21T02:50:53Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

      free(envOptions);
    }

    for (j = 0; j < argc; j++)
    {
+      /*
+       * Read the _last_ options file only and only at startup.


This breaks compatibility. Previously, only the first options file was used, now it's the last one. Is that a good idea? At least it would make nxagent more compatible with other tools, since most programs allow users to override values for previous parameters by specifying them again with only the last one taking effect.

To be precise: it may look like it's compatible to the previous behavior, but it isn't. Previously, the code scanned for -option(s) and took the first match, then parsed it as an options file. Afterwards, subsequent uses of ddxProcessArgument() overrode the internal nxagentOptionFile (or nowadays nxagentOptionsFilenameOrString) variable. That's an important difference because it allows for a very special combination:

start up nxagent with a special options file (first -option(s) argument)

override it again via subsequent -option(s) arguments, which will only change the internal data, but NOT lead to immediate parsing

on reconnects ONLY parse the newly registered options file

With your changes, that would not be possible.

I wonder if that behavior is a bug or a feature (i.e., whether this was intended or just an unintentional feature).

making it behave like most other tools was the intention of my change.

You are also right about changing the options file name for reconnect (only). But consider this an unintended behaviour. The whole options parsing code is very ugly anyway... Besides: that is not documented anywhere, AFAIKS.

Ionic · 2017-11-21T02:55:13Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

-
-      nxagentDisplayName[1023] = '\0';
-
+      strncpy(nxagentDisplayName, argv[i], sizeof(nxagentDisplayName) - 1);


Will cause hard to debug problems if we ever decide to change nxagentDisplayName to a pure pointer instead of an array. The alternatives are either hardcoding a value (like previously), or using a length macro. Both aren't very nice, but I guess the latter is the safest and best-looking route.

(changing those array to pointers is on my list already)

Well, if we change nxagentDisplayName to be a pointer using a hardcoded value will also be wrong. So the situation does not get worse with my change. It only makes it easier to change that hardcoded value because it is now at only one location.

Besides, all this nullbyte stuffing will be gone once we are using pointers. We can even get rid of it now by using s(n)printf() instead of str(n)cpy().

Well, if we change nxagentDisplayName to be a pointer using a hardcoded value will also be wrong. So the situation does not get worse with my change. It only makes it easier to change that hardcoded value because it is now at only one location.

Yes, that's why I recommend using a macro defining each length, e.g., NXAGENTDISPLAYNAME_LENGTH = ... in the header file and using that throughout the code instead of a hardcoded value.

Even if we change the variables to pure pointers, the situation wouldn't really change. We probably should bound the data length with pure pointers as well. Otherwise, str(n)cpy will crash on invalid input data. With bounding, we will in the worst case get LEN-1 bytes of garbage data, but no actual crashes.

sizeof(ptr) doesn't work for pure pointers, so we'd have to change these sections again anyway.

I agree that dropping the hardcoded numbers is a good idea, but IMHO the best way (also taking into account further changes to pure pointers) is to use macros instead.

Ionic · 2017-11-21T02:56:02Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

-
-      *(nxagentSessionId + 255) = '\0';
-
+      strncpy(nxagentSessionId, argv[i], sizeof(nxagentSessionId) - 1);


Ionic · 2017-11-22T03:13:44Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

    return 1;
  }

  if (!strcmp(argv[i], "-keystrokefile"))
  {
    if (i + 1 < argc)
    {
-      if (NULL != (nxagentKeystrokeFile = strdup(argv[i + 1])))
+      if ((nxagentKeystrokeFile = strdup(argv[i + 1])) == NULL)


I'd keep that as NULL == ...

I fully get you point here, but I want to keep the code uniform within a file. And the rest of the code has the constant at the right side. The Xorg code is also using this technique.

Welll... okay.

Personally, I'll be keeping this style for new submissions, particularly since it's just a cosmetic issue and provides a potential benefit.

I totally respect your decision, though. It's fine.

Ionic · 2017-11-22T03:16:08Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

@@ -1435,31 +1311,30 @@ static void nxagentParseOptions(char *name, char *value)
      errno = 0;
      tolerance_parse = strtol(value, NULL, 10);

-      if ((errno) && (0 == tolerance_parse))
+      if ((errno) && (tolerance_parse == 0))


Ionic · 2017-11-22T03:17:56Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

-      strncpy(nxagentWindowName, "NX", 255);
-
-      *(nxagentWindowName + 255) = '\0';
+      strncpy(nxagentWindowName, "NX", sizeof(nxagentWindowName) - 1);


Same issue here as some hunks above.

Ionic · 2017-11-22T03:18:47Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

@@ -2144,29 +2002,29 @@ static int nxagentGetDialogName()
 {
  strcpy(nxagentDialogName, "NX");

-  *(nxagentDialogName + 255) = '\0';
+  nxagentDialogName[sizeof(nxagentDialogName) - 1] = '\0';


Same problem as some hunks above.

Ionic · 2017-11-22T03:32:51Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

    {
-      strncat(nxagentDialogName, nxagentSessionId, length - (MD5_LENGTH * 2 + 1));
+      strncat(nxagentDialogName, nxagentSessionId,
+              MIN(length, sizeof(nxagentDialogName) - 1 - strlen(prefix)));


Same problem as above.

Bounding the size is actually a pretty good idea here, though.

Ionic · 2017-11-22T03:38:32Z

Took me a while go through all of this, sorry...

uli42 · 2017-11-23T14:48:02Z

pushed updated version

Ionic · 2017-12-06T04:22:39Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

-
-      nxagentDisplayName[1023] = '\0';
-
+      strncpy(nxagentDisplayName, argv[i], sizeof(nxagentDisplayName) - 1);


Well, if we change nxagentDisplayName to be a pointer using a hardcoded value will also be wrong. So the situation does not get worse with my change. It only makes it easier to change that hardcoded value because it is now at only one location.

Yes, that's why I recommend using a macro defining each length, e.g., NXAGENTDISPLAYNAME_LENGTH = ... in the header file and using that throughout the code instead of a hardcoded value.

Even if we change the variables to pure pointers, the situation wouldn't really change. We probably should bound the data length with pure pointers as well. Otherwise, str(n)cpy will crash on invalid input data. With bounding, we will in the worst case get LEN-1 bytes of garbage data, but no actual crashes.

sizeof(ptr) doesn't work for pure pointers, so we'd have to change these sections again anyway.

I agree that dropping the hardcoded numbers is a good idea, but IMHO the best way (also taking into account further changes to pure pointers) is to use macros instead.

Ionic · 2017-12-06T04:33:06Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

    return 1;
  }

  if (!strcmp(argv[i], "-keystrokefile"))
  {
    if (i + 1 < argc)
    {
-      if (NULL != (nxagentKeystrokeFile = strdup(argv[i + 1])))
+      if ((nxagentKeystrokeFile = strdup(argv[i + 1])) == NULL)


Welll... okay.

Personally, I'll be keeping this style for new submissions, particularly since it's just a cosmetic issue and provides a potential benefit.

I totally respect your decision, though. It's fine.

sunweaver · 2018-05-24T07:13:07Z

@Ionic: @uli42: ping on the status of this PR.

sunweaver · 2018-07-22T14:50:07Z

@Ionic: @uli42: ping (2) on the status of this PR.

Warnings that are guarded by ifdefs are for the users. Therefore they must not contain the function name. Also some of the messages had not been prefixed by "Warning".

we already know the exact value of the name variable so there's no need to validate it.

Within Args.c values in messages are not enclosed by [] but ''. The [] format is only used for debugging/testing messages.

Use strndup instead of malloc + strncpy. Don't issue just a warning if that fails but issue a FatalError.

We use (variable == constant) everywhere.

An options string/filename can only be provided via the command line or environment. Both are handled in the one-time init part of ddxProcessArguments() and will not change during runtime. As we do not want an options file to include another "options" option there's no need to handle that a second time. Due to the (kind of crazy) way nxagent handles all this option stuff we cannot remove the second check completely. This also removes the 1024 byte limit for nxagentOptionsFilenameOrString.

Could not find a reason to limit that string.

uli42 · 2018-08-23T21:54:28Z

rebased

sunweaver · 2018-08-24T11:41:18Z

@uli42: Is this PR outdated now with #725 merged?

uli42 · 2018-08-24T12:00:20Z

No, this is a different topic Mike Gabriel <[email protected]> schrieb am Fr., 24. Aug. 2018, 13:41:

…

@uli42 <https://github.com/uli42>: Is this PR outdate now with #725 <#725> merged? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#557 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGBw5H3w2dVMCfxdMoQqiFI5tfmf7JERks5uT-ZfgaJpZM4QjN-K> .

uli42 requested a review from Ionic November 19, 2017 01:35

uli42 force-pushed the pr/improve_option_handling branch from 561c93e to 66db29c Compare November 19, 2017 13:15

sunweaver requested changes Nov 20, 2017

View reviewed changes

sunweaver added the code cleanup label Nov 20, 2017

uli42 force-pushed the pr/improve_option_handling branch 4 times, most recently from 836225d to 320f413 Compare November 20, 2017 23:49

uli42 force-pushed the pr/improve_option_handling branch from 320f413 to 5d022f7 Compare November 21, 2017 21:34

Ionic requested changes Nov 22, 2017

View reviewed changes

uli42 force-pushed the pr/improve_option_handling branch 3 times, most recently from a973993 to 39fe99d Compare November 23, 2017 14:47

Ionic requested changes Dec 6, 2017

View reviewed changes

uli42 force-pushed the pr/improve_option_handling branch 2 times, most recently from ee8d03d to f5346e4 Compare December 11, 2017 22:08

uli42 force-pushed the pr/improve_option_handling branch 2 times, most recently from f5346e4 to d11ebd1 Compare February 27, 2018 21:43

uli42 added 5 commits August 23, 2018 23:53

Args.c: Fix warnings containing function name

32c8bd8

Warnings that are guarded by ifdefs are for the users. Therefore they must not contain the function name. Also some of the messages had not been prefixed by "Warning".

Args.c: re-use name variable for warnings

e4b1348

Args.c: remove variable validation

19142bd

we already know the exact value of the name variable so there's no need to validate it.

Args.c: do not use [] for warnings

e6f7dce

Within Args.c values in messages are not enclosed by [] but ''. The [] format is only used for debugging/testing messages.

Args.c: shorten string allocations and abort on failure

1011895

Use strndup instead of malloc + strncpy. Don't issue just a warning if that fails but issue a FatalError.

uli42 added 9 commits August 23, 2018 23:53

Args.c: formatting and whitespace improvements

2ee5b55

Args.c: swap comparison order

5c0345f

We use (variable == constant) everywhere.

Args.c, Binder.c: replace deprecated (r)index() by str(r)chr()

426cd2f

Args.c: use char '\0' instead of 0 where appropriate

19ef57d

hw/nxagent: make nxagentVerbose a Boolean

dda7d42

Args.c: add some comments and FIXMEs

ea9a206

nxagent manpage: add copysize option

1e89e2b

Args.c: drop 255 byte limit for -keyboard option

19126af

Could not find a reason to limit that string.

uli42 force-pushed the pr/improve_option_handling branch from d11ebd1 to 19126af Compare August 23, 2018 21:54


		nxagentDisplayName[1023] = '\0';

		strncpy(nxagentDisplayName, argv[i], sizeof(nxagentDisplayName) - 1);


		*(nxagentSessionId + 255) = '\0';

		strncpy(nxagentSessionId, argv[i], sizeof(nxagentSessionId) - 1);

Pr/improve option handling #557

Are you sure you want to change the base?

Pr/improve option handling #557

Conversation

uli42 commented Nov 19, 2017

sunweaver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uli42 Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uli42 commented Nov 20, 2017

uli42 commented Nov 21, 2017

uli42 commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ionic commented Nov 22, 2017

uli42 commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunweaver commented May 24, 2018

sunweaver commented Jul 22, 2018

uli42 commented Aug 23, 2018

sunweaver commented Aug 24, 2018 • edited Loading

uli42 commented Aug 24, 2018 via email

uli42 Nov 21, 2017 •

edited

Loading

sunweaver commented Aug 24, 2018 •

edited

Loading