2011/5/16 Eric Blake <ebl...@redhat.com>: > 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?
Is strict now, see https://www.redhat.com/archives/libvir-list/2011-May/msg01432.html >> @@ -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'). Done, see https://www.redhat.com/archives/libvir-list/2011-May/msg01434.html >> +++ 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. I handled this differently now, because we cannot change the order in the public API anyway. >> +++ 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. > See this series as a v2 with some additions: https://www.redhat.com/archives/libvir-list/2011-May/msg01427.html Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list