Re: [PATCH v3] mountmgr.sys: Implement StorageDeviceSeekPenaltyProperty

[Zebediah Figura (she/her)]

Hello, apologies for the very late reply.

On 5/1/21 6:54 PM, David Koolhoven wrote:
> On Sat, May 01, 2021 at 02:53:57PM -0500, Zebediah Figura (she/her) wrote:
> > Hello David, thanks for the patch!
> > Hello thanks for the review Zebediah!
> > I'm curious how the application is using this information, and if it
> > makes sense to just fake it instead, for simplicity?
> > It's not certain how it's being used, but it solves this
> > bug: https://bugs.winehq.org/show_bug.cgi?id=51065
>
> It could be used to collect performance statistics, or to
> deploy an application level policy on data access and memory
> management. I'm uncertain.
>
> I was recently informed this patch doesn't function on LVM
> partitians, so it'll need a new version anyway.
>
> Spoofing a response would certainly make it easier to
> implement and maintain, it sort of depends on the goals of
> the WINE project for a feature like this, which leads to a
> question:
>
> Would it be better for this to spoof an answer to the query
> if platform specific testing fails, rather than reporting a
> failure to the Windows program?

I think there's nothing wrong with implementing things like this 
properly in general, but stubs do tend to be easier.

The awkward thing in this case is that mountmgr is kind of a mess wrt 
how we track disks and partitions; that should probably be cleaned up to 
some degree first.
> If spoofing is better, should it spoof a seek penalty exists
> or not? I think it should spoof that it does exist, but I'm
> open to any implementation.

I don't have strong opinions here.
> The Linux platform logic was put into it's own file because
> I wanted to attempt a FreeBSD version of the same logic too.
> Would renaming the function with a prefix of "wineint_" work
> to clarify what it is?

There's no reason to split platform-specific logic into their own files; 
we leave them in the same file almost everywhere else in Wine.

Wrt naming, I mean that "device_seek_penalty" does not semantically 
communicate what the function does. A predicate should generally be 
named like "device_has_seek_penalty".
> I'll submit a spoofing only version and float a testing
> implementation later.
>
> Thank you!
>
> I'll work out the rest.