| Message ID | 20151022094330.GD9079@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 22 October 2015 at 10:43, Daniel P. Berrange <berrange@redhat.com> wrote: > For reasons I don't understand, it appears that even string > fields which are marked as optional get a 'has_XXX' flag, > to distinguish betweeen a NULL string and an unset string. > I struggle to imagine why we need this. It makes sense for > integer/boolean types, but I would have thought just checking > for NULL was sufficient for string types. I think the argument here is pure consistency. *Every* optional field should be implemented as has_fieldname (bool) plus fieldname (actual value). Special casing string types because we happen to have a special value we can use to indicate 'no string' seems to me like it would cause more confusion than it would be worth. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 22 October 2015 at 10:43, Daniel P. Berrange <berrange@redhat.com> wrote: >> For reasons I don't understand, it appears that even string >> fields which are marked as optional get a 'has_XXX' flag, >> to distinguish betweeen a NULL string and an unset string. >> I struggle to imagine why we need this. It makes sense for >> integer/boolean types, but I would have thought just checking >> for NULL was sufficient for string types. > > I think the argument here is pure consistency. *Every* optional > field should be implemented as has_fieldname (bool) plus fieldname > (actual value). Special casing string types because we happen > to have a special value we can use to indicate 'no string' > seems to me like it would cause more confusion than it would > be worth. Having a has_FOO for pointer-valued FOOs also causes confusion, because it begs questions like "does has_FOO imply FOO != NULL?" and "if !has_FOO, do I have to worry about freeing FOO?". I believe we have these has_FOOs not because someone tried to minimize confusion, but due to laziness: it was simpler to blindly generate the has_FOO for every FOO. Since I don't think distinguishing "pointer argument omitted" from "pointer argument is null" makes sense, I want the has_FOO killed for pointer FOOs. Perhaps it's even in the QAPI review queue already, which Eric keeps filling faster than I can drain it :)
On Thu, Oct 22, 2015 at 02:10:26PM +0200, Markus Armbruster wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > > > On 22 October 2015 at 10:43, Daniel P. Berrange <berrange@redhat.com> wrote: > >> For reasons I don't understand, it appears that even string > >> fields which are marked as optional get a 'has_XXX' flag, > >> to distinguish betweeen a NULL string and an unset string. > >> I struggle to imagine why we need this. It makes sense for > >> integer/boolean types, but I would have thought just checking > >> for NULL was sufficient for string types. > > > > I think the argument here is pure consistency. *Every* optional > > field should be implemented as has_fieldname (bool) plus fieldname > > (actual value). Special casing string types because we happen > > to have a special value we can use to indicate 'no string' > > seems to me like it would cause more confusion than it would > > be worth. > > Having a has_FOO for pointer-valued FOOs also causes confusion, because > it begs questions like "does has_FOO imply FOO != NULL?" and "if > !has_FOO, do I have to worry about freeing FOO?". > > I believe we have these has_FOOs not because someone tried to minimize > confusion, but due to laziness: it was simpler to blindly generate the > has_FOO for every FOO. > > Since I don't think distinguishing "pointer argument omitted" from > "pointer argument is null" makes sense, I want the has_FOO killed for > pointer FOOs. Perhaps it's even in the QAPI review queue already, which > Eric keeps filling faster than I can drain it :) Yep, Eric just replied to my patch for this to the effect that he's planning to fix this for 2.6. So we can just do this trivial revert for 2.5 and fix it properly in 2.6 Regards, Daniel
On 10/22/2015 06:10 AM, Markus Armbruster wrote: > > I believe we have these has_FOOs not because someone tried to minimize > confusion, but due to laziness: it was simpler to blindly generate the > has_FOO for every FOO. > > Since I don't think distinguishing "pointer argument omitted" from > "pointer argument is null" makes sense, I want the has_FOO killed for > pointer FOOs. Perhaps it's even in the QAPI review queue already, which > Eric keeps filling faster than I can drain it :) It's not there yet (so it's missed 2.5), but it IS on my plate for 2.6, to add a per-struct attribute on whether the struct prefers has_FOO vs FOO==NULL for marking optional strings/objects (has_FOO will remain mandatory for optional numbers). Per-struct attribute, because there are too many structs to change in one patch, although if I scrub the entire tree, we may remove the attribute at the end of the conversion.
diff --git a/qapi-schema.json b/qapi-schema.json index f60be29..c613dfd 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2615,8 +2615,6 @@ # @host: host part of the address # # @port: port part of the address, or lowest port if @to is present. -# Kernel selects a free port if omitted for listener addresses. -# #optional # # @to: highest port to try # @@ -2631,7 +2629,7 @@ { 'struct': 'InetSocketAddress', 'data': { 'host': 'str', - '*port': 'str', + 'port': 'str', '*to': 'uint16', '*ipv4': 'bool', '*ipv6': 'bool' } }