> -----Original Message----- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Wednesday, March 27, 2019 7:24 PM > To: Xin Li (Talons) <xin...@citrix.com> > Cc: Andrew Cooper <andrew.coop...@citrix.com>; Igor Druzhinin > <igor.druzhi...@citrix.com>; Sergey Dyasli <sergey.dya...@citrix.com>; Xin Li > <talons....@gmail.com>; xen-de...@lists.xen.org > Subject: RE: [Xen-devel] [PATCH v1 1/1] hvmloader: allow overriding SMBIOS > type 2 info > >> > - /* Only present when passed in */ > >> > + s = xenstore_read(HVM_XS_BASEBOARD_MANUFACTURER, NULL); > >> > + if ( (s != NULL) && (*s != '\0') ) > >> > >> Is it really a good idea to key everything else off of the presence > >> of this one string in xenstore? Shouldn't it rather be that the > >> structure gets instantiated whenever any of the strings are there? > > OK. > > I wanted to avoid structure without manufacturer, the app we met only > > read this field. > > > > Can I iterate the 6 keys twice? > > first iteration to decide if any string is provided, the second > > iteration to initialize structure. > > I'd suggest to avoid this. Fill the structure without the surrounding if(), > and > simply determine the function's return values based on whether counter is > non-zero. OK. Other structs can override this header.
> >> > --- a/xen/include/public/hvm/hvm_xs_strings.h > >> > +++ b/xen/include/public/hvm/hvm_xs_strings.h > >> > @@ -62,18 +62,24 @@ > >> > /* The following xenstore values are used to override some of the > default > >> > * string values in the SMBIOS table constructed in hvmloader. > >> > */ > >> > -#define HVM_XS_BIOS_STRINGS "bios-strings" > >> > -#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor" > >> > -#define HVM_XS_BIOS_VERSION "bios-strings/bios-version" > >> > -#define HVM_XS_SYSTEM_MANUFACTURER "bios-strings/system- > >> manufacturer" > >> > -#define HVM_XS_SYSTEM_PRODUCT_NAME "bios-strings/system- > >> product-name" > >> > -#define HVM_XS_SYSTEM_VERSION "bios-strings/system- > version" > >> > -#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios-strings/system- > serial- > >> number" > >> > -#define HVM_XS_ENCLOSURE_MANUFACTURER "bios- > strings/enclosure- > >> manufacturer" > >> > -#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios- > strings/enclosure- > >> serial-number" > >> > -#define HVM_XS_ENCLOSURE_ASSET_TAG "bios-strings/enclosure- > >> asset-tag" > >> > -#define HVM_XS_BATTERY_MANUFACTURER "bios-strings/battery- > >> manufacturer" > >> > -#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery- > >> device-name" > >> > +#define HVM_XS_BIOS_STRINGS "bios-strings" > >> > +#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor" > >> > +#define HVM_XS_BIOS_VERSION "bios-strings/bios-version" > >> > +#define HVM_XS_SYSTEM_MANUFACTURER "bios- > strings/system- > >> manufacturer" > >> > +#define HVM_XS_SYSTEM_PRODUCT_NAME "bios- > strings/system- > >> product-name" > >> > +#define HVM_XS_SYSTEM_VERSION "bios-strings/system- > >> version" > >> > +#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios- > strings/system- > >> serial-number" > >> > +#define HVM_XS_BASEBOARD_MANUFACTURER "bios- > >> strings/baseboard-manufacturer" > >> > +#define HVM_XS_BASEBOARD_PRODUCT_NAME "bios- > >> strings/baseboard-product-name" > >> > +#define HVM_XS_BASEBOARD_VERSION "bios- > strings/baseboard- > >> version" > >> > +#define HVM_XS_BASEBOARD_SERIAL_NUMBER "bios- > >> strings/baseboard-serial-number" > >> > +#define HVM_XS_BASEBOARD_ASSET_TAG "bios- > >> strings/baseboard-asset-tag" > >> > +#define HVM_XS_BASEBOARD_LOCATION_IN_CHASSIS "bios- > >> strings/baseboard-location-in-chassis" > >> > +#define HVM_XS_ENCLOSURE_MANUFACTURER "bios- > >> strings/enclosure-manufacturer" > >> > +#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios- > >> strings/enclosure-serial-number" > >> > +#define HVM_XS_ENCLOSURE_ASSET_TAG "bios- > strings/enclosure- > >> asset-tag" > >> > +#define HVM_XS_BATTERY_MANUFACTURER "bios- > strings/battery- > >> manufacturer" > >> > +#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery- > >> device-name" > >> > >> To be honest I'd prefer if you avoided the re-formatting, accepting > >> the one definition that then doesn't properly align with the rest. > >> But if others think differently, so be it. > > Can I keep this style? This seems fit current code style. > > I'm afraid I don't understand the question in the light of me having asked to > avoid the re-formatting. Is the question perhaps targeted at others, not me? OK. Then HVM_XS_BASEBOARD_LOCATION_IN_CHASSIS won't align with the rest. > Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel