Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc fixes for qrild #21

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Misc fixes for qrild #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ca18508
qmi: export packet header
calebccff Jun 7, 2022
d86dacc
lib/logging.h: extern C
calebccff Jul 18, 2022
4246d2a
Add support for the qmi_tlv based QMI accessor
calebccff Jul 18, 2022
9e9b6ba
qmi_tlv: fix dump and improve formatting
calebccff Jul 27, 2022
8ade7c5
tlv_dump: fix off-by-1, add string decode, add dump helper
calebccff Aug 3, 2022
b501651
drop tlv_dump stuff, it should be implemented where needed
calebccff Aug 4, 2022
bec5bb2
libqrtr: add qmi_tlv_msg_name to map msg_id to name
calebccff Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ $(proj)-lookup-cflags := -Ilib

lib$(proj).so-srcs := \
lib/logging.c \
lib/qmi_tlv.c \
lib/qrtr.c \
lib/qmi.c

Expand Down
49 changes: 49 additions & 0 deletions lib/libqrtr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ extern "C" {
#endif

struct sockaddr_qrtr;
struct qmi_tlv;

struct qrtr_packet {
int type;
Expand All @@ -40,6 +41,20 @@ struct qrtr_packet {
size_t data_len;
};

/**
* qmi_header - wireformat header of QMI messages
* @type: type of message
* @txn_id: transaction id
* @msg_id: message id
* @msg_len: length of message payload following header
*/
struct qmi_header {
uint8_t type;
uint16_t txn_id;
uint16_t msg_id;
uint16_t msg_len;
} __attribute__((packed));

#define DEFINE_QRTR_PACKET(pkt, size) \
char pkt ## _buf[size]; \
struct qrtr_packet pkt = { .data = pkt ##_buf, \
Expand Down Expand Up @@ -141,14 +156,48 @@ int qrtr_poll(int sock, unsigned int ms);
int qrtr_decode(struct qrtr_packet *dest, void *buf, size_t len,
const struct sockaddr_qrtr *sq);

const struct qmi_header *qmi_get_header(const struct qrtr_packet *pkt);
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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

unsigned short *txn_id);
int qmi_decode_message(void *c_struct, unsigned int *txn,
const struct qrtr_packet *pkt,
int type, int id, struct qmi_elem_info *ei);
ssize_t qmi_encode_message(struct qrtr_packet *pkt, int type, int msg_id,
int txn_id, const void *c_struct,
struct qmi_elem_info *ei);

struct qmi_tlv {
void *allocated;
void *buf;
size_t size;
int error;
};

struct qmi_tlv_item {
uint8_t key;
uint16_t len;
uint8_t data[];
} __attribute__((__packed__));

struct qmi_tlv_msg_name {
int msg_id;
const char *msg_name;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who is the user of this struct?


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_get(struct qmi_tlv *tlv, uint8_t id, size_t *len);
void *qmi_tlv_get_array(struct qmi_tlv *tlv, uint8_t id, size_t len_size,
size_t *len, size_t *size);
int qmi_tlv_set(struct qmi_tlv *tlv, uint8_t id, void *buf, size_t len);
int qmi_tlv_set_array(struct qmi_tlv *tlv, uint8_t id, size_t len_size,
void *buf, size_t len, size_t size);
struct qmi_response_type_v01 *qmi_tlv_get_result(struct qmi_tlv *tlv);


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say, also squash

/* Initial kernel header didn't expose these */
#ifndef QRTR_NODE_BCAST

Expand Down
8 changes: 8 additions & 0 deletions lib/logging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
#include <stdlib.h>
#include <syslog.h>

#ifdef __cplusplus
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

extern "C" {
#endif

#if defined(__GNUC__) || defined(__clang__)
#define __PRINTF__(fmt, args) __attribute__((format(__printf__, fmt, args)))
#else
Expand Down Expand Up @@ -33,4 +37,8 @@ void qlog(int priority, const char *format, ...) __PRINTF__(2, 3);
exit(1); \
} while(0)

#ifdef __cplusplus
}
#endif

#endif
45 changes: 27 additions & 18 deletions lib/qmi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,6 @@
#include "libqrtr.h"
#include "logging.h"

/**
* qmi_header - wireformat header of QMI messages
* @type: type of message
* @txn_id: transaction id
* @msg_id: message id
* @msg_len: length of message payload following header
*/
struct qmi_header {
uint8_t type;
uint16_t txn_id;
uint16_t msg_id;
uint16_t msg_len;
} __attribute__((packed));


#define QMI_ENCDEC_ENCODE_TLV(type, length, p_dst) do { \
*p_dst++ = type; \
*p_dst++ = ((uint8_t)((length) & 0xFF)); \
Expand Down Expand Up @@ -794,20 +779,44 @@ ssize_t qmi_encode_message(struct qrtr_packet *pkt, int type, int msg_id,
return pkt->data_len;
}

int qmi_decode_header(const struct qrtr_packet *pkt, unsigned int *msg_id)
const struct qmi_header *qmi_get_header(const struct qrtr_packet *pkt)
{
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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LOGW("[QRTR] Invalid length of incoming qmi request\n");
return NULL;
}

return qmi;
}

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return 0;
}

int qmi_decode_header2(const struct qrtr_packet *pkt, unsigned int *msg_id, unsigned char *type,
unsigned short *txn_id)
{
const struct qmi_header *qmi = qmi_get_header(pkt);
if (!qmi)
return -1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (type)
*type = qmi->type;
if (msg_id)
*msg_id = qmi->msg_id;
if (txn_id)
*txn_id = qmi->txn_id;

return 0;
}

/**
* qmi_decode_message() - Decode QMI encoded message to C structure
* @buf: Buffer with encoded message
Expand Down
Loading