udftune: Add initial implementation

[jtru]

This patch adds a new udftools command/executable, udftune.

It implements two useful operations: "--mark-ro" and "--mark-rw", for setting FSD and LVID Domain flags in a compatible UDF filesystem to indicate either read-only or read/write characteristics.

The code borrows heavily from udflabel, which contains nearly all of the required logic to modify a UDF target in this manner by itself.

Since udflabel is to be used for interacting with UDF label information only, udftune was created in its current form.

[jtru]

This hopes to replace #62 and fix/provide for the feature requested in #60.

[jtru]

Is this more to your liking than my first approach that would extend udflabel, @pali?

If there's a prospect of getting this merged, I would try to work on having the common code between udflabel and udftune properly shared between the two utilities, instead of duplicated from one into the other.

[pali]

Yea, this is better approach. But code is still in WIP state and needs cleanups and separating duplicate code into separate shared file (like is readdisc.c used in more tools).

[jtru]

Thanks for the feedback! :)

Would you prefer for the shared code that is to be split out to end up in ...

1. readdisc.c? I think it's a bit of an odd fit in there, since some of the code that is to be moved does not deal with reading, but writing UDF structures. On the other hand, not all code in readdisc.c as of today does concern itself only with reading either, afaiui (everything calling set_extent, for example).
2. A separate, newly created $some_fitting_name.c file that all those udftools that use the common code in it would include? The follow-up question then is, where should that yet-to-be-named file be "at home"; under which pre-existing tool's sub-directory (udfinfo and udflabel share code that would move already, and udftune would join them)?
3. libudffs - I think that would be a natural fit for at least some of the concerned functions - but the existing code in the library keeps stdout/stderr clean, and the functions that should be merged and move don't do that.
4. Some other option I have not considered yet?

(I have option 3 ready locally, but without having taken care of the the output-on-stdio-descriptors problem.)

[pali]

This common code is for modifying existing udf fs, so it should not be in udfinfo dir. udflabel dir is for label related stuff. So put it into new udftune dir into newly created file (e.g. updatedisc.c?)

[jtru]

Does this look OK yet?

[jtru]

Is there anything else obviously missing/wrong and in need of fixing before this can be merged?

[pali]

I was waiting until you finish it. There are still lot of duplicated copy+paste code in udftune/main.c file. For example code for checking access type or code which do descriptors updates. Also look that you have copied code which is never called.

[jtru]

I'm aware that there are superfluous checks and functionality in the current udftune/main.c:main() (which was mostly lifted off of udflabel's current source), but that was intentional - as soon as anyone implements a feature that requires touching something other than the fsd and/or the lvd, whatever gets cut away now is going to have to be re-introduced (hence my original argument that udflabel is so feature-complete that it should probably have been the original udftune all along... ;)) at that point in time. Sure I can trim all that to get rid of the duplication if you like, but then whoever comes after will have a harder time getting their intended udftune feature done.

Can you please elaborate which code you are referring to in your last sentece, where you say it is "never called"? I'm afraid my attempt at diffing the existing with the added-on isn't quite good enough to figure it out... (and I'm not familiar enough with suitable static analysis tools to figure out which code can't possibly be called either).

[pali]

You already figured out that it is that non-fsd/lvd code.

[jtru]

Hrmm, the failing CI checks look like they could be related to old git releases in use on the failing platforms? (I did a rebase to fix up my commit messages to be more in style with prior commits that I had read, and force-pushed the result - something along those lines seems to have messed with the repo's internals enough to cause this little mess...)

[jtru]

Please help me to figure out how to proceed, @pali - the build failure is not related to building the source code itself, but to fetching the build input artifacts via git. I think the checks would actually pass if it came to the compiler even having a go at it... but I don't know how I can achieve that.

OTOH, maybe it's about time to drop these long-out-of-support OS (Ubuntu 18.04 is dropping out of Canonical's support these next few days, and the failing environments are based on Ubuntu 14.04 and 12.04) releases from the CI gauntlet altogether anyway?

Apart from that, is there any improvement you need made before this can get merged?

Thanks for taking the time to answer my questions.

[pali]

Apart from that, is there any improvement you need made before this can get merged?

Yes, I have already wrote that there is duplicated code for checking access type. Really copy+paste is bad idea here, it should be moved to common function like you did it for other functions moved to updatedisc.c.

[jtru]

I would like to have these changes try to survive a pass through the CI pipeline, but I have no idea how (or even if) I can request that. Can you make that happen, @pali?

Also, do you have an idea what I might do to make the old git releases installed in these CI environments survive the current repo content? (According to the SO post linked to earlier, making git clone the repo without history (i.e., shallow) could probably do the trick, but I am not familiar enough with travis to know how to tell it to do that.)

[pali]

I will look at the changes carefully later. But it looks better now. And about CI, just ignore it, I will take care about Travis CI issues.

[jtru]

OK, thanks.

[jtru]

If you need me to revisit anything in order to get this merged, please let me know. I have some time off during the next weeks.

[pali]

Hello, sorry for a longer delay. I was looking at it. It looks better, I have few comments below and ignore CI issue for now.

• Rename sync_device to something like sync_and_close as it does not only sync.
• You are leaking file descriptors in open_existing_disc() during error handling (they should be closed).
• main cannot return negative value, this needs to be fixed.
• You have put code for checking of some descriptors into helper functions (e.g. verify_fsd, verify_lvd) but not for iuvd.
• I think that whole code which do "Updating ...\n" should be put into one helper function which would take all update_pvd, update_lvd, ... variables. And this function would be called from both udflabel and udftune. Try to think about it.
• New udftune does not do anything with new_vid or new_fsid. Check all variables and those which are unused (e.g. only set, but never read) completely remove from udftune code. If they would be needed in future, they can be added.
• Update open_existing_disc() to do not pass all fd flags. Pass just "int rw" variable to indicate if device would be opened in r/w mode or read-only mode; and compose fd flags in open_existing_disc().
• Move code for checking VAT and PseudoOverWrite into check_access_type() function.
• udftune needs all "Block Options" which has udflabel, as those specify how to read existing udf fs.
• And about code style: always write newline after "if"... so following line if (func()) return; should be split into if (func()) and on next line return;.

[pali]

And also there is missing udftune documentation - manual page - in similar form as there is existing one for udflabel (explanation of all command line options; plus description what is tool doing).

[pali]

Now I see that you pushed updated to this PR (github did not sent me notification). Could you squash relevant changes together and do force-push? Because for example now there is commit which adds some code and then in later commit you completely remove that code due to refactoring. What makes sense is to have separate commit which moves shared code into common files and then commit which adds a new tool -- meaning logically different changes in separate/different commits.

[jtru]

Oh yes I did, but I was actually still pondering some of the comments you had made :) I plan to work on it some more this coming weekend.

Thanks for having a look and the advice so far!

[jtru]

Hey @pali, if you could take another look at the current commit history and tell me if that's what you had in mind, that would be great! Thanks! :)

(I am sorry about once again making a mess of the older CI envs, but I just don't see a way to dig the repo's/PR's state out of this particular hole...)

[jtru]

Ping :) With the holiday season coming up, I guess I would have once again some more time to invest in getting this merged. Please let me know what you still need done, @pali.

Thanks!

[jtru]

Just in case anyone is wondering what has become of this PR - @pali contacted me via email, stating that he had lost access to his Github account due to some forced-MFA related problem that appears unsolvable, despite the involvement of Github support. Given these circumstances, I am not sure if udftools development will continue here, or at all :(