RE: [PATCH 03/13] rasdaemon: cxl: Add automatic indexing for storing CXL fields in SQLite database

[Shiju Jose <shiju.jose@huawei.com>]

>-----Original Message-----
>From: Jonathan Cameron <jonathan.cameron@huawei.com>
>Sent: 21 November 2024 15:17
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-edac@vger.kernel.org; linux-cxl@vger.kernel.org;
>mchehab@kernel.org; dave.jiang@intel.com; dan.j.williams@intel.com;
>alison.schofield@intel.com; nifan.cxl@gmail.com; vishal.l.verma@intel.com;
>ira.weiny@intel.com; dave@stgolabs.net; linux-kernel@vger.kernel.org;
>Linuxarm <linuxarm@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>;
>Zengtao (B) <prime.zeng@hisilicon.com>
>Subject: Re: [PATCH 03/13] rasdaemon: cxl: Add automatic indexing for storing
>CXL fields in SQLite database
>
>On Wed, 20 Nov 2024 09:59:13 +0000
><shiju.jose@huawei.com> wrote:
>
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> When the CXL specification adds new fields to the common header of CXL
>> event records, manual updates to the indexing are required to store
>> these CXL fields in the SQLite database. This update introduces
>> automatic indexing to facilitate the storage of CXL fields in the
>> SQLite database, eliminating the need for manual update to indexing.
>>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>
>Using 0 as an error code seems odd, maybe a negative instead?
>With that changed to say -1 then this looks good to me.

Will change error code to -1

Thanks,
Shiju
>
>Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
>
>> ---
>>  ras-record.c | 134
>> ++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 74 insertions(+), 60 deletions(-)
>>
>> diff --git a/ras-record.c b/ras-record.c index b4a1abd..9e68158 100644
>> --- a/ras-record.c
>> +++ b/ras-record.c
>> @@ -780,23 +780,25 @@ int ras_store_cxl_overflow_event(struct
>> ras_events *ras, struct ras_cxl_overflow
>>
>>  static int ras_store_cxl_common_hdr(sqlite3_stmt *stmt, struct
>> ras_cxl_event_common_hdr *hdr)  {
>> +	int idx = 1;
>> +
>>  	if (!stmt || !hdr)
>>  		return 0;
>>
>> -	sqlite3_bind_text(stmt, 1, hdr->timestamp, -1, NULL);
>> -	sqlite3_bind_text(stmt, 2, hdr->memdev, -1, NULL);
>> -	sqlite3_bind_text(stmt, 3, hdr->host, -1, NULL);
>> -	sqlite3_bind_int64(stmt, 4, hdr->serial);
>> -	sqlite3_bind_text(stmt, 5, hdr->log_type, -1, NULL);
>> -	sqlite3_bind_text(stmt, 6, hdr->hdr_uuid, -1, NULL);
>> -	sqlite3_bind_int(stmt, 7, hdr->hdr_flags);
>> -	sqlite3_bind_int(stmt, 8, hdr->hdr_handle);
>> -	sqlite3_bind_int(stmt, 9, hdr->hdr_related_handle);
>> -	sqlite3_bind_text(stmt, 10, hdr->hdr_timestamp, -1, NULL);
>> -	sqlite3_bind_int(stmt, 11, hdr->hdr_length);
>> -	sqlite3_bind_int(stmt, 12, hdr->hdr_maint_op_class);
>> -
>> -	return 0;
>> +	sqlite3_bind_text(stmt, idx++, hdr->timestamp, -1, NULL);
>> +	sqlite3_bind_text(stmt, idx++, hdr->memdev, -1, NULL);
>> +	sqlite3_bind_text(stmt, idx++, hdr->host, -1, NULL);
>> +	sqlite3_bind_int64(stmt, idx++, hdr->serial);
>> +	sqlite3_bind_text(stmt, idx++, hdr->log_type, -1, NULL);
>> +	sqlite3_bind_text(stmt, idx++, hdr->hdr_uuid, -1, NULL);
>> +	sqlite3_bind_int(stmt, idx++, hdr->hdr_flags);
>> +	sqlite3_bind_int(stmt, idx++, hdr->hdr_handle);
>> +	sqlite3_bind_int(stmt, idx++, hdr->hdr_related_handle);
>> +	sqlite3_bind_text(stmt, idx++, hdr->hdr_timestamp, -1, NULL);
>> +	sqlite3_bind_int(stmt, idx++, hdr->hdr_length);
>> +	sqlite3_bind_int(stmt, idx++, hdr->hdr_maint_op_class);
>> +
>> +	return idx;
>>  }
>>
>>  /*
>> @@ -827,15 +829,18 @@ static const struct db_table_descriptor
>> cxl_generic_event_tab = {
>>
>>  int ras_store_cxl_generic_event(struct ras_events *ras, struct
>> ras_cxl_generic_event *ev)  {
>> -	int rc;
>> +	int rc, idx;
>>  	struct sqlite3_priv *priv = ras->db_priv;
>>
>>  	if (!priv || !priv->stmt_cxl_generic_event)
>>  		return 0;
>>  	log(TERM, LOG_INFO, "cxl_generic_event store: %p\n",
>> priv->stmt_cxl_generic_event);
>>
>> -	ras_store_cxl_common_hdr(priv->stmt_cxl_generic_event, &ev->hdr);
>> -	sqlite3_bind_blob(priv->stmt_cxl_generic_event, 13, ev->data,
>> +	idx = ras_store_cxl_common_hdr(priv->stmt_cxl_generic_event, &ev-
>>hdr);
>> +	if (!idx)
>> +		return 0;
>Seems like an odd form of error code as without looking at implementation it
>might seem reasonable for that call to return 0 because it didn't add anythin to
>idx?
>
>> +
>> +	sqlite3_bind_blob(priv->stmt_cxl_generic_event, idx++, ev->data,
>>  			  CXL_EVENT_RECORD_DATA_LENGTH, NULL);
>>
>>  	rc = sqlite3_step(priv->stmt_cxl_generic_event);
>> @@ -891,7 +896,7 @@ static const struct db_table_descriptor
>> cxl_general_media_event_tab = {  int
>ras_store_cxl_general_media_event(struct ras_events *ras,
>>  				      struct ras_cxl_general_media_event *ev)  {
>> -	int rc;
>> +	int rc, idx;
>>  	struct sqlite3_priv *priv = ras->db_priv;
>>
>>  	if (!priv || !priv->stmt_cxl_general_media_event)
>> @@ -899,20 +904,23 @@ int ras_store_cxl_general_media_event(struct
>ras_events *ras,
>>  	log(TERM, LOG_INFO, "cxl_general_media_event store: %p\n",
>>  	    priv->stmt_cxl_general_media_event);
>>
>> -	ras_store_cxl_common_hdr(priv->stmt_cxl_general_media_event, &ev-
>>hdr);
>> -	sqlite3_bind_int64(priv->stmt_cxl_general_media_event, 13, ev->dpa);
>> -	sqlite3_bind_int(priv->stmt_cxl_general_media_event, 14, ev-
>>dpa_flags);
>> -	sqlite3_bind_int(priv->stmt_cxl_general_media_event, 15, ev-
>>descriptor);
>> -	sqlite3_bind_int(priv->stmt_cxl_general_media_event, 16, ev->type);
>> -	sqlite3_bind_int(priv->stmt_cxl_general_media_event, 17, ev-
>>transaction_type);
>> -	sqlite3_bind_int(priv->stmt_cxl_general_media_event, 18, ev->channel);
>> -	sqlite3_bind_int(priv->stmt_cxl_general_media_event, 19, ev->rank);
>> -	sqlite3_bind_int(priv->stmt_cxl_general_media_event, 20, ev->device);
>> -	sqlite3_bind_blob(priv->stmt_cxl_general_media_event, 21, ev-
>>comp_id,
>> +	idx = ras_store_cxl_common_hdr(priv->stmt_cxl_general_media_event,
>&ev->hdr);
>> +	if (!idx)
>As above,
>> +		return 0;
>> +
>> +	sqlite3_bind_int64(priv->stmt_cxl_general_media_event, idx++, ev-
>>dpa);
>> +	sqlite3_bind_int(priv->stmt_cxl_general_media_event, idx++, ev-
>>dpa_flags);
>> +	sqlite3_bind_int(priv->stmt_cxl_general_media_event, idx++, ev-
>>descriptor);
>> +	sqlite3_bind_int(priv->stmt_cxl_general_media_event, idx++, ev->type);
>> +	sqlite3_bind_int(priv->stmt_cxl_general_media_event, idx++, ev-
>>transaction_type);
>> +	sqlite3_bind_int(priv->stmt_cxl_general_media_event, idx++, ev-
>>channel);
>> +	sqlite3_bind_int(priv->stmt_cxl_general_media_event, idx++, ev->rank);
>> +	sqlite3_bind_int(priv->stmt_cxl_general_media_event, idx++, ev-
>>device);
>> +	sqlite3_bind_blob(priv->stmt_cxl_general_media_event, idx++,
>> +ev->comp_id,
>>  			  CXL_EVENT_GEN_MED_COMP_ID_SIZE, NULL);
>> -	sqlite3_bind_int64(priv->stmt_cxl_general_media_event, 22, ev->hpa);
>> -	sqlite3_bind_text(priv->stmt_cxl_general_media_event, 23, ev->region,
>-1, NULL);
>> -	sqlite3_bind_text(priv->stmt_cxl_general_media_event, 24, ev-
>>region_uuid, -1, NULL);
>> +	sqlite3_bind_int64(priv->stmt_cxl_general_media_event, idx++, ev-
>>hpa);
>> +	sqlite3_bind_text(priv->stmt_cxl_general_media_event, idx++, ev-
>>region, -1, NULL);
>> +	sqlite3_bind_text(priv->stmt_cxl_general_media_event, idx++,
>> +ev->region_uuid, -1, NULL);
>>
>>  	rc = sqlite3_step(priv->stmt_cxl_general_media_event);
>>  	if (rc != SQLITE_OK && rc != SQLITE_DONE) @@ -970,7 +978,7 @@
>static
>> const struct db_table_descriptor cxl_dram_event_tab = {
>>
>>  int ras_store_cxl_dram_event(struct ras_events *ras, struct
>> ras_cxl_dram_event *ev)  {
>> -	int rc;
>> +	int rc, idx;
>>  	struct sqlite3_priv *priv = ras->db_priv;
>>
>>  	if (!priv || !priv->stmt_cxl_dram_event) @@ -978,24 +986,27 @@ int
>> ras_store_cxl_dram_event(struct ras_events *ras, struct ras_cxl_dram_event
>*
>>  	log(TERM, LOG_INFO, "cxl_dram_event store: %p\n",
>>  	    priv->stmt_cxl_dram_event);
>>
>> -	ras_store_cxl_common_hdr(priv->stmt_cxl_dram_event, &ev->hdr);
>> -	sqlite3_bind_int64(priv->stmt_cxl_dram_event, 13, ev->dpa);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 14, ev->dpa_flags);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 15, ev->descriptor);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 16, ev->type);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 17, ev->transaction_type);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 18, ev->channel);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 19, ev->rank);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 20, ev->nibble_mask);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 21, ev->bank_group);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 22, ev->bank);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 23, ev->row);
>> -	sqlite3_bind_int(priv->stmt_cxl_dram_event, 24, ev->column);
>> -	sqlite3_bind_blob(priv->stmt_cxl_dram_event, 25, ev->cor_mask,
>> +	idx = ras_store_cxl_common_hdr(priv->stmt_cxl_dram_event, &ev-
>>hdr);
>> +	if (!idx)
>As above.
>
>> +		return 0;
>> +
>> +	sqlite3_bind_int64(priv->stmt_cxl_dram_event, idx++, ev->dpa);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev->dpa_flags);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev->descriptor);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev->type);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev-
>>transaction_type);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev->channel);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev->rank);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev->nibble_mask);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev->bank_group);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev->bank);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev->row);
>> +	sqlite3_bind_int(priv->stmt_cxl_dram_event, idx++, ev->column);
>> +	sqlite3_bind_blob(priv->stmt_cxl_dram_event, idx++, ev->cor_mask,
>>  			  CXL_EVENT_DER_CORRECTION_MASK_SIZE, NULL);
>> -	sqlite3_bind_int64(priv->stmt_cxl_dram_event, 26, ev->hpa);
>> -	sqlite3_bind_text(priv->stmt_cxl_dram_event, 27, ev->region, -1, NULL);
>> -	sqlite3_bind_text(priv->stmt_cxl_dram_event, 28, ev->region_uuid, -1,
>NULL);
>> +	sqlite3_bind_int64(priv->stmt_cxl_dram_event, idx++, ev->hpa);
>> +	sqlite3_bind_text(priv->stmt_cxl_dram_event, idx++, ev->region, -1,
>NULL);
>> +	sqlite3_bind_text(priv->stmt_cxl_dram_event, idx++, ev->region_uuid,
>> +-1, NULL);
>>
>>  	rc = sqlite3_step(priv->stmt_cxl_dram_event);
>>  	if (rc != SQLITE_OK && rc != SQLITE_DONE) @@ -1047,7 +1058,7 @@
>> static const struct db_table_descriptor cxl_memory_module_event_tab =
>> {  int ras_store_cxl_memory_module_event(struct ras_events *ras,
>>  				      struct ras_cxl_memory_module_event *ev)
>{
>> -	int rc;
>> +	int rc, idx;
>>  	struct sqlite3_priv *priv = ras->db_priv;
>>
>>  	if (!priv || !priv->stmt_cxl_memory_module_event)
>> @@ -1055,16 +1066,19 @@ int ras_store_cxl_memory_module_event(struct
>ras_events *ras,
>>  	log(TERM, LOG_INFO, "cxl_memory_module_event store: %p\n",
>>  	    priv->stmt_cxl_memory_module_event);
>>
>> -	ras_store_cxl_common_hdr(priv->stmt_cxl_memory_module_event,
>&ev->hdr);
>> -	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, 13, ev-
>>event_type);
>> -	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, 14, ev-
>>health_status);
>> -	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, 15, ev-
>>media_status);
>> -	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, 16, ev-
>>life_used);
>> -	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, 17, ev-
>>dirty_shutdown_cnt);
>> -	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, 18, ev-
>>cor_vol_err_cnt);
>> -	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, 19, ev-
>>cor_per_err_cnt);
>> -	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, 20, ev-
>>device_temp);
>> -	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, 21, ev-
>>add_status);
>> +	idx = ras_store_cxl_common_hdr(priv-
>>stmt_cxl_memory_module_event, &ev->hdr);
>> +	if (!idx)
>as above
>> +		return 0;
>> +
>> +	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, idx++, ev-
>>event_type);
>> +	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, idx++, ev-
>>health_status);
>> +	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, idx++, ev-
>>media_status);
>> +	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, idx++, ev-
>>life_used);
>> +	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, idx++, ev-
>>dirty_shutdown_cnt);
>> +	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, idx++, ev-
>>cor_vol_err_cnt);
>> +	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, idx++, ev-
>>cor_per_err_cnt);
>> +	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, idx++, ev-
>>device_temp);
>> +	sqlite3_bind_int(priv->stmt_cxl_memory_module_event, idx++,
>> +ev->add_status);
>>
>>  	rc = sqlite3_step(priv->stmt_cxl_memory_module_event);
>>  	if (rc != SQLITE_OK && rc != SQLITE_DONE)