From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v5 RFC 05/14] tools/libxc: noarch common code
Date: Tue, 17 Jun 2014 17:28:59 +0100
Message-ID: <53A06CCB.9010304@citrix.com>
References: <1402510482-21099-1-git-send-email-andrew.cooper3@citrix.com>
	<1402510482-21099-6-git-send-email-andrew.cooper3@citrix.com>
	<1403021435.25771.15.camel@kazak.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1403021435.25771.15.camel@kazak.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <Ian.Campbell@citrix.com>
Cc: Frediano Ziglio <frediano.ziglio@citrix.com>, David Vrabel <david.vrabel@citrix.com>, Xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

On 17/06/14 17:10, Ian Campbell wrote:
> 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)

It does indeed.  I will change it to a calculation.

>
>> +
>> +    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.

Ok.

>
>> +    {
>> +        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?

The comment by the prototype explains that this is for use by "callers
doing complicated records" (i.e. PAGE_INFO) and that the caller is
responsible for ensuring that length is correct.

>
>> +    {
>> +        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?

Not really.  There are enough other areas of libxc which still use these
macros, and I can't go and simply update all other areas as struct
context is meaningless outside of libxc/saverestore.

>
>> +     * 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.
>
>

Noted.

~Andrew