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.

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

Make the iso9660 extfs helper read-only #200

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

slowpeek
Copy link
Contributor

mc should NOT edit iso images

Fundamental problem

Quoting man xorriso:

The method of growing adds new data to the existing data on the medium. These data comprise of new file content and they override the existing ISO 9660 + Rock Ridge directory tree. It is possible to hide files from previous sessions but they still exist on the medium and with many types of optical media it is quite easy to recover them by mounting older sessions. Growing is achieved by command -dev.

Modifying takes place if input drive and output drive are not the same and if command -grow_blindly is set to its default "off". This is achieved by commands -indev and -outdev.

Modified images comprise a single session. Grown ones bear a session per a set of commited changes.

A session consists of a 64K superblock, directory data (if any) for the whole image and new (if any) data blocks. There is also 300K padding by default, it can be disabled with -padding 0. In the end it is aligned to 64K boundary.

In the best case (the image is empty and we only change something in the superblock), a session adds 64K to the image. For example, updating the volume id with -volid for an empty image is +64K. When there is some directory data,
it is +128K at least. The session overhead could be pretty big: it is over 11M for debian-12.5.0-amd64-DVD-1.iso.

The good point of growing is speed: it only writes the delta (plus the session overhead). It is suitable for adding data to real media. But there is no point in using it with images since modified images are smaller (no extra session overhead, no blocks of removed files).

In both cases, there is a showstopper to use it with mc:

  • removed files should actually be removed. When a user removes a big file from some iso image, it is expected for the image to become significantly smaller, not slightly larger. Hence, growing is not an option
  • simple changes should not result in lots of writes. When a user creates a dir in some 4G image it is NOT expected to trigger 4G of writes. Hence, modifying is not an option

Appendix: Side effects of simple commands mc currently uses

Any changes should preserve other attributes e.g. El Torito data, Rock Ridge and Joliet presence. For example, such innocent looking command (in this case it does not matter if changes are made in-place or not):

xorriso -dev 1.iso -mkdir /test

wipes El Torito data, enables Rock Ridge and disables Joliet. To make it not alter those, it should be at least like this:

features=$(xorriso -dev 1.iso -toc 2>/dev/null | grep '^ISO offers')

extra=(-boot_image any replay)
[[ $features == *Rock_Ridge* ]] || extra+=(-rockridge off)
[[ $features == *Joliet* ]] && extra+=(-joliet on)

xorriso -dev 1.iso "${extra[@]}" -mkdir /test

@zyv
Copy link
Member

zyv commented Jun 20, 2024

It seems that editing was implemented by Slava, and probably he didn't look too much into what actually happens when you use it:

https://midnight-commander.org/ticket/3027

I won't be very happy removing this functionality.

I agree that growing would be problematic, because most users probably wouldn't expect what actually happens. But I don't agree that having slow editing via modification is worse than no editing at all, as long as this editing is not brutally destructive, only slow.

Why not just add the options you mention to the commands (not using Bash arrays, but just POSIX sh variables for compatibility)?

@slowpeek
Copy link
Contributor Author

But I don't agree that having slow editing via modification is worse than no editing at all

But what is the target audience? I dont see any bug reports related to the obvious current problems: bootable images becoming not bootable, removing a file does not make the image smaller. How come noone ever havent noticed it? The answer is probably the feature has no use among the users.

There is one more evident problem with the current approach: renaming a file makes the image bigger by the file size because it is accomplished as copyout - copyin - rm.

As for the slow modifying approach. It is not only slow, but it takes extra space since the writes go to another file. What if an image is really big? What if, in addition, MC_TMPDIR (or its /tmp fallback) is RAM backed? Like, if it is a 20GB image, is it really okay to require 20GB of extra space to just mkdir or copyin a small file?

Why not just add the options you mention to the commands (not using Bash arrays, but just POSIX sh variables for compatibility)?

I think the idea to implement iso editing in mc was a bad one. Improving on the current state as-is is promoting the bad idea.

@zyv
Copy link
Member

zyv commented Jun 21, 2024

@aborodin what do you think?

@aborodin
Copy link
Member

aborodin commented Jun 22, 2024 via email

@zyv
Copy link
Member

zyv commented Jun 22, 2024

This is not what I'm asking. If you are of the opinion, that this stuff should stay, there is no need for a ticket, and we can reject the PR. If you think it makes sense to remove it, then we already have a ticket - #3027 - and it can be reopened.

@aborodin
Copy link
Member

aborodin commented Jun 22, 2024 via email

@slowpeek
Copy link
Contributor Author

Actually, it is even worse than I thought.

In extfs (or vfs as a whole?) logic it is one item per action. For example, when I copy a dir containing 9 files into an iso image, it results in an mkdir action and 9 copyin ones. When I delete it from an image, it is 9 rm actions and an rmdir one. So, with the growing approach, it is 10x the session overhead. And with the modifying approach it is 10x the amount of writes.

@zyv zyv force-pushed the master branch 2 times, most recently from e9e72d9 to de7d72c Compare October 17, 2024 19:17
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.

3 participants