Image file loaders may add a series of roms.  If an error occurs partway
through loading there is no easy way to drop previously added roms.

This patch adds a transaction mechanism that works like this:

  rom_transaction_begin();
  ...call rom_add_*()...
  rom_transaction_end(ok);

If ok is false then roms added in this transaction are dropped.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/loader.h | 19 +++++++++++++++++++
 hw/core/loader.c    | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index e98b84b8f9..5235f119a3 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -225,6 +225,25 @@ int rom_check_and_register_reset(void);
 void rom_set_fw(FWCfgState *f);
 void rom_set_order_override(int order);
 void rom_reset_order_override(void);
+
+/**
+ * rom_transaction_begin:
+ *
+ * Call this before of a series of rom_add_*() calls.  Call
+ * rom_transaction_end() afterwards to commit or abort.  These functions are
+ * useful for undoing a series of rom_add_*() calls if image file loading fails
+ * partway through.
+ */
+void rom_transaction_begin(void);
+
+/**
+ * rom_transaction_end:
+ * @commit: true to commit added roms, false to drop added roms
+ *
+ * Call this after a series of rom_add_*() calls.  See rom_transaction_begin().
+ */
+void rom_transaction_end(bool commit);
+
 int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
 void *rom_ptr(hwaddr addr, size_t size);
 void hmp_info_roms(Monitor *mon, const QDict *qdict);
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 0c72e7c05a..612420b870 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -840,6 +840,8 @@ struct Rom {
     char *fw_dir;
     char *fw_file;
 
+    bool committed;
+
     hwaddr addr;
     QTAILQ_ENTRY(Rom) next;
 };
@@ -877,6 +879,8 @@ static void rom_insert(Rom *rom)
         rom->as = &address_space_memory;
     }
 
+    rom->committed = false;
+
     /* List is ordered by load address in the same address space */
     QTAILQ_FOREACH(item, &roms, next) {
         if (rom_order_compare(rom, item)) {
@@ -1168,6 +1172,34 @@ void rom_reset_order_override(void)
     fw_cfg_reset_order_override(fw_cfg);
 }
 
+void rom_transaction_begin(void)
+{
+    Rom *rom;
+
+    /* Ignore ROMs added without the transaction API */
+    QTAILQ_FOREACH(rom, &roms, next) {
+        rom->committed = true;
+    }
+}
+
+void rom_transaction_end(bool commit)
+{
+    Rom *rom;
+    Rom *tmp;
+
+    QTAILQ_FOREACH_SAFE(rom, &roms, next, tmp) {
+        if (rom->committed) {
+            continue;
+        }
+        if (commit) {
+            rom->committed = true;
+        } else {
+            QTAILQ_REMOVE(&roms, rom, next);
+            rom_free(rom);
+        }
+    }
+}
+
 static Rom *find_rom(hwaddr addr, size_t size)
 {
     Rom *rom;
-- 
2.17.1

On Fri, Aug 3, 2018 at 7:47 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Image file loaders may add a series of roms.  If an error occurs partway
> through loading there is no easy way to drop previously added roms.
>
> This patch adds a transaction mechanism that works like this:
>
>   rom_transaction_begin();
>   ...call rom_add_*()...
>   rom_transaction_end(ok);
>
> If ok is false then roms added in this transaction are dropped.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/hw/loader.h | 19 +++++++++++++++++++
>  hw/core/loader.c    | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index e98b84b8f9..5235f119a3 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -225,6 +225,25 @@ int rom_check_and_register_reset(void);
>  void rom_set_fw(FWCfgState *f);
>  void rom_set_order_override(int order);
>  void rom_reset_order_override(void);
> +
> +/**
> + * rom_transaction_begin:
> + *
> + * Call this before of a series of rom_add_*() calls.  Call
> + * rom_transaction_end() afterwards to commit or abort.  These functions are
> + * useful for undoing a series of rom_add_*() calls if image file loading fails
> + * partway through.
> + */
> +void rom_transaction_begin(void);
> +
> +/**
> + * rom_transaction_end:
> + * @commit: true to commit added roms, false to drop added roms
> + *
> + * Call this after a series of rom_add_*() calls.  See rom_transaction_begin().
> + */
> +void rom_transaction_end(bool commit);
> +
>  int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
>  void *rom_ptr(hwaddr addr, size_t size);
>  void hmp_info_roms(Monitor *mon, const QDict *qdict);
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 0c72e7c05a..612420b870 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -840,6 +840,8 @@ struct Rom {
>      char *fw_dir;
>      char *fw_file;
>
> +    bool committed;
> +
>      hwaddr addr;
>      QTAILQ_ENTRY(Rom) next;
>  };
> @@ -877,6 +879,8 @@ static void rom_insert(Rom *rom)
>          rom->as = &address_space_memory;
>      }
>
> +    rom->committed = false;
> +
>      /* List is ordered by load address in the same address space */
>      QTAILQ_FOREACH(item, &roms, next) {
>          if (rom_order_compare(rom, item)) {
> @@ -1168,6 +1172,34 @@ void rom_reset_order_override(void)
>      fw_cfg_reset_order_override(fw_cfg);
>  }
>
> +void rom_transaction_begin(void)
> +{
> +    Rom *rom;
> +
> +    /* Ignore ROMs added without the transaction API */
> +    QTAILQ_FOREACH(rom, &roms, next) {
> +        rom->committed = true;

My only thought is that maybe this should produce a warning or error
if a ROM isn't committed.

Alistair

> +    }
> +}
> +
> +void rom_transaction_end(bool commit)
> +{
> +    Rom *rom;
> +    Rom *tmp;
> +
> +    QTAILQ_FOREACH_SAFE(rom, &roms, next, tmp) {
> +        if (rom->committed) {
> +            continue;
> +        }
> +        if (commit) {
> +            rom->committed = true;
> +        } else {
> +            QTAILQ_REMOVE(&roms, rom, next);
> +            rom_free(rom);
> +        }
> +    }
> +}
> +
>  static Rom *find_rom(hwaddr addr, size_t size)
>  {
>      Rom *rom;
> --
> 2.17.1
>
>

On Wed, Aug 8, 2018 at 10:32 PM Alistair Francis <alistair23@gmail.com> wrote:
> On Fri, Aug 3, 2018 at 7:47 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > @@ -1168,6 +1172,34 @@ void rom_reset_order_override(void)
> >      fw_cfg_reset_order_override(fw_cfg);
> >  }
> >
> > +void rom_transaction_begin(void)
> > +{
> > +    Rom *rom;
> > +
> > +    /* Ignore ROMs added without the transaction API */
> > +    QTAILQ_FOREACH(rom, &roms, next) {
> > +        rom->committed = true;
>
> My only thought is that maybe this should produce a warning or error
> if a ROM isn't committed.

Not all loaders use the transaction API.  Therefore it is likely that
some pre-existing ROMs will have ->committed = false.

For example, imagine a firmware ROM is loaded by the machine type and
then -kernel is used to load a file.  The -kernel loader shouldn't
worry about the firmware ROM, which was added without the transaction
API.

If we want to be strict I'd have to audit all ROM API users and wrap
them in add rom_transaction_begin/end() even if they cannot fail.
This is why I decided to simply ignore pre-existing ROMs.

Stefan