Attention is currently required from: laforge, pespin, fixeria, dexter.

View Change

11 comments:

• Patchset:

  • Patch Set #11:

    how does it work that only one phy ops entry is added?
    ...by only linking one of the phy implementation .c files, right?

• File src/osmobts_sock.c:

  • Patch Set #11, Line 114: the_pcu->phy_ops->l1if_close_pdch(bts->trx[trx].fl1h);

    if (the_pcu->phy_ops->l1if_close_pdch)
            the_pcu->phy_ops->l1if_close_pdch(bts->trx[trx].fl1h);
     
    in general, cb functions may be left NULL if nothing should happen

• File src/pcu_l1_if.cpp:

  • Patch Set #11, Line 218: the_pcu->phy_ops->l1if_pdch_req(bts->trx[trx].fl1h, ts, 0, fn, arfcn, block_nr,

    if (...
     
    check against NULL cb

  • Patch Set #11, Line 247: the_pcu->phy_ops->l1if_pdch_req(bts->trx[trx].fl1h, ts, 1, fn, arfcn, block_nr, data, data_len);

    check NULL cb

  • Patch Set #11, Line 869: if (!bts->trx[trx_nr].fl1h)

    && cb != NULL

• File src/pcu_l1_if_phy.c:

  • Patch Set #11, Line 2: * (C) 2023 by sysmocom s.f.m.c. GmbH <info@sysmocom.de>

    back in the days Holger asked me to add a dash

      sysmocom - s.f.m.c. GmbH

  • Patch Set #11, Line 51: l1if_phy_entry = talloc_zero(NULL, struct l1if_phy_entry);

    (i wonder if -fsanitize will report this as leaked memory whenever osmo-pcu exits .. i guess it would be better to either add a ctx arg to talloc the entry from, or to add a pcu_l1if_cleanup() that talloc_free()s all list entries that the main() calls before exiting)

  • Patch Set #11, Line 71: LOGPC(DLGLOBAL, LOGL_NOTICE, "\n");

    please let's never ever use LOGPC() anymore! It was a messed up concept from the start, and in particular is very awkward in gsmtap_log.

    either a static buffer with OSMO_STRBUF_PRINTF() or

      char *msg = NULL;
      llist_for... {
         osmo_talloc_asprintf(OTC_SELECT, msg, "foo");
      }
      LOGP(msg)

    and maybe add, at the start,

      if (!log_check_level(DLGLOBAL, LOGL_NOTICE))
         return;

  • Patch Set #11, Line 75: ops

    suggest name

      singleton_ops

    ...but seems to me the task this return arg solves could be achieved more elegantly?
    I'd like to see some code that acts on non-singleton phy lists, i guess will come up in another patch.

  • Patch Set #11, Line 81: *ops = NULL;

    let's add

            /* make sure a singleton is not mixed with other PHY */
     	llist_for_each_entry(l1if_phy_entry, &phy_list, list)
    		if (l1if_phy_entry->ops->singleton)
    		         OSMO_ASSERT(llist_count(&phy_list) == 1);
    		         
    without this check, below loop would initialize other non-singleton phy before exiting the list on the first singleton phy, and would skip any other entries ... i.e. act weird without complaining.

  • Patch Set #11, Line 100: if (*ops) {

    so the indication whether a phy is in use depends on the argument that the caller passes in to be zero initialized? seems awkward to me / gives weird errors when forgetting to zero initialize. When i see non-singleton code i might have a suggestion for a more elegant solution... for now i can only complain =)

To view, visit change 31341. To unsubscribe, or for help writing mail filters, visit settings.