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

[pulkomandy]

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.

[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.

[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.

[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.

[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.

[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 :-)

[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.

[korli]

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

[jua]

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

[vivek-roy]

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

[vivek-roy]

Fixed long lines as suggested by Barrett

[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.

[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?

[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).

[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

[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!

[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.

[Barrett]

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

[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)

[vivek-roy]

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

[pulkomandy]

Applied in hrev51011. Thanks!