2011/5/16 Eric Blake <eblake@xxxxxxxxxx>:
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list