[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