[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