Re: [PATCH 1/13] msidb: Add stub tool for manipulating MSI databases.

[Hans Leidekker]

On Sun, 2017-04-09 at 13:12 -0600, Erich E. Hoover wrote:
> +static int open_database( struct msidb_state *state )
> +{
> +    LPCWSTR db_mode = state->create_database ? MSIDBOPEN_CREATEDIRECT : MSIDBOPEN_DIRECT;
> +    UINT ret;
> +
> +    ret = MsiOpenDatabaseW( state->database_file, db_mode, &state->database_handle );

Databases opened in direct mode should always be committed. You should either do that
in following patches, or open them in transacted mode.

[PATCH 1/13] msidb: Add stub tool for manipulating MSI databases.

[Erich E. Hoover]

On Wed, Apr 26, 2017 at 3:39 AM, Hans Leidekker <hans(a)codeweavers.com> wrote:
> On Sun, 2017-04-09 at 13:12 -0600, Erich E. Hoover wrote:
> > +static int open_database( struct msidb_state *state )
> > +{
> > +    LPCWSTR db_mode = state->create_database ? MSIDBOPEN_CREATEDIRECT : MSIDBOPEN_DIRECT;
> > +    UINT ret;
> > +
> > +    ret = MsiOpenDatabaseW( state->database_file, db_mode, &state->database_handle );
>
> Databases opened in direct mode should always be committed. You should either do that
> in following patches, or open them in transacted mode.

Thanks for the review!  MsiDatabaseCommit is called by the
close_database routine (called as long as we actually open a
database).  I did things this way so that if any of the requested
operations (in the next patches) fail then the database does not get
committed (to prevent creating "partially updated" databases).  Is
there something that I can do to improve this or make that clearer?

Best,
Erich

[PATCH 1/13] msidb: Add stub tool for manipulating MSI databases.

[Hans Leidekker]

On Thu, 2017-04-27 at 07:47 -0600, Erich E. Hoover wrote:
> On Wed, Apr 26, 2017 at 3:39 AM, Hans Leidekker <hans(a)codeweavers.com> wrote:
> > On Sun, 2017-04-09 at 13:12 -0600, Erich E. Hoover wrote:
> > > +static int open_database( struct msidb_state *state )
> > > +{
> > > +    LPCWSTR db_mode = state->create_database ? MSIDBOPEN_CREATEDIRECT : MSIDBOPEN_DIRECT;
> > > +    UINT ret;
> > > +
> > > +    ret = MsiOpenDatabaseW( state->database_file, db_mode, &state->database_handle );
> >
> > Databases opened in direct mode should always be committed. You should either do that
> > in following patches, or open them in transacted mode.
>
> Thanks for the review!  MsiDatabaseCommit is called by the
> close_database routine (called as long as we actually open a
> database).  I did things this way so that if any of the requested
> operations (in the next patches) fail then the database does not get
> committed (to prevent creating "partially updated" databases).  Is
> there something that I can do to improve this or make that clearer?

Opening the database in direct mode does not guarantee that partial
updates are not written if you don't commit. MSDN warns that the
database may become corrupt if you don't always commit before closing.

You should to open the database in transacted mode if you want to avoid
partially updated databases.