Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#9672 closed bug (fixed)

False positive of alert prompt.

Reported by: mmadia Owned by: axeld
Priority: normal Milestone: R1
Component: Applications/DriveSetup Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

This issue is introduced with hrev45277.

When using DriveSetup to mount any partition (BFS or non-BFS), an informational alert of 'Could not mount partition "<name>"' will needlessly display. The partition is mounted successfully.

Attachments (2)

fixv4.patch (2.3 KB ) - added by puckipedia 6 years ago.
Fix
fixv5.patch (2.2 KB ) - added by puckipedia 6 years ago.
Second version of the fix

Download all attachments as: .zip

Change History (14)

by puckipedia, 6 years ago

Attachment: fixv4.patch added

Fix

comment:1 by puckipedia, 6 years ago

Has a Patch: set

comment:3 by korli, 6 years ago

src/kits/storage/disk_device/Partition.cpp: Line 574, a space after if is missing. Also only set *devicePointer in case of success.

This is an API change, this means you checked that no code used the dev_t returned by this method, right?

in reply to:  3 ; comment:4 by siarzhuk, 6 years ago

Replying to korli:

This is an API change, this means you checked that no code used the dev_t returned by this method, right?

BTW, dev_t returned by this method is casted silently into status_t and checked for error as (result < B_OK). After style fixing it in hrev45277 to more correct using of status_t as (result != B_OK) DriveSetup started to complain about failed mount operation.

by puckipedia, 6 years ago

Attachment: fixv5.patch added

Second version of the fix

in reply to:  4 comment:5 by puckipedia, 6 years ago

Replying to korli:

This is an API change, this means you checked that no code used the dev_t returned by this method, right?

Yes, I checked that no code used the dev_t, all only used the status_t.

Replying to siarzhuk:

BTW, dev_t returned by this method is casted silently into status_t and checked for error as (result < B_OK). After style fixing it in hrev45277 to more correct using of status_t as (result != B_OK) DriveSetup started to complain about failed mount operation.

This was the original patch (see GCI page), but I changed it mostly because all uses just use the status_t portion.

I added a new patch containing the two style fixes.

comment:6 by jessicah, 5 years ago

Resolution: fixed
Status: newclosed

Applied in hrev47853.

comment:7 by axeld, 5 years ago

Unfortunately, I missed this ticket; I'm not really fond of the suggested solution. Moving the dev_t to an argument doesn't really improve anything, so I think it should be reverted again, and the DriveSetup bug fixed instead (sorry for that one).

in reply to:  7 ; comment:8 by korli, 5 years ago

Replying to axeld:

Unfortunately, I missed this ticket; I'm not really fond of the suggested solution. Moving the dev_t to an argument doesn't really improve anything, so I think it should be reverted again, and the DriveSetup bug fixed instead (sorry for that one).

In this case, the documentation for BPartition::Mount() needs to be fixed too.

in reply to:  8 ; comment:9 by jessicah, 5 years ago

Replying to korli:

Replying to axeld:

Unfortunately, I missed this ticket; I'm not really fond of the suggested solution. Moving the dev_t to an argument doesn't really improve anything, so I think it should be reverted again, and the DriveSetup bug fixed instead (sorry for that one).

In this case, the documentation for BPartition::Mount() needs to be fixed too.

The documentation was updated. Also, the already existing documentation says it returns B_OK on success, not a dev_t; so whilst it has returned a dev_t since forever, it also says it returns B_OK on success since forever.

Is there any reason we even need to expose the dev_t handle here anyway?

in reply to:  9 comment:10 by bonefish, 5 years ago

Replying to jessicah:

Is there any reason we even need to expose the dev_t handle here anyway?

When you check the users of this method in our sources and don't find any actually requiring the dev_t, then I'd say it isn't needed and could as well return a status_t. There's a BPartition::GetVolume() that allows indirect access to the dev_t. An optional BVolume* return parameter for Mount() would be another option.

comment:11 by axeld, 5 years ago

Just FYI, I'd be okay with all proposed solutions (ie. keep dev_t, return status_t, have an optional BVolume return parameter).

I think returning a status_t without any extra parameters would actually be the best choice, though.

comment:12 by jessicah, 5 years ago

Updated API applied in hrev47861. Removed extra parameter to return the dev_t handle, leaving the function to only return a status_t, since the dev_t handle currently isn't used, and as mentioned, can be retrieved via GetVolume() instead.

Note: See TracTickets for help on using tickets.