Seems like by the time this is called, we know service is valid so I suppose you can reasonably assume any failures are due to Access Denied (however, I didn't look at all the possible return values from the win32 apis). However, I think you should at least add a comment about why you're not checking the lastError code. Applies to the other ones below as well.

I'm not checking lastError because the documentation says If the function fails, the return value is NULL. Source and also because the other service classes called the function the same way.

I thought it would be better to have the potential exception speak for itself rather than assuming that the error is a Access Denied.

But I might be wrong. If I am, please tell me.

Thanks for the explanation, I'm ok with this.

I don't have a strong opinion on this one, but my preference is to have it as part of StartupType similar to what you see in services.msc

Can we extend Enums using PowerShell somehow?
C# does not seem to be able to do it using just inheritance.
Currently the ServiceStartMode class does not support Automatic (Delayed start).
I could also make my own class, but that would be a breaking change, correct?

Seems like having it separate would be simplest.

If seems like the QueryServiceConfig2W api along with the error handling could be an internal helper api since it's repeated pretty often.

Same with QueryServiceConfigW as helper api

I think rather than writing a non-terminating error for each property that fails to be retrieved, it might be better to just write one non-terminating error if any fail.

Because there's very similar code for these test cases, you could turn them into -TestCases like:

# move user creation to BeforeAll{} and cleanup in AfterAll{}

It "Get-Service can get the <property> of a service" -TestCases @(
  @{property="username"; value="user1"; parameters = @{ Credential = $creds },
  @{property="binpath";value="$PSHOME\powershell.exe"},
 ...
){
 param ($property, $value, $parameters)

  if ($parameters -eq $null) {
    $parameters = @{}
  }
  $parameters += @{Name=$servicename;binarypathname="...}
  $service = New-Service @parameters
  ...
  $service.$property | Should BeExactly $value
}

It seems the change is not related to the PR purpose. I'd prefer to move it in separate PR.

Shouldn't this be related to DelayedAutoStart?

Shouldn't this be related to DelayedAutoStart?

Please remove or correct.

I wonder where we destroy and free?

@daxian-dbw Could you please comment?

Good catch! I don't find the allocated memory is freed anywhere. FreeCoTaskMem should be called once done using the memory. The same issue also happens in some other places in this file.

I found AllocCoTaskMem in 4 files and FreeCoTaskMem in 17 files - It seems we should widely review. Maybe don't block the PR and open a tracking Issue for new PR?

Have we the ApiSet on Windows 7?
/cc @mirichmo

The API set is included for Win 7

Is there MSDN public docs? I only see for Windows 10 and Windows 8.1. 😕

Why is GitHub paints '&' in red?

Please add spaces - use common formatting convention. Below too.

Extra space before "-TestCases".

Please use indentation with 4 spaces. Below too.

I'd like to see this protected within a finally.

Finally here as well

@daxian-dbw Are we still following the PInvokeDLLNames convention? It no longer makes sense since we stripped out the full CLR code

@daxian-dbw Could you please comment?

I think there is still one benefit -- you can easily know what native dependencies powershell has.

A check should be made to confirm that status == ERROR_INSUFFICIENT_BUFFER before proceeding.

https://msdn.microsoft.com/en-us/library/windows/desktop/ms684935.aspx

Interesting, what is best practice? Could we exclude second call by pre-allocate memory buffer?

This technique is a common pattern in Windows APIs. It is designed to be called twice with the first call returning the buffer size needed so the correct buffer size can be allocated and supplied on the second call. You can do it all in one call, but you have to either guess how big the buffer should be or allocate a max size buffer. If you guess wrong, you'll need to resize it and call the method again anyway. A max size buffer will work, but will incur unnecessary memory allocation. With that in mind, it is optimal to call it twice and follow the expected pattern.

I can't find this. I think your comment might be on an outdated version of the code. I made the check here and here

Agreed. Steve and I made the same review comment around the same time. This has been addressed.

Same issue here. It should not proceed unless it gets the correct status value.

I'd like to see these method calls organized differently to protect against potential memory leaks. The pattern is repeated in NativeMethods.QueryServiceConfig2 and I'd like all instances of it fixed. Since NativeMethods.QueryServiceConfig() is a helper function, its output parameter, instead of being an IntPtr structurePointer, should be the managed object NativeMethods.SERVICE_DESCRIPTIONW. That way, the unmanaged memory that is allocated within that function is also cleaned up within that function. The managed object carries the values out of the function and eventually gets garbage collected. To fix this instance, line 741 should be moved within the helper function. You can probably make Nativemethods.QueryServiceConfig2() a generic method as well to avoid having to use a switch for infoLevel.

This sounds like a great idea and I'm all for it.
Could you describe a bit more what you would change?
Like somewhere with an example.
The problem I'm having now is if I return the struct as an out, I have to set the variable before returning. I could set default values, but that means I have to check the values when adding the properties to my Service-object.

@mirichmo, could you comment on my last comment?

The idea is to put (NativeMethods.QUERY_SERVICE_CONFIG)Marshal.PtrToStructure(serviceConfigStructPtr, typeof(NativeMethods.QUERY_SERVICE_CONFIG)); and Marshal.FreeCoTaskMem in QueryServiceConfig2 and return the managed object NativeMethods.SERVICE_DESCRIPTIONW. On error return Null.

I had something like this in mind. The code is a bit messy and should be cleaned up, but illustrates the idea. The idea is that the helper function is a generic method that handles all the types in a similar manner. It also keeps allocation and freeing within the same method.

Method

internal static bool QueryServiceConfig2<T>(NakedWin32Handle hService, DWORD infolevel, out T configStructure)
{
            IntPtr lpBuffer = IntPtr.Zero;
            structurePointer = IntPtr.Zero;
            DWORD bufferSize, bufferSizeNeeded = 0;

            bool status = NativeMethods.QueryServiceConfig2W(
                hService: hService,
                dwInfoLevel: infolevel,
                lpBuffer: lpBuffer,
                cbBufSize: 0,
                pcbBytesNeeded: out bufferSizeNeeded);

            if (status != true && Marshal.GetLastWin32Error() != ERROR_INSUFFICIENT_BUFFER) {
                return status;
            }

    try {
            lpBuffer = Marshal.AllocCoTaskMem((int)bufferSizeNeeded);
            bufferSize = bufferSizeNeeded;
           status = NativeMethods.QueryServiceConfig2W(
               hService,
               infolevel,
                lpBuffer,
                bufferSize,
                out bufferSizeNeeded);

        configStructure = (T)Marshal.PtrToStructure(lpBuffer, typeof(T));
    }
    finally
    {
        Marshal.FreeCoTaskMem(lpBuffer);
    }
            return status;
}

Caller

NativeMethods.SERVICE_DESCRIPTIONW descriptionStruct = new NativeMethods.SERVICE_DESCRIPTIONW();

bool querySuccessful = NativeMethods.QueryServiceConfig2<NativeMethods.SERVICE_DESCRIPTIONW>(hService, NativeMethods.SERVICE_CONFIG_DESCRIPTION, out descriptionStruct);

// No free is needed here

Thank you! 👍 I made the updates. Could you please review @mirichmo

Is there MSDN public docs? I only see for Windows 10 and Windows 8.1. 😕

Closed.

@joandrsn Could you please address @mirichmo previous request:

I'd like to see these method calls organized differently to protect against potential memory leaks. The pattern is repeated in NativeMethods.QueryServiceConfig2 and I'd like all instances of it fixed. Since NativeMethods.QueryServiceConfig() is a helper function, its output parameter, instead of being an IntPtr structurePointer, should be the managed object NativeMethods.SERVICE_DESCRIPTIONW. That way, the unmanaged memory that is allocated within that function is also cleaned up within that function. The managed object carries the values out of the function and eventually gets garbage collected. To fix this instance, line 741 should be moved within the helper function. You can probably make Nativemethods.QueryServiceConfig2() a generic method as well to avoid having to use a switch for infoLevel.