Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/sys/windows: break compat and use implicit mksyscall string conversion #34309

Closed
zx2c4 opened this issue Sep 15, 2019 · 3 comments
Closed

x/sys/windows: break compat and use implicit mksyscall string conversion #34309

zx2c4 opened this issue Sep 15, 2019 · 3 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Sep 15, 2019 •
edited

x/sys/windows doesn't have a mod version, so maybe there's some leeway for breaking stuff. Here's the mischief I have in mind:

All functions that take strings in x/sys/windows take a *uint16. It's then up to the caller to do the rather cumbersome:

//sys    DoWithFooBar(foo *uint16, bar *uint16) = user32.DoWithFooBarW

foo16, err := windows.UTF16PtrFromString(foo)
if err != nil {
    return err
}
bar16, err := windows.UTF16PtrFromString(bar)
if err != nil {
    return err
}
return windows.DoWithFooBar(foo16, bar16)

When you have lots of strings, this is really a drag.

As it turns out, mksyscall.go will automatically generate the conversion. So instead these work:

//sys    DoWithFooBar(foo string, bar string) = user32.DoWithFooBarW

return windows.DoWithFooBar(foo, bar)

Under the hood, the generated bindings do the right thing.

Now to be clear, I'm not proposing that we change mksyscall.go to generate the string conversion code. Why? Because this is already implemented!

What I am suggesting is that we change all of the definitions in x/sys/windows to take a simple string type instead of forcing the user to muck with *uint16. I assume that the conversion code was added to mksyscall.go for this reason, but then for some reason it never made it to x/sys/windows.

cc @alexbrainman @bradfitz
@gopherbot gopherbot added this to the Unreleased milestone Sep 15, 2019
@ianlancetaylor
Copy link
Contributor

It sounds like that would change a large number of existing functions, and therefore break a large number of existing programs. We can break compatibility in x/sys/windows, but I don't think we can break it to that extent.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Sep 16, 2019

It's a lot of functions, but not all are so commonly used. Here's a list:

TranslateName
GetUserNameEx
NetUserGetInfo
NetGetJoinInformation
LookupAccountSid
LookupAccountName
ConvertSidToStringSid
ConvertStringSidToSid
LookupPrivilegeValue
GetUserProfileDirectory
CreateFile
GetCurrentDirectory
SetCurrentDirectory
CreateDirectory
RemoveDirectory
DeleteFile
MoveFile
MoveFileEx
GetComputerName
GetComputerNameEx
CreateProcess
ShellExecute
GetTempPath
CryptAcquireContext
GetEnvironmentStrings
FreeEnvironmentStrings
GetEnvironmentVariable
SetEnvironmentVariable
CreateEnvironmentBlock
DestroyEnvironmentBlock
GetFileAttributes
SetFileAttributes
GetFileAttributesEx
GetCommandLine
CommandLineToArgv
GetFullPathName
GetLongPathName
GetShortPathName
CreateFileMapping
CertOpenSystemStore
RegOpenKeyEx
RegQueryInfoKey
RegEnumKeyEx
RegQueryValueEx
WriteConsole
ReadConsole
CreateSymbolicLink
CreateHardLink
CreateEvent
CreateEventEx
OpenEvent
CreateMutex
CreateMutexEx
OpenMutex
CreateJobObject
DefineDosDevice
DeleteVolumeMountPoint
FindFirstVolume
FindFirstVolumeMountPoint
FindNextVolume
FindNextVolumeMountPoint
GetDriveType
GetLogicalDriveStrings
GetVolumeInformation
GetVolumeInformationByHandle
GetVolumeNameForVolumeMountPoint
GetVolumePathName
GetVolumePathNamesForVolumeName
QueryDosDevice
SetVolumeLabel
SetVolumeMountPoint
MessageBox
DnsNameCompare
GetAddrInfoW
MultiByteToWideChar
OpenSCManager
CreateService
OpenService
StartService
ChangeServiceConfig
EnumServicesStatusEx
RegisterEventSource
ReportEvent

@alexbrainman
Copy link
Member

I agree with Ian - let sleeping dogs lie.

golang.org/x/sys/windows package started its life as copy of windows version of syscall package. And windows version of syscall package was only intended to be used by other packages in standard library. It was not intended to be used directly by any programs. If you want 'nice' interface, we should not put into golang.org/x/sys/windows package. See golang.org/x/sys/windows/registry or golang.org/x/sys/windows/svc for an example of a nice package.

Alex

@zx2c4 zx2c4 closed this as completed Sep 16, 2019
@zx2c4 zx2c4 mentioned this issue Oct 1, 2019
Closed
@golang golang locked and limited conversation to collaborators Sep 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@zx2c4 @ianlancetaylor @gopherbot @alexbrainman