| Message ID | 20220915204317.3766007-7-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: Elide redundant has_FOO in generated C | expand |
On Thu, 15 Sep 2022 22:42:56 +0200 Markus Armbruster <armbru@redhat.com> wrote: > The has_FOO for pointer-valued FOO are redundant, except for arrays. > They are also a nuisance to work with. Recent commit "qapi: Start to > elide redundant has_FOO in generated C" provided the means to elide > them step by step. This is the step for qapi/acpi.py. > > Said commit explains the transformation in more detail. The invariant > violations mentioned there do not occur here. > > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Ani Sinha <ani@anisinha.ca> > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/acpi/core.c | 14 +++++++------- > hw/acpi/cpu.c | 1 - > hw/acpi/memory_hotplug.c | 1 - > scripts/qapi/schema.py | 1 - > 4 files changed, 7 insertions(+), 10 deletions(-) > > diff --git a/hw/acpi/core.c b/hw/acpi/core.c > index 3e811bf03c..6da275c599 100644 > --- a/hw/acpi/core.c > +++ b/hw/acpi/core.c > @@ -185,7 +185,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, > changed_fields = 0; > ext_hdr->_length = cpu_to_le16(acpi_payload_size); > > - if (hdrs->has_sig) { > + if (hdrs->sig) { > strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); > ++changed_fields; > } > @@ -204,11 +204,11 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, > > ext_hdr->checksum = 0; > > - if (hdrs->has_oem_id) { > + if (hdrs->oem_id) { > strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id); > ++changed_fields; > } > - if (hdrs->has_oem_table_id) { > + if (hdrs->oem_table_id) { > strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id, > sizeof ext_hdr->oem_table_id); > ++changed_fields; > @@ -217,7 +217,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, > ext_hdr->oem_revision = cpu_to_le32(hdrs->oem_rev); > ++changed_fields; > } > - if (hdrs->has_asl_compiler_id) { > + if (hdrs->asl_compiler_id) { > strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id, > sizeof ext_hdr->asl_compiler_id); > ++changed_fields; > @@ -255,12 +255,12 @@ void acpi_table_add(const QemuOpts *opts, Error **errp) > if (!hdrs) { > goto out; > } > - if (hdrs->has_file == hdrs->has_data) { > + if (!hdrs->file == !hdrs->data) { > error_setg(errp, "'-acpitable' requires one of 'data' or 'file'"); > goto out; > } > > - pathnames = g_strsplit(hdrs->has_file ? hdrs->file : hdrs->data, ":", 0); > + pathnames = g_strsplit(hdrs->file ?: hdrs->data, ":", 0); > if (pathnames == NULL || pathnames[0] == NULL) { > error_setg(errp, "'-acpitable' requires at least one pathname"); > goto out; > @@ -297,7 +297,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp) > close(fd); > } > > - acpi_table_install(blob, bloblen, hdrs->has_file, hdrs, errp); > + acpi_table_install(blob, bloblen, !!hdrs->file, hdrs, errp); > > out: > g_free(blob); > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > index 3646dbfe68..4e580959a2 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -35,7 +35,6 @@ static ACPIOSTInfo *acpi_cpu_device_status(int idx, AcpiCpuStatus *cdev) > DeviceState *dev = DEVICE(cdev->cpu); > if (dev->id) { > info->device = g_strdup(dev->id); > - info->has_device = true; > } > } > return info; > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index 0a7e89a13e..a7476330a8 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -44,7 +44,6 @@ static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev) > DeviceState *dev = DEVICE(mdev->dimm); > if (dev->id) { > info->device = g_strdup(dev->id); > - info->has_device = true; > } > } > return info; > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index 3ae94300c4..5160a659b7 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -759,7 +759,6 @@ def need_has(self): > assert self.type > # Temporary hack to support dropping the has_FOO in reviewable chunks > opt_out = [ > - 'qapi/acpi.json', > 'qapi/audio.json', > 'qapi/block-core.json', > 'qapi/block-export.json',
On Fri, Sep 16, 2022 at 2:13 AM Markus Armbruster <armbru@redhat.com> wrote: > > The has_FOO for pointer-valued FOO are redundant, except for arrays. > They are also a nuisance to work with. Recent commit "qapi: Start to > elide redundant has_FOO in generated C" Commit is referenced using <commit hash 13 chars min> ("commit header") provided the means to elide > them step by step. This is the step for qapi/acpi.py. > > Said commit explains the transformation in more detail. The invariant > violations mentioned there do not occur here. > > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Ani Sinha <ani@anisinha.ca> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/acpi/core.c | 14 +++++++------- > hw/acpi/cpu.c | 1 - > hw/acpi/memory_hotplug.c | 1 - > scripts/qapi/schema.py | 1 - > 4 files changed, 7 insertions(+), 10 deletions(-) > > diff --git a/hw/acpi/core.c b/hw/acpi/core.c > index 3e811bf03c..6da275c599 100644 > --- a/hw/acpi/core.c > +++ b/hw/acpi/core.c > @@ -185,7 +185,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, > changed_fields = 0; > ext_hdr->_length = cpu_to_le16(acpi_payload_size); > > - if (hdrs->has_sig) { > + if (hdrs->sig) { > strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); > ++changed_fields; > } > @@ -204,11 +204,11 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, > > ext_hdr->checksum = 0; > > - if (hdrs->has_oem_id) { > + if (hdrs->oem_id) { > strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id); > ++changed_fields; > } > - if (hdrs->has_oem_table_id) { > + if (hdrs->oem_table_id) { > strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id, > sizeof ext_hdr->oem_table_id); > ++changed_fields; > @@ -217,7 +217,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, > ext_hdr->oem_revision = cpu_to_le32(hdrs->oem_rev); > ++changed_fields; > } > - if (hdrs->has_asl_compiler_id) { > + if (hdrs->asl_compiler_id) { > strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id, > sizeof ext_hdr->asl_compiler_id); > ++changed_fields; > @@ -255,12 +255,12 @@ void acpi_table_add(const QemuOpts *opts, Error **errp) > if (!hdrs) { > goto out; > } > - if (hdrs->has_file == hdrs->has_data) { > + if (!hdrs->file == !hdrs->data) { > error_setg(errp, "'-acpitable' requires one of 'data' or 'file'"); > goto out; > } > > - pathnames = g_strsplit(hdrs->has_file ? hdrs->file : hdrs->data, ":", 0); > + pathnames = g_strsplit(hdrs->file ?: hdrs->data, ":", 0); > if (pathnames == NULL || pathnames[0] == NULL) { > error_setg(errp, "'-acpitable' requires at least one pathname"); > goto out; > @@ -297,7 +297,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp) > close(fd); > } > > - acpi_table_install(blob, bloblen, hdrs->has_file, hdrs, errp); > + acpi_table_install(blob, bloblen, !!hdrs->file, hdrs, errp); > > out: > g_free(blob); > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > index 3646dbfe68..4e580959a2 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -35,7 +35,6 @@ static ACPIOSTInfo *acpi_cpu_device_status(int idx, AcpiCpuStatus *cdev) > DeviceState *dev = DEVICE(cdev->cpu); > if (dev->id) { > info->device = g_strdup(dev->id); > - info->has_device = true; > } > } > return info; > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index 0a7e89a13e..a7476330a8 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -44,7 +44,6 @@ static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev) > DeviceState *dev = DEVICE(mdev->dimm); > if (dev->id) { > info->device = g_strdup(dev->id); > - info->has_device = true; > } > } > return info; > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index 3ae94300c4..5160a659b7 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -759,7 +759,6 @@ def need_has(self): > assert self.type > # Temporary hack to support dropping the has_FOO in reviewable chunks > opt_out = [ > - 'qapi/acpi.json', > 'qapi/audio.json', > 'qapi/block-core.json', > 'qapi/block-export.json', > -- > 2.37.2 >
Ani Sinha <ani@anisinha.ca> writes: > On Fri, Sep 16, 2022 at 2:13 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> The has_FOO for pointer-valued FOO are redundant, except for arrays. >> They are also a nuisance to work with. Recent commit "qapi: Start to >> elide redundant has_FOO in generated C" > > Commit is referenced using <commit hash 13 chars min> ("commit header") Apply my patches in your tree, and your hashes will differ from mine. Hashes can serve as stable reference only when we git-fetch patches, not when we git-send-email then. >> provided the means to elide >> them step by step. This is the step for qapi/acpi.py. >> >> Said commit explains the transformation in more detail. The invariant >> violations mentioned there do not occur here. >> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Cc: Igor Mammedov <imammedo@redhat.com> >> Cc: Ani Sinha <ani@anisinha.ca> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
On Fri, Sep 16, 2022 at 1:48 PM Markus Armbruster <armbru@redhat.com> wrote: > > Ani Sinha <ani@anisinha.ca> writes: > > > On Fri, Sep 16, 2022 at 2:13 AM Markus Armbruster <armbru@redhat.com> wrote: > >> > >> The has_FOO for pointer-valued FOO are redundant, except for arrays. > >> They are also a nuisance to work with. Recent commit "qapi: Start to > >> elide redundant has_FOO in generated C" > > > > Commit is referenced using <commit hash 13 chars min> ("commit header") > > Apply my patches in your tree, and your hashes will differ from mine. > Hashes can serve as stable reference only when we git-fetch patches, not > when we git-send-email then. Has the referenced patch not been pushed upstream already? I thought it was and hence was asking for the reference. > > >> provided the means to elide > >> them step by step. This is the step for qapi/acpi.py. > >> > >> Said commit explains the transformation in more detail. The invariant > >> violations mentioned there do not occur here. > >> > >> Cc: Michael S. Tsirkin <mst@redhat.com> > >> Cc: Igor Mammedov <imammedo@redhat.com> > >> Cc: Ani Sinha <ani@anisinha.ca> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >
On Fri, Sep 16, 2022 at 01:51:14PM +0530, Ani Sinha wrote: > On Fri, Sep 16, 2022 at 1:48 PM Markus Armbruster <armbru@redhat.com> wrote: > > > > Ani Sinha <ani@anisinha.ca> writes: > > > > > On Fri, Sep 16, 2022 at 2:13 AM Markus Armbruster <armbru@redhat.com> wrote: > > >> > > >> The has_FOO for pointer-valued FOO are redundant, except for arrays. > > >> They are also a nuisance to work with. Recent commit "qapi: Start to > > >> elide redundant has_FOO in generated C" > > > > > > Commit is referenced using <commit hash 13 chars min> ("commit header") > > > > Apply my patches in your tree, and your hashes will differ from mine. > > Hashes can serve as stable reference only when we git-fetch patches, not > > when we git-send-email then. > > Has the referenced patch not been pushed upstream already? I thought > it was and hence was asking for the reference. Err, it is just a few patches earlier in this very series you're looking at. With regards, Daniel
On Fri, Sep 16, 2022 at 14:08 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Fri, Sep 16, 2022 at 01:51:14PM +0530, Ani Sinha wrote: > > On Fri, Sep 16, 2022 at 1:48 PM Markus Armbruster <armbru@redhat.com> > wrote: > > > > > > Ani Sinha <ani@anisinha.ca> writes: > > > > > > > On Fri, Sep 16, 2022 at 2:13 AM Markus Armbruster <armbru@redhat.com> > wrote: > > > >> > > > >> The has_FOO for pointer-valued FOO are redundant, except for arrays. > > > >> They are also a nuisance to work with. Recent commit "qapi: Start > to > > > >> elide redundant has_FOO in generated C" > > > > > > > > Commit is referenced using <commit hash 13 chars min> ("commit > header") > > > > > > Apply my patches in your tree, and your hashes will differ from mine. > > > Hashes can serve as stable reference only when we git-fetch patches, > not > > > when we git-send-email then. > > > > Has the referenced patch not been pushed upstream already? I thought > > it was and hence was asking for the reference. > > Err, it is just a few patches earlier in this very series you're > looking at. Hmm ok. It’s missing from my inbox. Probably was not CC’d to me.
On Thu, Sep 15, 2022 at 10:42:56PM +0200, Markus Armbruster wrote: > The has_FOO for pointer-valued FOO are redundant, except for arrays. > They are also a nuisance to work with. Recent commit "qapi: Start to > elide redundant has_FOO in generated C" provided the means to elide > them step by step. This is the step for qapi/acpi.py. > > Said commit explains the transformation in more detail. The invariant > violations mentioned there do not occur here. > > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Ani Sinha <ani@anisinha.ca> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/acpi/core.c | 14 +++++++------- > hw/acpi/cpu.c | 1 - > hw/acpi/memory_hotplug.c | 1 - > scripts/qapi/schema.py | 1 - > 4 files changed, 7 insertions(+), 10 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
On Fri, Sep 16, 2022 at 14:15 Ani Sinha <ani@anisinha.ca> wrote: > > > On Fri, Sep 16, 2022 at 14:08 Daniel P. Berrangé <berrange@redhat.com> > wrote: > >> On Fri, Sep 16, 2022 at 01:51:14PM +0530, Ani Sinha wrote: >> > On Fri, Sep 16, 2022 at 1:48 PM Markus Armbruster <armbru@redhat.com> >> wrote: >> > > >> > > Ani Sinha <ani@anisinha.ca> writes: >> > > >> > > > On Fri, Sep 16, 2022 at 2:13 AM Markus Armbruster < >> armbru@redhat.com> wrote: >> > > >> >> > > >> The has_FOO for pointer-valued FOO are redundant, except for >> arrays. >> > > >> They are also a nuisance to work with. Recent commit "qapi: Start >> to >> > > >> elide redundant has_FOO in generated C" >> > > > >> > > > Commit is referenced using <commit hash 13 chars min> ("commit >> header") >> > > >> > > Apply my patches in your tree, and your hashes will differ from mine. >> > > Hashes can serve as stable reference only when we git-fetch patches, >> not >> > > when we git-send-email then. >> > >> > Has the referenced patch not been pushed upstream already? I thought >> > it was and hence was asking for the reference. >> >> Err, it is just a few patches earlier in this very series you're >> looking at. > > > Hmm ok. It’s missing from my inbox. Probably was not CC’d to me. > Indeed. https://lore.kernel.org/all/20220915204317.3766007-5-armbru@redhat.com/ >
Ani Sinha <ani@anisinha.ca> writes: > On Fri, Sep 16, 2022 at 14:08 Daniel P. Berrangé <berrange@redhat.com> > wrote: > >> On Fri, Sep 16, 2022 at 01:51:14PM +0530, Ani Sinha wrote: >> > On Fri, Sep 16, 2022 at 1:48 PM Markus Armbruster <armbru@redhat.com> wrote: >> > > >> > > Ani Sinha <ani@anisinha.ca> writes: >> > > >> > > > On Fri, Sep 16, 2022 at 2:13 AM Markus Armbruster <armbru@redhat.com> wrote: >> > > >> >> > > >> The has_FOO for pointer-valued FOO are redundant, except for arrays. >> > > >> They are also a nuisance to work with. Recent commit "qapi: Start to >> > > >> elide redundant has_FOO in generated C" >> > > > >> > > > Commit is referenced using <commit hash 13 chars min> ("commit header") >> > > >> > > Apply my patches in your tree, and your hashes will differ from mine. >> > > Hashes can serve as stable reference only when we git-fetch patches, not >> > > when we git-send-email then. >> > >> > Has the referenced patch not been pushed upstream already? I thought >> > it was and hence was asking for the reference. >> >> Err, it is just a few patches earlier in this very series you're >> looking at. > > > Hmm ok. It’s missing from my inbox. Probably was not CC’d to me. Yes. I'm sorry for the confusion!
diff --git a/hw/acpi/core.c b/hw/acpi/core.c index 3e811bf03c..6da275c599 100644 --- a/hw/acpi/core.c +++ b/hw/acpi/core.c @@ -185,7 +185,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, changed_fields = 0; ext_hdr->_length = cpu_to_le16(acpi_payload_size); - if (hdrs->has_sig) { + if (hdrs->sig) { strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig); ++changed_fields; } @@ -204,11 +204,11 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, ext_hdr->checksum = 0; - if (hdrs->has_oem_id) { + if (hdrs->oem_id) { strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id); ++changed_fields; } - if (hdrs->has_oem_table_id) { + if (hdrs->oem_table_id) { strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id, sizeof ext_hdr->oem_table_id); ++changed_fields; @@ -217,7 +217,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen, ext_hdr->oem_revision = cpu_to_le32(hdrs->oem_rev); ++changed_fields; } - if (hdrs->has_asl_compiler_id) { + if (hdrs->asl_compiler_id) { strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id, sizeof ext_hdr->asl_compiler_id); ++changed_fields; @@ -255,12 +255,12 @@ void acpi_table_add(const QemuOpts *opts, Error **errp) if (!hdrs) { goto out; } - if (hdrs->has_file == hdrs->has_data) { + if (!hdrs->file == !hdrs->data) { error_setg(errp, "'-acpitable' requires one of 'data' or 'file'"); goto out; } - pathnames = g_strsplit(hdrs->has_file ? hdrs->file : hdrs->data, ":", 0); + pathnames = g_strsplit(hdrs->file ?: hdrs->data, ":", 0); if (pathnames == NULL || pathnames[0] == NULL) { error_setg(errp, "'-acpitable' requires at least one pathname"); goto out; @@ -297,7 +297,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp) close(fd); } - acpi_table_install(blob, bloblen, hdrs->has_file, hdrs, errp); + acpi_table_install(blob, bloblen, !!hdrs->file, hdrs, errp); out: g_free(blob); diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index 3646dbfe68..4e580959a2 100644 --- a/hw/acpi/cpu.c +++ b/hw/acpi/cpu.c @@ -35,7 +35,6 @@ static ACPIOSTInfo *acpi_cpu_device_status(int idx, AcpiCpuStatus *cdev) DeviceState *dev = DEVICE(cdev->cpu); if (dev->id) { info->device = g_strdup(dev->id); - info->has_device = true; } } return info; diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index 0a7e89a13e..a7476330a8 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -44,7 +44,6 @@ static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev) DeviceState *dev = DEVICE(mdev->dimm); if (dev->id) { info->device = g_strdup(dev->id); - info->has_device = true; } } return info; diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 3ae94300c4..5160a659b7 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -759,7 +759,6 @@ def need_has(self): assert self.type # Temporary hack to support dropping the has_FOO in reviewable chunks opt_out = [ - 'qapi/acpi.json', 'qapi/audio.json', 'qapi/block-core.json', 'qapi/block-export.json',
The has_FOO for pointer-valued FOO are redundant, except for arrays. They are also a nuisance to work with. Recent commit "qapi: Start to elide redundant has_FOO in generated C" provided the means to elide them step by step. This is the step for qapi/acpi.py. Said commit explains the transformation in more detail. The invariant violations mentioned there do not occur here. Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Ani Sinha <ani@anisinha.ca> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/acpi/core.c | 14 +++++++------- hw/acpi/cpu.c | 1 - hw/acpi/memory_hotplug.c | 1 - scripts/qapi/schema.py | 1 - 4 files changed, 7 insertions(+), 10 deletions(-)