Re: [PATCH 01/18] qapi: Smarter camel_to_upper() to reduce need for 'prefix'

[Markus Armbruster <armbru@redhat.com>]

Avihai, there's a question for you on VfioMigrationState.

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jul 30, 2024 at 10:10:15AM +0200, Markus Armbruster wrote:
>> camel_to_upper() converts its argument from camel case to upper case
>> with '_' between words.  Used for generated enumeration constant
>> prefixes.
>> 
>> When some of the words are spelled all caps, where exactly to insert
>> '_' is guesswork.  camel_to_upper()'s guesses are bad enough in places
>> to make people override them with a 'prefix' in the schema.
>> 
>> Rewrite it to guess better:
>> 
>> 1. Insert '_' after a non-upper case character followed by an upper
>>    case character:
>> 
>>        OneTwo -> ONE_TWO
>>        One2Three -> ONE2_THREE
>> 
>> 2. Insert '_' before the last upper case character followed by a
>>    non-upper case character:
>> 
>>        ACRONYMWord -> ACRONYM_Word
>> 
>>    Except at the beginning (as in OneTwo above), or when there is
>>    already one:
>> 
>>        AbCd -> AB_CD
>> 
>> This changes the default enumeration constant prefix for a number of
>> enums.  Generated enumeration constants change only where the default
>> is not overridden with 'prefix'.
>> 
>> The following enumerations without a 'prefix' change:
>> 
>>     enum       	     	 	    old camel_to_upper()
>>     				    new camel_to_upper()
>>     ------------------------------------------------------------------
>>     DisplayGLMode                   DISPLAYGL_MODE
>> 				    DISPLAY_GL_MODE
>>     EbpfProgramID                   EBPF_PROGRAMID
>> 				    EBPF_PROGRAM_ID
>>     HmatLBDataType                  HMATLB_DATA_TYPE
>> 				    HMAT_LB_DATA_TYPE
>>     HmatLBMemoryHierarchy           HMATLB_MEMORY_HIERARCHY
>> 				    HMAT_LB_MEMORY_HIERARCHY
>>     MultiFDCompression              MULTIFD_COMPRESSION
>> 				    MULTI_FD_COMPRESSION
>>     OffAutoPCIBAR                   OFF_AUTOPCIBAR
>> 				    OFF_AUTO_PCIBAR
>>     QCryptoBlockFormat              Q_CRYPTO_BLOCK_FORMAT
>> 				    QCRYPTO_BLOCK_FORMAT
>>     QCryptoBlockLUKSKeyslotState    Q_CRYPTO_BLOCKLUKS_KEYSLOT_STATE
>> 				    QCRYPTO_BLOCK_LUKS_KEYSLOT_STATE
>>     QKeyCode                        Q_KEY_CODE
>>     				    QKEY_CODE
>>     XDbgBlockGraphNodeType          X_DBG_BLOCK_GRAPH_NODE_TYPE
>> 				    XDBG_BLOCK_GRAPH_NODE_TYPE
>>     TestUnionEnumA		    TEST_UNION_ENUMA
>>     				    TEST_UNION_ENUM_A
>> 
>> Add a 'prefix' so generated code doesn't change now.  Subsequent
>> commits will remove most of them again.  Two will remain:
>> MULTIFD_COMPRESSION, because migration code generally spells "multifd"
>> that way, and Q_KEY_CODE, because that one is baked into
>> subprojects/keycodemapdb/tools/keymap-gen.
>> 
>> The following enumerations with a 'prefix' change so that the prefix
>> is now superfluous:
>> 
>>     enum       	     	 	    old camel_to_upper()
>>     				    new camel_to_upper() [equal to prefix]
>>     ------------------------------------------------------------------
>>     BlkdebugIOType                  BLKDEBUGIO_TYPE
>> 				    BLKDEBUG_IO_TYPE
>>     QCryptoTLSCredsEndpoint         Q_CRYPTOTLS_CREDS_ENDPOINT
>> 				    QCRYPTO_TLS_CREDS_ENDPOINT
>>     QCryptoSecretFormat             Q_CRYPTO_SECRET_FORMAT
>> 				    QCRYPTO_SECRET_FORMAT
>>     QCryptoCipherMode               Q_CRYPTO_CIPHER_MODE
>> 				    QCRYPTO_CIPHER_MODE
>>     QCryptodevBackendType           Q_CRYPTODEV_BACKEND_TYPE
>> 				    QCRYPTODEV_BACKEND_TYPE
>>     QType [builtin]                 Q_TYPE
>> 				    QTYPE
>> 
>> Drop these prefixes.
>> 
>> The following enumerations with a 'prefix' change without making the
>> 'prefix' superfluous:
>> 
>>     enum       	     	 	    old camel_to_upper()
>>     				    new camel_to_upper() [equal to prefix]
>> 				    prefix
>>     ------------------------------------------------------------------
>>     CpuS390Entitlement              CPUS390_ENTITLEMENT
>> 				    CPU_S390_ENTITLEMENT
>> 				    S390_CPU_ENTITLEMENT
>>     CpuS390Polarization             CPUS390_POLARIZATION
>> 				    CPU_S390_POLARIZATION
>> 				    S390_CPU_POLARIZATION
>>     CpuS390State                    CPUS390_STATE
>> 				    CPU_S390_STATE
>> 				    S390_CPU_STATE
>>     QAuthZListFormat                Q_AUTHZ_LIST_FORMAT
>> 				    QAUTH_Z_LIST_FORMAT
>> 				    QAUTHZ_LIST_FORMAT
>>     QAuthZListPolicy                Q_AUTHZ_LIST_POLICY
>> 				    QAUTH_Z_LIST_POLICY
>> 				    QAUTHZ_LIST_POLICY
>>     QCryptoAkCipherAlgorithm        Q_CRYPTO_AK_CIPHER_ALGORITHM
>> 				    QCRYPTO_AK_CIPHER_ALGORITHM
>> 				    QCRYPTO_AKCIPHER_ALG
>>     QCryptoAkCipherKeyType          Q_CRYPTO_AK_CIPHER_KEY_TYPE
>> 				    QCRYPTO_AK_CIPHER_KEY_TYPE
>> 				    QCRYPTO_AKCIPHER_KEY_TYPE
>>     QCryptoCipherAlgorithm          Q_CRYPTO_CIPHER_ALGORITHM
>> 				    QCRYPTO_CIPHER_ALGORITHM
>> 				    QCRYPTO_CIPHER_ALG
>>     QCryptoHashAlgorithm            Q_CRYPTO_HASH_ALGORITHM
>> 				    QCRYPTO_HASH_ALGORITHM
>> 				    QCRYPTO_HASH_ALG
>>     QCryptoIVGenAlgorithm           Q_CRYPTOIV_GEN_ALGORITHM
>> 				    QCRYPTO_IV_GEN_ALGORITHM
>> 				    QCRYPTO_IVGEN_ALG
>>     QCryptoRSAPaddingAlgorithm      Q_CRYPTORSA_PADDING_ALGORITHM
>> 				    QCRYPTO_RSA_PADDING_ALGORITHM
>> 				    QCRYPTO_RSA_PADDING_ALG
>>     QCryptodevBackendAlgType        Q_CRYPTODEV_BACKEND_ALG_TYPE
>> 				    QCRYPTODEV_BACKEND_ALG_TYPE
>> 				    QCRYPTODEV_BACKEND_ALG
>>     QCryptodevBackendServiceType    Q_CRYPTODEV_BACKEND_SERVICE_TYPE
>> 				    QCRYPTODEV_BACKEND_SERVICE_TYPE
>> 				    QCRYPTODEV_BACKEND_SERVICE
>> 
>> Subsequent commits will tweak things to remove most of these prefixes.
>> Only QAUTHZ_LIST_FORMAT and QAUTHZ_LIST_POLICY will remain.
>
> IIUC from above those two result in 
>
> 			    QAUTH_Z_LIST_FORMAT
> 			    QAUTH_Z_LIST_POLICY
>
> Is it possible to add a 3rd rule
>
>  *  Single uppercase letter folds into the previous word

I guess we could.

> or are there valid cases where we have a single uppercase
> that we want to preserve ?

Not now, but I'd prefer to leave predictions to economists.

> It sure would be nice to eliminate the 'prefix' concept,
> that we've clearly over-used, if we can kill the only 2
> remaining examples.

There are a few more, actually.  After this series and outside tests:

    enum       	     	 	    default prefix camel_to_upper()
				    prefix override
    ------------------------------------------------------------------
    BlkdebugEvent                   BLKDEBUG_EVENT
                                    BLKDBG
    IscsiHeaderDigest               ISCSI_HEADER_DIGEST
                                    QAPI_ISCSI_HEADER_DIGEST
    MultiFDCompression              MULTI_FD_COMPRESSION
                                    MULTIFD_COMPRESSION
    QAuthZListFormat                QAUTH_Z_LIST_FORMAT
				    QAUTHZ_LIST_FORMAT
    QAuthZListPolicy                QAUTH_Z_LIST_POLICY
				    QAUTHZ_LIST_POLICY
    QKeyCode                        QKEY_CODE
                                    Q_KEY_CODE
    VfioMigrationState              VFIO_MIGRATION_STATE
                                    QAPI_VFIO_MIGRATION_STATE

Reasons for 'prefix', and what could be done instead of 'prefix':

* BlkdebugEvent: shorten the prefix.

  Could live with the longer names instead.  Some 90 occurences...

* IscsiHeaderDigest

  QAPI version of enum iscsi_header_digest from libiscsi's
  iscsi/iscsi.h.  We use 'prefix' to avoid name clashes.

  Could rename the type to QapiIscsiHeaderDigest instead.

* MultiFDCompression

  Migration code consistently uses prefixes multifd_, MULTIFD_, and
  MultiFD_.

  Could rename the type to MultifdCompression instead, but that just
  moves the inconsistency to the type name.

* QAuthZListFormat and QAuthZListPolicy

  The authz code consistently uses QAuthZ.

  Could make camel_to_upper() avoid the lone Z instead (and hope that'll
  remain what we want).

* QKeyCode

  Q_KEY_CODE is baked into subprojects/keycodemapdb/tools/keymap-gen.

  Could adjust the subproject instead.

* VfioMigrationState

  Can't see why this one has a prefix.  Avihai, can you enlighten me?

Daniel, thoughts?

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qapi/block-core.json                     |  3 +-
>>  qapi/common.json                         |  1 +
>>  qapi/crypto.json                         |  6 ++--
>>  qapi/cryptodev.json                      |  1 -
>>  qapi/ebpf.json                           |  1 +
>>  qapi/machine.json                        |  1 +
>>  qapi/migration.json                      |  1 +
>>  qapi/ui.json                             |  2 ++
>>  scripts/qapi/common.py                   | 42 ++++++++++++++----------
>>  scripts/qapi/schema.py                   |  2 +-
>>  tests/qapi-schema/alternate-array.out    |  1 -
>>  tests/qapi-schema/comments.out           |  1 -
>>  tests/qapi-schema/doc-good.out           |  1 -
>>  tests/qapi-schema/empty.out              |  1 -
>>  tests/qapi-schema/include-repetition.out |  1 -
>>  tests/qapi-schema/include-simple.out     |  1 -
>>  tests/qapi-schema/indented-expr.out      |  1 -
>>  tests/qapi-schema/qapi-schema-test.json  |  1 +
>>  tests/qapi-schema/qapi-schema-test.out   |  2 +-
>>  19 files changed, 37 insertions(+), 33 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks!