[Qemu-devel] Re: [PATCH 16/28] migration: use global variable directly

[Anthony Liguori]

On 02/23/2011 04:46 PM, Juan Quintela wrote:
> Anthony Liguori<address@hidden>  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<address@hidden>
> > > ---
> > >    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;
> > >
> > >