| Message ID | 20181207170400.5129-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() | expand |
On 12/07/18 18:03, Philippe Mathieu-Daudé wrote: > Since af1f60a4022, the fw_cfg field is always created in machvirt_init(). > There is no need to null check it. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/arm/virt.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index f69e7eb399..36303ed59c 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1279,10 +1279,6 @@ static void virt_build_smbios(VirtMachineState *vms) > size_t smbios_tables_len, smbios_anchor_len; > const char *product = "QEMU Virtual Machine"; > > - if (!vms->fw_cfg) { > - return; > - } > - > if (kvm_enabled()) { > product = "KVM Virtual Machine"; > } > This patch looks correct to me; however, the fact that fw_cfg is always present on the virt board does not seem related to commit af1f60a4022 af1f60a40226 ("hw/arm/virt: remove VirtGuestInfo", 2017-01-09). Said commit only flattened the contents of VirtGuestInfo into VirtMachineState; the conditions under which fw_cfg would be created were not changed. As far as I can see (from a number of git-blame / git-show commands), fw_cfg was added unconditionally to the virt board in commit 578f3c7b0835 ("arm: add fw_cfg to "virt" board", 2014-12-22). And the check that you are now (correctly) removing has always been superfluous -- it was superfluous already when it was first added in commit c30e15658b1b ("smbios: implement smbios support for mach-virt", 2015-09-07). As far as I can tell, anyway. If you agree, I suggest mentioning commit c30e15658b1b in the commit message, rather than commit af1f60a4022. With such an update: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo
Hi Laszlo, On 12/10/18 5:02 PM, Laszlo Ersek wrote: > On 12/07/18 18:03, Philippe Mathieu-Daudé wrote: >> Since af1f60a4022, the fw_cfg field is always created in machvirt_init(). >> There is no need to null check it. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/arm/virt.c | 4 ---- >> 1 file changed, 4 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index f69e7eb399..36303ed59c 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -1279,10 +1279,6 @@ static void virt_build_smbios(VirtMachineState *vms) >> size_t smbios_tables_len, smbios_anchor_len; >> const char *product = "QEMU Virtual Machine"; >> >> - if (!vms->fw_cfg) { >> - return; >> - } >> - >> if (kvm_enabled()) { >> product = "KVM Virtual Machine"; >> } >> > > This patch looks correct to me; however, the fact that fw_cfg is always > present on the virt board does not seem related to commit af1f60a4022 > af1f60a40226 ("hw/arm/virt: remove VirtGuestInfo", 2017-01-09). Said > commit only flattened the contents of VirtGuestInfo into > VirtMachineState; the conditions under which fw_cfg would be created > were not changed. This is certainly not related to commit af1f60a4022... This is not the commit I remember I wanted to refer to, I suppose I did a copy/paste mistake, thanks for carefully reviewing me and catching this! > As far as I can see (from a number of git-blame / git-show commands), > fw_cfg was added unconditionally to the virt board in commit > 578f3c7b0835 ("arm: add fw_cfg to "virt" board", 2014-12-22). And the > check that you are now (correctly) removing has always been superfluous > -- it was superfluous already when it was first added in commit > c30e15658b1b ("smbios: implement smbios support for mach-virt", 2015-09-07). > > As far as I can tell, anyway. > > If you agree, I suggest mentioning commit c30e15658b1b in the commit > message, rather than commit af1f60a4022. Yes, this one makes more sense ;) > > With such an update: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Phil. > > Thanks > Laszlo >
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index f69e7eb399..36303ed59c 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1279,10 +1279,6 @@ static void virt_build_smbios(VirtMachineState *vms) size_t smbios_tables_len, smbios_anchor_len; const char *product = "QEMU Virtual Machine"; - if (!vms->fw_cfg) { - return; - } - if (kvm_enabled()) { product = "KVM Virtual Machine"; }
Since af1f60a4022, the fw_cfg field is always created in machvirt_init(). There is no need to null check it. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/arm/virt.c | 4 ---- 1 file changed, 4 deletions(-)