Re: [PATCH 04/17] p2p: Add peer WSC device type properties

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6829 bytes --]

Hi Andrew,

On 4/3/20 11:14 AM, Andrew Zaborowski wrote:
> ---
>   src/p2p.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 208 insertions(+)
> 
> diff --git a/src/p2p.c b/src/p2p.c
> index 957482eb..49111a98 100644
> --- a/src/p2p.c
> +++ b/src/p2p.c
> @@ -68,10 +68,156 @@ struct p2p_peer {
>   	struct scan_bss *bss;
>   	struct p2p_device *dev;
>   	char *name;
> +	struct wsc_primary_device_type primary_device_type;
>   };
>   
>   static struct l_queue *p2p_device_list;
>   
> +struct device_type_category_info {
> +	char *category_str;

const char?

> +	unsigned int subcategory_max;
> +	char **subcategory_str;

const char **?

> +};
> +
> +/* Wi-Fi Simple Configuration Technical Specification v2.0.5 Table 41 */
> +struct device_type_category_info device_type_categories[] = {
> +	[1] = {
> +		"Computer",
> +		10,
> +		(char *[]) {
> +			[1] = "PC",
> +			[2] = "Server",
> +			[3] = "Media Center",
> +			[4] = "Ultra-mobile PC",
> +			[5] = "Notebook",
> +			[6] = "Desktop",
> +			[7] = "Mobile Internet Device",
> +			[8] = "Netbook",
> +			[9] = "Tablet",
> +			[10] = "Ultrabook",
> +		},
> +	},
> +	[2] = {
> +		"Input Device",
> +		9,
> +		(char *[]) {
> +			[1] = "Keyboard",
> +			[2] = "Mouse",
> +			[3] = "Joystick",
> +			[4] = "Trackball",
> +			[5] = "Gaming controller",
> +			[6] = "Remote",
> +			[7] = "Touchscreen",
> +			[8] = "Biometric reader",
> +			[9] = "Barcode reader",
> +		},
> +	},
> +	[3] = {
> +		"Printer or Scanner",
> +		5,
> +		(char *[]) {
> +			[1] = "Printer/Print Server",
> +			[2] = "Scanner",
> +			[3] = "Fax",
> +			[4] = "Copier",
> +			[5] = "All-in-one printer, scanner, fax, copier",
> +		},
> +	},
> +	[4] = {
> +		"Camera",
> +		4,
> +		(char *[]) {
> +			[1] = "Digital Still Camera",
> +			[2] = "Video Camera",
> +			[3] = "Web Camera",
> +			[4] = "Security Camera",
> +		},
> +	},
> +	[5] = {
> +		"Storage",
> +		1,
> +		(char *[]) {
> +			[1] = "NAS",
> +		},
> +	},
> +	[6] = {
> +		"Network Infrastructure",
> +		5,
> +		(char *[]) {
> +			[1] = "AP",
> +			[2] = "Router",
> +			[3] = "Switch",
> +			[4] = "Gateway",
> +			[5] = "Bridge",
> +		},
> +	},
> +	[7] = {
> +		"Display",
> +		4,
> +		(char *[]) {
> +			[1] = "Television",
> +			[2] = "Electronic Picture Frame",
> +			[3] = "Projector",
> +			[4] = "Monitor",
> +		},
> +	},
> +	[8] = {
> +		"Multimedia Device",
> +		6,
> +		(char *[]) {
> +			[1] = "DAR",
> +			[2] = "PVR",
> +			[3] = "MCX",
> +			[4] = "Set-top box",
> +			[5] = "Media Server/Adapter/Extender",
> +			[6] = "Portable Video Player",
> +		},
> +	},
> +	[9] = {
> +		"Gaming device",
> +		5,
> +		(char *[]) {
> +			[1] = "Xbox",
> +			[2] = "Xbox360",
> +			[3] = "Playstation",
> +			[4] = "Game Console/Console Adapter",
> +			[5] = "Portable Gaming Device",
> +		},
> +	},
> +	[10] = {
> +		"Telephone",
> +		5,
> +		(char *[]) {
> +			[1] = "Windows Mobile",
> +			[2] = "Single mode phone",
> +			[3] = "Dual mode phone",
> +			[4] = "Single mode smartphone",
> +			[5] = "Dual mode smartphone",
> +		},
> +	},
> +	[11] = {
> +		"Audio Device",
> +		7,
> +		(char *[]) {
> +			[1] = "Audio tuner/receiver",
> +			[2] = "Speakers",
> +			[3] = "Portable Music Player",
> +			[4] = "Headset",
> +			[5] = "Headphones",
> +			[6] = "Microphone",
> +			[7] = "Home Theater System",
> +		},
> +	},
> +	[12] = {
> +		"Docking Device",
> +		2,
> +		(char *[]) {
> +			[1] = "Computer docking station",
> +			[2] = "Media kiosk",
> +		},
> +	},
> +};
> +

So two things I see here:

1. Does this belong in wscutil.c instead?
2. Your intent seems to be to expose these as D-Bus API values.  If so, 
then this gets a bit tricky.  Our D-Bus APIs do not support translation. 
  This is by design, hence all values need to be defined in 
doc/foo-api.txt.  The values must also be all lower case.

>   static bool p2p_device_match(const void *a, const void *b)
>   {
>   	const struct p2p_device *dev = a;
> @@ -301,6 +447,64 @@ static bool p2p_peer_get_name(struct l_dbus *dbus,
>   	return true;
>   }
>   
> +static bool p2p_peer_get_category(struct l_dbus *dbus,
> +					struct l_dbus_message *message,
> +					struct l_dbus_message_builder *builder,
> +					void *user_data)
> +{
> +	struct p2p_peer *peer = user_data;
> +	uint16_t category_id = peer->primary_device_type.category;
> +	const char *category = NULL;
> +
> +	if (category_id == 255)
> +		category = "Other";
> +	else if (category_id < L_ARRAY_SIZE(device_type_categories))
> +		category = device_type_categories[category_id].category_str;
> +
> +	if (!category)
> +		category = "Unknown";
> +
> +	l_dbus_message_builder_append_basic(builder, 's', category);
> +	return true;
> +}
> +
> +static bool p2p_peer_get_subcategory(struct l_dbus *dbus,
> +					struct l_dbus_message *message,
> +					struct l_dbus_message_builder *builder,
> +					void *user_data)
> +{
> +	struct p2p_peer *peer = user_data;
> +	uint16_t category_id = peer->primary_device_type.category;
> +	uint16_t subcategory_id = peer->primary_device_type.subcategory;
> +	const char *subcategory;
> +
> +	/*
> +	 * Should we generate subcategory strings with the numerical
> +	 * values for the subcategories we don't know, such as
> +	 * "Vendor-specific 00:11:22:33 44" ?
> +	 */
> +
> +	if (memcmp(peer->primary_device_type.oui, microsoft_oui, 3) ||
> +			peer->primary_device_type.oui_type != 4)
> +		return false;
> +
> +	if (category_id >= L_ARRAY_SIZE(device_type_categories))
> +		return false;
> +
> +	if (!device_type_categories[category_id].subcategory_str ||
> +			subcategory_id >
> +			device_type_categories[category_id].subcategory_max)
> +		return false;
> +
> +	subcategory = device_type_categories[category_id].
> +		subcategory_str[subcategory_id];
> +	if (!subcategory)
> +		return false;
> +
> +	l_dbus_message_builder_append_basic(builder, 's', subcategory);
> +	return true;
> +}
> +
>   static bool p2p_peer_get_connected(struct l_dbus *dbus,
>   					struct l_dbus_message *message,
>   					struct l_dbus_message_builder *builder,
> @@ -316,6 +520,10 @@ static void p2p_peer_interface_setup(struct l_dbus_interface *interface)
>   {
>   	l_dbus_interface_property(interface, "Name", 0, "s",
>   					p2p_peer_get_name, NULL);
> +	l_dbus_interface_property(interface, "DeviceCategory", 0, "s",
> +					p2p_peer_get_category, NULL);
> +	l_dbus_interface_property(interface, "DeviceSubcategory", 0, "s",
> +					p2p_peer_get_subcategory, NULL);
>   	l_dbus_interface_property(interface, "Connected", 0, "b",
>   					p2p_peer_get_connected, NULL);
>   }
> 

Regards,
-Denis