Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disable --rm on -o command #3450

Merged
merged 2 commits into from
Jan 26, 2023
Merged

disable --rm on -o command #3450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c85b29
disable --rm on -o command
Cyan4973 Jan 24, 2023
02434e0
enforce a hard fail when input files are set to be erased
Cyan4973 Jan 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 71 additions & 40 deletions programs/fileio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,7 @@ FIO_openDstFile(FIO_ctx_t* fCtx, FIO_prefs_t* const prefs,
if (!prefs->overwrite) {
if (g_display_prefs.displayLevel <= 1) {
/* No interaction possible */
DISPLAY("zstd: %s already exists; not overwritten \n",
DISPLAYLEVEL(1, "zstd: %s already exists; not overwritten \n",
dstFileName);
return NULL;
}
Expand Down Expand Up @@ -723,7 +723,7 @@ int FIO_checkFilenameCollisions(const char** filenameTable, unsigned nbFiles) {

filenameTableSorted = (const char**) malloc(sizeof(char*) * nbFiles);
if (!filenameTableSorted) {
DISPLAY("Unable to malloc new str array, not checking for name collisions\n");
DISPLAYLEVEL(1, "Allocation error during filename collision checking \n");
return 1;
}

Expand All @@ -740,7 +740,7 @@ int FIO_checkFilenameCollisions(const char** filenameTable, unsigned nbFiles) {
prevElem = filenameTableSorted[0];
for (u = 1; u < nbFiles; ++u) {
if (strcmp(prevElem, filenameTableSorted[u]) == 0) {
DISPLAY("WARNING: Two files have same filename: %s\n", prevElem);
DISPLAYLEVEL(2, "WARNING: Two files have same filename: %s\n", prevElem);
}
prevElem = filenameTableSorted[u];
}
Expand Down Expand Up @@ -823,41 +823,71 @@ static void FIO_adjustMemLimitForPatchFromMode(FIO_prefs_t* const prefs,
FIO_setMemLimit(prefs, (unsigned)maxSize);
}

/* FIO_removeMultiFilesWarning() :
/* FIO_multiFilesConcatWarning() :
* This function handles logic when processing multiple files with -o or -c, displaying the appropriate warnings/prompts.
* Returns 1 if the console should abort, 0 if console should proceed.
* This function handles logic when processing multiple files with -o, displaying the appropriate warnings/prompts.
*
* If -f is specified, or there is just 1 file, zstd will always proceed as usual.
* If --rm is specified, there will be a prompt asking for user confirmation.
* If -f is specified with --rm, zstd will proceed as usual
* If -q is specified with --rm, zstd will abort pre-emptively
* If neither flag is specified, zstd will prompt the user for confirmation to proceed.
* If --rm is not specified, then zstd will print a warning to the user (which can be silenced with -q).
* Note : --rm in combination with stdout is not allowed.
* If output is stdout or test mode is active, check that `--rm` disabled.
*
* If there is just 1 file to process, zstd will proceed as usual.
* If each file get processed into its own separate destination file, proceed as usual.
*
* When multiple files are processed into a single output,
* display a warning message, then disable --rm if it's set.
*
* If -f is specified or if output is stdout, just proceed.
* If output is set with -o, prompt for confirmation.
*/
static int FIO_removeMultiFilesWarning(FIO_ctx_t* const fCtx, const FIO_prefs_t* const prefs, const char* outFileName, int displayLevelCutoff)
{
int error = 0;
if (fCtx->nbFilesTotal > 1 && !prefs->overwrite) {
if (g_display_prefs.displayLevel <= displayLevelCutoff) {
if (prefs->removeSrcFile) {
DISPLAYLEVEL(1, "zstd: Aborting... not deleting files and processing into dst: %s\n", outFileName);
error = 1;
}
} else {
if (!strcmp(outFileName, stdoutmark)) {
DISPLAYLEVEL(2, "zstd: WARNING: all input files will be processed and concatenated into stdout. \n");
} else {
DISPLAYLEVEL(2, "zstd: WARNING: all input files will be processed and concatenated into a single output file: %s \n", outFileName);
}
DISPLAYLEVEL(2, "The concatenated output CANNOT regenerate original file names nor directory structure. \n")
if (prefs->removeSrcFile) {
assert(fCtx->hasStdoutOutput == 0); /* not possible : never erase source files when output == stdout */
error = (g_display_prefs.displayLevel > displayLevelCutoff) && UTIL_requireUserConfirmation("This is a destructive operation. Proceed? (y/n): ", "Aborting...", "yY", fCtx->hasStdinInput);
}
}
static int FIO_multiFilesConcatWarning(const FIO_ctx_t* fCtx, FIO_prefs_t* prefs, const char* outFileName, int displayLevelCutoff)
{
if (fCtx->hasStdoutOutput) {
if (prefs->removeSrcFile)
/* this should not happen ; hard fail, to protect user's data
* note: this should rather be an assert(), but we want to be certain that user's data will not be wiped out in case it nonetheless happen */
EXM_THROW(43, "It's not allowed to remove input files when processed output is piped to stdout. "
"This scenario is not supposed to be possible. "
"This is a programming error. File an issue for it to be fixed.");
}
if (prefs->testMode) {
if (prefs->removeSrcFile)
/* this should not happen ; hard fail, to protect user's data
* note: this should rather be an assert(), but we want to be certain that user's data will not be wiped out in case it nonetheless happen */
EXM_THROW(43, "Test mode shall not remove input files! "
"This scenario is not supposed to be possible. "
"This is a programming error. File an issue for it to be fixed.");
return 0;
}
return error;

if (fCtx->nbFilesTotal == 1) return 0;
assert(fCtx->nbFilesTotal > 1);

if (!outFileName) return 0;

if (fCtx->hasStdoutOutput) {
DISPLAYLEVEL(2, "zstd: WARNING: all input files will be processed and concatenated into stdout. \n");
} else {
DISPLAYLEVEL(2, "zstd: WARNING: all input files will be processed and concatenated into a single output file: %s \n", outFileName);
}
DISPLAYLEVEL(2, "The concatenated output CANNOT regenerate original file names nor directory structure. \n")

/* multi-input into single output : --rm is not allowed */
if (prefs->removeSrcFile) {
DISPLAYLEVEL(2, "Since it's a destructive operation, input files will not be removed. \n");
prefs->removeSrcFile = 0;
}

if (fCtx->hasStdoutOutput) return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we not prompting if we have stdout as an output? I think the same behavior as -o for -c would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it would seem logical to have -o and -c sharing the same behavior.
But for -c, we are following the established behavior of gzip, and gzip features no prompt in this case.
-o on the other hand only exists for zstd, so we can select a more protective behavior.

if (prefs->overwrite) return 0;

/* multiple files concatenated into single destination file using -o without -f */
if (g_display_prefs.displayLevel <= displayLevelCutoff) {
/* quiet mode => no prompt => fail automatically */
DISPLAYLEVEL(1, "Concatenating multiple processed inputs into a single output loses file metadata. \n");
DISPLAYLEVEL(1, "Aborting. \n");
return 1;
}
/* normal mode => prompt */
return UTIL_requireUserConfirmation("Proceed? (y/n): ", "Aborting...", "yY", fCtx->hasStdinInput);
}

static ZSTD_inBuffer setInBuffer(const void* buf, size_t s, size_t pos)
Expand Down Expand Up @@ -1767,9 +1797,9 @@ FIO_compressFilename_srcFile(FIO_ctx_t* const fCtx,
&srcFileStat, compressionLevel);
AIO_ReadPool_closeFile(ress.readCtx);

if ( prefs->removeSrcFile /* --rm */
&& result == 0 /* success */
&& strcmp(srcFileName, stdinmark) /* exception : don't erase stdin */
if ( prefs->removeSrcFile /* --rm */
&& result == 0 /* success */
&& strcmp(srcFileName, stdinmark) /* exception : don't erase stdin */
) {
/* We must clear the handler, since after this point calling it would
* delete both the source and destination files.
Expand All @@ -1791,7 +1821,8 @@ checked_index(const char* options[], size_t length, size_t index) {

#define INDEX(options, index) checked_index((options), sizeof(options) / sizeof(char*), (size_t)(index))

void FIO_displayCompressionParameters(const FIO_prefs_t* prefs) {
void FIO_displayCompressionParameters(const FIO_prefs_t* prefs)
{
static const char* formatOptions[5] = {ZSTD_EXTENSION, GZ_EXTENSION, XZ_EXTENSION,
LZMA_EXTENSION, LZ4_EXTENSION};
static const char* sparseOptions[3] = {" --no-sparse", "", " --sparse"};
Expand Down Expand Up @@ -1920,7 +1951,7 @@ int FIO_compressMultipleFilenames(FIO_ctx_t* const fCtx,
assert(outFileName != NULL || suffix != NULL);
if (outFileName != NULL) { /* output into a single destination (stdout typically) */
FILE *dstFile;
if (FIO_removeMultiFilesWarning(fCtx, prefs, outFileName, 1 /* displayLevelCutoff */)) {
if (FIO_multiFilesConcatWarning(fCtx, prefs, outFileName, 1 /* displayLevelCutoff */)) {
FIO_freeCResources(&ress);
return 1;
}
Expand Down Expand Up @@ -2742,7 +2773,7 @@ FIO_decompressMultipleFilenames(FIO_ctx_t* const fCtx,
dRess_t ress = FIO_createDResources(prefs, dictFileName);

if (outFileName) {
if (FIO_removeMultiFilesWarning(fCtx, prefs, outFileName, 1 /* displayLevelCutoff */)) {
if (FIO_multiFilesConcatWarning(fCtx, prefs, outFileName, 1 /* displayLevelCutoff */)) {
FIO_freeDResources(ress);
return 1;
}
Expand Down
2 changes: 1 addition & 1 deletion programs/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ int UTIL_requireUserConfirmation(const char* prompt, const char* abortMsg,
ch = getchar();
result = 0;
if (strchr(acceptableLetters, ch) == NULL) {
UTIL_DISPLAY("%s", abortMsg);
UTIL_DISPLAY("%s \n", abortMsg);
result = 1;
}
/* flush the rest */
Expand Down
38 changes: 22 additions & 16 deletions programs/zstdcli.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ static const char* lastNameFromPath(const char* path)

static void errorOut(const char* msg)
{
DISPLAY("%s \n", msg); exit(1);
DISPLAYLEVEL(1, "%s \n", msg); exit(1);
}

/*! readU32FromCharChecked() :
Expand Down Expand Up @@ -786,13 +786,13 @@ static unsigned init_nbThreads(void) {
} else { \
argNb++; \
if (argNb >= argCount) { \
DISPLAY("error: missing command argument \n"); \
DISPLAYLEVEL(1, "error: missing command argument \n"); \
CLEAN_RETURN(1); \
} \
ptr = argv[argNb]; \
assert(ptr != NULL); \
if (ptr[0]=='-') { \
DISPLAY("error: command cannot be separated from its argument by another command \n"); \
DISPLAYLEVEL(1, "error: command cannot be separated from its argument by another command \n"); \
CLEAN_RETURN(1); \
} } }

Expand Down Expand Up @@ -859,6 +859,7 @@ int main(int argCount, const char* argv[])

FIO_prefs_t* const prefs = FIO_createPreferences();
FIO_ctx_t* const fCtx = FIO_createContext();
FIO_progressSetting_e progress = FIO_ps_auto;
zstd_operation_mode operation = zom_compress;
ZSTD_compressionParameters compressionParams;
int cLevel = init_cLevel();
Expand Down Expand Up @@ -898,7 +899,7 @@ int main(int argCount, const char* argv[])
(void)recursive; (void)cLevelLast; /* not used when ZSTD_NOBENCH set */
(void)memLimit;
assert(argCount >= 1);
if ((filenames==NULL) || (file_of_names==NULL)) { DISPLAY("zstd: allocation error \n"); exit(1); }
if ((filenames==NULL) || (file_of_names==NULL)) { DISPLAYLEVEL(1, "zstd: allocation error \n"); exit(1); }
programName = lastNameFromPath(programName);
#ifdef ZSTD_MULTITHREAD
nbWorkers = init_nbThreads();
Expand Down Expand Up @@ -999,8 +1000,8 @@ int main(int argCount, const char* argv[])
if (!strcmp(argument, "--rsyncable")) { rsyncable = 1; continue; }
if (!strcmp(argument, "--compress-literals")) { literalCompressionMode = ZSTD_ps_enable; continue; }
if (!strcmp(argument, "--no-compress-literals")) { literalCompressionMode = ZSTD_ps_disable; continue; }
if (!strcmp(argument, "--no-progress")) { FIO_setProgressSetting(FIO_ps_never); continue; }
if (!strcmp(argument, "--progress")) { FIO_setProgressSetting(FIO_ps_always); continue; }
if (!strcmp(argument, "--no-progress")) { progress = FIO_ps_never; continue; }
if (!strcmp(argument, "--progress")) { progress = FIO_ps_always; continue; }
if (!strcmp(argument, "--exclude-compressed")) { FIO_setExcludeCompressedFile(prefs, 1); continue; }
if (!strcmp(argument, "--fake-stdin-is-console")) { UTIL_fakeStdinIsConsole(); continue; }
if (!strcmp(argument, "--fake-stdout-is-console")) { UTIL_fakeStdoutIsConsole(); continue; }
Expand Down Expand Up @@ -1057,7 +1058,7 @@ int main(int argCount, const char* argv[])
if (longCommandWArg(&argument, "--output-dir-flat")) {
NEXT_FIELD(outDirName);
if (strlen(outDirName) == 0) {
DISPLAY("error: output dir cannot be empty string (did you mean to pass '.' instead?)\n");
DISPLAYLEVEL(1, "error: output dir cannot be empty string (did you mean to pass '.' instead?)\n");
CLEAN_RETURN(1);
}
continue;
Expand All @@ -1073,7 +1074,7 @@ int main(int argCount, const char* argv[])
if (longCommandWArg(&argument, "--output-dir-mirror")) {
NEXT_FIELD(outMirroredDirName);
if (strlen(outMirroredDirName) == 0) {
DISPLAY("error: output dir cannot be empty string (did you mean to pass '.' instead?)\n");
DISPLAYLEVEL(1, "error: output dir cannot be empty string (did you mean to pass '.' instead?)\n");
CLEAN_RETURN(1);
}
continue;
Expand Down Expand Up @@ -1349,7 +1350,7 @@ int main(int argCount, const char* argv[])
int const ret = FIO_listMultipleFiles((unsigned)filenames->tableSize, filenames->fileNames, g_displayLevel);
CLEAN_RETURN(ret);
#else
DISPLAY("file information is not supported \n");
DISPLAYLEVEL(1, "file information is not supported \n");
CLEAN_RETURN(1);
#endif
}
Expand Down Expand Up @@ -1480,24 +1481,29 @@ int main(int argCount, const char* argv[])

if (showDefaultCParams) {
if (operation == zom_decompress) {
DISPLAY("error : can't use --show-default-cparams in decompression mode \n");
DISPLAYLEVEL(1, "error : can't use --show-default-cparams in decompression mode \n");
CLEAN_RETURN(1);
}
}

if (dictFileName != NULL && patchFromDictFileName != NULL) {
DISPLAY("error : can't use -D and --patch-from=# at the same time \n");
DISPLAYLEVEL(1, "error : can't use -D and --patch-from=# at the same time \n");
CLEAN_RETURN(1);
}

if (patchFromDictFileName != NULL && filenames->tableSize > 1) {
DISPLAY("error : can't use --patch-from=# on multiple files \n");
DISPLAYLEVEL(1, "error : can't use --patch-from=# on multiple files \n");
CLEAN_RETURN(1);
}

/* No status message in pipe mode (stdin - stdout) */
/* No status message by default when output is stdout */
hasStdout = outFileName && !strcmp(outFileName,stdoutmark);
if ((hasStdout || !UTIL_isConsole(stderr)) && (g_displayLevel==2)) g_displayLevel=1;
if (hasStdout && (g_displayLevel==2)) g_displayLevel=1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is the line that is causing issues in the test. It is producing logs in the cli-tests when we didn't expect them because stderr is not a console.

When !UTIL_isConsole(stderr) we can instead set the --no-progress flag. Though we should make sure that using --progress will still re-enable progress messages.

Can you separate this change into a separate PR? Since this could change the CLI tests output. And otherwise the CLI tests should be unaffected by this PR.

Copy link
Contributor Author

@Cyan4973 Cyan4973 Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an issue with some tests though.

When displayLevel==2 (default), the CLI expects interactive mode, and will sometimes prompt for confirmation.
There are a few tests which expect that to happen, and pipe a y into stdin to continue the operation successfully.

It happens here: when a user wants to concatenate multiple frames into a single output,
the CLI warns that it's probably not a good idea, and then asks the user for confirmation of this operation.

However, with displayLevel==1, this is interpreted as --quiet, in which case no prompt ever happens, and instead it's an instant failure. Since the test was expecting "success" (by providing the y to the prompt), the test now fails.

With the !UTIL_isConsole(stderr) automatic change of verbosity, CLI tests behave one way locally, and differently on Github Actions. That's a problem, tests require a unified behavior.

There are probably several ways to get around this problem.

For example, one could simply avoid testing interactive prompt scenarios, by forcing such scenarios to be run exclusively with -q or -v.
Of course, such a strategy would make the test suite blind to interactive scenarios.

So I was more in favor of trying to "fix" this behavior, since the intention was just to remove the status updates from stderr when stderr is a log. But if it triggers too many complications, then maybe the answer will be to test less scenarios...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you're trying to test, it makes sense to combine it into this PR then.


/* when stderr is not the console, do not pollute it with status updates
* Note : the below code actually also silence more stuff, including completion report. */
if (!UTIL_isConsole(stderr) && (g_displayLevel==2)) g_displayLevel=1;
FIO_setProgressSetting(progress);

/* don't remove source files when output is stdout */;
if (hasStdout && removeSrcFile) {
Expand Down Expand Up @@ -1569,7 +1575,7 @@ int main(int argCount, const char* argv[])
operationResult = FIO_compressMultipleFilenames(fCtx, prefs, filenames->fileNames, outMirroredDirName, outDirName, outFileName, suffix, dictFileName, cLevel, compressionParams);
#else
(void)contentSize; (void)suffix; (void)adapt; (void)rsyncable; (void)ultra; (void)cLevel; (void)ldmFlag; (void)literalCompressionMode; (void)targetCBlockSize; (void)streamSrcSize; (void)srcSizeHint; (void)ZSTD_strategyMap; (void)useRowMatchFinder; /* not used when ZSTD_NOCOMPRESS set */
DISPLAY("Compression not supported \n");
DISPLAYLEVEL(1, "Compression not supported \n");
#endif
} else { /* decompression or test */
#ifndef ZSTD_NODECOMPRESS
Expand All @@ -1579,7 +1585,7 @@ int main(int argCount, const char* argv[])
operationResult = FIO_decompressMultipleFilenames(fCtx, prefs, filenames->fileNames, outMirroredDirName, outDirName, outFileName, dictFileName);
}
#else
DISPLAY("Decompression not supported \n");
DISPLAYLEVEL(1, "Decompression not supported \n");
#endif
}

Expand Down
2 changes: 2 additions & 0 deletions tests/cli-tests/file-stat/compress-file-to-stdout.sh.stderr.exact
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ Trace:FileStat: > UTIL_isLink(file)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_isConsole(1)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_isConsole(2)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_getFileSize(file)
Trace:FileStat: > UTIL_stat(file)
Trace:FileStat: < 1
Expand Down
2 changes: 2 additions & 0 deletions tests/cli-tests/file-stat/compress-stdin-to-stdout.sh.stderr.exact
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ Trace:FileStat: > UTIL_isConsole(0)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_isConsole(1)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_isConsole(2)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_getFileSize(/*stdin*\)
Trace:FileStat: > UTIL_stat(/*stdin*\)
Trace:FileStat: < 0
Expand Down
2 changes: 2 additions & 0 deletions tests/cli-tests/file-stat/decompress-file-to-stdout.sh.stderr.exact
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ Trace:FileStat: > UTIL_isLink(file.zst)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_isConsole(1)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_isConsole(2)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_isDirectory(file.zst)
Trace:FileStat: > UTIL_stat(file.zst)
Trace:FileStat: < 1
Expand Down
2 changes: 2 additions & 0 deletions tests/cli-tests/file-stat/decompress-stdin-to-stdout.sh.stderr.exact
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ Trace:FileStat: > UTIL_isConsole(0)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_isConsole(1)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_isConsole(2)
Trace:FileStat: < 0
Trace:FileStat: > UTIL_isDirectory(/*stdin*\)
Trace:FileStat: > UTIL_stat(/*stdin*\)
Trace:FileStat: < 0
Expand Down
Loading