Anthony, thanks for your explaination.IMO, patch 1  and patch 2 need your detailed review.. IMO the reset patches are good in general..Emil, if patch 1 / patch 2 are reviewed from anthony, could you send out v10? :) i know it's not an easy task, thanks in advence!!
Quan

On Mon, 18 Jul 2016 15:50:27 +0100, anthony.perard<anthony.perard@citrix.com> wrote:> On Sun, Jul 17, 2016 at 03:41:26PM +0800, Quan Xu wrote:
> -int xenstore_write_int(const char *base, const char *node, int ival)
> -{
> -    char val[12];
> -
> [Quan:]: why 12 ? what about XEN_BUFSIZE? 

That is the number of digit in INT_MAX (10) + 1 for the sign + 1 for '\0'.

> -    snprintf(val, sizeof(val), "%d", ival);
> -    return xenstore_write_str(base, node, val);
> -}
> -
> -int xenstore_write_int64(const char *base, const char *node, int64_t ival)
> -{
> -    char val[21];
> -
> [Quan:]: why 21 ? what about XEN_BUFSIZE?

Same with INT64_MAX (19 digits).

> 
> -    snprintf(val, sizeof(val), "%"PRId64, ival);
> -    return xenstore_write_str(base, node, val);
> -}
> -
> -int xenstore_read_int(const char *base, const char *node, int *ival)
> -{
> -    char *val;
> -    int rc = -1;
> -
> -    val = xenstore_read_str(base, node);
> [Quan:]:  IMO, it is better to initialize val when declares.

I think I prefer it this way.

> -    if (val && 1 == sscanf(val, "%d", ival)) {
> -        rc = 0;
> -    }
> -    g_free(val);
> -    return rc;
> -}

-- 
Anthony PERARD