Custom action library, initial version

installer/src/installer-lib/property.cpp

+/**
+ * \file property.cpp Implementation of Property class etc.
+ */
+
+#include "property.h"
+#include "session.h"
+#include "msiquery.h"
+#include <memory>
+
+//-----------------------------------------------------------------------------------------
+// Property
+//-----------------------------------------------------------------------------------------
+Property::Property( Session & session, std::wstring name )
+  // VSE 2012 shows an IntelliSense error here. Ignore it. The compiler properly sees the 'friend' declaration.
+  : handle( session.handle ), name( name )
+{}
+
+/**
+ * \par Implementation
+ * The center of the implementation is the <a href="http://msdn.microsoft.com/en-us/library/windows/desktop/aa370134%28v=vs.85%29.aspx">MsiGetProperty function</a>.
+ */
+Property::operator std::wstring() const
+{
+  /*
+   * The screwy logic below arises from how the API works.
+   * MsiGetProperty insists on copying into your buffer, but you don't know how long that buffer needs to be in advance.
+   * The first call gets the size, but also the actual value if it's short enough.
+   * A second call, if necessary, gets the actual value after allocat
+   */

[Wladimir Palant, 2013/10/29 08:49:26]

The logic is actually common for any API that doesn't allocate the buffer itself
(and thus mandates which deallocator you have to use). Please reformulate this
comment to use a neutral tone. Dropping the first two lines should be
sufficient, these don't really have any informational value.

[Eric, 2013/10/29 14:00:58]

I'll rewrite the comment. Reading it again, it doesn't capture the API behavior
adequately to explain the code.

FYI, the Windows registry call does something similar, but with behavior
different enough that you can't use generic code to handle both cases. That's
what's the screwy part.

+  // We only need a modest fixed-size buffer here, because we handle arbitrary-length property values in a second step.
+  // It has 'auto' allocation, so we don't want it too large.
+  TCHAR buffer1[ 64 ] = { L'\0' } ;
+  DWORD length = sizeof( buffer1 ) / sizeof( TCHAR ) ;

[Wladimir Palant, 2013/10/29 08:49:26]

sizeof(TCHAR) => sizeof(buffer1[0])?

[Eric, 2013/10/29 14:00:58]

TCHAR is, as I recall, a wchar_t for Unicode compiles and a char otherwise. The
syntax is a way of avoiding another line of code to define the buffer length 64
separately.

I don't know how much using TCHAR matters here. I cribbed using this type from
the WiX CA code.

[Wladimir Palant, 2013/10/29 15:06:38]

What I meant: you can replace sizeof(TCHAR) by sizeof(buffer1[0]) - this is the
generic way of calculating the size of an array, without having to mention its
type again.

But now that you mention it: yes, you should use WCHAR instead of TCHAR and call
MsiGetPropertyW() explicitly. Given that the result is a wstring, we don't want
generic behavior here - we want to get Unicode no matter what.

+  switch ( MsiGetProperty( handle, name.c_str(), buffer1, & length ) ) 
+  {
+  case ERROR_SUCCESS:
+    // This call might succeed, which means the return value was short enough to fit into the buffer.
+    return std::wstring( buffer1, length ) ;
+  case ERROR_MORE_DATA:
+    // Do nothing yet.
+    break ;
+  default:
+    throw std::runtime_error( "Error getting property" ) ;
+  }
+  // Assert we received ERROR_MORE_DATA
+  // unique_ptr handles deallocation transparently
+  std::unique_ptr< TCHAR[] > buffer2( new TCHAR[ length ] );
+  switch ( MsiGetProperty( handle, name.c_str(), buffer2.get(), & length ) )
+  {
+  case ERROR_SUCCESS:
+    return std::wstring( buffer2.get(), length ) ;
+  default:
+    throw std::runtime_error( "Error getting property" ) ;
+  }

[Wladimir Palant, 2013/10/29 08:49:26]

Any reason why this isn't an if..else.. block? Also, this exception isn't very
helpful, it should contain the error code at the very least.

[Eric, 2013/10/29 14:00:58]

There are three other error codes defined for the API. (As also for the above
switch statement.) I hadn't added them in yet; I don't know if they'll matter.

See also below.

+}
+
+/**
+ * \par Implementation
+ * The center of the implementation is the <a href="http://msdn.microsoft.com/en-us/library/windows/desktop/aa370391%28v=vs.85%29.aspx">MsiSetProperty function</a>.
+ */
+void Property::operator=( const std::wstring & value )
+{
+  if ( MsiSetProperty( handle, name.c_str(), value.c_str() ) != ERROR_SUCCESS )
+  {
+    throw std::runtime_error( "Error setting property" ) ;

[Wladimir Palant, 2013/10/29 08:49:26]

This exception isn't very helpful, it should contain the error code at the very
least.

[Eric, 2013/10/29 14:00:58]

Admittedly, the exceptions here are stubs. I don't think that any of the errors
ever occur unless the API itself crashes (a Windows defect) or the code calling
them messes up (an application defect). This comment applies to a most of the
API calls in this code base.

Given how prevalent this pattern is, though, what's needed an exception class
that takes a message and a Windows error code. I haven't written it yet.

[Wladimir Palant, 2013/10/29 15:06:38]

Forgot to mention that: where are these exceptions caught? If they aren't (which
is what it looks like right now), does it mean that the installer will crash if
it hits an unexpected situation?

[Eric, 2014/03/28 13:56:38]

The answer is a general one. 

Every time you use C++ outside of your own main(), you need to provide a
top-level try statement with a catch-all (that is, "catch(...)") block to catch
exceptions. You need this even if your own code doesn't throw, since both the
standard library and even the language itself can throw. (Divide by zero
evidently throws on certain platforms.)

For a DLL, the top levels include the DLL initialization code and the DLL entry
points. In addition, callbacks from the operating system should be considered
top-level. The rule of thumb is that if you need __stdcall (or WINAPI, a typedef
to it) to declare a function, you need a top-level try statement around the body
of that function.

+  }
+}