Problems updating type information for app binaries if a corresponding package exists

[anevilyak]

If a package is installed for an application that has a signature and is registered, and one then tries to e.g. build it in haikuports, a mimeset -f to set its icon can fail. After tracking it down for a bit, what appears to occur is that in MimeInfoUpdater::Do(), SetSupportedTypes() returns 'Operation not permitted', which can actually be traced back to Database::DeleteSupportedTypes() in storage/mime/Database.cpp. fLocation->DeleteAttribute() returns the above value, and this error consequently causes the rest of the mime setting operations to be skipped, including setting the icons.

Moving the corresponding package out of the packages dir and then repeating the mimeset in question succeeds. Not sure as to the best resolution to that problem though, should that error simply be suppressed as is frequently done for e.g. B_ENTRY_NOT_FOUND surrounding mime operations?

[bonefish]

Mmh, there seem to be a few questions/problems. First of all, if not invoked with "-a" or "-A", should mimeset touch the MIME DB at all? It doesn't seem right. Not sure what BeOS, though.

Assuming we don't want that, update_mime_info() and consequently MimeInfoUpdater shouldn't do it either. I'm not even sure that it was implemented intentionally the way it works now. It's just that MimeInfoUpdater::Do() uses BAppFileInfo::SetSupportedTypes(), which, somewhat surprisingly, also updates the MIME DB entry. Most of its methods don't. In fact only SetIconForType() also does that. As it is now, the functionality also doesn't work correctly when using mimeset with a custom MIME DB, since it always uses the system MIME DB.

I think private functionality should be added to BAppFileInfo to support a custom MIME DB and optionally avoid writing to it.

That being said, the behavior of DatabaseLocation::DeleteAttribute() is incorrect. It just uses _OpenType(), but it actually needs a writable type. OpenOrCreateType() would be more appropriate, but not quite, since actually we don't want to create a type when it doesn't exist yet. Renaming the method to (or adding a new) OpenWritableType() with an additional "bool create" parameter would be the best solution.

Finally there's the general question what is supposed to happen when an application is installed/available multiple times, as there can only be one MIME DB entry for the signature. Having mimeset only manipulate the MIME DB when "-a" or "-A" is specified makes a lot of sense in that light.

[anevilyak]

Replying to bonefish:

Mmh, there seem to be a few questions/problems. First of all, if not invoked with "-a" or "-A", should mimeset touch the MIME DB at all? It doesn't seem right. Not sure what BeOS, though.

Regarding BeOS, I'm uncertain there as well, but I would say that I wouldn't have expected mimeset to do so without passing one of the options that explicitly mentions the database.

That being said, the behavior of DatabaseLocation::DeleteAttribute() is incorrect. It just uses _OpenType(), but it actually needs a writable type. OpenOrCreateType() would be more appropriate, but not quite, since actually we don't want to create a type when it doesn't exist yet. Renaming the method to (or adding a new) OpenWritableType() with an additional "bool create" parameter would be the best solution.

Understood, I can look into that.

Finally there's the general question what is supposed to happen when an application is installed/available multiple times, as there can only be one MIME DB entry for the signature. Having mimeset only manipulate the MIME DB when "-a" or "-A" is specified makes a lot of sense in that light.

+1. On a related note regarding MimeInfoUpdater::Do(), a question about the conditional block ​here: Do you know if it's intentional that all those potential failures are suppressed? Currently, if anyone of those fails, none of the encapsulated update operations will wind up actually taking place, but the caller will have no way of detecting it.

[bonefish]

Replying to anevilyak:

+1. On a related note regarding MimeInfoUpdater::Do(), a question about the conditional block ​here: Do you know if it's intentional that all those potential failures are suppressed? Currently, if anyone of those fails, none of the encapsulated update operations will wind up actually taking place, but the caller will have no way of detecting it.

I don't see a reason why the errors are suppressed in this case, so I guess the subconditions should simply be replaced by (err = ...) == B_OK. I suppose a reason for the somewhat inconsistent error handling might be that the API (update_mime_info()) supports recursive operation without any actual control over the recursion or possible handling of errors. E.g. if some file has an incorrect (application) MIME type, the whole recursive operation fails without providing the caller any information about what happened where.

[anevilyak]

(In #10441) The package needs to be rebuilt once #10435 is resolved.

[anevilyak]

Does attachment:10435.patch​ look reasonable? Appears to work correctly over here. It does not, however, touch the private mime database issues you brought up, but I think that would be better served as the subject of a separate ticket.

[bonefish]

Yep, looks good.

[anevilyak]

Thanks for the review :) Will apply that after work tonight.

[anevilyak]

Patch applied in hrev46724. I guess the question of whether things should be refactored to allow mimeset -f to avoid touching the database at all is probably best addressed on the development list.