-
Notifications
You must be signed in to change notification settings - Fork 30
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
udftune: Add initial implementation #63
base: master
Are you sure you want to change the base?
Conversation
Is this more to your liking than my first approach that would extend If there's a prospect of getting this merged, I would try to work on having the common code between |
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). |
Thanks for the feedback! :) Would you prefer for the shared code that is to be split out to end up in ...
(I have option 3 ready locally, but without having taken care of the the output-on-stdio-descriptors problem.) |
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?) |
Does this look OK yet? |
Is there anything else obviously missing/wrong and in need of fixing before this can be merged? |
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. |
I'm aware that there are superfluous checks and functionality in the current 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). |
You already figured out that it is that non-fsd/lvd code. |
Hrmm, the failing CI checks look like they could be related to old |
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. |
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. |
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 |
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. |
OK, thanks. |
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. |
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.
|
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). |
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. |
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! |
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.
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...) |
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! |
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 :( |
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.