From 15d2f947f51a0ea72e9938f5648fd5133950ecdb Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Wed, 1 May 2013 17:20:39 +0100 Subject: [PATCH] [settings] Eliminate settings "tag magic" Create an explicit concept of "settings scope" and eliminate the magic values used for numerical setting tags. Signed-off-by: Michael Brown --- src/arch/i386/interface/vmware/guestinfo.c | 2 +- src/core/nvo.c | 6 +- src/core/settings.c | 44 ++++--------- src/drivers/net/phantom/phantom.c | 13 ++-- src/include/ipxe/net80211.h | 21 ------ src/include/ipxe/netdevice.h | 32 --------- src/include/ipxe/settings.h | 75 +++++++++++++--------- src/interface/smbios/smbios_settings.c | 32 ++++----- src/net/80211/net80211.c | 3 - src/net/dhcppkt.c | 7 +- src/net/netdev_settings.c | 17 ----- 11 files changed, 82 insertions(+), 170 deletions(-) diff --git a/src/arch/i386/interface/vmware/guestinfo.c b/src/arch/i386/interface/vmware/guestinfo.c index 5e08d947..da013ca2 100644 --- a/src/arch/i386/interface/vmware/guestinfo.c +++ b/src/arch/i386/interface/vmware/guestinfo.c @@ -224,7 +224,7 @@ static int guestinfo_net_probe ( struct net_device *netdev ) { rc = -ENOMEM; goto err_alloc; } - settings_init ( settings, &guestinfo_settings_operations, NULL, 0 ); + settings_init ( settings, &guestinfo_settings_operations, NULL, NULL ); /* Register settings */ if ( ( rc = register_settings ( settings, netdev_settings ( netdev ), diff --git a/src/core/nvo.c b/src/core/nvo.c index 5383fe5c..4c871b2f 100644 --- a/src/core/nvo.c +++ b/src/core/nvo.c @@ -195,7 +195,8 @@ static int nvo_save ( struct nvo_block *nvo ) { int nvo_applies ( struct settings *settings __unused, struct setting *setting ) { - return dhcpopt_applies ( setting->tag ); + return ( ( setting->scope == NULL ) && + dhcpopt_applies ( setting->tag ) ); } /** @@ -274,7 +275,8 @@ void nvo_init ( struct nvo_block *nvo, struct nvs_device *nvs, nvo->len = len; nvo->resize = resize; dhcpopt_init ( &nvo->dhcpopts, NULL, 0, nvo_realloc_dhcpopt ); - settings_init ( &nvo->settings, &nvo_settings_operations, refcnt, 0 ); + settings_init ( &nvo->settings, &nvo_settings_operations, + refcnt, NULL ); } /** diff --git a/src/core/settings.c b/src/core/settings.c index 43715fbd..54d48d32 100644 --- a/src/core/settings.c +++ b/src/core/settings.c @@ -984,7 +984,7 @@ void clear_settings ( struct settings *settings ) { int setting_cmp ( struct setting *a, struct setting *b ) { /* If the settings have tags, compare them */ - if ( a->tag && ( a->tag == b->tag ) ) + if ( a->tag && ( a->tag == b->tag ) && ( a->scope == b->scope ) ) return 0; /* Otherwise, if the settings have names, compare them */ @@ -1099,19 +1099,17 @@ struct setting * find_setting ( const char *name ) { /** * Parse setting name as tag number * - * @v settings Settings block * @v name Name * @ret tag Tag number, or 0 if not a valid number */ -static unsigned int parse_setting_tag ( struct settings *settings, - const char *name ) { +static unsigned int parse_setting_tag ( const char *name ) { char *tmp = ( ( char * ) name ); unsigned int tag = 0; while ( 1 ) { tag = ( ( tag << 8 ) | strtoul ( tmp, &tmp, 0 ) ); if ( *tmp == 0 ) - return ( tag | settings->tag_magic ); + return tag; if ( *tmp != '.' ) return 0; tmp++; @@ -1193,7 +1191,8 @@ parse_setting_name ( const char *name, } /* Identify setting */ - setting->tag = parse_setting_tag ( *settings, setting_name ); + setting->tag = parse_setting_tag ( setting_name ); + setting->scope = (*settings)->default_scope; setting->name = setting_name; for_each_table_entry ( named_setting, SETTINGS ) { /* Matches a defined named setting; use that setting */ @@ -1998,26 +1997,15 @@ struct builtin_setting_operation { int ( * fetch ) ( void *data, size_t len ); }; -/** Built-in setting tag magic */ -#define BUILTIN_SETTING_TAG_MAGIC 0xb1 - -/** - * Construct built-in setting tag - * - * @v id Unique identifier - * @ret tag Setting tag - */ -#define BUILTIN_SETTING_TAG( id ) ( ( BUILTIN_SETTING_TAG_MAGIC << 24 ) | (id) ) - -/** "errno" setting tag */ -#define BUILTIN_SETTING_TAG_ERRNO BUILTIN_SETTING_TAG ( 0x01 ) +/** Built-in setting scope */ +static struct settings_scope builtin_scope; /** Error number setting */ struct setting errno_setting __setting ( SETTING_MISC ) = { .name = "errno", .description = "Last error", - .tag = BUILTIN_SETTING_TAG_ERRNO, .type = &setting_type_uint32, + .scope = &builtin_scope, }; /** @@ -2038,15 +2026,12 @@ static int errno_fetch ( void *data, size_t len ) { return sizeof ( content ); } -/** "buildarch" setting tag */ -#define BUILTIN_SETTING_TAG_BUILDARCH BUILTIN_SETTING_TAG ( 0x02 ) - /** Build architecture setting */ struct setting buildarch_setting __setting ( SETTING_MISC ) = { .name = "buildarch", .description = "Build architecture", - .tag = BUILTIN_SETTING_TAG_BUILDARCH, .type = &setting_type_string, + .scope = &builtin_scope, }; /** @@ -2063,15 +2048,12 @@ static int buildarch_fetch ( void *data, size_t len ) { return ( sizeof ( buildarch ) - 1 /* NUL */ ); } -/** "platform" setting tag */ -#define BUILTIN_SETTING_TAG_PLATFORM BUILTIN_SETTING_TAG ( 0x03 ) - /** Platform setting */ struct setting platform_setting __setting ( SETTING_MISC ) = { .name = "platform", .description = "Platform", - .tag = BUILTIN_SETTING_TAG_PLATFORM, .type = &setting_type_string, + .scope = &builtin_scope, }; /** @@ -2128,11 +2110,8 @@ static int builtin_fetch ( struct settings *settings __unused, */ static int builtin_applies ( struct settings *settings __unused, struct setting *setting ) { - unsigned int tag_magic; - /* Check tag magic */ - tag_magic = ( setting->tag >> 24 ); - return ( tag_magic == BUILTIN_SETTING_TAG_MAGIC ); + return ( setting->scope == &builtin_scope ); } /** Built-in settings operations */ @@ -2144,7 +2123,6 @@ static struct settings_operations builtin_settings_operations = { /** Built-in settings */ static struct settings builtin_settings = { .refcnt = NULL, - .tag_magic = BUILTIN_SETTING_TAG ( 0 ), .siblings = LIST_HEAD_INIT ( builtin_settings.siblings ), .children = LIST_HEAD_INIT ( builtin_settings.children ), .op = &builtin_settings_operations, diff --git a/src/drivers/net/phantom/phantom.c b/src/drivers/net/phantom/phantom.c index 1c798e04..7f2fe0b6 100644 --- a/src/drivers/net/phantom/phantom.c +++ b/src/drivers/net/phantom/phantom.c @@ -1454,11 +1454,8 @@ static struct net_device_operations phantom_operations = { * */ -/** Phantom CLP settings tag magic */ -#define PHN_CLP_TAG_MAGIC 0xc19c1900UL - -/** Phantom CLP settings tag magic mask */ -#define PHN_CLP_TAG_MAGIC_MASK 0xffffff00UL +/** Phantom CLP settings scope */ +static struct settings_scope phantom_settings_scope; /** Phantom CLP data * @@ -1689,8 +1686,8 @@ phantom_clp_setting ( struct phantom_nic *phantom, struct setting *setting ) { } /* Allow for use of numbered settings */ - if ( ( setting->tag & PHN_CLP_TAG_MAGIC_MASK ) == PHN_CLP_TAG_MAGIC ) - return ( setting->tag & ~PHN_CLP_TAG_MAGIC_MASK ); + if ( setting->scope == &phantom_settings_scope ) + return setting->tag; DBGC2 ( phantom, "Phantom %p has no \"%s\" setting\n", phantom, setting->name ); @@ -2075,7 +2072,7 @@ static int phantom_probe ( struct pci_device *pci ) { assert ( phantom->port < PHN_MAX_NUM_PORTS ); settings_init ( &phantom->settings, &phantom_settings_operations, - &netdev->refcnt, PHN_CLP_TAG_MAGIC ); + &netdev->refcnt, &phantom_settings_scope ); /* Fix up PCI device */ adjust_pci_device ( pci ); diff --git a/src/include/ipxe/net80211.h b/src/include/ipxe/net80211.h index 771872c8..dd8bd228 100644 --- a/src/include/ipxe/net80211.h +++ b/src/include/ipxe/net80211.h @@ -1183,25 +1183,4 @@ static inline u16 net80211_cts_duration ( struct net80211_device *dev, net80211_duration ( dev, size, dev->rates[dev->rate] ) ); } -/** 802.11 device setting tag magic */ -#define NET80211_SETTING_TAG_MAGIC 0x8211 - -/** - * Construct 802.11 setting tag - * - * @v id Unique identifier - * @ret tag Setting tag - */ -#define NET80211_SETTING_TAG( id ) \ - NETDEV_SETTING_TAG ( ( NET80211_SETTING_TAG_MAGIC << 8 ) | (id) ) - -/** SSID setting tag */ -#define NET80211_SETTING_TAG_SSID NET80211_SETTING_TAG ( 0x01 ) - -/** Active scanning setting tag */ -#define NET80211_SETTING_TAG_ACTIVE_SCAN NET80211_SETTING_TAG ( 0x02 ) - -/** Wireless key setting tag */ -#define NET80211_SETTING_TAG_KEY NET80211_SETTING_TAG ( 0x03 ) - #endif diff --git a/src/include/ipxe/netdevice.h b/src/include/ipxe/netdevice.h index 21754fe6..0daa1d62 100644 --- a/src/include/ipxe/netdevice.h +++ b/src/include/ipxe/netdevice.h @@ -411,38 +411,6 @@ struct net_driver { /** Declare a network driver */ #define __net_driver __table_entry ( NET_DRIVERS, 01 ) -/** Network device setting tag magic - * - * All DHCP option settings are deemed to be valid as network device - * settings. There are also some extra non-DHCP settings (such as - * "mac"), which are marked as being valid network device settings by - * using a magic tag value. - */ -#define NETDEV_SETTING_TAG_MAGIC 0xeb - -/** - * Construct network device setting tag - * - * @v id Unique identifier - * @ret tag Setting tag - */ -#define NETDEV_SETTING_TAG( id ) ( ( NETDEV_SETTING_TAG_MAGIC << 24 ) | (id) ) - -/** - * Check if tag is a network device setting tag - * - * @v tag Setting tag - * @ret is_ours Tag is a network device setting tag - */ -#define IS_NETDEV_SETTING_TAG( tag ) \ - ( ( (tag) >> 24 ) == NETDEV_SETTING_TAG_MAGIC ) - -/** MAC address setting tag */ -#define NETDEV_SETTING_TAG_MAC NETDEV_SETTING_TAG ( 0x01 ) - -/** Bus ID setting tag */ -#define NETDEV_SETTING_TAG_BUS_ID NETDEV_SETTING_TAG ( 0x02 ) - extern struct list_head net_devices; extern struct net_device_operations null_netdev_operations; extern struct settings_operations netdev_settings_operations; diff --git a/src/include/ipxe/settings.h b/src/include/ipxe/settings.h index 3f887602..9b7404ac 100644 --- a/src/include/ipxe/settings.h +++ b/src/include/ipxe/settings.h @@ -38,27 +38,14 @@ struct setting { * The setting tag is a numerical description of the setting * (such as a DHCP option number, or an SMBIOS structure and * field number). - * - * Users can construct tags for settings that are not - * explicitly known to iPXE using the generic syntax for - * numerical settings. For example, the setting name "60" - * will be interpreted as referring to DHCP option 60 (the - * vendor class identifier). - * - * This creates a potential for namespace collisions, since - * the interpretation of the numerical description will vary - * according to the settings block. When a user attempts to - * fetch a generic numerical setting, we need to ensure that - * only the intended settings block interprets the numerical - * description. (For example, we do not want to attempt to - * retrieve the subnet mask from SMBIOS, or the system UUID - * from DHCP.) - * - * This potential problem is resolved by allowing the setting - * tag to include a "magic" value indicating the - * interpretation to be placed upon the numerical description. */ unsigned int tag; + /** Setting scope (or NULL) + * + * For historic reasons, a NULL scope with a non-zero tag + * indicates a DHCPv4 option setting. + */ + struct settings_scope *scope; }; /** Configuration setting table */ @@ -134,12 +121,6 @@ struct settings { struct refcnt *refcnt; /** Name */ const char *name; - /** Tag magic - * - * This value will be ORed in to any numerical tags - * constructed by parse_setting_name(). - */ - unsigned int tag_magic; /** Parent settings block */ struct settings *parent; /** Sibling settings blocks */ @@ -148,8 +129,44 @@ struct settings { struct list_head children; /** Settings block operations */ struct settings_operations *op; + /** Default scope for numerical settings constructed for this block */ + struct settings_scope *default_scope; }; +/** + * A setting scope + * + * Users can construct tags for settings that are not explicitly known + * to iPXE using the generic syntax for numerical settings. For + * example, the setting name "60" will be interpreted as referring to + * DHCP option 60 (the vendor class identifier). + * + * This creates a potential for namespace collisions, since the + * interpretation of the numerical description will vary according to + * the settings block. When a user attempts to fetch a generic + * numerical setting, we need to ensure that only the intended + * settings blocks interpret this numerical description. (For + * example, we do not want to attempt to retrieve the subnet mask from + * SMBIOS, or the system UUID from DHCP.) + * + * This potential problem is resolved by including a user-invisible + * "scope" within the definition of each setting. Settings blocks may + * use this to determine whether or not the setting is applicable. + * Any settings constructed from a numerical description + * (e.g. "smbios/1.4.0") will be assigned the default scope of the + * settings block specified in the description (e.g. "smbios"); this + * provides behaviour matching the user's expectations in most + * circumstances. + */ +struct settings_scope { + /** Dummy field + * + * This is included only to ensure that pointers to different + * scopes always compare differently. + */ + uint8_t dummy; +} __attribute__ (( packed )); + /** * A setting type * @@ -329,17 +346,17 @@ extern struct setting busid_setting __setting ( SETTING_NETDEV ); * @v settings Settings block * @v op Settings block operations * @v refcnt Containing object reference counter, or NULL - * @v tag_magic Tag magic + * @v default_scope Default scope */ static inline void settings_init ( struct settings *settings, struct settings_operations *op, struct refcnt *refcnt, - unsigned int tag_magic ) { + struct settings_scope *default_scope ) { INIT_LIST_HEAD ( &settings->siblings ); INIT_LIST_HEAD ( &settings->children ); settings->op = op; settings->refcnt = refcnt; - settings->tag_magic = tag_magic; + settings->default_scope = default_scope; } /** @@ -351,7 +368,7 @@ static inline void settings_init ( struct settings *settings, static inline void generic_settings_init ( struct generic_settings *generics, struct refcnt *refcnt ) { settings_init ( &generics->settings, &generic_settings_operations, - refcnt, 0 ); + refcnt, NULL ); INIT_LIST_HEAD ( &generics->list ); } diff --git a/src/interface/smbios/smbios_settings.c b/src/interface/smbios/smbios_settings.c index 663da968..7b2efcd7 100644 --- a/src/interface/smbios/smbios_settings.c +++ b/src/interface/smbios/smbios_settings.c @@ -27,15 +27,8 @@ FILE_LICENCE ( GPL2_OR_LATER ); #include #include -/** SMBIOS settings tag magic number */ -#define SMBIOS_TAG_MAGIC 0x5B /* "SmBios" */ - -/** - * Construct SMBIOS empty tag - * - * @ret tag SMBIOS setting tag - */ -#define SMBIOS_EMPTY_TAG ( SMBIOS_TAG_MAGIC << 24 ) +/** SMBIOS settings scope */ +static struct settings_scope smbios_settings_scope; /** * Construct SMBIOS raw-data tag @@ -46,8 +39,7 @@ FILE_LICENCE ( GPL2_OR_LATER ); * @ret tag SMBIOS setting tag */ #define SMBIOS_RAW_TAG( _type, _structure, _field ) \ - ( ( SMBIOS_TAG_MAGIC << 24 ) | \ - ( (_type) << 16 ) | \ + ( ( (_type) << 16 ) | \ ( offsetof ( _structure, _field ) << 8 ) | \ ( sizeof ( ( ( _structure * ) 0 )->_field ) ) ) @@ -60,8 +52,7 @@ FILE_LICENCE ( GPL2_OR_LATER ); * @ret tag SMBIOS setting tag */ #define SMBIOS_STRING_TAG( _type, _structure, _field ) \ - ( ( SMBIOS_TAG_MAGIC << 24 ) | \ - ( (_type) << 16 ) | \ + ( ( (_type) << 16 ) | \ ( offsetof ( _structure, _field ) << 8 ) ) /** @@ -73,11 +64,8 @@ FILE_LICENCE ( GPL2_OR_LATER ); */ static int smbios_applies ( struct settings *settings __unused, struct setting *setting ) { - unsigned int tag_magic; - /* Check tag magic */ - tag_magic = ( setting->tag >> 24 ); - return ( tag_magic == SMBIOS_TAG_MAGIC ); + return ( setting->scope == &smbios_settings_scope ); } /** @@ -93,18 +81,15 @@ static int smbios_fetch ( struct settings *settings __unused, struct setting *setting, void *data, size_t len ) { struct smbios_structure structure; - unsigned int tag_magic; unsigned int tag_type; unsigned int tag_offset; unsigned int tag_len; int rc; /* Split tag into type, offset and length */ - tag_magic = ( setting->tag >> 24 ); tag_type = ( ( setting->tag >> 16 ) & 0xff ); tag_offset = ( ( setting->tag >> 8 ) & 0xff ); tag_len = ( setting->tag & 0xff ); - assert ( tag_magic == SMBIOS_TAG_MAGIC ); /* Find SMBIOS structure */ if ( ( rc = find_smbios_structure ( tag_type, &structure ) ) != 0 ) @@ -170,10 +155,10 @@ static struct settings_operations smbios_settings_operations = { /** SMBIOS settings */ static struct settings smbios_settings = { .refcnt = NULL, - .tag_magic = SMBIOS_EMPTY_TAG, .siblings = LIST_HEAD_INIT ( smbios_settings.siblings ), .children = LIST_HEAD_INIT ( smbios_settings.children ), .op = &smbios_settings_operations, + .default_scope = &smbios_settings_scope, }; /** Initialise SMBIOS settings */ @@ -200,6 +185,7 @@ struct setting uuid_setting __setting ( SETTING_HOST ) = { .tag = SMBIOS_RAW_TAG ( SMBIOS_TYPE_SYSTEM_INFORMATION, struct smbios_system_information, uuid ), .type = &setting_type_uuid, + .scope = &smbios_settings_scope, }; /** Other SMBIOS named settings */ @@ -211,6 +197,7 @@ struct setting smbios_named_settings[] __setting ( SETTING_HOST_EXTRA ) = { struct smbios_system_information, manufacturer ), .type = &setting_type_string, + .scope = &smbios_settings_scope, }, { .name = "product", @@ -219,6 +206,7 @@ struct setting smbios_named_settings[] __setting ( SETTING_HOST_EXTRA ) = { struct smbios_system_information, product ), .type = &setting_type_string, + .scope = &smbios_settings_scope, }, { .name = "serial", @@ -227,6 +215,7 @@ struct setting smbios_named_settings[] __setting ( SETTING_HOST_EXTRA ) = { struct smbios_system_information, serial ), .type = &setting_type_string, + .scope = &smbios_settings_scope, }, { .name = "asset", @@ -235,5 +224,6 @@ struct setting smbios_named_settings[] __setting ( SETTING_HOST_EXTRA ) = { struct smbios_enclosure_information, asset_tag ), .type = &setting_type_string, + .scope = &smbios_settings_scope, }, }; diff --git a/src/net/80211/net80211.c b/src/net/80211/net80211.c index 2181fc4a..54df7905 100644 --- a/src/net/80211/net80211.c +++ b/src/net/80211/net80211.c @@ -208,7 +208,6 @@ struct setting net80211_ssid_setting __setting ( SETTING_NETDEV_EXTRA ) = { .name = "ssid", .description = "Wireless SSID", .type = &setting_type_string, - .tag = NET80211_SETTING_TAG_SSID, }; /** Whether to use active scanning @@ -221,7 +220,6 @@ struct setting net80211_active_setting __setting ( SETTING_NETDEV_EXTRA ) = { .name = "active-scan", .description = "Actively scan for wireless networks", .type = &setting_type_int8, - .tag = NET80211_SETTING_TAG_ACTIVE_SCAN, }; /** The cryptographic key to use @@ -234,7 +232,6 @@ struct setting net80211_key_setting __setting ( SETTING_NETDEV_EXTRA ) = { .name = "key", .description = "Wireless encryption key", .type = &setting_type_string, - .tag = NET80211_SETTING_TAG_KEY, }; /** @} */ diff --git a/src/net/dhcppkt.c b/src/net/dhcppkt.c index 528f9003..3722c09e 100644 --- a/src/net/dhcppkt.c +++ b/src/net/dhcppkt.c @@ -230,7 +230,8 @@ static int dhcppkt_settings_applies ( struct settings *settings, struct dhcp_packet *dhcppkt = container_of ( settings, struct dhcp_packet, settings ); - return dhcppkt_applies ( dhcppkt, setting->tag ); + return ( ( setting->scope == NULL ) && + dhcppkt_applies ( dhcppkt, setting->tag ) ); } /** @@ -299,6 +300,6 @@ void dhcppkt_init ( struct dhcp_packet *dhcppkt, struct dhcphdr *data, dhcpopt_init ( &dhcppkt->options, &dhcppkt->dhcphdr->options, ( len - offsetof ( struct dhcphdr, options ) ), dhcpopt_no_realloc ); - settings_init ( &dhcppkt->settings, - &dhcppkt_settings_operations, &dhcppkt->refcnt, 0 ); + settings_init ( &dhcppkt->settings, &dhcppkt_settings_operations, + &dhcppkt->refcnt, NULL ); } diff --git a/src/net/netdev_settings.c b/src/net/netdev_settings.c index 9efe6811..8758e980 100644 --- a/src/net/netdev_settings.c +++ b/src/net/netdev_settings.c @@ -39,29 +39,13 @@ struct setting mac_setting __setting ( SETTING_NETDEV ) = { .name = "mac", .description = "MAC address", .type = &setting_type_hex, - .tag = NETDEV_SETTING_TAG_MAC, }; struct setting busid_setting __setting ( SETTING_NETDEV ) = { .name = "busid", .description = "Bus ID", .type = &setting_type_hex, - .tag = NETDEV_SETTING_TAG_BUS_ID, }; -/** - * Check applicability of network device setting - * - * @v settings Settings block - * @v setting Setting - * @ret applies Setting applies within this settings block - */ -static int netdev_applies ( struct settings *settings __unused, - struct setting *setting ) { - - return ( IS_NETDEV_SETTING_TAG ( setting->tag ) || - dhcpopt_applies ( setting->tag ) ); -} - /** * Store value of network device setting * @@ -134,7 +118,6 @@ static void netdev_clear ( struct settings *settings ) { /** Network device configuration settings operations */ struct settings_operations netdev_settings_operations = { - .applies = netdev_applies, .store = netdev_store, .fetch = netdev_fetch, .clear = netdev_clear,