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 5 years ago.
Fix
fixv5.patch (2.2 KB) - added by puckipedia 5 years ago.
Second version of the fix

Download all attachments as: .zip

Change History (14)

Changed 5 years ago by puckipedia

Attachment: fixv4.patch added

Fix

comment:1 Changed 5 years ago by puckipedia

Has a Patch: set

comment:3 Changed 5 years ago by korli

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?

comment:4 in reply to:  3 ; Changed 5 years ago by siarzhuk

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.

Changed 5 years ago by puckipedia

Attachment: fixv5.patch added

Second version of the fix

comment:5 in reply to:  4 Changed 5 years ago by puckipedia

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 Changed 5 years ago by jessicah

Resolution: fixed
Status: newclosed

Applied in hrev47853.

comment:7 Changed 5 years ago by 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).

comment:8 in reply to:  7 ; Changed 5 years ago by 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.

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

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?

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

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 Changed 5 years ago by axeld

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 Changed 5 years ago by jessicah

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.