[PATCH 1/2] ui/ncurses: Add ui and functions to send url to server

Jeremy Kerr jk at ozlabs.org
Tue Jul 22 19:06:51 EST 2014


Hi Sam,

A few comments, but one general item: would it make sense to call this
"add by URL" instead of "retrieve"? or something reflecting
configuration? (but not to confuse with the system configuration...).


> diff --git a/lib/pb-protocol/pb-protocol.c b/lib/pb-protocol/pb-protocol.c
> index 67e1f9e..b721c2a 100644
> --- a/lib/pb-protocol/pb-protocol.c
> +++ b/lib/pb-protocol/pb-protocol.c
> @@ -286,6 +286,12 @@ int pb_protocol_config_len(const struct config *config)
>  	return len;
>  }
>  
> +int pb_protocol_url_len(const char *url)
> +{
> +	/* url + length field */
> +	return strlen(url) + sizeof(uint32_t);
> +}
> +

Following the other _len functions, this could be:

	return 4 + optional_strlen(url);

- which gives us a little safety for NULL urls.



> @@ -38,6 +39,7 @@ int pb_protocol_boot_len(const struct boot_command *boot);
>  int pb_protocol_boot_status_len(const struct boot_status *status);
>  int pb_protocol_system_info_len(const struct system_info *sysinfo);
>  int pb_protocol_config_len(const struct config *config);
> +int pb_protocol_url_len(const char *url);
>  int pb_protocol_device_cmp(const struct device *a, const struct device *b);
>  
>  int pb_protocol_boot_option_cmp(const struct boot_option *a,
> @@ -59,6 +61,7 @@ int pb_protocol_serialise_system_info(const struct system_info *sysinfo,
>  		char *buf, int buf_len);
>  int pb_protocol_serialise_config(const struct config *config,
>  		char *buf, int buf_len);
> +int pb_protocol_serialise_retrieve(const char *url, char *buf, int buf_len);
>  
>  int pb_protocol_write_message(int fd, struct pb_protocol_message *message);
>  

Would be best to use consistent naming for the _len and _serialise
functions, whatever we end up calling the general action.

> +struct retrieve_screen {
> +	struct nc_scr		scr;
> +	struct cui		*cui;
> +	struct nc_widgetset	*widgetset;
> +	WINDOW			*pad;
> +
> +	bool			exit;
> +	bool			show_help;
> +	void			(*on_exit)(struct cui *);
> +
> +	int			scroll_y;

I don't think we'll need scrolling for this screen; does it simplify the
code much to remove this (along with the _focus code, and the pad)?

Otherwise, looks good!

Cheers,


Jeremy


More information about the Petitboot mailing list