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

Re: [libvirt] [PATCH 4/5] remote generator: Handle stream-using functions



On 05/15/2011 12:23 AM, Matthias Bolte wrote:
> Reorder signature of virDrvDomainOpenConsole to match the common pattern.
> Add special case code to handle deviation in the public API version.
> 
> Adds a missing remoteStreamRelease to remoteDomainScreenshot error path.
> ---
>  daemon/remote.c              |  181 ------------------------------------
>  daemon/remote_generator.pl   |  104 +++++++++++++++++++--
>  src/driver.h                 |    2 +-
>  src/libvirt.c                |    2 +-
>  src/lxc/lxc_driver.c         |    2 +-
>  src/qemu/qemu_driver.c       |    2 +-
>  src/remote/remote_driver.c   |  207 ------------------------------------------
>  src/remote/remote_protocol.x |   20 +++--
>  src/uml/uml_driver.c         |    2 +-
>  src/xen/xen_driver.c         |    2 +-
>  10 files changed, 113 insertions(+), 411 deletions(-)
> 

> diff --git a/daemon/remote.c b/daemon/remote.c
> index e3bd4a2..f8db6fe 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -1171,50 +1171,6 @@ cleanup:
>  }
>  
>  static int
> -remoteDispatchDomainMigratePrepareTunnel(struct qemud_server *server ATTRIBUTE_UNUSED,

Hmm, your patch was written before Dan's migration v3 patch; you may
need to tweak it to cover PrepareTunnel3 in the same manner.  However,
of the functions you moved, I validated that the generated version has
the same behavior as the deleted version.

> +++ b/daemon/remote_generator.pl
> @@ -43,7 +43,7 @@ sub name_to_ProcName {
>  
>  # Read the input file (usually remote_protocol.x) and form an
>  # opinion about the name, args and return type of each RPC.
> -my ($name, $ProcName, $id, $flags, $gen, %calls, @calls);
> +my ($name, $ProcName, $id, $flags, %calls, @calls);
>  
>  # only generate a close method if -c was passed
>  if ($opt_c) {
> @@ -135,19 +135,26 @@ while (<PROTOCOL>) {
>          $ProcName = name_to_ProcName ($name);
>  
>          if ($opt_b or $opt_k) {
> -            if (!($flags =~ m/^\s*\/\*\s*(\S+)\s+(\S+)\s*\*\/\s*$/)) {
> +            if (!($flags =~ m/^\s*\/\*\s*(\S+)\s+(\S+)\s*(.*)\*\/\s*$/)) {
>                  die "invalid generator flags for ${procprefix}_PROC_${name}"
>              }
>  
> -            $gen = $opt_b ? $1 : $2;
> +            my $genmode = $opt_b ? $1 : $2;
> +            my $genflags = $3;
>  
> -            if ($gen eq "autogen") {
> +            if ($genmode eq "autogen") {
>                  push(@autogen, $ProcName);
> -            } elsif ($gen eq "skipgen") {
> +            } elsif ($genmode eq "skipgen") {
>                  # ignore it
>              } else {
>                  die "invalid generator flags for ${procprefix}_PROC_${name}"
>              }
> +
> +            if (defined $genflags and $genflags =~ m/\|\s*(read|write)stream/) {
> +                $calls{$name}->{streamflag} = $1;
> +            } else {
> +                $calls{$name}->{streamflag} = "none";
> +            }

Really, we have readstream, writestream, or empty.  Should we make this
stricter and reject unknown words, rather than silently translating them
to none?

> @@ -854,9 +908,14 @@ elsif ($opt_k) {
>                          }
>                      }
>  
> -                    if ($call->{ProcName} eq "DomainMigrateSetMaxDowntime" and
> -                        $arg_name eq "downtime") {
> -                        $type_name = "unsigned long long";
> +                    # SPECIAL: some hyper parameters map to long longs
> +                    if (($call->{ProcName} eq "DomainMigrateSetMaxDowntime" and
> +                         $arg_name eq "downtime") or
> +                        ($call->{ProcName} eq "StorageVolUpload" and
> +                         ($arg_name eq "offset" or $arg_name eq "length")) or
> +                        ($call->{ProcName} eq "StorageVolDownload" and
> +                         ($arg_name eq "offset" or $arg_name eq "length"))) {
> +                        $type_name .= " long";

Idea for a future patch - should we add annotations to remote_protocol.h
for which hyper fields map to 'long' (and thus need overflow checking on
32-bit hosts) and which are 'long long'?  That is, instead of
special-casing per function name in remote_generator.pl, it might be
nice to annotate it directly in the .x file (and error out if you use
'[unsigned] hyper' but don't say whether it maps to 'long' or 'long long').

> +++ b/src/driver.h
> @@ -516,8 +516,8 @@ typedef int
>  
>  typedef int
>      (*virDrvDomainOpenConsole)(virDomainPtr dom,
> -                               const char *devname,
>                                 virStreamPtr st,
> +                               const char *devname,
>                                 unsigned int flags);

I would have preferred seeing this split into two patches; one for the
driver.h callback reorganization, and the other for the generator
functions.  I think it's okay as one, but it makes this patch rather
large; especially since you have to rebase anyways for picking up
migration v3.

> +++ b/src/remote/remote_protocol.x
> @@ -1965,7 +1965,10 @@ const REMOTE_PROTOCOL_VERSION = 1;
>  enum remote_procedure {
>      /* Each function must have a two-word comment.  The first word is
>       * whether remote_generator.pl handles daemon, the second whether
> -     * it handles src/remote.  */
> +     * it handles src/remote.  Additional flags can be specified after a
> +     * pipe. The readstream/writestream flag lets daemon and src/remote
> +     * create a stream. The direction is defined from the src/remote point of
> +     * view. A readstream transfers data from daemon to src/remote.  */
>      REMOTE_PROC_OPEN = 1, /* skipgen skipgen */
>      REMOTE_PROC_CLOSE = 2, /* skipgen skipgen */
>      REMOTE_PROC_GET_TYPE = 3, /* skipgen skipgen */
> @@ -2127,7 +2130,7 @@ enum remote_procedure {
>      REMOTE_PROC_SECRET_GET_VALUE = 145, /* skipgen skipgen */
>      REMOTE_PROC_SECRET_UNDEFINE = 146, /* autogen autogen */
>      REMOTE_PROC_SECRET_LOOKUP_BY_USAGE = 147, /* autogen autogen */
> -    REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL = 148, /* skipgen skipgen */
> +    REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL = 148, /* autogen autogen | writestream */
>      REMOTE_PROC_IS_SECURE = 149, /* autogen skipgen */
>      REMOTE_PROC_DOMAIN_IS_ACTIVE = 150, /* autogen autogen */
>  
> @@ -2186,18 +2189,18 @@ enum remote_procedure {
>      REMOTE_PROC_DOMAIN_SET_VCPUS_FLAGS = 199, /* autogen autogen */
>      REMOTE_PROC_DOMAIN_GET_VCPUS_FLAGS = 200, /* autogen autogen */
>  
> -    REMOTE_PROC_DOMAIN_OPEN_CONSOLE = 201, /* skipgen skipgen */
> +    REMOTE_PROC_DOMAIN_OPEN_CONSOLE = 201, /* autogen autogen | readstream */
>      REMOTE_PROC_DOMAIN_IS_UPDATED = 202, /* autogen autogen */
>      REMOTE_PROC_GET_SYSINFO = 203, /* autogen autogen */
>      REMOTE_PROC_DOMAIN_SET_MEMORY_FLAGS = 204, /* autogen autogen */
>      REMOTE_PROC_DOMAIN_SET_BLKIO_PARAMETERS = 205, /* skipgen skipgen */
>      REMOTE_PROC_DOMAIN_GET_BLKIO_PARAMETERS = 206, /* skipgen skipgen */
>      REMOTE_PROC_DOMAIN_MIGRATE_SET_MAX_SPEED = 207, /* autogen autogen */
> -    REMOTE_PROC_STORAGE_VOL_UPLOAD = 208, /* skipgen skipgen */
> -    REMOTE_PROC_STORAGE_VOL_DOWNLOAD = 209, /* skipgen skipgen */
> +    REMOTE_PROC_STORAGE_VOL_UPLOAD = 208, /* autogen autogen | writestream */
> +    REMOTE_PROC_STORAGE_VOL_DOWNLOAD = 209, /* autogen autogen | readstream */
>      REMOTE_PROC_DOMAIN_INJECT_NMI = 210, /* autogen autogen */
>  
> -    REMOTE_PROC_DOMAIN_SCREENSHOT = 211 /* skipgen skipgen */
> +    REMOTE_PROC_DOMAIN_SCREENSHOT = 211 /* skipgen autogen | readstream */

Merge conflict due to migration v3.

>      /*
>       * Notice how the entries are grouped in sets of 10 ?
>       * Nice isn't it. Please keep it this way when adding more.
> @@ -2205,7 +2208,10 @@ enum remote_procedure {
>  
>      /* Each function must have a two-word comment.  The first word is

Should these two comments (grouping in sets of 10, and details about the
comment) be joined into one?

While this patch is accurate as-is, I had enough comments (especially
regarding splitting this into two after rebasing onto the latest
upstream) that it is worth seeing a v2.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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