Opened 6 years ago

Closed 6 years ago

#10435 closed bug (fixed)

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

Reported by: anevilyak Owned by: anevilyak
Priority: normal Milestone: R1
Component: Servers/registrar Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

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?

Attachments (1)

10435.patch (7.8 KB ) - added by anevilyak 6 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 by bonefish, 6 years ago

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.

in reply to:  1 ; comment:2 by anevilyak, 6 years ago

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.

in reply to:  2 comment:3 by bonefish, 6 years ago

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.

comment:4 by anevilyak, 6 years ago

Blocking: 10441 added

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

by anevilyak, 6 years ago

Attachment: 10435.patch added

comment:5 by anevilyak, 6 years ago

Has a Patch: set

comment:6 by anevilyak, 6 years ago

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.

comment:7 by anevilyak, 6 years ago

Blocking: 10441 removed

comment:8 by bonefish, 6 years ago

Yep, looks good.

comment:9 by anevilyak, 6 years ago

Owner: changed from bonefish to anevilyak
Status: newin-progress

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

comment:10 by anevilyak, 6 years ago

Resolution: fixed
Status: in-progressclosed

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.

Note: See TracTickets for help on using tickets.