False positive of alert prompt.

[mmadia]

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.

[puckipedia]

Fix

[puckipedia]

Attached fix (GCI task ​https://google-melange.appspot.com/gci/task/view/google/gci2013/5243327079251968)

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

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

[puckipedia]

Second version of the fix

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

[jessicah]

Applied in hrev47853.

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

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

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

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

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

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