Re: [Xen-devel] [PATCH v5 RFC 05/14] tools/libxc: noarch common code

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-06-11 at 19:14 +0100, Andrew Cooper wrote:
> +int write_split_record(struct context *ctx, struct record *rec,
> +                       void *buf, size_t sz)
> +{
> +    static const char zeroes[7] = { 0 };
> +    xc_interface *xch = ctx->xch;
> +    uint32_t combined_length = rec->length + sz;
> +    size_t record_length = ROUNDUP(combined_length, REC_ALIGN_ORDER);

I suppose the [7] must relate to REC_ALIGN_ORDER somehow, can you not
derive it? (1<<REC_ALIGN_ORDER perhaps)

> +
> +    if ( record_length > REC_LENGTH_MAX )
> +    {
> +        ERROR("Record (0x%08"PRIx32", %s) length 0x%"PRIx32
> +              " exceeds max (0x%"PRIx32")", rec->type,
> +              rec_type_to_str(rec->type), rec->length, REC_LENGTH_MAX);
> +        return -1;
> +    }
> +
> +    if ( rec->length )
> +        assert(rec->data);
> +    if ( sz )
> +        assert(buf);
> +
> +    if ( write_exact(ctx->fd, &rec->type, sizeof(rec->type)) ||
> +         write_exact(ctx->fd, &combined_length, sizeof(rec->length)) ||
> +         (rec->length && write_exact(ctx->fd, rec->data, rec->length)) ||
> +         (sz && write_exact(ctx->fd, buf, sz)) ||
> +         write_exact(ctx->fd, zeroes, record_length - combined_length) )

For clarity I'd be inclined to split this into 4 separate ifs:
  write type & length
  optionally write data
  optionally write extra buf
  write padding.

either goto err or a context specific PERROR.

> +    {
> +        PERROR("Unable to write record to stream");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +int write_record_header(struct context *ctx, struct record *rec)
> +{
> +    xc_interface *xch = ctx->xch;
> +
> +    if ( write_exact(ctx->fd, &rec->type, sizeof(rec->type)) ||
> +         write_exact(ctx->fd, &rec->length, sizeof(rec->length)) )

No need to round this one up? Or assert that it is already aligned?

> +    {
> +        PERROR("Unable to write record to stream");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  static void __attribute__((unused)) build_assertions(void)
>  {
>      XC_BUILD_BUG_ON(sizeof(struct ihdr) != 24);
> diff --git a/tools/libxc/saverestore/common.h 
> b/tools/libxc/saverestore/common.h
> index cbecf0a..d9a3655 100644
> --- a/tools/libxc/saverestore/common.h
> +++ b/tools/libxc/saverestore/common.h
> @@ -1,7 +1,20 @@
>  #ifndef __COMMON__H
>  #define __COMMON__H
>  
> +#include <stdbool.h>
> +
> +// Hack out junk from the namespace

Do you have a plan to not need these hacks?

> +     * required, the callee should leave '*pages' unouched.

untouched.

> +     * callee enounceters an error, it should *NOT* free() the memory it

encounters.

> + * ensuring that they subseqently write the correct amount of data into the

subsequently.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel