[PATCH v3] mountmgr.sys: Implement StorageDeviceSeekPenaltyProperty
Zebediah Figura (she/her)
zfigura at codeweavers.com
Sat May 1 14:53:57 CDT 2021
Hello David, thanks for the patch!
I'm curious how the application is using this information, and if it
makes sense to just fake it instead, for simplicity?
I have some further comments inlined below.
On 4/27/21 4:31 PM, David Koolhoven wrote:
> Allows programs to query if disk has a seek penalty.
>
> Signed-off-by: David Koolhoven <david at koolhoven-home.net>
You could add a Wine-Bug tag for this; see e.g.
<https://source.winehq.org/patches/data/204879> for an example.
> ---
> v3: Linux side/sysfs logic is contained file.
> ---
> dlls/mountmgr.sys/Makefile.in | 1 +
> dlls/mountmgr.sys/device.c | 24 +++
> dlls/mountmgr.sys/unix/device_seek_penalty.c | 145 +++++++++++++++++++
> include/ntddstor.h | 9 +-
> 4 files changed, 178 insertions(+), 1 deletion(-)
> create mode 100644 dlls/mountmgr.sys/unix/device_seek_penalty.c
>
> diff --git a/dlls/mountmgr.sys/Makefile.in b/dlls/mountmgr.sys/Makefile.in
> index 83204e66504..61b462ddd7b 100644
> --- a/dlls/mountmgr.sys/Makefile.in
> +++ b/dlls/mountmgr.sys/Makefile.in
> @@ -7,6 +7,7 @@ EXTRALIBS = $(DISKARBITRATION_LIBS) $(SYSTEMCONFIGURATION_LIBS) $(CORESERVICES_L
>
> C_SRCS = \
> dbus.c \
> + unix/device_seek_penalty.c \
> device.c \
> diskarb.c \
> mountmgr.c
I don't think there's a good enough reason to do this; leaving it in
device.c would be better.
> diff --git a/dlls/mountmgr.sys/device.c b/dlls/mountmgr.sys/device.c
> index 04e8fe3c0f5..90c73b13139 100644
> --- a/dlls/mountmgr.sys/device.c
> +++ b/dlls/mountmgr.sys/device.c
> @@ -1894,6 +1894,30 @@ static NTSTATUS query_property( struct disk_device *device, IRP *irp )
>
> break;
> }
> + case StorageDeviceSeekPenaltyProperty:
> + {
> + DEVICE_SEEK_PENALTY_DESCRIPTOR *descriptor;
> + int ret;
Please use consistent four-space indentation, and avoid tabs.
> +
> + memset( irp->AssociatedIrp.SystemBuffer, 0, irpsp->Parameters.DeviceIoControl.OutputBufferLength );
> + descriptor = irp->AssociatedIrp.SystemBuffer;
> + descriptor->Version = sizeof(DEVICE_SEEK_PENALTY_DESCRIPTOR);
That seems wrong.
> + descriptor->Size = sizeof(DEVICE_SEEK_PENALTY_DESCRIPTOR);
> + irp->IoStatus.Information = sizeof(DEVICE_SEEK_PENALTY_DESCRIPTOR);
> +
> + ret = device_seek_penalty(device->unix_mount);
This function is badly named, especially considering that it's a predicate.
> + if (ret < 0) {
> + FIXME( "Unsupported property StorageDeviceSeekPenalty\n" );
> + status = STATUS_NOT_SUPPORTED;
> + } else if (ret == 0) {
> + descriptor->IncursSeekPenalty = FALSE;
> + status = STATUS_SUCCESS;
> + } else if (ret == 1) {
> + descriptor->IncursSeekPenalty = TRUE;
> + status = STATUS_SUCCESS;
> + }
> + break;
> + }
> default:
> FIXME( "Unsupported property %#x\n", query->PropertyId );
> status = STATUS_NOT_SUPPORTED;
> diff --git a/dlls/mountmgr.sys/unix/device_seek_penalty.c b/dlls/mountmgr.sys/unix/device_seek_penalty.c
> new file mode 100644
> index 00000000000..eefb4d67e65
> --- /dev/null
> +++ b/dlls/mountmgr.sys/unix/device_seek_penalty.c
> @@ -0,0 +1,145 @@
> +/*
> + * System information APIs
> + *
> + * Copyright 1996-1998 Marcus Meissner
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> + */
> +
> +#include "config.h"
> +#include "wine/port.h"
> +
> +#ifdef linux
> +#include <assert.h>
> +#include <errno.h>
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <sys/stat.h>
> +#include <sys/sysmacros.h>
> +
> +#define NONAMELESSUNION
> +#include "ntstatus.h"
> +#include "mountmgr.h"
> +#define WIN32_NO_STATUS
> +#include "windef.h"
> +#include "winternl.h"
> +#endif
> +#include "wine/debug.h"
> +
> +WINE_DEFAULT_DEBUG_CHANNEL(mountmgr);
> +
> +int device_seek_penalty (char *unix_mount)
> +{
> +#ifdef linux
> + FILE *fp;
> + char isrotapathstr[260];
> + char evpath[260];
> + char ueventbufstr[260];
> + char isrotastrbuf[2];
> + char *fgetsret = NULL;
> + int ret = 0;
> + char *rptr = NULL;
> + char *sptr = NULL;
> + char *devstr_edited = NULL;
> + char mountpath[260];
> + char *part_dev = NULL;
> + int c = 0;
> + struct stat statbuf;
> + int seekpen;
> +
> + const char *home = getenv( "HOME" );
This function is full of inconsistent spacing; please try to match the
surrounding code style.
> + const char *prefix = getenv( "WINEPREFIX" );
> + size_t len = (prefix ? strlen(prefix) : strlen(home) + strlen("/.wine")) + sizeof("/dosdevices/com256");
> + char *path = HeapAlloc( GetProcessHeap(), 0, len );
> +
> + if (path)
> + {
> + if (prefix) strcpy( path, prefix );
> + else
> + {
> + strcpy( path, home );
> + strcat( path, "/.wine" );
> + }
> + strcat( path, "/dosdevices/" );
> + }
> +
> + if (!unix_mount) {
> + return -1;
> + }
> +
> + snprintf (mountpath, 260, "%s%s", path, unix_mount);
> + ret = stat (mountpath, &statbuf);
This doesn't make any sense; unix_mount is the absolute Unix mount
point. Did you test this code?
> + if (ret == -1) {
> + return -1;
> + }
> + HeapFree( GetProcessHeap(), 0, path );
> + sprintf (evpath, "/sys/dev/block/%d:%d/uevent", major(statbuf.st_dev), minor(statbuf.st_dev));
> + fp = fopen(evpath, "r");
> + if (!fp) {
> + return -1;
> + }
> + while ((rptr = fgets (ueventbufstr, 260, fp))) {
> + sptr = strstr (rptr, "DEVNAME=");
> + if (sptr) {
> + sptr += strlen ("DEVNAME=");
> + break;
> + }
> + }
> + fclose (fp);
> + devstr_edited = sptr;
> + if (!devstr_edited) {
> + return -1;
> + }
> + /* Find first character after forwardslash '/' */
> + part_dev = strrchr (devstr_edited, '/');
> + if (!part_dev || (part_dev == devstr_edited)) part_dev = devstr_edited;
> + else part_dev++;
> + /* Trim off trailing digits and whitespace */
> + c = strlen (devstr_edited);
> + c--;
> + while ((devstr_edited[c] >= '0' && devstr_edited[c] <= '9')
> + || (devstr_edited[c] == '\n' || devstr_edited[c] == '\r'))
> + devstr_edited[c--] = '\0';
This isn't reliable; there's no guarantee that partition devices are
named in that scheme (and I've seen partitions that aren't in practice).
> +
> + ret = snprintf (isrotapathstr, 260, "/sys/block/%s/queue/rotational", part_dev);
> + if (ret < 1 || ret == 260) {
> + return -1;
> + }
"ret >= 260"
> +
> + fp = fopen(isrotapathstr, "r");
> + if (!fp) {
> + return -1;
> + }
> +
> + fgetsret = fgets(isrotastrbuf, 2, fp);
> + if (!fgetsret) {
> + return -1;
> + }
fgetc() seems even easier.
> +
> + fclose (fp);
> +
> + if (isrotastrbuf[0] == '1') {
> + seekpen = 1;
> + } else if (isrotastrbuf[0] == '0') {
> + seekpen = 0;
> + } else {
> + return -1;
> + }
> +
> + return seekpen;
> +#else
> + return -1;
> +#endif
> +}
> diff --git a/include/ntddstor.h b/include/ntddstor.h
> index b8c4bb73b0d..836def413fe 100644
> --- a/include/ntddstor.h
> +++ b/include/ntddstor.h
> @@ -214,7 +214,8 @@ typedef enum _STORAGE_QUERY_TYPE {
>
> typedef enum _STORAGE_PROPERTY_ID {
> StorageDeviceProperty = 0,
> - StorageAdapterProperty
> + StorageAdapterProperty = 1,
> + StorageDeviceSeekPenaltyProperty = 7,
> } STORAGE_PROPERTY_ID, *PSTORAGE_PROPERTY_ID;
>
> typedef struct _STORAGE_PROPERTY_QUERY {
> @@ -272,6 +273,12 @@ typedef struct _STORAGE_ADAPTER_DESCRIPTOR {
> USHORT BusMinorVersion;
> } STORAGE_ADAPTER_DESCRIPTOR, *PSTORAGE_ADAPTER_DESCRIPTOR;
>
> +typedef struct _DEVICE_SEEK_PENALTY_DESCRIPTOR {
> + ULONG Version;
> + ULONG Size;
> + BOOLEAN IncursSeekPenalty;
> +} DEVICE_SEEK_PENALTY_DESCRIPTOR, *PDEVICE_SEEK_PENALTY_DESCRIPTOR;
> +
> #ifdef __cplusplus
> }
> #endif
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20210501/e70af362/attachment.sig>
More information about the wine-devel
mailing list