Wouldn't it be cleaner to return an error from initializeVssCOMInterface when it gets a nil? Just asking, I've no idea what the effect on HasSufficientPrivilegesForVSS would be.

Wouldn't it be cleaner to return an error from initializeVssCOMInterface when it gets a nil? Just asking, I've no idea what the effect on HasSufficientPrivilegesForVSS would be.

Actually the HRESULT code is correct and fall back to E_ACCESSDENIED case before crashing. It seems possible to treat all result code and error case in initializeVssCOMInterface() and returning only oleIUnknown or an error but you finally have to type check errors to keep actual behaviour of HasSufficientPrivilegesForVSS().

I'm not sure what you mean. Is there a case where oleIUnknown is nil but still usable? I.e., is the following incorrect?

// initializeCOMInterface initialize an instance of the VSS COM api
func initializeVssCOMInterface() (*ole.IUnknown, uintptr, error) {
	vssInstance, err := loadIVssBackupComponentsConstructor()
	if err != nil {
		return nil, 0, err
	}

	// ensure COM is initialized before use
	ole.CoInitializeEx(0, ole.COINIT_MULTITHREADED)

	var oleIUnknown *ole.IUnknown
	result, _, _ := vssInstance.Call(uintptr(unsafe.Pointer(&oleIUnknown)))
	if oleIUnknown == nil {
		return nil, 0, errors.Errorf("VSS: got nil OLE object") // or whatever
	}

	return oleIUnknown, result, nil
}

Ideally HRESULT should be returned only as part of vssError (functions like Cancel, Wait, QueryStatus must be changed) and all function such as DoSnapshotSet, GatherWriterMetadata, PrepareForBackup, BackupComplete, initializeVssCOMInterface, queryInterface should call Release itself and return nil, err. Also situation where returned COMinterface!=nil and err==nil or COMinterface==nil and err!=nil should be impossible.

@DRON-666 PR?

Yes @greatroar, I should write PR, but problem is that my developer skills predates git, github and golang. I want to contribute to this project, but definitely not today.

I've cleaned it up. It now handles errors in initializeVssCOMInterface() properly. I've tested it by removing the call to HasSufficientPrivilegesForVSS(). This leads to a to expected error:

VSS error: Failed to initialize COM interface: E_ACCESSDENIED (0x80070005)

I've cleaned it up.

After that switch HRESULT(result) cases become mostly unreachable.

@DRON-666 To make it clean we should also move the error handling in the switch statement into initializeVssCOMInterface(). I'd like to keep the special error handling telling the user about necessary privileges or being an administrator for easy troubleshooting.

I've move error handling to initializeVssCOMInterface(). It will still return a the result because it is needed in HasSufficientPrivilegesForVSS().

Is there much left to do on this or is it ready for merging soon?

I'm right now thinking about @DRON-666's remarks.

Let us know when you're done :)

Just waiting for @DRON-666 approve my changes.

I also thought about it but I can't easily get around it. Its essentially an implementation detail. In case multiple other things might fail before permissions are checked the API might return a different error. All other possible errors won't give any information about permissions. Actually checking for correct permissions is a bit more involved on windows. I implemented this in #2875 which is still open for discussion.

I also thought about it but I can't easily get around it. Its essentially an implementation detail. In case multiple other things might fail before permissions are checked the API might return a different error. All other possible errors won't give any information about permissions. Actually checking for correct permissions is a bit more involved on windows. I implemented this in #2875 which is still open for discussion.

The cleanest fix would be to let HasSufficientPrivilegesForVSS return true/false/"something else went wrong". That is return (bool, err). That way the initial permissions check could already return allowed/denied/some other error to the user.

The only call site turns a false result into a fatal error anyway, so just returning the error would be enough :)

If err != nil but of a different type, the function returns true. That doesn't seem right.

Yes and code from cmd_backup.go should be

if err := fs.HasSufficientPrivilegesForVSS(); err != nil {
    return err
}

Now it's convert HRESULT to error, then to bool and then to different error.

HasSufficientPrivilegesForVSS() now returns only an error

@DRON-666 and @greatroar Does your thumbs-up clickety-clicks mean that you have no further comments on this PR, that you think it's fine to merge?