Re: [Qemu-devel] [PATCH v6 23/29] tests/acpi-utils: Drop dependence on global_qtest

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> As a general rule, we prefer avoiding implicit global state
> because it makes code harder to safely copy and paste without
> thinking about the global state.  Adjust the helper code to
> use explicit state instead, and update all callers.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  tests/acpi-utils.h       |  65 +++++++++----------
>  tests/acpi-utils.c       |  19 +++---
>  tests/bios-tables-test.c | 161 
> ++++++++++++++++++++++++-----------------------
>  tests/vmgenid-test.c     |  18 +++---
>  4 files changed, 133 insertions(+), 130 deletions(-)
[...]
> @@ -87,7 +87,7 @@ static void test_acpi_rsdp_table(test_data *data)
>      AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table;
>      uint32_t addr = data->rsdp_addr;
> 
> -    acpi_parse_rsdp_table(addr, rsdp_table);
> +    acpi_parse_rsdp_table(global_qtest, addr, rsdp_table);
> 
>      /* rsdp checksum is not for the whole table, but for the first 20 bytes 
> */
>      g_assert(!acpi_calc_checksum((uint8_t *)rsdp_table, 20));
> @@ -102,7 +102,7 @@ static void test_acpi_rsdt_table(test_data *data)
>      uint8_t checksum;
> 
>      /* read the header */
> -    ACPI_READ_TABLE_HEADER(rsdt_table, addr);
> +    ACPI_READ_TABLE_HEADER(global_qtest, rsdt_table, addr);
>      ACPI_ASSERT_CMP(rsdt_table->signature, "RSDT");
> 
>      /* compute the table entries in rsdt */
> @@ -112,7 +112,7 @@ static void test_acpi_rsdt_table(test_data *data)
> 
>      /* get the addresses of the tables pointed by rsdt */
>      tables = g_new0(uint32_t, tables_nr);
> -    ACPI_READ_ARRAY_PTR(tables, tables_nr, addr);
> +    ACPI_READ_ARRAY_PTR(global_qtest, tables, tables_nr, addr);
> 
>      checksum = acpi_calc_checksum((uint8_t *)rsdt_table, rsdt_table->length) 
> +
>                 acpi_calc_checksum((uint8_t *)tables,

Wouldn't it be better to use a "QTestState *qts = global_qtest" variable
in this function, like you do it below in test_acpi_fadt_table()? Or
even pass it via test_data already? Otherwise you've got to change all
lines again when finally getting rid of global_qtest ...

> @@ -128,63 +128,64 @@ static void test_acpi_fadt_table(test_data *data)
>  {
>      AcpiFadtDescriptorRev3 *fadt_table = &data->fadt_table;
>      uint32_t addr;
> +    QTestState *qts = global_qtest;
> 
>      /* FADT table comes first */
>      addr = data->rsdt_tables_addr[0];
> -    ACPI_READ_TABLE_HEADER(fadt_table, addr);
> +    ACPI_READ_TABLE_HEADER(qts, fadt_table, addr);
> 
> -    ACPI_READ_FIELD(fadt_table->firmware_ctrl, addr);
> -    ACPI_READ_FIELD(fadt_table->dsdt, addr);
> -    ACPI_READ_FIELD(fadt_table->model, addr);
> -    ACPI_READ_FIELD(fadt_table->reserved1, addr);
> -    ACPI_READ_FIELD(fadt_table->sci_int, addr);
> -    ACPI_READ_FIELD(fadt_table->smi_cmd, addr);
> -    ACPI_READ_FIELD(fadt_table->acpi_enable, addr);
> -    ACPI_READ_FIELD(fadt_table->acpi_disable, addr);
> -    ACPI_READ_FIELD(fadt_table->S4bios_req, addr);
> -    ACPI_READ_FIELD(fadt_table->reserved2, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1a_evt_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1b_evt_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1a_cnt_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1b_cnt_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm2_cnt_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm_tmr_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->gpe0_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->gpe1_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1_evt_len, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1_cnt_len, addr);
> -    ACPI_READ_FIELD(fadt_table->pm2_cnt_len, addr);
> -    ACPI_READ_FIELD(fadt_table->pm_tmr_len, addr);
> -    ACPI_READ_FIELD(fadt_table->gpe0_blk_len, addr);
> -    ACPI_READ_FIELD(fadt_table->gpe1_blk_len, addr);
> -    ACPI_READ_FIELD(fadt_table->gpe1_base, addr);
> -    ACPI_READ_FIELD(fadt_table->reserved3, addr);
> -    ACPI_READ_FIELD(fadt_table->plvl2_lat, addr);
> -    ACPI_READ_FIELD(fadt_table->plvl3_lat, addr);
> -    ACPI_READ_FIELD(fadt_table->flush_size, addr);
> -    ACPI_READ_FIELD(fadt_table->flush_stride, addr);
> -    ACPI_READ_FIELD(fadt_table->duty_offset, addr);
> -    ACPI_READ_FIELD(fadt_table->duty_width, addr);
> -    ACPI_READ_FIELD(fadt_table->day_alrm, addr);
> -    ACPI_READ_FIELD(fadt_table->mon_alrm, addr);
> -    ACPI_READ_FIELD(fadt_table->century, addr);
> -    ACPI_READ_FIELD(fadt_table->boot_flags, addr);
> -    ACPI_READ_FIELD(fadt_table->reserved, addr);
> -    ACPI_READ_FIELD(fadt_table->flags, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->reset_register, addr);
> -    ACPI_READ_FIELD(fadt_table->reset_value, addr);
> -    ACPI_READ_FIELD(fadt_table->arm_boot_flags, addr);
> -    ACPI_READ_FIELD(fadt_table->minor_revision, addr);
> -    ACPI_READ_FIELD(fadt_table->x_facs, addr);
> -    ACPI_READ_FIELD(fadt_table->x_dsdt, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1a_event_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1b_event_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1a_control_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1b_control_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm2_control_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm_timer_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xgpe0_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xgpe1_block, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->firmware_ctrl, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->dsdt, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->model, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->reserved1, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->sci_int, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->smi_cmd, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->acpi_enable, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->acpi_disable, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->S4bios_req, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->reserved2, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->pm1a_evt_blk, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->pm1b_evt_blk, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->pm1a_cnt_blk, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->pm1b_cnt_blk, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->pm2_cnt_blk, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->pm_tmr_blk, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->gpe0_blk, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->gpe1_blk, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->pm1_evt_len, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->pm1_cnt_len, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->pm2_cnt_len, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->pm_tmr_len, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->gpe0_blk_len, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->gpe1_blk_len, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->gpe1_base, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->reserved3, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->plvl2_lat, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->plvl3_lat, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->flush_size, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->flush_stride, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->duty_offset, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->duty_width, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->day_alrm, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->mon_alrm, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->century, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->boot_flags, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->reserved, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->flags, addr);
> +    ACPI_READ_GENERIC_ADDRESS(qts, fadt_table->reset_register, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->reset_value, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->arm_boot_flags, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->minor_revision, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->x_facs, addr);
> +    ACPI_READ_FIELD(qts, fadt_table->x_dsdt, addr);
> +    ACPI_READ_GENERIC_ADDRESS(qts, fadt_table->xpm1a_event_block, addr);
> +    ACPI_READ_GENERIC_ADDRESS(qts, fadt_table->xpm1b_event_block, addr);
> +    ACPI_READ_GENERIC_ADDRESS(qts, fadt_table->xpm1a_control_block, addr);
> +    ACPI_READ_GENERIC_ADDRESS(qts, fadt_table->xpm1b_control_block, addr);
> +    ACPI_READ_GENERIC_ADDRESS(qts, fadt_table->xpm2_control_block, addr);
> +    ACPI_READ_GENERIC_ADDRESS(qts, fadt_table->xpm_timer_block, addr);
> +    ACPI_READ_GENERIC_ADDRESS(qts, fadt_table->xgpe0_block, addr);
> +    ACPI_READ_GENERIC_ADDRESS(qts, fadt_table->xgpe1_block, addr);
> 
>      ACPI_ASSERT_CMP(fadt_table->signature, "FACP");
>      g_assert(!acpi_calc_checksum((uint8_t *)fadt_table, fadt_table->length));
> @@ -195,13 +196,13 @@ static void test_acpi_facs_table(test_data *data)
>      AcpiFacsDescriptorRev1 *facs_table = &data->facs_table;
>      uint32_t addr = data->fadt_table.firmware_ctrl;
> 
> -    ACPI_READ_FIELD(facs_table->signature, addr);
> -    ACPI_READ_FIELD(facs_table->length, addr);
> -    ACPI_READ_FIELD(facs_table->hardware_signature, addr);
> -    ACPI_READ_FIELD(facs_table->firmware_waking_vector, addr);
> -    ACPI_READ_FIELD(facs_table->global_lock, addr);
> -    ACPI_READ_FIELD(facs_table->flags, addr);
> -    ACPI_READ_ARRAY(facs_table->resverved3, addr);
> +    ACPI_READ_FIELD(global_qtest, facs_table->signature, addr);
> +    ACPI_READ_FIELD(global_qtest, facs_table->length, addr);
> +    ACPI_READ_FIELD(global_qtest, facs_table->hardware_signature, addr);
> +    ACPI_READ_FIELD(global_qtest, facs_table->firmware_waking_vector, addr);
> +    ACPI_READ_FIELD(global_qtest, facs_table->global_lock, addr);
> +    ACPI_READ_FIELD(global_qtest, facs_table->flags, addr);
> +    ACPI_READ_ARRAY(global_qtest, facs_table->resverved3, addr);

Dito, i.e. use a local "QTestState *qts = global_qtest" variable instead?

Same comment applies to the other functions where you were using
global_test like this.

 Thomas