HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Expecting working sample of DeviceIoControl reviewed

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
samplerevieweddeviceiocontrolworkingexpecting

Problem

I put the full code at the rear of this post.

I've recently answered a question, and the detail explanation is posted in

  • Physical disk size not correct (IoCtlDiskGetDriveGeometry)



And en passant answer another one

  • DeviceIoControl does not set output buffer



I'd like to know that if any one think there's any problem or improvement of the code.

code:

```
using System.Runtime.InteropServices;
using System.ComponentModel;
using System;

namespace DiskManagement {
using Microsoft.Win32.SafeHandles;

using LPSECURITY_ATTRIBUTES=IntPtr;
using LPOVERLAPPED=IntPtr;
using LPVOID=IntPtr;
using HANDLE=IntPtr;

using LARGE_INTEGER=Int64;
using DWORD=UInt32;
using LPCTSTR=String;

public static partial class IoCtl / methods / {
[DllImport("kernel32.dll", SetLastError=true)]
static extern SafeFileHandle CreateFile(
LPCTSTR lpFileName,
DWORD dwDesiredAccess,
DWORD dwShareMode,
LPSECURITY_ATTRIBUTES lpSecurityAttributes,
DWORD dwCreationDisposition,
DWORD dwFlagsAndAttributes,
HANDLE hTemplateFile
);

[DllImport("kernel32.dll", SetLastError=true)]
static extern DWORD DeviceIoControl(
SafeFileHandle hDevice,
DWORD dwIoControlCode,
LPVOID lpInBuffer,
DWORD nInBufferSize,
LPVOID lpOutBuffer,
int nOutBufferSize,
ref DWORD lpBytesReturned,
LPOVERLAPPED lpOverlapped
);

static DWORD CTL_CODE(DWORD DeviceType, DWORD Function, DWORD Method, DWORD Access) {
return (((DeviceType)(
ref T x,
DWORD dwIoControlCode,
LPCTSTR lpFileName,
DWORD dwDesiredAccess=GENERIC_READ,
DWORD dwShareMode=FILE_SHARE_WRITE|FILE_SHARE_READ,
LPSECURITY_ATTRIBUTES lpSecurityAttributes=default(LPSECURITY_ATTRIBUTES),
DWORD dwCrea

Solution

As most people in the comments suggest, it is missing a few conventions that are pretty generally accepting across all C# developers. This will mostly be looking at stylistic points as there isn't really much in wrong with the implementation.

General notes

Most of the bracket/whitespace-related points are enforced by Visual Studio when you close a code block ({).

  • Types should be in PascalCase



  • Public member names should be in PascalCase



  • Hungarian notation (dwDesiredAccess) is discourages except for when naming controls in WebForms and WinForms. Go with camelCase.



-
Your spacing is a bit off, all curly braces should be on separate lines. Methods generally should look like this:

public void Method(Type param)
{
    // Implementation
}


-
Generally you should have 1 (partial) class per file, this may not be applicable but wanted to mention.

  • Placing a space between most symbols is encourages.



-
Order your type's members like this:

  • Variables (pref. const/static first)



  • Constructors/Properties (either way)



  • Methods



-
Normally ALL_CAPS-style naming is not used in C#, sometimes it is for constants but generally not.

Line-by-line

Normally comments are made on the line above class definitions, you may be better putting this into an IoCtrl.Methods.cs file or something if you really want to separate the different sections of the file. I usually stay away from partial unless the file gets ridiculously long and can't be factored out.

// methods
public static partial class IoCtl {


While I'm not particularly a fan of it, your 'defines' at the top make sense given their application and the documentation of the kernal32.dll assembly.

using LPSECURITY_ATTRIBUTES = IntPtr;
using LPOVERLAPPED = IntPtr;
using LPVOID = IntPtr;
using HANDLE = IntPtr;

using LARGE_INTEGER = Int64;
using DWORD = UInt32;
using LPCTSTR = String;


Structure your braces like this:

public static void Execute(
    ref T x,
    DWORD dwIoControlCode,
    LPCTSTR lpFileName,
    DWORD dwDesiredAccess = GENERIC_READ,
    DWORD dwShareMode = FILE_SHARE_WRITE | FILE_SHARE_READ,
    LPSECURITY_ATTRIBUTES lpSecurityAttributes = default(LPSECURITY_ATTRIBUTES),
    DWORD dwCreationDisposition = OPEN_EXISTING,
    DWORD dwFlagsAndAttributes = 0,
    HANDLE hTemplateFile = default(IntPtr)) 
{


Try not to span too many lines when calling functions

using (var hDevice = CreateFile(lpFileName, dwDesiredAccess, dwShareMode,
        lpSecurityAttributes, dwCreationDisposition, dwFlagsAndAttributes,
        hTemplateFile)) 
    {

        if (null == hDevice || hDevice.IsInvalid)
            throw new Win32Exception(Marshal.GetLastWin32Error());

        var nOutBufferSize = Marshal.SizeOf(typeof(T));
        var lpOutBuffer = Marshal.AllocHGlobal(nOutBufferSize);
        var lpBytesReturned = default(DWORD);


Using IntPtr.Zero rather than an alias (NULL) makes it easier to understand what the value is later on.

var result = DeviceIoControl(hDevice, dwIoControlCode, IntPtr.Zero, 0,
        lpOutBuffer, nOutBufferSize, ref lpBytesReturned, IntPtr.Zero);


Always prefer var == over == var, this type of condition is affectionately known as a 'yoda condition'.

if (result == 0)
            throw new Win32Exception(Marshal.GetLastWin32Error());

        x = (T)Marshal.PtrToStructure(lpOutBuffer, typeof(T));
        Marshal.FreeHGlobal(lpOutBuffer);
    }
}


Use pascal case for all types.

public enum MediaType : int
{
    Unknown = 0,
    F5_1Pt2_512 = 1,
    F3_1Pt44_512 = 2,
    F3_2Pt88_512 = 3,
    F3_20Pt8_512 = 4,
    F3_720_512 = 5,
    F5_360_512 = 6,
    F5_320_512 = 7,
    F5_320_1024 = 8,
    F5_180_512 = 9,
    F5_160_512 = 10,
    RemovableMedia = 11,
    FixedMedia = 12,
    F3_120M_512 = 13,
    F3_640_512 = 14,
    F5_640_512 = 15,
    F5_720_512 = 16,
    F3_1Pt2_512 = 17,
    F3_1Pt23_1024 = 18,
    F5_1Pt23_1024 = 19,
    F3_128Mb_512 = 20,
    F3_230Mb_512 = 21,
    F8_256_128 = 22,
    F3_200Mb_512 = 23,
    F3_240M_512 = 24,
    F3_32M_512 = 25
}


Always specify access modifiers.

// Structures
internal partial class DiskGeometry
{
    [StructLayout(LayoutKind.Sequential)]
    internal struct DISK_GEOMETRY 
    {
        internal LARGE_INTEGER Cylinders;
        internal MEDIA_TYPE MediaType;
        internal DWORD TracksPerCylinder;
        internal DWORD SectorsPerTrack;
        internal DWORD BytesPerSector;
    }

    [StructLayout(LayoutKind.Sequential)]
    internal struct DISK_GEOMETRY_EX 
    {
        internal DISK_GEOMETRY Geometry;
        internal LARGE_INTEGER DiskSize;

        [MarshalAs(UnmanagedType.ByValArray, SizeConst=1)]
        internal byte[] Data;
    }
}


There isn't really a standard on this but I prefer to keep simple getters on a single line. Also keep variables at the top of the file and use pascalCase;

```
// properties and fields
partial class DiskGeometry
{
private CubicAddress maximumCub

Code Snippets

public void Method(Type param)
{
    // Implementation
}
// methods
public static partial class IoCtl {
using LPSECURITY_ATTRIBUTES = IntPtr;
using LPOVERLAPPED = IntPtr;
using LPVOID = IntPtr;
using HANDLE = IntPtr;

using LARGE_INTEGER = Int64;
using DWORD = UInt32;
using LPCTSTR = String;
public static void Execute<T>(
    ref T x,
    DWORD dwIoControlCode,
    LPCTSTR lpFileName,
    DWORD dwDesiredAccess = GENERIC_READ,
    DWORD dwShareMode = FILE_SHARE_WRITE | FILE_SHARE_READ,
    LPSECURITY_ATTRIBUTES lpSecurityAttributes = default(LPSECURITY_ATTRIBUTES),
    DWORD dwCreationDisposition = OPEN_EXISTING,
    DWORD dwFlagsAndAttributes = 0,
    HANDLE hTemplateFile = default(IntPtr)) 
{
using (var hDevice = CreateFile(lpFileName, dwDesiredAccess, dwShareMode,
        lpSecurityAttributes, dwCreationDisposition, dwFlagsAndAttributes,
        hTemplateFile)) 
    {

        if (null == hDevice || hDevice.IsInvalid)
            throw new Win32Exception(Marshal.GetLastWin32Error());

        var nOutBufferSize = Marshal.SizeOf(typeof(T));
        var lpOutBuffer = Marshal.AllocHGlobal(nOutBufferSize);
        var lpBytesReturned = default(DWORD);

Context

StackExchange Code Review Q#23264, answer score: 4

Revisions (0)

No revisions yet.