[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 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/cfg.mk b/cfg.mk
> index 23564f1..5c3f3ab 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -907,7 +907,7 @@ exclude_file_name_regexp--sc_avoid_strcase = ^tools/virsh\.h$$
>  _src1=libvirt|fdstream|qemu/qemu_monitor|util/(vircommand|virfile)|xen/xend_internal|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon
>  _test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock
>  exclude_file_name_regexp--sc_avoid_write = \
> -  ^(src/($(_src1))|daemon/libvirtd|tools/console|tests/($(_test1)))\.c$$
> +  ^(src/($(_src1))|daemon/libvirtd|tools/virsh-console|tests/($(_test1)))\.c$$
> 
>  exclude_file_name_regexp--sc_bindtextdomain = ^(tests|examples)/
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 9a83069..aa604fd 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -213,10 +213,10 @@ src/xenapi/xenapi_driver.c
>  src/xenapi/xenapi_utils.c
>  src/xenxs/xen_sxpr.c
>  src/xenxs/xen_xm.c
> -tools/console.c
>  tools/libvirt-guests.sh.in
>  tools/virsh.c
>  tools/virsh.h
> +tools/virsh-console.c
>  tools/virsh-domain-monitor.c
>  tools/virsh-domain.c
>  tools/virsh-edit.c
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index 74fae2d..cd4cb31 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -161,8 +161,8 @@ virt_login_shell_CFLAGS =					\
>  		$(COVERAGE_CFLAGS)
> 
>  virsh_SOURCES =							\
> -		console.c console.h				\
>  		virsh.c virsh.h					\
> +		virsh-console.c virsh-console.h			\
>  		virsh-domain.c virsh-domain.h			\
>  		virsh-domain-monitor.c virsh-domain-monitor.h	\
>  		virsh-host.c virsh-host.h			\
> 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.

> @@ -37,7 +37,7 @@
>  # include <c-ctype.h>
> 
>  # include "internal.h"
> -# include "console.h"
> +# include "virsh-console.h"
>  # include "virlog.h"
>  # include "virfile.h"
>  # include "viralloc.h"
> @@ -58,6 +58,7 @@ struct virConsoleBuffer {
>      char *data;
>  };
> 
> +
>  typedef struct virConsole virConsole;
>  typedef virConsole *virConsolePtr;
>  struct virConsole {
> @@ -75,11 +76,15 @@ struct virConsole {
>      char escapeChar;
>  };
> 
> +
>  static int got_signal = 0;
> -static void do_signal(int sig ATTRIBUTE_UNUSED) {
> +static void
> +virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED)
> +{
>      got_signal = 1;
>  }
> 
> +
>  # ifndef HAVE_CFMAKERAW
>  static void
>  cfmakeraw(struct termios *attr)
> @@ -93,6 +98,7 @@ cfmakeraw(struct termios *attr)
>  }
>  # endif /* !HAVE_CFMAKERAW */
> 
> +
>  static void
>  virConsoleShutdown(virConsolePtr con)
>  {
> @@ -114,6 +120,21 @@ virConsoleShutdown(virConsolePtr con)
>      virCondSignal(&con->cond);
>  }
> 
> +
> +static void
> +virConsoleFree(virConsolePtr con)
> +{
> +    if (!con)
> +        return;
> +
> +    if (con->st)
> +        virStreamFree(con->st);
> +    virMutexDestroy(&con->lock);
> +    virCondDestroy(&con->cond);
> +    VIR_FREE(con);
> +}
> +
> +
>  static void
>  virConsoleEventOnStream(virStreamPtr st,
>                          int events, void *opaque)
> @@ -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.

>          }
>      }
> @@ -187,6 +207,7 @@ virConsoleEventOnStream(virStreamPtr st,
>      }
>  }
> 
> +
>  static void
>  virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>                         int fd ATTRIBUTE_UNUSED,
> @@ -242,6 +263,7 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>      }
>  }
> 
> +
>  static void
>  virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
>                          int fd,
> @@ -270,9 +292,8 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
> 
>          avail = con->streamToTerminal.length - con->streamToTerminal.offset;
>          if (avail > 1024) {
> -            if (VIR_REALLOC_N(con->streamToTerminal.data,
> -                              con->streamToTerminal.offset + 1024) < 0)
> -            {}
> +            ignore_value(VIR_REALLOC_N(con->streamToTerminal.data,
> +                                       con->streamToTerminal.offset + 1024));
>              con->streamToTerminal.length = con->streamToTerminal.offset + 1024;

And again.

>          }
>      }
> @@ -296,6 +317,7 @@ vshGetEscapeChar(const char *s)
>      return *s;
>  }
> 
> +
>  int
>  vshMakeStdinRaw(struct termios *ttyattr, bool report_errors)
>  {
> @@ -322,10 +344,12 @@ vshMakeStdinRaw(struct termios *ttyattr, bool report_errors)
>      return 0;
>  
> 
> -int vshRunConsole(virDomainPtr dom,
> -                  const char *dev_name,
> -                  const char *escape_seq,
> -                  unsigned int flags)
> +
> +int
> +vshRunConsole(virDomainPtr dom,
> +              const char *dev_name,
> +              const char *escape_seq,
> +              unsigned int flags)
>  {
>      int ret = -1;
>      struct termios ttyattr;
> @@ -348,11 +372,11 @@ int vshRunConsole(virDomainPtr dom,
>         the original terminal settings on STDIN before the
>         process exits - people don't like being left with a
>         messed up terminal ! */
> -    old_sigquit = signal(SIGQUIT, do_signal);
> -    old_sigterm = signal(SIGTERM, do_signal);
> -    old_sigint = signal(SIGINT, do_signal);
> -    old_sighup = signal(SIGHUP, do_signal);
> -    old_sigpipe = signal(SIGPIPE, do_signal);
> +    old_sigquit = signal(SIGQUIT, virConsoleHandleSignal);
> +    old_sigterm = signal(SIGTERM, virConsoleHandleSignal);
> +    old_sigint = signal(SIGINT, virConsoleHandleSignal);
> +    old_sighup = signal(SIGHUP, virConsoleHandleSignal);
> +    old_sigpipe = signal(SIGPIPE, virConsoleHandleSignal);
>      got_signal = 0;
> 
>      if (VIR_ALLOC(con) < 0)
> @@ -396,15 +420,8 @@ int vshRunConsole(virDomainPtr dom,
> 
>      ret = 0;
> 
> - cleanup:
> -
> -    if (con) {
> -        if (con->st)
> -            virStreamFree(con->st);
> -        virMutexDestroy(&con->lock);
> -        virCondDestroy(&con->cond);
> -        VIR_FREE(con);
> -    }
> +cleanup:
> +    virConsoleFree(con);
> 
>      /* Restore original signal handlers */
>      signal(SIGPIPE, old_sigpipe);
> 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.

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

#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


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