lixa-general Mailing List for lixa

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

Dear Lawrence,I'm replying you through LIXA mailing list because I like to keep this type of discussion in the public space.
Here is my review about your proposed changes:
Structural notes:
1. don't create a couple of new files (tx_lixa.c/h) for your extension, they can be put directly in tx.c/tx.h
    by the way, there are already some extensions contributed by Globetom some years ago like tx_join, tx_resume, etc..

Interface notes:
1. I don't like the idea of overloading the original tx_open and creating a "black swan" function named tx_open_something()
    I don't like the idea of passing an array of structs to the function just for configuration: in the end, the developer would have to
      allocate the array static/dynamic
      populate the array because the source information probably comes from a different place
      pass the array to the function
      call tx_open_something()
      destroy the array that is not anymore useful
   to stay aligned with the original TX design, I prefer an interface splitted in two parts:
      tx_add_resource_manager(...);
      tx_open();
   tx_add_resource_manager() will be coherent with similar standard functions like tx_set_transaction_control(), tx_set_transaction_timeout(), etc...
      following the above approach, the resulting application code will be:

      tx_add_resource_manager(...); /* any number of times, depending on the use case */
      tx_open();

   tx_add_resource_manager(...) will save the data in the thread context (client_status_t)
   tx_open() will pick-up the additional resource managers from the thread context (client_status_t), if any

   after all the above, the signature of tx_add_resource_manager() becomes:

   extern int tx_add_resource_manager(const char *profile, const char *name, const char *switch_file, const char *xa_open_info, const char *xa_close_info);

   the return code must be in the set of TX standard return codes, see file tx.h. Examples: TX_OK, TX_ERROR. It must *not* be a LIXA_RC_something

   Warnings: the final destination is a struct of type rsrmgr_config_s
      you have to use proper functions to duplicate to xmlChar destinations for name and switch_file, see http://xmlsoft.org/html/libxml-xmlstring.html#xmlCharStrdup  
      you have to pay attention xa_open_info and xa_close_info are defined as char[MAXINFOSIZE] by the XA specification
      as you proposed with tx_open_lixa(), don't put logic in tx.c because it's just an adapter: real logic must be in a new
      function lixa_tx_add_resource_manager in file lixa_tx.c

2. the same principle applies to tx_begin_lixa(): at the end of the day, the application want to specify a prefix that many times will stay stable in a context
   I prefer an interface splitted in two parts and aligned with TX original design:
      tx_set_gtrid_prefix();
      tx_begin();
   even in this case, tx_set_gtrid_prefix() will store the prefix in the context of the thread and can be reused by as many tx_begin() calls as necessary, without the burden to scatter the prefix in different parts of the application program. That's a typical info coming from the configuration, while tx_begin() is a function you typically call from a piece of business logic.

   the signature becomes:

   extern int tx_set_gtrid_prefix(const char *gtrid_prefix, long gtrid_prefix_length);

   as above the return code must be TX_* as described in file tx.h 

   Warnings: the XA specification REQUIRES the XID to be a BINARY arrays, an array of octets, NOT a string of characters, even if it's implemented using "char" type instead of something like "unsigned char" or "byte_t"
      This is why LIXA traces it in exadecimal format; don't mind PostgreSQL uses the ASCII subset, the XA specification is much more generic

   Once you have the gtrid_prefix and gtrid_prefix_length in the thread context (client_status_t) you must intercept XID generation and override the first gtrid_prefix_length byte with memcpy...
   PAY ATTENTION gtrid_prefix_length MUST NOT exceed the "gtrid_length" so the algorith should be:
      - generate the XID with the current algorithm
      - if(gtrid_prefix_length > 0)
            replace the first N bytes where N = min(XID.gtrid_length, gtrid_prefix_length)
   WARNING: tainting the "bqual" part of the XID can produce UNEXPECTED behaviors

3. with all the above, creating the equivalent COBOL TX interface for tx_add_resource_manager() and tx_set_gtrid_prefix() will be quite straighforward if someone asks for it
4. the new functions must be documented in the LIXA reference manual (docbook format) like these ones https://www.tiian.org/lixa/manuals/html/ch07.html 
5. the new functions should carry at least one sample program to put in doc/examples directory
6. the new functions must be tested with dedicated test cases tx_....at; from the description in the email you are implementing a non trivial use case:
   ALL THE POSSIBLE CODE PATHS MUST BE COVERED BY TEST CASES AND DOUBLE CHECKED FOR MEMORY LEAKS

Coding style notes:
1. XA and TX standard have their own coding style, CaMeLiZaTion is never used in the LIXA C source code and
   not used by XA and TX specification:
   rmNname --> rm_name
   rmSwitchFile --> rm_switch_file
   and so on...
2. don't put "_" in front of a type or a variable, that's typically dedicated to internal special stuff like __GNUC__ and others
3. LIXA_TRACE has a strict format:
   - the message prefix is the name of the function followed by a colon like "lixa_tx_open:      " otherwise, DIFFICULT to understand where the message comes from
   - strings are typically surronded by single quotes like '%s' to figure they are values and not constants when the trace must be read
   - numbers are typically wrote without quotes like %d (it's unlikely there are constant numbers in trace messages)

Syntax notes:
1. Doxygen autodocumentation requires strict match between the name of the parameter in the functon declaration and the same name in the documentation comment
2. Doxygen comments must stay only in .h files not in both .h and .c
3. When a function throw the same "exception" several times, it MUST be enumerated, otherwise NO way to understand the exception point from the trace file.
   Example:
        bad
                    if (NULL == (rsrmgr = g_try_malloc(
                                    sizeof(struct rsrmgr_config_s))))
                        THROW(G_TRY_MALLOC_ERROR);
                    if (NULL == (act_rsrmgr = g_try_malloc(
                                    sizeof(struct act_rsrmgr_config_s))))
                        THROW(G_TRY_MALLOC_ERROR);
        good
                    if (NULL == (rsrmgr = g_try_malloc(
                                    sizeof(struct rsrmgr_config_s))))
                        THROW(G_TRY_MALLOC_ERROR1);
                    if (NULL == (act_rsrmgr = g_try_malloc(
                                    sizeof(struct act_rsrmgr_config_s))))
                        THROW(G_TRY_MALLOC_ERROR2);

Process notes:
1. as soon as your code reach enough maturity, fork the master trunk of the source code on github and merge from the master
   as frequently as possible before creating the pull request
2. I'm currently working on version 1.9.3; it basically contains refinements like CentOS 8 support and it could be a good candidate to host your extension
   Alternatively, a new 1.11.0 could be created to underline the new capabilities
3. sanitize the code using valgrind, the automatic testing suite is already instrumented, just activate the env var
      SERVER_CHECK_TYPE=memory  (see script functions tests/lixa_test_functions.sh)
   The critical point of your extension is manipulating the list of resource managers defined in the thread context: it's very
   easy to introduce memory leaks in that piece of code. In the past it required several fixes.
   The other change tx_set_gtrid_prefix() is easier and safer to implement.
Hoping it will help.Kind RegardsCh.F.

   ----- Messaggio inoltrato -----Inviato: martedì 20 ottobre 2020, 10:07:10 CESTOggetto: LIXA changes for Resource Manager definitions

Dear Christian,

I attach some updated files for your comments. These are based on v1.9.0.

I have added a new function tx_open_lixa() which has parameters to specify (a) an optional profile name and (b) a list of Resource Managers.

·        The profile name acts like, and overrides, the LIXA_PROFILE environment variable. This is done by passing the profile from tx_open_lixa() to client_config().

·        If specified, the list of RMs is then added to the RMs loaded from the profile. These are added as ‘dynamically-defined’ RMs.

·        The RM list entry (see tx_lixa.h) contains the same as in the RM definition in lixac_conf.xml: Name, SwitchFile, OpenInfo and CloseInfo.

·        If Name is specified, it must exist as a definition in lixac_conf, and from that definition the switch_file, open_info and close_info can be overridden if desired. The act_rsrmgr is created and linked to the existing rsrmgr.

·        If no name is specified (NULL), then the other 3 parameters must be specified. An ‘anonymous’ rsrmgr structure is created, and the act_rsrmgr links to that.

So the result is similar to the enlist function in xta.

The tx_close() function then removes the dynamically-defined act_rsrmgr objects, and any anonymous rsrmgr objects.

The main code for this has been added to lixa_tx_open() in lixa_tx.c, which now has profile and RM list parameters. You can search for “Wilkinson” in this file to find what I have added, but I will copy the important parts at the bottom of this email.

The tx_lixa.h file also includes a new tx_begin function with a GTrid parameter, but I have not started on that yet.

I still need to do a check for leaks and other errors, but I would like to verify that I haven’t done anything stupid with my changes and that you are happy with function names etc.

Best regards,

Lawrence

ypedef struct _RMspec {

    const char* rmName;         /* Must be specified */

    const char* rmSwitchFile;   /* Optional if rmName is in profile */

    const char* rmOpenInfo;     /* ditto */

    const char* rmCloseInfo;    /* ditto */

} RMspec;

extern int tx_open_lixa( const char* profile, int numRM, const RMspec *listRM );

int lixa_tx_open(int *txrc, int mmode, const char* profile, int numRM, const RMspec *listRM)

(numRM and listRM are the user’s RM list)

            /* Add user-supplied RMs */

            int iRM;

            if (NULL != listRM) {

                for (iRM = 0; iRM < numRM; iRM++) {

                    struct rsrmgr_config_s *rsrmgr = NULL;      // Meant for Xml, but we use it here

                    struct act_rsrmgr_config_s *act_rsrmgr = NULL;

                    const RMspec *currentRM = &listRM[iRM];

                    /* allocate a new record for the Resource Manager description */

                    if (NULL == (rsrmgr = g_try_malloc(

                                    sizeof(struct rsrmgr_config_s))))

                        THROW(G_TRY_MALLOC_ERROR);

                    if (NULL == (act_rsrmgr = g_try_malloc(

                                    sizeof(struct act_rsrmgr_config_s))))

                        THROW(G_TRY_MALLOC_ERROR);

                    /* Populate the rsrmgr_config_s from the RM in the list */

                    /* First try to find an existing rsrmgr definition */

                    if (currentRM->rmName) {

                        rsrmgr->name = NULL;    /* Flag we have not found it yet */

                        /* Loop through specified rsrmgrs */

                        int i;

                        for (i = 0; i < global_ccc.rsrmgrs->len; ++i) {

                            struct rsrmgr_config_s *templateRsrmgr =

                                (struct rsrmgr_config_s *)

                                g_ptr_array_index(global_ccc.rsrmgrs, i);

                            if (strcmp((char*)templateRsrmgr->name, currentRM->rmName) == 0) {

                                /* Found it - duplicate it and use it as a template */

                                if (NULL == (rsrmgr->name = (xmlChar*)strdup( currentRM->rmName ))) {

                                    g_free(rsrmgr);

                                    THROW(STRDUP_ERROR);

                                }

                                if (currentRM->rmSwitchFile) {

                                    if (NULL == (rsrmgr->switch_file = (xmlChar*)strdup( currentRM->rmSwitchFile ))) {

                                        g_free(rsrmgr);

                                        THROW(STRDUP_ERROR);

                                    }

                                } else {

                                    if (NULL == (rsrmgr->switch_file = xmlStrdup( templateRsrmgr->switch_file ))) {

                                        g_free(rsrmgr);

                                        THROW(XML_STRDUP_ERROR);

                                    }

                                }

                                if (currentRM->rmOpenInfo) {

                                    strncpy( rsrmgr->xa_open_info, currentRM->rmOpenInfo, sizeof(rsrmgr->xa_open_info) );

                                } else {

                                    rsrmgr->xa_open_info[0] = 0;

                                }

                                if (currentRM->rmCloseInfo) {

                                    strncpy( rsrmgr->xa_close_info, currentRM->rmCloseInfo, sizeof(rsrmgr->xa_close_info) );

                                } else {

                                    rsrmgr->xa_close_info[0] = 0;

                                }

                                LIXA_TRACE(("RM %s (%s) created\n", rsrmgr->name, rsrmgr->xa_open_info));

                                break;

                            }

                        }

                        if (rsrmgr->name == NULL) {

                            /* Not found - error */

                            LIXA_TRACE(("RM %s not defined\n", currentRM->rmName));

                            THROW(INVALID_RM_ERROR);

                        }

                    } else {

                        rsrmgr->name = NULL;    /* Anonymous */

                        rsrmgr->switch_file = (xmlChar*)strdup( currentRM->rmSwitchFile );

                        strncpy( rsrmgr->xa_open_info, currentRM->rmOpenInfo, sizeof(rsrmgr->xa_open_info) );

                        strncpy( rsrmgr->xa_close_info, currentRM->rmCloseInfo, sizeof(rsrmgr->xa_close_info) );

                        LIXA_TRACE(("RM %s created", rsrmgr->switch_file));

                    }

                    act_rsrmgr->generic = rsrmgr;

                    act_rsrmgr->module = NULL;

                    memset(&act_rsrmgr->lixa_iface, 0, sizeof(lixa_iface_t));

                    act_rsrmgr->dynamically_defined = TRUE;

                    /* load the switch file for the resource manager,

                    * set it as "dynamically defined"  */

                    if (LIXA_RC_OK != (ret_cod = client_config_load_switch_file(

                                        act_rsrmgr,

                                        TRUE)))

                        THROW(CLIENT_CONFIG_LOAD_SWITCH_FILE_ERROR);

                    client_config_display_rsrmgr( act_rsrmgr );

                    /* append the resource manager to the list of actual configured

                    resource managers */

                    client_config_append_rsrmgr(&global_ccc, rsrmgr,

                                                act_rsrmgr);

                }

            }

lixa_tx_close()

        /* remove dynamically added RMs */

        int i;

        for (i = 0; i < global_ccc.actconf.rsrmgrs->len; i++) {

            struct act_rsrmgr_config_s *act_rsrmgr =

                (struct act_rsrmgr_config_s *)

                g_ptr_array_index(global_ccc.rsrmgrs, i);

            if (act_rsrmgr->dynamically_defined) {

                ret_cod = client_config_unload_switch_file(

                                       act_rsrmgr);

                if (NULL == act_rsrmgr->generic->name) {

                    // Anonymous RM - remove it

                    g_ptr_array_remove(global_ccc.rsrmgrs, act_rsrmgr->generic);

                    g_free(act_rsrmgr->generic);

                }

                g_ptr_array_remove_index(global_ccc.actconf.rsrmgrs, i);

                g_free(act_rsrmgr);

            }

        }

int client_config(client_config_coll_t *ccc, int global_config, const char* profile)

(profile is the parameter from tx_open_lixa())

        if (profile) {

            LIXA_TRACE(("client_config: using transactional profile '%s' for "

                        "subsequent operations\n", profile));

            if (NULL == (ccc->profile = g_strdup(profile)))

                THROW(G_STRDUP_ERROR);

        } else if (NULL == (tmp_str = getenv(LIXA_PROFILE_ENV_VAR))) {

            /* use empty string instead of NULL to avoid allocation issues */

            ccc->profile = tmp_str;

            LIXA_TRACE(("client_config: '%s' environment variable not found, "

                        "using default profile for this client\n",

                        LIXA_PROFILE_ENV_VAR));

        } else {

            LIXA_TRACE(("client_config: using transactional profile '%s' for "

                        "subsequent operations\n", tmp_str));

            if (NULL == (ccc->profile = g_strdup(tmp_str)))

                THROW(G_STRDUP_ERROR);

        }

_______________________________________

Lawrence Wilkinson
Senior Developer

LzLabs GmbH
Richtiarkade 16
CH-8304 Wallisellen
Switzerland

web      www.lzlabs.com

2011	Jan	Feb	Mar	Apr	May	Jun	Jul	Aug	Sep	Oct	Nov (1)	Dec
2012	Jan (2)	Feb (1)	Mar (3)	Apr (3)	May (5)	Jun	Jul	Aug	Sep	Oct	Nov	Dec
2014	Jan	Feb	Mar	Apr	May (2)	Jun	Jul	Aug	Sep	Oct (1)	Nov	Dec
2016	Jan	Feb	Mar	Apr	May	Jun	Jul	Aug	Sep	Oct	Nov	Dec (5)
2017	Jan (3)	Feb	Mar	Apr	May	Jun (3)	Jul	Aug	Sep	Oct	Nov	Dec
2019	Jan (1)	Feb	Mar	Apr	May	Jun	Jul	Aug	Sep	Oct	Nov	Dec
2020	Jan	Feb	Mar	Apr	May	Jun	Jul	Aug	Sep	Oct (4)	Nov	Dec

lixa-general Mailing List for lixa

LIXA, LIbre XA, is a free and open source XA transaction manager

lixa-general — General discussion related to LIXA project