From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:43274)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1Ws8Hi-0005hu-Vc
	for qemu-devel@nongnu.org; Wed, 04 Jun 2014 06:19:14 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1Ws8Hb-0006jV-Go
	for qemu-devel@nongnu.org; Wed, 04 Jun 2014 06:19:06 -0400
Received: from mx1.redhat.com ([209.132.183.28]:61714)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1Ws8Hb-0006jI-6o
	for qemu-devel@nongnu.org; Wed, 04 Jun 2014 06:18:59 -0400
From: Juan Quintela <quintela@redhat.com>
In-Reply-To: <1401863911-5947-3-git-send-email-sanidhya.iiith@gmail.com>
	(Sanidhya Kashyap's message of "Wed, 4 Jun 2014 12:08:25 +0530")
References: <1401863911-5947-1-git-send-email-sanidhya.iiith@gmail.com>
	<1401863911-5947-3-git-send-email-sanidhya.iiith@gmail.com>
Date: Wed, 04 Jun 2014 12:18:56 +0200
Message-ID: <87zjhtrn8v.fsf@troll.troll>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH v2 2/8] bitmap dump code via QAPI framework
Reply-To: quintela@redhat.com
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
Cc: qemu list <qemu-devel@nongnu.org>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Sanidhya Kashyap <sanidhya.iiith@gmail.com> wrote:
> Following are the changes made with respect to the previous version:
> Chen's advice
> 1) Replaced DIRTY_MEMORY_LOG_BITMAP with DIRTY_MEMORY_MIGRATION and
> completely removed the DIRTY_MEMORY_LOG_BITMAP flag.
>
> Eric's advice
> 2) Replaced FILE pointer with file descriptor.
> 3) Replaced fopen/fclose with qemu_open / qemu_close.
> 4) Removed text format, output only in machine-readable format.
> 5) Defined constants.



> +
> +static inline bool check_value(int64_t value, int64_t min_value,
> +                               const char *str, Error **errp)

If you pass min_value, you should also pass max_value, no?

> +{
> +    if (value < min_value) {
> +        error_setg(errp, "%s's value must be greater than %ld",
> +                         str, min_value);
> +        return false;
> +    }
> +    if (value > LOG_SIZE_MAX) {
> +        error_setg(errp, "%s's value must be less than %ld",
> +                         str, LOG_SIZE_MAX);

You use it for more things that LOG, right?

Name of the function can be changed:

   value_in_range()?


> +    g_free(logging_bitmap);
> +    qemu_close(b->fd);
> +    logging_bitmap = NULL;

Change orders of this line with previous one?

> +    b = NULL;

Why?  b is a local variable.
Do you mean:

	b->fd = -1;?

> +     */
> + log_thread_end:
> +    logging_bitmap_close(b);
> +    if (b->state == LOG_BITMAP_STATE_ACTIVE) {
> +        logging_state_set_status(b, LOG_BITMAP_STATE_ACTIVE,
> +                                    LOG_BITMAP_STATE_COMPLETED);
> +    } else if (b->state == LOG_BITMAP_STATE_CANCELING) {
> +        logging_state_set_status(b, LOG_BITMAP_STATE_CANCELING,
> +                                    LOG_BITMAP_STATE_COMPLETED);
> +    } else if (b->state == LOG_BITMAP_STATE_ERROR) {
> +        logging_state_set_status(b, LOG_BITMAP_STATE_ERROR,
> +                                    LOG_BITMAP_STATE_COMPLETED);
> +    }

can you use a switch here, please?

> +{
> +    int fd = -1;
> +    BitmapLogState *b = logging_current_state();
> +    Error *local_err = NULL;
> +    if (b->state == LOG_BITMAP_STATE_ACTIVE ||
> +            b->state == LOG_BITMAP_STATE_SETUP ||
> +            b->state == LOG_BITMAP_STATE_CANCELING) {
> +        b = NULL;

Not needed.  This is repeated in more places.

> +    b->total_epochs = epochs;
> +    b->current_frequency = frequency;

I would have written it as:

	if (has_epochs) {
             b->total_epochs = epochs;
        } else {
             b->total_epochs IN_EPOCH_VALUE;
        }

The same with current frequency.

Thanks, Juan.