[libvirt] [PATCH] conf: Fix initialization value of 'multi' in PCI address
Signed-off-by: Xian Han Yu <xhyubj@linux.vnet.ibm.com> --- src/conf/node_device_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index a23d8ef..96fdb6f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1163,7 +1163,7 @@ virNodeDevCapPCIDevIommuGroupParseXML(xmlXPathContextPtr ctxt, goto cleanup; for (i = 0; i < nAddrNodes; i++) { - virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 }; + virPCIDeviceAddress addr = { 0, 0, 0, 0, false }; if (virPCIDeviceAddressParseXML(addrNodes[i], &addr) < 0) goto cleanup; if (VIR_ALLOC(pciAddr) < 0) -- 2.5.5
On Tue, Aug 09, 2016 at 10:49:04AM +0200, Xian Han Yu wrote:
Signed-off-by: Xian Han Yu <xhyubj@linux.vnet.ibm.com> ---
You could've been more descriptive in the commit message. Like describing what's the reason for this change? Just to initialize bool to 'false' instead 0? Is there something more to it?
src/conf/node_device_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index a23d8ef..96fdb6f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1163,7 +1163,7 @@ virNodeDevCapPCIDevIommuGroupParseXML(xmlXPathContextPtr ctxt, goto cleanup;
for (i = 0; i < nAddrNodes; i++) { - virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 }; + virPCIDeviceAddress addr = { 0, 0, 0, 0, false };
Honestly, I have no idea what preferences we have for such initializations, but I for one prefer initialization to '{0}' which guarantees everything to be zeroed anyway. And will be readable the same way even when we change the structure. Would that work for you as well?
if (virPCIDeviceAddressParseXML(addrNodes[i], &addr) < 0) goto cleanup; if (VIR_ALLOC(pciAddr) < 0) -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2016-08-09 at 11:19 +0200, Martin Kletzander wrote:
for (i = 0; i < nAddrNodes; i++) { - virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 }; + virPCIDeviceAddress addr = { 0, 0, 0, 0, false }; Honestly, I have no idea what preferences we have for such initializations, but I for one prefer initialization to '{0}' which guarantees everything to be zeroed anyway. And will be readable the same way even when we change the structure. Would that work for you as well?
I think it should either be 0 (as the structure member is defined as int) or VIR_TRISTATE_SWITCH_ABSENT (as it is used as virTristateSwitch, according to the comment and other bits of code). false definitely seems out of place. -- Andrea Bolognani / Red Hat / Virtualization
On 08/09/2016 11:25 AM, Andrea Bolognani wrote:
On Tue, 2016-08-09 at 11:19 +0200, Martin Kletzander wrote:
for (i = 0; i < nAddrNodes; i++) { - virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 }; + virPCIDeviceAddress addr = { 0, 0, 0, 0, false };
Honestly, I have no idea what preferences we have for such initializations, but I for one prefer initialization to '{0}' which guarantees everything to be zeroed anyway. And will be readable the same way even when we change the structure. Would that work for you as well?
I think it should either be 0 (as the structure member is defined as int) or VIR_TRISTATE_SWITCH_ABSENT (as it is used as virTristateSwitch, according to the comment and other bits of code). false definitely seems out of place.
-- Andrea Bolognani / Red Hat / Virtualization
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Actually this fix was about aligning three code occurrences. These three initialisations can be found here: qemu/qemu_domain_address.c 1099: virPCIDeviceAddress addr = { 0, 0, 0, 0, false }; conf/node_device_conf.c 1166: virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 }; conf/domain_addr.c 572: virPCIDeviceAddress a = { 0, 0, 0, 0, false }; Setting the VIR_TRISTATE_SWITCH_ABSENT make sense from the data type point of view. Looking at it from the code readability point of view you would have to know that the default of the multifunction is Off and with that in mind it made more sense setting it to false. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Wed, 2016-08-10 at 17:16 +0200, Boris Fiuczynski wrote:
for (i = 0; i < nAddrNodes; i++) { - virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 }; + virPCIDeviceAddress addr = { 0, 0, 0, 0, false }; Honestly, I have no idea what preferences we have for such initializations, but I for one prefer initialization to '{0}' which guarantees everything to be zeroed anyway. And will be readable the same way even when we change the structure. Would that work for you as well? I think it should either be 0 (as the structure member is defined as int) or VIR_TRISTATE_SWITCH_ABSENT (as it is used as virTristateSwitch, according to the comment and other bits of code). false definitely seems out of place. Actually this fix was about aligning three code occurrences. These three initialisations can be found here: qemu/qemu_domain_address.c 1099: virPCIDeviceAddress addr = { 0, 0, 0, 0, false }; conf/node_device_conf.c 1166: virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 }; conf/domain_addr.c 572: virPCIDeviceAddress a = { 0, 0, 0, 0, false }; Setting the VIR_TRISTATE_SWITCH_ABSENT make sense from the data type point of view. Looking at it from the code readability point of view you would have to know that the default of the multifunction is Off and with that in mind it made more sense setting it to false.
The default is not OFF, though, it's ABSENT :) In fact, as far as I can tell, OFF isn't ever used explicitly either for assignment or comparison. And false is plain wrong from a datatype point of view. -- Andrea Bolognani / Red Hat / Virtualization
On 08/10/2016 11:34 AM, Andrea Bolognani wrote:
On Wed, 2016-08-10 at 17:16 +0200, Boris Fiuczynski wrote:
for (i = 0; i < nAddrNodes; i++) { - virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 }; + virPCIDeviceAddress addr = { 0, 0, 0, 0, false };
Honestly, I have no idea what preferences we have for such initializations, but I for one prefer initialization to '{0}' which guarantees everything to be zeroed anyway. And will be readable the same way even when we change the structure. Would that work for you as well?
I think it should either be 0 (as the structure member is defined as int) or VIR_TRISTATE_SWITCH_ABSENT (as it is used as virTristateSwitch, according to the comment and other bits of code). false definitely seems out of place.
Actually this fix was about aligning three code occurrences. These three initialisations can be found here:
qemu/qemu_domain_address.c 1099: virPCIDeviceAddress addr = { 0, 0, 0, 0, false };
conf/node_device_conf.c 1166: virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 };
conf/domain_addr.c 572: virPCIDeviceAddress a = { 0, 0, 0, 0, false };
Setting the VIR_TRISTATE_SWITCH_ABSENT make sense from the data type point of view. Looking at it from the code readability point of view you would have to know that the default of the multifunction is Off and with that in mind it made more sense setting it to false. The default is not OFF, though, it's ABSENT :)
In fact, as far as I can tell, OFF isn't ever used explicitly either for assignment or comparison. And false is plain wrong from a datatype point of view.
(a bit off-topic, but while we're talking about the multifunction option). For that matter, I've always thought the mere existence of the multifunction attribute was completely superfluous. Its proper setting can be completely 100% determined from context (I verified this with others who know more about PCI than I do). 1) It must not ever be set on a device with non-0 function. 2) It must not ever be set on function 0 if there are no other functions in use on the slot 3) It must *always* be set on function 0 if other functions of the slot are also used. For static configuration, we of course know whether or not there are other functions in use on the slot, and even once we do support hotplug of multiple functions, these multiple functions will *always* be plugged/unplugged as a single unit. So really, libvirt should just be setting the option according to these rules, and just logging an error if someone attempts to set it manually and gets it wrong.
On 8/10/2016 11:34 PM, Andrea Bolognani wrote:
On Wed, 2016-08-10 at 17:16 +0200, Boris Fiuczynski wrote:
for (i = 0; i < nAddrNodes; i++) { - virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 }; + virPCIDeviceAddress addr = { 0, 0, 0, 0, false };
Honestly, I have no idea what preferences we have for such initializations, but I for one prefer initialization to '{0}' which guarantees everything to be zeroed anyway. And will be readable the same way even when we change the structure. Would that work for you as well?
I think it should either be 0 (as the structure member is defined as int) or VIR_TRISTATE_SWITCH_ABSENT (as it is used as virTristateSwitch, according to the comment and other bits of code). false definitely seems out of place.
Actually this fix was about aligning three code occurrences. These three initialisations can be found here:
qemu/qemu_domain_address.c 1099: virPCIDeviceAddress addr = { 0, 0, 0, 0, false };
conf/node_device_conf.c 1166: virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 };
conf/domain_addr.c 572: virPCIDeviceAddress a = { 0, 0, 0, 0, false };
Setting the VIR_TRISTATE_SWITCH_ABSENT make sense from the data type point of view. Looking at it from the code readability point of view you would have to know that the default of the multifunction is Off and with that in mind it made more sense setting it to false. The default is not OFF, though, it's ABSENT :)
In fact, as far as I can tell, OFF isn't ever used explicitly either for assignment or comparison. And false is plain wrong from a datatype point of view.
-- Andrea Bolognani / Red Hat / Virtualization
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
How about we change all three occurrences as boris list above into VIR_TRISTATE_SWITCH_ABSENT. Xian Han
On Thu, 2016-08-11 at 14:48 +0800, Xian Han Yu wrote:
The default is not OFF, though, it's ABSENT :) In fact, as far as I can tell, OFF isn't ever used explicitly either for assignment or comparison. And false is plain wrong from a datatype point of view. How about we change all three occurrences as boris list above into VIR_TRISTATE_SWITCH_ABSENT.
Sure, that's exactly what I suggested :) -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Aug 11, 2016 at 10:55:44AM +0200, Andrea Bolognani wrote:
On Thu, 2016-08-11 at 14:48 +0800, Xian Han Yu wrote:
The default is not OFF, though, it's ABSENT :) In fact, as far as I can tell, OFF isn't ever used explicitly either for assignment or comparison. And false is plain wrong from a datatype point of view. How about we change all three occurrences as boris list above into VIR_TRISTATE_SWITCH_ABSENT.
Sure, that's exactly what I suggested :)
IMHO, we should just do what Michael suggested right at the start of this thread and use {0}, instead of manually initializing each field to 0, or a constant hiding the 0. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 8/11/2016 5:00 PM, Daniel P. Berrange wrote:
On Thu, 2016-08-11 at 14:48 +0800, Xian Han Yu wrote:
The default is not OFF, though, it's ABSENT :)
In fact, as far as I can tell, OFF isn't ever used explicitly either for assignment or comparison. And false is plain wrong from a datatype point of view.
How about we change all three occurrences as boris list above into VIR_TRISTATE_SWITCH_ABSENT. Sure, that's exactly what I suggested :) IMHO, we should just do what Michael suggested right at the start of this
On Thu, Aug 11, 2016 at 10:55:44AM +0200, Andrea Bolognani wrote: thread and use {0}, instead of manually initializing each field to 0, or a constant hiding the 0.
Regards, Daniel
That maybe change back again in the future, if this struct add a new member or current member need to be not zero-initialized.
On Thu, Aug 11, 2016 at 05:39:06PM +0800, Xian Han Yu wrote:
On 8/11/2016 5:00 PM, Daniel P. Berrange wrote:
On Thu, 2016-08-11 at 14:48 +0800, Xian Han Yu wrote:
The default is not OFF, though, it's ABSENT :) In fact, as far as I can tell, OFF isn't ever used explicitly either for assignment or comparison. And false is plain wrong from a datatype point of view. How about we change all three occurrences as boris list above into VIR_TRISTATE_SWITCH_ABSENT. Sure, that's exactly what I suggested :) IMHO, we should just do what Michael suggested right at the start of this
On Thu, Aug 11, 2016 at 10:55:44AM +0200, Andrea Bolognani wrote: thread and use {0}, instead of manually initializing each field to 0, or a constant hiding the 0.
That maybe change back again in the future, if this struct add a new member or current member need to be not zero-initialized.
Lets do what makes sense now, not in some hypothetical future that may never happen. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Aug 11, 2016 at 10:46:28AM +0100, Daniel P. Berrange wrote:
On Thu, Aug 11, 2016 at 05:39:06PM +0800, Xian Han Yu wrote:
On 8/11/2016 5:00 PM, Daniel P. Berrange wrote:
On Thu, 2016-08-11 at 14:48 +0800, Xian Han Yu wrote:
The default is not OFF, though, it's ABSENT :) In fact, as far as I can tell, OFF isn't ever used explicitly either for assignment or comparison. And false is plain wrong from a datatype point of view. How about we change all three occurrences as boris list above into VIR_TRISTATE_SWITCH_ABSENT. Sure, that's exactly what I suggested :) IMHO, we should just do what Michael suggested right at the start of this
On Thu, Aug 11, 2016 at 10:55:44AM +0200, Andrea Bolognani wrote: thread and use {0}, instead of manually initializing each field to 0, or a constant hiding the 0.
That maybe change back again in the future, if this struct add a new member or current member need to be not zero-initialized.
Lets do what makes sense now, not in some hypothetical future that may never happen.
{0} will initialize the whole struct to zeroes as if you did memset(&s, 0, sizeof(s)); and structures in libvirt are designed so that all zeroes are defaults unless there are virStructNew() functions that properly initialize the non-zero values. So this will work as long as this structure is not the exception (in which case we need to redo all the initializations anyway). That's why {0} is the nicest solution, IMHO. Martin (not Michal ;) )
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 8/11/2016 9:54 PM, Martin Kletzander wrote:
On Thu, Aug 11, 2016 at 10:46:28AM +0100, Daniel P. Berrange wrote:
On Thu, Aug 11, 2016 at 05:39:06PM +0800, Xian Han Yu wrote:
On 8/11/2016 5:00 PM, Daniel P. Berrange wrote:
On Thu, 2016-08-11 at 14:48 +0800, Xian Han Yu wrote:
> The default is not OFF, though, it's ABSENT :) > In fact, as far as I can tell, OFF isn't ever used explicitly > either for assignment or comparison. And false is plain wrong > from a datatype point of view. How about we change all three occurrences as boris list above into VIR_TRISTATE_SWITCH_ABSENT. Sure, that's exactly what I suggested :) IMHO, we should just do what Michael suggested right at the start of this
On Thu, Aug 11, 2016 at 10:55:44AM +0200, Andrea Bolognani wrote: thread and use {0}, instead of manually initializing each field to 0, or a constant hiding the 0.
That maybe change back again in the future, if this struct add a new member or current member need to be not zero-initialized.
Lets do what makes sense now, not in some hypothetical future that may never happen.
{0} will initialize the whole struct to zeroes as if you did
memset(&s, 0, sizeof(s));
and structures in libvirt are designed so that all zeroes are defaults unless there are virStructNew() functions that properly initialize the non-zero values. So this will work as long as this structure is not the exception (in which case we need to redo all the initializations anyway). That's why {0} is the nicest solution, IMHO.
OK, I understand, we will change these three initializations to {0}. Regards, Xian Han
Martin (not Michal ;) )
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (6)
-
Andrea Bolognani -
Boris Fiuczynski -
Daniel P. Berrange -
Laine Stump -
Martin Kletzander -
Xian Han Yu