[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/3] tools: rename console.[ch] to virsh-console.[ch] and fix coding style



On 09/03/13 11:44, Michal Privoznik wrote:
> On 29.08.2013 18:36, Peter Krempa wrote:
>> ---
>>  cfg.mk                               |  2 +-
>>  po/POTFILES.in                       |  2 +-
>>  tools/Makefile.am                    |  2 +-
>>  tools/{console.c => virsh-console.c} | 73 ++++++++++++++++++++++--------------
>>  tools/{console.h => virsh-console.h} |  4 +-
>>  tools/virsh-domain.c                 |  2 +-
>>  tools/virsh.c                        |  2 +-
>>  7 files changed, 52 insertions(+), 35 deletions(-)
>>  rename tools/{console.c => virsh-console.c} (90%)
>>  rename tools/{console.h => virsh-console.h} (91%)
>>

...

>> diff --git a/tools/console.c b/tools/virsh-console.c
>> similarity index 90%
>> rename from tools/console.c
>> rename to tools/virsh-console.c
>> index 6c24fcf..debf12c 100644
>> --- a/tools/console.c
>> +++ b/tools/virsh-console.c
>> @@ -1,7 +1,7 @@
>>  /*
>> - * console.c: A dumb serial console client
>> + * virsh-console.c: A dumb serial console client
>>   *
>> - * Copyright (C) 2007-2008, 2010-2012 Red Hat, Inc.
>> + * Copyright (C) 2007-2008, 2010-2013 Red Hat, Inc.
>>   *
>>   * This library is free software; you can redistribute it and/or
>>   * modify it under the terms of the GNU Lesser General Public
> 
> Not shown in the context, but there should be 'Authors:' line just above
> Dan's name.
> 

Okay, added.

...


>> @@ -171,9 +192,8 @@ virConsoleEventOnStream(virStreamPtr st,
>>
>>          avail = con->terminalToStream.length - con->terminalToStream.offset;
>>          if (avail > 1024) {
>> -            if (VIR_REALLOC_N(con->terminalToStream.data,
>> -                              con->terminalToStream.offset + 1024) < 0)
>> -            {}
>> +            ignore_value(VIR_REALLOC_N(con->terminalToStream.data,
>> +                                       con->terminalToStream.offset + 1024));
>>              con->terminalToStream.length = con->terminalToStream.offset + 1024;
> 
> I don't think this is quite right. If the VIR_REALLOC fails, why are we
> increasing the .lenght? I know this is pre-existing, but if we are
> touching this we should fix this.

This is actually decreasing the size of the allocated memory. It's hard
to see here, but if there's more than 1024 bytes left free in the
buffer, the size is decreased to exactly 1024 extra bytes. Thus this
call should be always safe and I just changed the way the result from
VIR_REALLOC is ignored.

> 
>>          }
>>      }

...

>> diff --git a/tools/console.h b/tools/virsh-console.h
>> similarity index 91%
>> rename from tools/console.h
>> rename to tools/virsh-console.h
>> index 46255b8..96ef235 100644
>> --- a/tools/console.h
>> +++ b/tools/virsh-console.h
>> @@ -1,7 +1,7 @@
>>  /*
>> - * console.c: A dumb serial console client
>> + * virsh-console.h: A dumb serial console client
>>   *
>> - * Copyright (C) 2007, 2010, 2012 Red Hat, Inc.
>> + * Copyright (C) 2007, 2010, 2012-2013 Red Hat, Inc.
>>   *
>>   * This library is free software; you can redistribute it and/or
>>   * modify it under the terms of the GNU Lesser General Public
> 
> Not shown in the context, but there should be 'Authors:' line just above
> Dan's name.

Okay, done.

> 
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index c87cf6a..60eed51 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -41,7 +41,7 @@
>>  #include "virbuffer.h"
>>  #include "c-ctype.h"
>>  #include "conf/domain_conf.h"
>> -#include "console.h"
>> +#include "virsh-console.h"
> 
> This should go a few lines down, just before the line:
> 

Fixed.

> #include "virsh-domain-monitor.h"
> 
>>  #include "viralloc.h"
>>  #include "vircommand.h"
>>  #include "virfile.h"
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index 2f04e6a..0cc9bdd 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -57,7 +57,6 @@
>>  #include "virerror.h"
>>  #include "base64.h"
>>  #include "virbuffer.h"
>> -#include "console.h"
>>  #include "viralloc.h"
>>  #include "virxml.h"
>>  #include <libvirt/libvirt-qemu.h>
>> @@ -73,6 +72,7 @@
>>  #include "virtypedparam.h"
>>  #include "virstring.h"
>>
>> +#include "virsh-console.h"
>>  #include "virsh-domain.h"
>>  #include "virsh-domain-monitor.h"
>>  #include "virsh-host.h"
>>
> 
> I've pointed a few issues but I believe that you will fix them right so
> I don't need to see a v2.
> 
> ACK
> 
> Michal
> 

And pushed.

Peter

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]