Misc fixes for qrild #21

calebccff · 2022-07-18T14:19:33Z

Export the whole QMI header, and add qmi_tlv to libqrtr so projects using the "accessor" style QMI code generator don't have to vendor it.

I've made some changes to the qmi_tlv_* interface which are reflected in my fork of qmic: https://github.com/calebccff/qmic/tree/nested_structs, I'll also be opening a PR for that soon, any feedback on the qmi_tlv_* interface would be good before opening that.

Allow clients to read the whole packet header instead of just the message ID, also extend qmi_decode_header with a second getter to fetch all of the header properties

andersson · 2022-07-18T20:10:30Z

lib/libqrtr.h

 int qmi_decode_header(const struct qrtr_packet *pkt, unsigned int *msg_id);
+int qmi_decode_header2(const struct qrtr_packet *pkt, unsigned int *msg_id, unsigned char *type,


It seems that either you could expose the struct qmi_header and qmi_get_head() or implement this qmi_decode_header2(), I don't think we need both ways to do the same thing.

andersson · 2022-07-18T20:10:34Z

lib/qmi.c

+int qmi_decode_header(const struct qrtr_packet *pkt, unsigned int *msg_id)
+{
+	const struct qmi_header *qmi = qmi_get_header(pkt);
+
 	*msg_id = qmi->msg_id;


qmi_get_header() will return NULL on failure, in which cause this will fault. Better check for !qmi and return -EINVAL in this case.

andersson · 2022-07-18T20:12:19Z

lib/qmi.c

+{
+	const struct qmi_header *qmi = qmi_get_header(pkt);
+	if (!qmi)
+		return -1;


Currently these errors return -EINVAL, it could be argued that returning -1 and setting errno is more idiomatic - but I think you should stick to the current semantics.

andersson · 2022-07-18T20:19:01Z

lib/logging.h

@@ -38,7 +38,7 @@ void qlog(int priority, const char *format, ...) __PRINTF__(2, 3);
 	} while(0)

 #ifdef __cplusplus
-extern "C" {


This would indicate that patch 2 doesn't build. Can you please update that instead?

andersson · 2022-07-18T20:21:47Z

lib/logging.h

@@ -5,6 +5,10 @@
 #include <stdlib.h>
 #include <syslog.h>

+#ifdef __cplusplus


Please provide a commit message with a motivation of why this change is done - same with the last patch.

lumag · 2024-04-09T10:31:07Z

lib/qmi.c

 {
 	const struct qmi_header *qmi = pkt->data;

 	if (qmi->msg_len != pkt->data_len - sizeof(*qmi)) {
-		LOGW("[RMTFS] Invalid length of incoming qmi request\n");
-		return -EINVAL;


While being correct, this is unrelated to qmi_header rework. Please split to a separate commit.

lumag · 2024-04-09T10:33:02Z

lib/qmi_tlv.c

+	printf("<<<    msg_len : %u\n", pkt->msg_len);
+	printf("<<<    msg_id  : 0x%04x\n", pkt->msg_id);
+	printf("<<<    txn_id  : %u\n", pkt->txn_id);
+	printf("<<< TLVs:\n");


Please pass struct FILE to the function and use fprintf + fflush at the end.

lumag · 2024-04-09T10:33:27Z

lib/qmi_tlv.c

@@ -43,7 +43,6 @@ struct qmi_tlv *qmi_tlv_init(uint16_t txn, uint32_t msg_id, uint32_t msg_type)

 struct qmi_tlv *qmi_tlv_decode(void *buf, size_t len)
 {
-	struct qmi_header *pkt = buf;


Squash this commit into the previous one.

lumag · 2024-04-09T10:34:07Z

lib/libqrtr.h

+
+	return 0;
+}
+


I'd say, also squash

lumag · 2024-04-09T10:34:16Z

lib/libqrtr.h

+static inline int qmi_tlv_dump_buf(void *buf, size_t len) {
+	struct qmi_tlv *tlv = qmi_tlv_decode(buf, len);
+	if (!tlv)
+		return -1;


lumag · 2024-04-09T10:34:43Z

lib/qmi_tlv.c

@@ -254,10 +255,11 @@ void qmi_tlv_dump(struct qmi_tlv *tlv) {
 	printf("<<<    msg_id  : 0x%1$04x (%1$u)\n", pkt->msg_id);
 	printf("<<<    txn_id  : 0x%1$04x (%1$u)\n", pkt->txn_id);
 	printf("<<< TLVs:\n");
-	while (offset < tlv->size - sizeof(struct qmi_header)) {
+	// I do not understand why this -1 is needed


lumag · 2024-04-09T10:35:19Z

lib/qmi_tlv.c

+			for (; k < LINE_LENGTH; k++) {
+				line[li++] = ' ';
+				line[li++] = ' ';
+				line[li++] = ' ';


Is there a possibility to overflow it?

lumag · 2024-04-09T10:36:02Z

lib/libqrtr.h

 struct qmi_tlv *qmi_tlv_init(uint16_t txn, uint32_t msg_id, uint32_t msg_type);
 void *qmi_tlv_encode(struct qmi_tlv *tlv, size_t *len);
 struct qmi_tlv *qmi_tlv_decode(void *buf, size_t len);
 void qmi_tlv_free(struct qmi_tlv *tlv);
-void qmi_tlv_dump(struct qmi_tlv *tlv);


Oh, my. I started to like the function and now it is gone. Squash everything together so that the remnants of it won't make me mourn the loss each time I look at the git history.

lumag · 2024-04-09T10:38:13Z

lib/libqrtr.h

+struct qmi_tlv_msg_name {
+	int msg_id;
+	const char *msg_name;
+};


Who is the user of this struct?

calebccff · 2024-04-10T03:23:30Z

@lumag thanks for reviewing all this, I'll see if i can clean it up a bit.

the tlv dump feature can be kept i guess, it's certainly nice to have. although it's not immensely useful without the qmic changes to include the name of the message and of each value, otherwise you stil have to manually decode the output.

and at this point the full fat freedesktop libqmi is probably better.

lumag · 2024-05-21T08:22:01Z

@calebccff I think we are still using qrtr in some of our tools. Would you suggest switching to libqmi and possibly abandoning qrtr completely?

calebccff added 2 commits July 18, 2022 10:02

qmi: export packet header

ca18508

Allow clients to read the whole packet header instead of just the message ID, also extend qmi_decode_header with a second getter to fetch all of the header properties

lib/logging.h: extern C

d86dacc

andersson requested changes Jul 18, 2022

View reviewed changes

Add support for the qmi_tlv based QMI accessor

4246d2a

calebccff force-pushed the caleb/qrild-fixes branch from e12dfab to 4246d2a Compare July 19, 2022 14:30

qmi_tlv: fix dump and improve formatting

9e9b6ba

calebccff force-pushed the caleb/qrild-fixes branch 2 times, most recently from 7c8e14e to a354790 Compare August 3, 2022 15:11

tlv_dump: fix off-by-1, add string decode, add dump helper

8ade7c5

calebccff force-pushed the caleb/qrild-fixes branch from a354790 to 8ade7c5 Compare August 3, 2022 16:18

calebccff added 2 commits August 4, 2022 21:55

drop tlv_dump stuff, it should be implemented where needed

b501651

libqrtr: add qmi_tlv_msg_name to map msg_id to name

bec5bb2

konradybcio requested a review from andersson April 9, 2024 09:27

lumag requested changes Apr 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc fixes for qrild #21

Misc fixes for qrild #21

calebccff commented Jul 18, 2022

andersson Jul 18, 2022

lumag Apr 9, 2024

andersson Jul 18, 2022

andersson Jul 18, 2022

andersson Jul 18, 2022

andersson Jul 18, 2022

lumag Apr 9, 2024

lumag Apr 9, 2024

lumag Apr 9, 2024

lumag Apr 9, 2024

lumag Apr 9, 2024

lumag Apr 9, 2024

lumag Apr 9, 2024

lumag Apr 9, 2024

lumag Apr 9, 2024

calebccff commented Apr 10, 2024

lumag commented May 21, 2024

		int qmi_decode_header(const struct qrtr_packet pkt, unsigned int msg_id);
		int qmi_decode_header2(const struct qrtr_packet pkt, unsigned int msg_id, unsigned char *type,

Misc fixes for qrild #21

Are you sure you want to change the base?

Misc fixes for qrild #21

Conversation

calebccff commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebccff commented Apr 10, 2024

lumag commented May 21, 2024