Patchwork [16/28] migration: use global variable directly

login
register
mail settings
Submitter Juan Quintela
Date Feb. 23, 2011, 9:47 p.m.
Message ID <8b675b6ffee29ec15f2b73a9fd47e334429f5c5c.1298492768.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/84244/
State New
Headers show

Comments

Juan Quintela - Feb. 23, 2011, 9:47 p.m.
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.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
Anthony Liguori - Feb. 23, 2011, 10:18 p.m.
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.

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;
>
Juan Quintela - Feb. 23, 2011, 10:46 p.m.
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.  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;
>>
Anthony Liguori - Feb. 23, 2011, 11:04 p.m.
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;
>>>
>>>
Juan Quintela - Feb. 23, 2011, 11:07 p.m.
Anthony Liguori <anthony@codemonkey.ws> wrote:
> 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.

Obviounly tastes change between us :-(

If I want to do that, instead of creating a local variable, I will just
add a parameter to the function and make the callers to pass it.

For me, a variable in a function is global or local, hidden it as a
local has no merit.

Later, Juan.
Markus Armbruster - Feb. 24, 2011, 7:09 a.m.
Anthony Liguori <anthony@codemonkey.ws> writes:

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

I disagree.  The use of global variables should be made as painfully
explicit as possible.  Hiding them behind local pointers is sweeping the
globals under the rug.

For completeness: a local variable may be necessary to convince the
optimizer that the value doesn't change.  Cases where this matters
exist, but they're rare.
Paolo Bonzini - Feb. 24, 2011, 7:58 a.m.
On 02/24/2011 08:09 AM, Markus Armbruster wrote:
> For completeness: a local variable may be necessary to convince the
> optimizer that the value doesn't change.  Cases where this matters
> exist, but they're rare.

In particular, for non-pointers they're nonexistent if the variable is 
static and you never use &current_migration, i.e. if the variable 
doesn't escape.

If the above conditions are satisfied and you have a loop that never 
"leaves" the C file, you could even see the compiler keep the variable 
in a register for the whole duration of the loop and store the modified 
change after the loop.

Paolo

Patch

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;