From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=49913 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PsNlH-0004Md-CX
	for qemu-devel@nongnu.org; Wed, 23 Feb 2011 18:04:48 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <anthony@codemonkey.ws>) id 1PsNlG-0004AY-4Y
	for qemu-devel@nongnu.org; Wed, 23 Feb 2011 18:04:47 -0500
Received: from mail-vw0-f45.google.com ([209.85.212.45]:51276)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <anthony@codemonkey.ws>) id 1PsNlG-0004AR-0c
	for qemu-devel@nongnu.org; Wed, 23 Feb 2011 18:04:46 -0500
Received: by vws19 with SMTP id 19so4301568vws.4
	for <qemu-devel@nongnu.org>; Wed, 23 Feb 2011 15:04:45 -0800 (PST)
Message-ID: <4D659295.1070400@codemonkey.ws>
Date: Wed, 23 Feb 2011 17:04:53 -0600
From: Anthony Liguori <anthony@codemonkey.ws>
MIME-Version: 1.0
References: <cover.1298492768.git.quintela@redhat.com>	<8b675b6ffee29ec15f2b73a9fd47e334429f5c5c.1298492768.git.quintela@redhat.com>	<4D6587C7.6030604@codemonkey.ws>
	<m3mxlm2wtm.fsf@trasno.org>
In-Reply-To: <m3mxlm2wtm.fsf@trasno.org>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: [Qemu-devel] Re: [PATCH 16/28] migration: use global variable
	directly
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/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: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: quintela@redhat.com
Cc: qemu-devel@nongnu.org

On 02/23/2011 04:46 PM, Juan Quintela wrote:
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>    
>> On 02/23/2011 03:47 PM, Juan Quintela wrote:
>>      
>>> We are setting a pointer to a local variable in the previous line, just use
>>> the global variable directly.  We remove the ->file test because it is already
>>> done inside qemu_file_set_rate_limit() function.
>>>
>>>        
>> I think this is bad form generally speaking.  Globals are not
>> something to be embraced but rather to be isolated as much as humanly
>> possible.
>>      
> current_migration is a global variable.
>
> And just doing:
>
> s = current_migration;
>
> foo(s);
>
> helps nothing.

It's still bad form IMHO.  You should always use local variables to 
reference global variables unless you're explicitly setting a global 
variable.

Regards,

Anthony Liguori

>   We are not going to happen more than one migration at
> the same time anytime soon.  Notice that everything that is outside of
> migration.c  already receives a pointer to it, i.e. not use the global
> one.  So, I claim this is a very special case, not the normal case about
> global variables.
>
> Later, Juan.
>
>    
>> Regards,
>>
>> Anthony Liguori
>>
>>      
>>> Signed-off-by: Juan Quintela<quintela@redhat.com>
>>> ---
>>>    migration.c |    6 ++----
>>>    1 files changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/migration.c b/migration.c
>>> index caf5044..21f5a9a 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -457,7 +457,6 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>    int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>    {
>>>        int64_t d;
>>> -    MigrationState *s;
>>>
>>>        d = qdict_get_int(qdict, "value");
>>>        if (d<   0) {
>>> @@ -465,9 +464,8 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>        }
>>>        max_throttle = d;
>>>
>>> -    s = current_migration;
>>> -    if (s&&   s->file) {
>>> -        qemu_file_set_rate_limit(s->file, max_throttle);
>>> +    if (current_migration) {
>>> +        qemu_file_set_rate_limit(current_migration->file, max_throttle);
>>>        }
>>>
>>>        return 0;
>>>
>>>