Skip to content

remove opt_nocopyzero setting globally, since unused#2187

Merged
ghaerr merged 2 commits intoghaerr:masterfrom
floriangit:rm_unused_code_cp
Jan 19, 2025
Merged

remove opt_nocopyzero setting globally, since unused#2187
ghaerr merged 2 commits intoghaerr:masterfrom
floriangit:rm_unused_code_cp

Conversation

@floriangit
Copy link
Copy Markdown
Contributor

@floriangit floriangit commented Jan 18, 2025

: #2182

@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Jan 18, 2025

Hi @floriangit,

I think we've had a miscommunication - the mfs -k option needs to stay, it is fully operational and sometimes used. Only the unused code in cp.c should be removed, since it wasn't ever enabled and uses extra image space in distributions.

Thank you!

@floriangit
Copy link
Copy Markdown
Contributor Author

floriangit commented Jan 18, 2025

Oops, ok. I guess I interpreted your "global" remark as a too strong hint then ;-)
As for the mkfs.c removal I fully understand to revert. The option is declared external coming in from the included protos.h, BUT then declared again with storage in that file and is actually used.

For the genfs.c change, which is almost same as cp.c you want to keep the removal, or not? It will be always 0 anyhow same as in cp.c?

Thanks

@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Jan 18, 2025

The option is declared external coming in from the included protos.h, BUT then declared again with storage in that file and is actually used.

Lets clear up confusion regarding "extern int opt_nocopyzero" and "int opt_nocopyzero = 0" first. An extern int declaration by the C compiler does nothing other than enter the type and size into the symbol table, so that should the symbol be seen again, such as in genfs.c, the compiler knows the type and size to generate code for it. In mkfs.c, the variable itself is seen as both extern int from protos.h, and then declared with initialization to 0, which causes the compiler to emit a non-common global symbol in the .data segment. The option itself is used in genfs.c to implement the non-copy operation. So the extern declaration in protos.h, the initialization in mkfs.c, and the actual use in genfs.c are all required. We don't want to remove any of this, since mfs accepts a -k option which is used to not copy files with zero length.

All the above code is in the tools/mfs/ directory is used to compile a completely separate host-based mfs program, which is used to create MINIX filesystems from scratch for creating distributions.

The cp.c code is entirely different, but looks the same, because it was copied from the mfs/ code when I was looking for a fast way to recursively traverse directories when implementing the cp -R option. What we are trying to do is to remove the code, in cp.c only, which you correctly identified as using an option that was never set. The cp.c code is never linked with the mfs code, it is used to build the ELKS cp binary.

For the genfs.c change, which is almost same as cp.c you want to keep the removal, or not? I will be always 0 anyhow same as in cp.c?

No, in the genfs,.c code, opt_nocopyzero will not always be zero - it will be 1 if the -k option is specified to mfs.

Bottom line, the only file that should be changed is cp.c, removing the code which can't ever execute. As previously discussed, since the code used a global non-static variable, it will never be optimized out.

@ghaerr
Copy link
Copy Markdown
Owner

ghaerr commented Jan 19, 2025

Thank you @floriangit!

@ghaerr ghaerr merged commit 1fb2a47 into ghaerr:master Jan 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants