winspool.drv APIs missing SetLastError attribute

[rul3rst4]

Actual behavior

Some methods from the Winspool.drv are not generated with SetLastError true, even tho the docs says that they should generate errors.

Expected behavior

The methods to be generated with SetLastError = true.

Repro steps

Some examples:

This method is alright.

But this one is not, and the docs says that it generates error in some cases: https://learn.microsoft.com/en-us/windows/win32/printdocs/addprinter

Does someone know why this happens? Should i fill an issue to the metadata team? Is this the expected behavior, and if so, why?

Thanks!

Context

• CsWin32 version: [e.g. 0.4.422-beta]
• Win32Metadata version (if explicitly set by project):
• Target Framework: [e.g. netstandard2.0]
• LangVersion (if explicitly set by project): [e.g. 9]

[AArnott]

SetLastError causes the marshaler to call GetLastError as soon as the function completes, and store the result in thread-local storage so that the next call to Marshal.GetLastWin32Error can return it. This is all only appropriate to do if the native function calls (or may call) SetLastError itself.

Such functions are (or should be) documented as doing that. Take CreateFile for instance. If you search that doc for "LastError", you'll see several hits where the docs say to use GetLastError to obtain info on the error.
But your AddPrinter example has docs that do not mention GetLastError anywhere. It talks about returning an error, but its only description is that it returns NULL in the error case.

Ultimately, the reason CsWin32 does not emit SetLastError = true is because the win32metadata from which CsWin32 gets info on the APIs doesn't indicate that SetLastError should be set. So if you think the docs are wrong, and the metadata is wrong, let me know and I'll move this issue to the win32metadata repo so they can fix it there.

[rul3rst4]

@AArnott, i've been using the winspool API at work for quite some time and we've always used the GetLastError to detect errors in these API methods calls, for example, if i try to add a printer with a name that already exists on the system, i get 0x00000709 through GetLastError which is a error that makes sense in this case, we actually have a list of the possible errors for most of the winspool methods.

Anyway, I think the metadata and docs are wrong in this case and not only for the AddPrinter method. The docs are wrong for not showing explicitly that the function uses the SetLastError.

I can share the list of methods that we have mapped the errors using GetLastError if it's necessary.

Even though i think the optimal solution would be fix the metadata, i don't know the complexity or heuristics that they use to generate this decorator, and if its possible...

Do you think it's possible to generate it manually somehow, with configuration? Some json key/val that forces CsWin32 to generate the method signature with the SetLastError, or even in the NativeMethods.txt with some hint after the method name.

Thanks!

[AArnott]

I can share the list of methods that we have mapped the errors using GetLastError if it's necessary.

That would be very helpful.

Do you think it's possible to generate it manually somehow, with configuration? Some json key/val that forces CsWin32 to generate the method signature with the SetLastError,

No, such a feature does not exist, and give that would only help one user of one projection, we're far more interested in fixing the metadata itself so everyone benefits.

I'll transfer the issue to the win32metadata repo, and if you can provide the list of APIs, I think we can get it fixed right away.

[riverar]

The linked AddPrinter documentation says nothing about setting the last error value. So this will require someone at Microsoft to go look at the implementation.

[kennykerr]

AddPrinterW does call SetLastError.

[riverar]

Great, thanks @kennykerr!

[mikebattista]

Is it just AddPrinter that needs a change?

[kennykerr]

I think we need a better way to figure out whether SetLastError is called by an API. For example, GetProcessHeap's documentation says that it does but that function is guaranteed to succeed and never calls SetLastError.

[rul3rst4]

Is it just AddPrinter that needs a change?

No, i was just cleaning a list that we have here at the company to give the methods that we are almost sure that call SetLastError because of the nature of the errors that they emit in specific scenarios:

Here is the list:
AddPort
AddMonitor
AddPrinter
AddPrinterDriver
ClosePrinter
DeleteMonitor
DeletePort
DeletePrinter
EnumJobs
EnumPrinterDrivers
EnumPrinters
GetPrinter
GetSpoolFileHandle
OpenPrinter
SetPort
SetPrinter