qapi: The #optional tag is redundant, drop

We traditionally mark optional members #optional in the doc comment.
Before commit 3313b612, this was entirely manual.

Commit 3313b612 added some automation because its qapi2texi.py relied
on #optional to determine whether a member is optional.  This is no
longer the case since the previous commit: the only thing qapi2texi.py
still does with #optional is stripping it out.  We still reject bogus
qapi-schema.json and six places for qga/qapi-schema.json.

Thus, you can't actually rely on #optional to see whether something is
optional.  Yet we still make people add it manually.  That's just
busy-work.

Drop the code to check, fix up and strip out #optional, along with all
instances of #optional.  To keep it out, add code to reject it, to be
dropped again once the dust settles.

No change to generated documentation.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Message-Id: <1489582656-31133-18-git-send-email-armbru@redhat.com>

docs/qapi-code-gen.txt

@@ -131,10 +131,8 @@ and optional tagged sections.
 
 FIXME: the parser accepts these things in almost any order.
 
-Optional arguments / members are tagged with the phrase '#optional',
-often with their default value; and extensions added after the
-expression was first released are also given a '(since x.y.z)'
-comment.
+Extensions added after the expression was first released carry a
+'(since x.y.z)' comment.
 
 A tagged section starts with one of the following words:
 "Note:"/"Notes:", "Since:", "Example"/"Examples", "Returns:", "TODO:".
@@ -150,10 +148,10 @@ For example:
 #
 # Statistics of a virtual block device or a block backing device.
 #
-# @device: #optional If the stats are for a virtual block device, the name
+# @device: If the stats are for a virtual block device, the name
 #          corresponding to the virtual block device.
 #
-# @node-name: #optional The node name of the device. (since 2.3)
+# @node-name: The node name of the device. (since 2.3)
 #
 # ... more members ...
 #
@@ -168,7 +166,7 @@ For example:
 #
 # Query the @BlockStats for all virtual block devices.
 #
-# @query-nodes: #optional If true, the command will query all the
+# @query-nodes: If true, the command will query all the
 #               block nodes ... explain, explain ...  (since 2.3)
 #
 # Returns: A list of @BlockStats for each virtual block devices.

docs/writing-qmp-commands.txt

@@ -252,7 +252,7 @@ here goes "hello-world"'s new entry for the qapi-schema.json file:
 #
 # Print a client provided string to the standard output stream.
 #
-# @message: #optional string to be printed
+# @message: string to be printed
 #
 # Returns: Nothing on success.
 #
@@ -358,7 +358,7 @@ The best way to return that data is to create a new QAPI type, as shown below:
 #
 # @clock-name: The alarm clock method's name.
 #
-# @next-deadline: #optional The time (in nanoseconds) the next alarm will fire.
+# @next-deadline: The time (in nanoseconds) the next alarm will fire.
 #
 # Since: 1.0
 ##

qapi/block.json

@@ -163,11 +163,11 @@
 #
 # Ejects a device from a removable drive.
 #
-# @device:  #optional Block device name (deprecated, use @id instead)
+# @device:  Block device name (deprecated, use @id instead)
 #
-# @id:      #optional The name or QOM path of the guest device (since: 2.8)
+# @id:      The name or QOM path of the guest device (since: 2.8)
 #
-# @force:   #optional If true, eject regardless of whether the drive is locked.
+# @force:   If true, eject regardless of whether the drive is locked.
 #           If not specified, the default value is false.
 #
 # Returns:  Nothing on success
@@ -215,7 +215,7 @@
 # @device: The device name or node name of the node to be exported
 #
 # @writable: Whether clients should be able to write to the device via the
-#     NBD connection (default false). #optional
+#     NBD connection (default false).
 #
 # Returns: error if the device is already marked for export.
 #

qapi/crypto.json

@@ -152,7 +152,7 @@
 #
 # The options that apply to QCow/QCow2 AES-CBC encryption format
 #
-# @key-secret: #optional the ID of a QCryptoSecret object providing the
+# @key-secret: the ID of a QCryptoSecret object providing the
 #              decryption key. Mandatory except when probing image for
 #              metadata only.
 #
@@ -166,7 +166,7 @@
 #
 # The options that apply to LUKS encryption format
 #
-# @key-secret: #optional the ID of a QCryptoSecret object providing the
+# @key-secret: the ID of a QCryptoSecret object providing the
 #              decryption key. Mandatory except when probing image for
 #              metadata only.
 # Since: 2.6
@@ -180,17 +180,17 @@
 #
 # The options that apply to LUKS encryption format initialization
 #
-# @cipher-alg: #optional the cipher algorithm for data encryption
+# @cipher-alg: the cipher algorithm for data encryption
 #              Currently defaults to 'aes'.
-# @cipher-mode: #optional the cipher mode for data encryption
+# @cipher-mode: the cipher mode for data encryption
 #               Currently defaults to 'cbc'
-# @ivgen-alg: #optional the initialization vector generator
+# @ivgen-alg: the initialization vector generator
 #             Currently defaults to 'essiv'
-# @ivgen-hash-alg: #optional the initialization vector generator hash
+# @ivgen-hash-alg: the initialization vector generator hash
 #                  Currently defaults to 'sha256'
-# @hash-alg: #optional the master key hash algorithm
+# @hash-alg: the master key hash algorithm
 #            Currently defaults to 'sha256'
-# @iter-time: #optional number of milliseconds to spend in
+# @iter-time: number of milliseconds to spend in
 #             PBKDF passphrase processing. Currently defaults
 #             to 2000. (since 2.8)
 # Since: 2.6
@@ -257,8 +257,8 @@
 #
 # @active: whether the key slot is currently in use
 # @key-offset: offset to the key material in bytes
-# @iters: #optional number of PBKDF2 iterations for key material
-# @stripes: #optional number of stripes for splitting key material
+# @iters: number of PBKDF2 iterations for key material
+# @stripes: number of stripes for splitting key material
 #
 # Since: 2.7
 ##
@@ -277,7 +277,7 @@
 # @cipher-alg: the cipher algorithm for data encryption
 # @cipher-mode: the cipher mode for data encryption
 # @ivgen-alg: the initialization vector generator
-# @ivgen-hash-alg: #optional the initialization vector generator hash
+# @ivgen-hash-alg: the initialization vector generator hash
 # @hash-alg: the master key hash algorithm
 # @payload-offset: offset to the payload data in bytes
 # @master-key-iters: number of PBKDF2 iterations for key material

qapi/event.json

@@ -186,7 +186,7 @@
 # At this point, it's safe to reuse the specified device ID. Device removal can
 # be initiated by the guest or by HMP/QMP commands.
 #
-# @device: #optional device name
+# @device: device name
 #
 # @path: device path
 #
@@ -209,7 +209,7 @@
 # Emitted once until the 'query-rx-filter' command is executed, the first event
 # will always be emitted
 #
-# @name: #optional net client name
+# @name: net client name
 #
 # @path: device path
 #
@@ -488,7 +488,7 @@
 #
 # @action: action that has been taken, currently always "pause"
 #
-# @info: #optional information about a panic (since 2.9)
+# @info: information about a panic (since 2.9)
 #
 # Since: 1.5
 #
@@ -533,7 +533,7 @@
 #
 # @type: quorum operation type (Since 2.6)
 #
-# @error: #optional error message. Only present on failure. This field
+# @error: error message. Only present on failure. This field
 #         contains a human-readable error message. There are no semantics other
 #         than that the block layer reported an error and clients should not
 #         try to interpret the error string.
@@ -620,7 +620,7 @@
 #
 # @result: DumpQueryResult type described in qapi-schema.json.
 #
-# @error: #optional human-readable error string that provides
+# @error: human-readable error string that provides
 #         hint on why dump failed. Only presents on failure. The
 #         user should not try to interpret the error string.
 #

qapi/introspect.json

@@ -163,10 +163,10 @@
 #
 # @members: the object type's (non-variant) members, in no particular order.
 #
-# @tag: #optional the name of the member serving as type tag.
+# @tag: the name of the member serving as type tag.
 #       An element of @members with this name must exist.
 #
-# @variants: #optional variant members, i.e. additional members that
+# @variants: variant members, i.e. additional members that
 #            depend on the type tag's value.  Present exactly when
 #            @tag is present.  The variants are in no particular order,
 #            and may even differ from the order of the values of the
@@ -190,7 +190,7 @@
 #
 # @type: the name of the member's type.
 #
-# @default: #optional default when used as command parameter.
+# @default: default when used as command parameter.
 #           If absent, the parameter is mandatory.
 #           If present, the value must be null.  The parameter is
 #           optional, and behavior when it's missing is not specified

qapi/rocker.json

@@ -121,23 +121,23 @@
 #
 # @tbl-id: flow table ID
 #
-# @in-pport: #optional physical input port
+# @in-pport: physical input port
 #
-# @tunnel-id: #optional tunnel ID
+# @tunnel-id: tunnel ID
 #
-# @vlan-id: #optional VLAN ID
+# @vlan-id: VLAN ID
 #
-# @eth-type: #optional Ethernet header type
+# @eth-type: Ethernet header type
 #
-# @eth-src: #optional Ethernet header source MAC address
+# @eth-src: Ethernet header source MAC address
 #
-# @eth-dst: #optional Ethernet header destination MAC address
+# @eth-dst: Ethernet header destination MAC address
 #
-# @ip-proto: #optional IP Header protocol field
+# @ip-proto: IP Header protocol field
 #
-# @ip-tos: #optional IP header TOS field
+# @ip-tos: IP header TOS field
 #
-# @ip-dst: #optional IP header destination address
+# @ip-dst: IP header destination address
 #
 # Note: optional members may or may not appear in the flow key
 # depending if they're relevant to the flow key.
@@ -155,19 +155,19 @@
 #
 # Rocker switch OF-DPA flow mask
 #
-# @in-pport: #optional physical input port
+# @in-pport: physical input port
 #
-# @tunnel-id: #optional tunnel ID
+# @tunnel-id: tunnel ID
 #
-# @vlan-id: #optional VLAN ID
+# @vlan-id: VLAN ID
 #
-# @eth-src: #optional Ethernet header source MAC address
+# @eth-src: Ethernet header source MAC address
 #
-# @eth-dst: #optional Ethernet header destination MAC address
+# @eth-dst: Ethernet header destination MAC address
 #
-# @ip-proto: #optional IP Header protocol field
+# @ip-proto: IP Header protocol field
 #
-# @ip-tos: #optional IP header TOS field
+# @ip-tos: IP header TOS field
 #
 # Note: optional members may or may not appear in the flow mask
 # depending if they're relevant to the flow mask.
@@ -184,17 +184,17 @@
 #
 # Rocker switch OF-DPA flow action
 #
-# @goto-tbl: #optional next table ID
+# @goto-tbl: next table ID
 #
-# @group-id: #optional group ID
+# @group-id: group ID
 #
-# @tunnel-lport: #optional tunnel logical port ID
+# @tunnel-lport: tunnel logical port ID
 #
-# @vlan-id: #optional VLAN ID
+# @vlan-id: VLAN ID
 #
-# @new-vlan-id: #optional new VLAN ID
+# @new-vlan-id: new VLAN ID
 #
-# @out-pport: #optional physical output port
+# @out-pport: physical output port
 #
 # Note: optional members may or may not appear in the flow action
 # depending if they're relevant to the flow action.
@@ -234,7 +234,7 @@
 #
 # @name: switch name
 #
-# @tbl-id: #optional flow table ID.  If tbl-id is not specified, returns
+# @tbl-id: flow table ID.  If tbl-id is not specified, returns
 # flow information for all tables.
 #
 # Returns: rocker OF-DPA flow information
@@ -268,27 +268,27 @@
 #
 # @type: group type
 #
-# @vlan-id: #optional VLAN ID
+# @vlan-id: VLAN ID
 #
-# @pport: #optional physical port number
+# @pport: physical port number
 #
-# @index: #optional group index, unique with group type
+# @index: group index, unique with group type
 #
-# @out-pport: #optional output physical port number
+# @out-pport: output physical port number
 #
-# @group-id: #optional next group ID
+# @group-id: next group ID
 #
-# @set-vlan-id: #optional VLAN ID to set
+# @set-vlan-id: VLAN ID to set
 #
-# @pop-vlan: #optional pop VLAN headr from packet
+# @pop-vlan: pop VLAN headr from packet
 #
-# @group-ids: #optional list of next group IDs
+# @group-ids: list of next group IDs
 #
-# @set-eth-src: #optional set source MAC address in Ethernet header
+# @set-eth-src: set source MAC address in Ethernet header
 #
-# @set-eth-dst: #optional set destination MAC address in Ethernet header
+# @set-eth-dst: set destination MAC address in Ethernet header
 #
-# @ttl-check: #optional perform TTL check
+# @ttl-check: perform TTL check
 #
 # Note: optional members may or may not appear in the group depending
 # if they're relevant to the group type.
@@ -310,7 +310,7 @@
 #
 # @name: switch name
 #
-# @type: #optional group type.  If type is not specified, returns
+# @type: group type.  If type is not specified, returns
 # group information for all group types.
 #
 # Returns: rocker OF-DPA group information

qapi/trace.json

@@ -48,7 +48,7 @@
 # Query the state of events.
 #
 # @name: Event name pattern (case-sensitive glob).
-# @vcpu: #optional The vCPU to query (any by default; since 2.7).
+# @vcpu: The vCPU to query (any by default; since 2.7).
 #
 # Returns: a list of @TraceEventInfo for the matching events
 #
@@ -81,8 +81,8 @@
 #
 # @name: Event name pattern (case-sensitive glob).
 # @enable: Whether to enable tracing.
-# @ignore-unavailable: #optional Do not match unavailable events with @name.
-# @vcpu: #optional The vCPU to act upon (all by default; since 2.7).
+# @ignore-unavailable: Do not match unavailable events with @name.
+# @vcpu: The vCPU to act upon (all by default; since 2.7).
 #
 # An event's state is modified if:
 # - its name matches the @name pattern, and

qga/qapi-schema.json

@@ -144,7 +144,7 @@
 # If that's the case users are advised to always pass a
 # value.
 #
-# @time: #optional time of nanoseconds, relative to the Epoch
+# @time: time of nanoseconds, relative to the Epoch
 #        of 1970-01-01 in UTC.
 #
 # Returns: Nothing on success.
@@ -203,7 +203,7 @@
 # Initiate guest-activated shutdown. Note: this is an asynchronous
 # shutdown request, with no guarantee of successful shutdown.
 #
-# @mode: #optional "halt", "powerdown" (default), or "reboot"
+# @mode: "halt", "powerdown" (default), or "reboot"
 #
 # This command does NOT return a response on success. Success condition
 # is indicated by the VM exiting with a zero exit status or, when
@@ -222,7 +222,7 @@
 #
 # @path: Full path to the file in the guest to open.
 #
-# @mode: #optional open mode, as per fopen(), "r" is the default.
+# @mode: open mode, as per fopen(), "r" is the default.
 #
 # Returns: Guest file handle on success.
 #
@@ -270,7 +270,7 @@
 #
 # @handle: filehandle returned by guest-file-open
 #
-# @count: #optional maximum number of bytes to read (default is 4KB)
+# @count: maximum number of bytes to read (default is 4KB)
 #
 # Returns: @GuestFileRead on success.
 #
@@ -304,7 +304,7 @@
 #
 # @buf-b64: base64-encoded string representing data to be written
 #
-# @count: #optional bytes to write (actual bytes, after base64-decode),
+# @count: bytes to write (actual bytes, after base64-decode),
 #         default is all content in buf-b64 buffer after base64 decoding
 #
 # Returns: @GuestFileWrite on success.
@@ -441,7 +441,7 @@
 #
 # Sync and freeze specified guest filesystems
 #
-# @mountpoints: #optional an array of mountpoints of filesystems to be frozen.
+# @mountpoints: an array of mountpoints of filesystems to be frozen.
 #               If omitted, every mounted filesystem is frozen.
 #
 # Returns: Number of file systems currently frozen. On error, all filesystems
@@ -670,7 +670,7 @@
 #
 # @online: Whether the VCPU is enabled.
 #
-# @can-offline: #optional Whether offlining the VCPU is possible. This member
+# @can-offline: Whether offlining the VCPU is possible. This member
 #               is always filled in by the guest agent when the structure is
 #               returned, and always ignored on input (hence it can be omitted
 #               then).
@@ -858,7 +858,7 @@
 #
 # @online: Whether the MEMORY BLOCK is enabled in guest.
 #
-# @can-offline: #optional Whether offlining the MEMORY BLOCK is possible.
+# @can-offline: Whether offlining the MEMORY BLOCK is possible.
 #               This member is always filled in by the guest agent when the
 #               structure is returned, and always ignored on input (hence it
 #               can be omitted then).
@@ -911,7 +911,7 @@
 #
 # @response: the result of memory block operation.
 #
-# @error-code: #optional the error number.
+# @error-code: the error number.
 #               When memory block operation fails, we assign the value of
 #               'errno' to this member, it indicates what goes wrong.
 #               When the operation succeeds, it will be omitted.
@@ -979,16 +979,16 @@
 # @GuestExecStatus:
 #
 # @exited: true if process has already terminated.
-# @exitcode: #optional process exit code if it was normally terminated.
-# @signal: #optional signal number (linux) or unhandled exception code
+# @exitcode: process exit code if it was normally terminated.
+# @signal: signal number (linux) or unhandled exception code
 #       (windows) if the process was abnormally terminated.
-# @out-data: #optional base64-encoded stdout of the process
-# @err-data: #optional base64-encoded stderr of the process
+# @out-data: base64-encoded stdout of the process
+# @err-data: base64-encoded stderr of the process
 #       Note: @out-data and @err-data are present only
 #       if 'capture-output' was specified for 'guest-exec'
-# @out-truncated: #optional true if stdout was not fully captured
+# @out-truncated: true if stdout was not fully captured
 #       due to size limitation.
-# @err-truncated: #optional true if stderr was not fully captured
+# @err-truncated: true if stderr was not fully captured
 #       due to size limitation.
 #
 # Since: 2.5
@@ -1028,10 +1028,10 @@
 # Execute a command in the guest
 #
 # @path: path or executable name to execute
-# @arg: #optional argument list to pass to executable
-# @env: #optional environment variables to pass to executable
-# @input-data: #optional data to be passed to process stdin (base64 encoded)
-# @capture-output: #optional bool flag to enable capture of
+# @arg: argument list to pass to executable
+# @env: environment variables to pass to executable
+# @input-data: data to be passed to process stdin (base64 encoded)
+# @capture-output: bool flag to enable capture of
 #                  stdout/stderr of running process. defaults to false.
 #
 # Returns: PID on success.

scripts/qapi.py

@@ -219,6 +219,10 @@ class QAPIDoc(object):
         if (in_arg or not self.section.name
                 or not self.section.name.startswith("Example")):
             line = line.strip()
+        # TODO Drop this once the dust has settled
+        if (isinstance(self.section, QAPIDoc.ArgSection)
+                and '#optional' in line):
+            raise QAPISemError(self.info, "Please drop the #optional tag")
         self.section.append(line)
 
     def connect_member(self, member):
@@ -985,25 +989,6 @@ def check_definition_doc(doc, expr, info):
             or (meta == 'union' and not expr.get('discriminator'))):
         args.append('type')
 
-    for arg in args:
-        if arg[0] == '*':
-            opt = True
-            desc = doc.args.get(arg[1:])
-        else:
-            opt = False
-            desc = doc.args.get(arg)
-        if not desc:
-            continue
-        desc.optional = opt
-        desc_opt = "#optional" in str(desc)
-        if desc_opt and not opt:
-            raise QAPISemError(info, "Description has #optional, "
-                               "but the declaration doesn't")
-        if not desc_opt and opt:
-            # TODO either fix the schema and make this an error,
-            # or drop #optional entirely
-            pass
-
     doc_args = set(doc.args.keys())
     args = set([name.strip('*') for name in args])
     if not doc_args.issubset(args):

scripts/qapi2texi.py

@@ -145,8 +145,7 @@ def texi_members(doc, member_func, show_undocumented):
     for section in doc.args.itervalues():
         if not section.content and not show_undocumented:
             continue          # Undocumented TODO require doc and drop
-        desc = re.sub(r'^ *#optional *\n?|\n? *#optional *$|#optional',
-                      '', str(section))
+        desc = str(section)
         items += member_func(section.member) + texi_format(desc) + '\n'
     if not items:
         return ''

tests/Makefile.include

@@ -385,7 +385,6 @@ qapi-schema += doc-missing.json
 qapi-schema += doc-missing-colon.json
 qapi-schema += doc-missing-expr.json
 qapi-schema += doc-missing-space.json
-qapi-schema += doc-optional.json
 qapi-schema += double-data.json
 qapi-schema += double-type.json
 qapi-schema += duplicate-key.json

tests/qapi-schema/doc-optional.err

-tests/qapi-schema/doc-optional.json:3: Description has #optional, but the declaration doesn't

tests/qapi-schema/doc-optional.exit

-1

tests/qapi-schema/doc-optional.json

-# Description #optional should match declaration
-
-##
-# @foo:
-# @a: a #optional
-##
-{ 'command': 'foo', 'data': {'a': 'int'} }