Opened 4 years ago

Closed 2 years ago

#12334 closed bug (fixed)

MediaConvert does not check for read only target directory (easy)

Reported by: pulkomandy Owned by: Barrett
Priority: normal Milestone: R1
Component: Applications/MediaConverter Version: R1/Development
Keywords: gsoc2017 Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Drag tracks from an audio cd to MediaConvert.

The default target is the audio CD mount directory, which is obviously not writable. MediaConvert allows to proceed anyway and will not detect any error. Of course the files are not written anywhere.

MediaConvert should not allow selecting a readonly folder as the target, and by default should use something else (maybe /boot/home) if the source dir is not writable. And it should also do more error checking when converting the files to detect the problem.

Attachments (2)

0001-Added-check-of-write-access-of-selected-output-folde.patch (1.4 KB) - added by vivek-roy 2 years ago.
Fixed long lines as suggested by Barrett
0001-Added-error-messages-when-converted-file-cannot-be-w.patch (3.3 KB) - added by vivek-roy 2 years ago.
I couldn't test it thoroughly because of the frequent crashes I was facing, but this should work.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 4 years ago by vidrep

Perhaps MediaConverter should create its own "Converted Media Files" directory in /boot/home. It would also solve ticket #11500, where converted video files residing in the same directory as the source files rendered the source file unplayable until it was moved into another directory.

comment:2 Changed 2 years ago by vivek-roy

Has a Patch: set

comment:3 Changed 2 years ago by Barrett

Has a Patch: unset
Owner: changed from stippi to Barrett
Status: newassigned

comment:4 Changed 2 years ago by Barrett

Some comments :

  • Fix else brackets
  • Use the BAlert constructor directly instead to create a BString
  • Use the BPath directly no need to use another string to Append (it handles slash automatically), see BPath::Append and BPath::Path which returns a string.
  • Use tempFile instead of fp as var name.

comment:5 Changed 2 years ago by pulkomandy

Are you sure all of this is required just to check that the directory is writable? Have you tried using entry.GetPermissions() instead? If there is write permission then we can proceed. There is no need to check if creating a file really works.

comment:6 Changed 2 years ago by vivek-roy

I tried mode_t perms; entry.GetPermission(&perms); if(perms & S_IWUSR) { } else { } , but then if I try to write to /boot/system/ it still doesn't complain. I guess this behavior is undesired and so I actually have to check if a directory is writable or not.

Last edited 2 years ago by vivek-roy (previous) (diff)

comment:7 Changed 2 years ago by vivek-roy

Has a Patch: set

comment:8 Changed 2 years ago by jua

Agreed with PulkoMandy, checking the permissions is better and cleaner, no need to create a temporary file.

If there is a problem with the write permission check of /boot/system returning a wrong value, then that is a separate bug, please add a bug ticket for it.

We generally try to avoid adding any workarounds for bugs in other places (creating a temporary file here would be such a workaround) -- it's better to solve the actual root cause of the problem then :-)

comment:9 Changed 2 years ago by jua

After further discussion with vivek-roy:

  • The permission check works as expected, no bug there.
  • However, as vivek-roy pointed out, the code would also have to check the current user and all its groups to determine if there really is write permission. That would add unnecessary complexity, so creating the temporary file looks like the simplest solution after all.

comment:10 Changed 2 years ago by korli

jua, access(path.Path(), W_OK) should be sufficient. If not, that should be a bug IMO.

comment:11 Changed 2 years ago by jua

Oh, right. Wasn't aware access() does all that, my POSIX-API-knowledge isn't very strong :-) So yes: what korli said.

comment:12 Changed 2 years ago by vivek-roy

Right. access() works. I tested. Implementing that. Thank you korli

Changed 2 years ago by vivek-roy

Fixed long lines as suggested by Barrett

comment:13 Changed 2 years ago by axeld

access() is not enough for this check! If you mount a writable file system read-only, it will still report the actual file permissions, not the effective ones. If you really want to know whether or not you can write in a certain position, you need to check if it's a read-only volume, first. It's the same on Linux, BTW.

In this particular case, IIRC the permissions in the packagefs are set in the package themselves; I think we have an open bug report that those are messed up when you build Haiku with a weird umask. Not sure if that only affects files, or directories as well, but I assume the latter.

comment:14 Changed 2 years ago by vivek-roy

axeld I am unable to reproduce what you said. I tried to mount a removable flash drive as read-only drive and then select that as the output folder and things worked correctly. access() did complain. Can you tell me the steps to reproduce the error. And what do you suggest as the solution to this? Do you suggest creating a temporary file?

comment:15 Changed 2 years ago by axeld

Sorry, I was completely on the wrong track -- access() should indeed work nicely. What does not work is evaluating stat() information; they will contain the readable bit no matter if the device is being mount read-only or not. access() on the other hand must handle this case. What I had in mind was that you'd check whether or not it's a read-only file system, before doing the check using access().

So the patch does indeed look nice now!

However, I don't think it fixes the main issue: MediaConverter does not report an error if it could not write the files. There are other reasons not being able to write to a volume (like when it's being full, or someone deleted your target parent directory).

comment:16 Changed 2 years ago by vivek-roy

axeld, yes stat() doesn't work but access() does.

In order to fix the other issues I have to check the amount of free space available on the location and if the location actually exists or not when I am actually going to create the converted file.

I understand what you want me to do. I'll give it a try.

Thank You

comment:17 Changed 2 years ago by axeld

Actually, instead of anticipating every possible fail in advance, it's much better to actually report an error when it happens :-)

IOW it's hard to know the amount of space in advance that you need, or whatever other problems might arise. But if errors occur, the user should always be notified about them.

In any case, thanks for working on this!

comment:18 Changed 2 years ago by vivek-roy

Yes. I do plan to popup alerts when the errors occur. I just want to have some meaningful messages for the errors which we have already anticipated.

I am glad to work on this, I am learning a lot.

comment:19 Changed 2 years ago by Barrett

Wouldn't be better to check this when the file is going to be converted?

comment:20 Changed 2 years ago by vivek-roy

Barrett, Yes, when the file is going to be converted, that is when we check if the directory actually exists or not. But whether there is sufficient space available for the new file has to be done during the file is getting converted I guess. (Not sure though if there is any other way)

Changed 2 years ago by vivek-roy

I couldn't test it thoroughly because of the frequent crashes I was facing, but this should work.

comment:21 Changed 2 years ago by pulkomandy

Keywords: gsoc2017 added
Resolution: fixed
Status: assignedclosed

Applied in hrev51011. Thanks!

Note: See TracTickets for help on using tickets.