From 8052cd0131a4f483ef14be3e564530c07ea382f5 Mon Sep 17 00:00:00 2001
From: Ruslan Sayfutdinov <sayfutdinov@fb.com>
Date: Sat, 16 Dec 2023 15:28:19 +0000
Subject: [PATCH] cli: better errors on arguent parsing

---
 programs/zstdcli.c                        | 48 +++++++++++++----------
 tests/cli-tests/basic/args.sh             | 10 +++++
 tests/cli-tests/basic/args.sh.exit        |  1 +
 tests/cli-tests/basic/args.sh.stderr.glob | 28 +++++++++++++
 4 files changed, 66 insertions(+), 21 deletions(-)
 create mode 100755 tests/cli-tests/basic/args.sh
 create mode 100644 tests/cli-tests/basic/args.sh.exit
 create mode 100644 tests/cli-tests/basic/args.sh.stderr.glob

diff --git a/programs/zstdcli.c b/programs/zstdcli.c
index 66952aa8293..3f0ae8bdd21 100644
--- a/programs/zstdcli.c
+++ b/programs/zstdcli.c
@@ -138,8 +138,8 @@ static int exeNameMatch(const char* exeName, const char* test)
 *  Command Line
 **************************************/
 /* print help either in `stderr` or `stdout` depending on originating request
- * error (badusage) => stderr
- * help (usage_advanced) => stdout
+ * error (badUsage) => stderr
+ * help (usageAdvanced) => stdout
  */
 static void usage(FILE* f, const char* programName)
 {
@@ -175,7 +175,7 @@ static void usage(FILE* f, const char* programName)
     DISPLAY_F(f, "\n");
 }
 
-static void usage_advanced(const char* programName)
+static void usageAdvanced(const char* programName)
 {
     DISPLAYOUT(WELCOME_MESSAGE);
     DISPLAYOUT("\n");
@@ -316,9 +316,9 @@ static void usage_advanced(const char* programName)
 
 }
 
-static void badusage(const char* programName)
+static void badUsage(const char* programName, const char* parameter)
 {
-    DISPLAYLEVEL(1, "Incorrect parameters \n");
+    DISPLAYLEVEL(1, "Incorrect parameter: %s\n", parameter);
     if (g_displayLevel >= 2) usage(stderr, programName);
 }
 
@@ -589,7 +589,7 @@ static ZDICT_fastCover_params_t defaultFastCoverParams(void)
 
 
 /** parseAdaptParameters() :
- *  reads adapt parameters from *stringPtr (e.g. "--zstd=min=1,max=19) and store them into adaptMinPtr and adaptMaxPtr.
+ *  reads adapt parameters from *stringPtr (e.g. "--adapt=min=1,max=19) and store them into adaptMinPtr and adaptMaxPtr.
  *  Both adaptMinPtr and adaptMaxPtr must be already allocated and correctly initialized.
  *  There is no guarantee that any of these values will be updated.
  *  @return 1 means that parsing was successful,
@@ -933,6 +933,7 @@ int main(int argCount, const char* argv[])
     /* command switches */
     for (argNb=1; argNb<argCount; argNb++) {
         const char* argument = argv[argNb];
+        const char* const originalArgument = argument;
         if (!argument) continue;   /* Protection if argument empty */
 
         if (nextArgumentsAreFiles) {
@@ -958,7 +959,7 @@ int main(int argCount, const char* argv[])
                 if (!strcmp(argument, "--uncompress")) { operation=zom_decompress; continue; }
                 if (!strcmp(argument, "--force")) { FIO_overwriteMode(prefs); forceStdin=1; forceStdout=1; followLinks=1; allowBlockDevices=1; continue; }
                 if (!strcmp(argument, "--version")) { printVersion(); CLEAN_RETURN(0); }
-                if (!strcmp(argument, "--help")) { usage_advanced(programName); CLEAN_RETURN(0); }
+                if (!strcmp(argument, "--help")) { usageAdvanced(programName); CLEAN_RETURN(0); }
                 if (!strcmp(argument, "--verbose")) { g_displayLevel++; continue; }
                 if (!strcmp(argument, "--quiet")) { g_displayLevel--; continue; }
                 if (!strcmp(argument, "--stdout")) { forceStdout=1; outFileName=stdoutmark; removeSrcFile=0; continue; }
@@ -983,7 +984,7 @@ int main(int argCount, const char* argv[])
                 if (!strcmp(argument, "--adapt")) { adapt = 1; continue; }
                 if (!strcmp(argument, "--no-row-match-finder")) { useRowMatchFinder = ZSTD_ps_disable; continue; }
                 if (!strcmp(argument, "--row-match-finder")) { useRowMatchFinder = ZSTD_ps_enable; continue; }
-                if (longCommandWArg(&argument, "--adapt=")) { adapt = 1; if (!parseAdaptParameters(argument, &adaptMin, &adaptMax)) { badusage(programName); CLEAN_RETURN(1); } continue; }
+                if (longCommandWArg(&argument, "--adapt=")) { adapt = 1; if (!parseAdaptParameters(argument, &adaptMin, &adaptMax)) { badUsage(programName, originalArgument); CLEAN_RETURN(1); } continue; }
                 if (!strcmp(argument, "--single-thread")) { nbWorkers = 0; singleThread = 1; continue; }
                 if (!strcmp(argument, "--format=zstd")) { suffix = ZSTD_EXTENSION; cType = FIO_zstdCompression; continue; }
                 if (!strcmp(argument, "--mmap-dict")) { mmapDict = ZSTD_ps_enable; continue; }
@@ -1022,8 +1023,8 @@ int main(int argCount, const char* argv[])
                   dict = cover;
                   /* Allow optional arguments following an = */
                   if (*argument == 0) { memset(&coverParams, 0, sizeof(coverParams)); }
-                  else if (*argument++ != '=') { badusage(programName); CLEAN_RETURN(1); }
-                  else if (!parseCoverParameters(argument, &coverParams)) { badusage(programName); CLEAN_RETURN(1); }
+                  else if (*argument++ != '=') { badUsage(programName, originalArgument); CLEAN_RETURN(1); }
+                  else if (!parseCoverParameters(argument, &coverParams)) { badUsage(programName, originalArgument); CLEAN_RETURN(1); }
                   continue;
                 }
                 if (longCommandWArg(&argument, "--train-fastcover")) {
@@ -1033,8 +1034,8 @@ int main(int argCount, const char* argv[])
                   dict = fastCover;
                   /* Allow optional arguments following an = */
                   if (*argument == 0) { memset(&fastCoverParams, 0, sizeof(fastCoverParams)); }
-                  else if (*argument++ != '=') { badusage(programName); CLEAN_RETURN(1); }
-                  else if (!parseFastCoverParameters(argument, &fastCoverParams)) { badusage(programName); CLEAN_RETURN(1); }
+                  else if (*argument++ != '=') { badUsage(programName, originalArgument); CLEAN_RETURN(1); }
+                  else if (!parseFastCoverParameters(argument, &fastCoverParams)) { badUsage(programName, originalArgument); CLEAN_RETURN(1); }
                   continue;
                 }
                 if (longCommandWArg(&argument, "--train-legacy")) {
@@ -1044,8 +1045,8 @@ int main(int argCount, const char* argv[])
                   dict = legacy;
                   /* Allow optional arguments following an = */
                   if (*argument == 0) { continue; }
-                  else if (*argument++ != '=') { badusage(programName); CLEAN_RETURN(1); }
-                  else if (!parseLegacyParameters(argument, &dictSelect)) { badusage(programName); CLEAN_RETURN(1); }
+                  else if (*argument++ != '=') { badUsage(programName, originalArgument); CLEAN_RETURN(1); }
+                  else if (!parseLegacyParameters(argument, &dictSelect)) { badUsage(programName, originalArgument); CLEAN_RETURN(1); }
                   continue;
                 }
 #endif
@@ -1056,7 +1057,7 @@ int main(int argCount, const char* argv[])
                 if (longCommandWArg(&argument, "--block-size")) { NEXT_TSIZE(blockSize); continue; }
                 if (longCommandWArg(&argument, "--maxdict")) { NEXT_UINT32(maxDictSize); continue; }
                 if (longCommandWArg(&argument, "--dictID")) { NEXT_UINT32(dictID); continue; }
-                if (longCommandWArg(&argument, "--zstd=")) { if (!parseCompressionParameters(argument, &compressionParams)) { badusage(programName); CLEAN_RETURN(1); } ; cType = FIO_zstdCompression; continue; }
+                if (longCommandWArg(&argument, "--zstd=")) { if (!parseCompressionParameters(argument, &compressionParams)) { badUsage(programName, originalArgument); CLEAN_RETURN(1); } ; cType = FIO_zstdCompression; continue; }
                 if (longCommandWArg(&argument, "--stream-size")) { NEXT_TSIZE(streamSrcSize); continue; }
                 if (longCommandWArg(&argument, "--target-compressed-block-size")) { NEXT_TSIZE(targetCBlockSize); continue; }
                 if (longCommandWArg(&argument, "--size-hint")) { NEXT_TSIZE(srcSizeHint); continue; }
@@ -1098,7 +1099,7 @@ int main(int argCount, const char* argv[])
                         ldmWindowLog = readU32FromChar(&argument);
                     } else if (*argument != 0) {
                         /* Invalid character following --long */
-                        badusage(programName);
+                        badUsage(programName, originalArgument);
                         CLEAN_RETURN(1);
                     } else {
                         ldmWindowLog = g_defaultMaxWindowLog;
@@ -1120,12 +1121,12 @@ int main(int argCount, const char* argv[])
                         if (fastLevel) {
                             dictCLevel = cLevel = -(int)fastLevel;
                         } else {
-                            badusage(programName);
+                            badUsage(programName, originalArgument);
                             CLEAN_RETURN(1);
                         }
                     } else if (*argument != 0) {
                         /* Invalid character following --fast */
-                        badusage(programName);
+                        badUsage(programName, originalArgument);
                         CLEAN_RETURN(1);
                     } else {
                         cLevel = -1;  /* default for --fast */
@@ -1141,11 +1142,13 @@ int main(int argCount, const char* argv[])
                     continue;
                 }
 
-                /* fall-through, will trigger bad_usage() later on */
+                badUsage(programName, originalArgument);
+                CLEAN_RETURN(1);
             }
 
             argument++;
             while (argument[0]!=0) {
+                char shortArgument[3];
 
 #ifndef ZSTD_NOCOMPRESS
                 /* compression Level */
@@ -1159,7 +1162,7 @@ int main(int argCount, const char* argv[])
                 {
                     /* Display help */
                 case 'V': printVersion(); CLEAN_RETURN(0);   /* Version Only */
-                case 'H': usage_advanced(programName); CLEAN_RETURN(0);
+                case 'H': usageAdvanced(programName); CLEAN_RETURN(0);
                 case 'h': usage(stdout, programName); CLEAN_RETURN(0);
 
                      /* Compress */
@@ -1277,7 +1280,10 @@ int main(int argCount, const char* argv[])
                     break;
 
                     /* unknown command */
-                default : badusage(programName); CLEAN_RETURN(1);
+                default : 
+                    sprintf(shortArgument, "-%c", argument[0]);
+                    badUsage(programName, shortArgument);
+                    CLEAN_RETURN(1);
                 }
             }
             continue;
diff --git a/tests/cli-tests/basic/args.sh b/tests/cli-tests/basic/args.sh
new file mode 100755
index 00000000000..94331014dc5
--- /dev/null
+++ b/tests/cli-tests/basic/args.sh
@@ -0,0 +1,10 @@
+#!/bin/sh
+
+println "+ zstd --blah" >&2
+zstd --blah
+println "+ zstd -xz" >&2
+zstd -xz
+println "+ zstd --adapt=min=1,maxx=2 file.txt" >&2
+zstd --adapt=min=1,maxx=2 file.txt
+println "+ zstd --train-cover=k=48,d=8,steps32 file.txt" >&2
+zstd --train-cover=k=48,d=8,steps32 file.txt
diff --git a/tests/cli-tests/basic/args.sh.exit b/tests/cli-tests/basic/args.sh.exit
new file mode 100644
index 00000000000..d00491fd7e5
--- /dev/null
+++ b/tests/cli-tests/basic/args.sh.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/cli-tests/basic/args.sh.stderr.glob b/tests/cli-tests/basic/args.sh.stderr.glob
new file mode 100644
index 00000000000..df275471ca8
--- /dev/null
+++ b/tests/cli-tests/basic/args.sh.stderr.glob
@@ -0,0 +1,28 @@
++ zstd --blah
+Incorrect parameter: --blah
+...
+Usage: zstd *
+
+Options:
+...
++ zstd -xz
+Incorrect parameter: -x
+...
+Usage: zstd *
+
+Options:
+...
++ zstd --adapt=min=1,maxx=2 file.txt
+Incorrect parameter: --adapt=min=1,maxx=2
+...
+Usage: zstd *
+
+Options:
+...
++ zstd --train-cover=k=48,d=8,steps32 file.txt
+Incorrect parameter: --train-cover=k=48,d=8,steps32
+...
+Usage: zstd *
+
+Options:
+...