0

こんにちは、要件に基づいてコードを作成しました。

(field1_6) (field2_30) (field3_16) (field4_16) (field5_1) (field6_6) (field7_2) (field8_1)..... これは 1 バケット (8 フィールド) のデータです。一度に 20 個のバケットを受け取ります。つまり、合計で 160 個のフィールドになります。定義済みの条件に基づいて、field3、field7、および fields8 の値を取得する必要があります。入力引数が N の場合は、1 番目のバケットから 3 つのフィールドを取得し、Y の場合は、1 番目以外のバケットから 3 つのフィールドを取得する必要があります。引数が Y の場合、20 個のバケットすべてを次々にスキャンし、バケットの最初のフィールドが 0 でないことを確認し、それが true の場合は、そのバケットの 3 つのフィールドをフェッチして終了する必要があります。私はコードを書きましたが、それもうまく機能しています..しかし、それが効果的であるとは確信していません。いつかクラッシュするのではないかと心配しています.以下のコードを提案してください.

int CMI9_auxc_parse_balance_info(char *i_balance_info,char  *i_use_balance_ind,char *o_balance,char *o_balance_change,char *o_balance_sign
)
{
  char *pch = NULL;
  char *balance_id[MAX_BUCKETS] = {NULL};
  char balance_info[BALANCE_INFO_FIELD_MAX_LENTH] = {0};
  char *str[160] = {NULL};
  int i=0,j=0,b_id=0,b_ind=0,bc_ind=0,bs_ind=0,rc;
  int total_bukets ;
  memset(balance_info,' ',BALANCE_INFO_FIELD_MAX_LENTH);
  memcpy(balance_info,i_balance_info,BALANCE_INFO_FIELD_MAX_LENTH);
  //balance_info[BALANCE_INFO_FIELD_MAX_LENTH]='\0';
  pch = strtok (balance_info,"*");
  while (pch != NULL && i < 160)
  {
     str[i]=(char*)malloc(strlen(pch) + 1);
     strcpy(str[i],pch);
     pch = strtok (NULL, "*");
     i++;
  }
total_bukets  = i/8  ;
  for (j=0;str[b_id]!=NULL,j<total_bukets;j++)
  {
  balance_id[j]=str[b_id];
  b_id=b_id+8;
  }
  if (!memcmp(i_use_balance_ind,"Y",1))
  {
     if (atoi(balance_id[0])==1)
     {
        memcpy(o_balance,str[2],16);
        memcpy(o_balance_change,str[3],16);
        memcpy(o_balance_sign,str[7],1);
        for(i=0;i<160;i++)
        free(str[i]);
        return 1;
     }
     else
     {
        for(i=0;i<160;i++)
        free(str[i]);
      return 0;
     }
  }
  else if (!memcmp(i_use_balance_ind,"N",1))
  {
      for (j=1;balance_id[j]!=NULL,j<MAX_BUCKETS;j++)
      {
        b_ind=(j*8)+2;
        bc_ind=(j*8)+3;
        bs_ind=(j*8)+7;
       if (atoi(balance_id[j])!=1 && atoi( str[bc_ind] )!=0)
       {
        memcpy(o_balance,str[b_ind],16);
        memcpy(o_balance_change,str[bc_ind],16);
        memcpy(o_balance_sign,str[bs_ind],1);
        for(i=0;i<160;i++)
        free(str[i]);
        return 1;
       }
      }
     for(i=0;i<160;i++)
     free(str[i]);
    return 0;
  }
 for(i=0;i<160;i++)
 free(str[i]);
return 0;
}
4

2 に答える 2

1

私の感じでは、このコードは非常にもろいです。適切な入力があればうまくいくかもしれませんが (机上で確認することはお勧めしません)、間違った入力をすると、クラッシュして燃えたり、誤解を招くような結果をもたらしたりします。

予期しない入力をテストしましたか? 例えば：

  • i_balance_info が null だとしますか?
  • i_balance_info が "" だとしますか?

  • 入力文字列の項目数が 8 未満であると仮定すると、このコード行は何を実行するでしょうか?

    memcpy(o_balance_sign,str[7],1);

  • str[3] の項目の長さが 16 文字未満であると仮定すると、このコード行は何を行うでしょうか?

    memcpy(o_balance_change,str[3],16);

そのようなコードを書くための私のアプローチは、そのようなすべての不測の事態から保護することです. 少なくとも、ASSERT() ステートメントを追加します。通常、明示的な入力検証を記述し、問題がある場合はエラーを返します。ここでの問題は、インターフェイスが不正な入力が存在する可能性をまったく許容していないように見えることです。

于 2009-10-09T08:00:55.240 に答える
0

私はあなたのコードを読むのに苦労しましたが、FWIW 私はいくつかのコメントを追加しました、HTH:

// do shorter functions, long functions are harder to follow and make errors harder to spot
// document all your variables, at the very least your function parameters
// also what the function is suppose to do and what it expects as input
int CMI9_auxc_parse_balance_info
(
  char *i_balance_info,
  char *i_use_balance_ind,
  char *o_balance,
  char *o_balance_change,
  char *o_balance_sign
)
{
  char *balance_id[MAX_BUCKETS] = {NULL};
  char balance_info[BALANCE_INFO_FIELD_MAX_LENTH] = {0};
  char *str[160] = {NULL};
  int i=0,j=0,b_id=0,b_ind=0,bc_ind=0,bs_ind=0,rc;
  int total_bukets=0; // good practice to initialize all variables

  //
  // check for null pointers in your arguments, and do sanity checks for any
  // calculations
  // also move variable declarations to just before they are needed
  //

  memset(balance_info,' ',BALANCE_INFO_FIELD_MAX_LENTH);
  memcpy(balance_info,i_balance_info,BALANCE_INFO_FIELD_MAX_LENTH);
  //balance_info[BALANCE_INFO_FIELD_MAX_LENTH]='\0';  // should be BALANCE_INFO_FIELD_MAX_LENTH-1

  char *pch = strtok (balance_info,"*"); // this will potentially crash since no ending \0

  while (pch != NULL && i < 160)
  {
    str[i]=(char*)malloc(strlen(pch) + 1);
    strcpy(str[i],pch);
    pch = strtok (NULL, "*");
    i++;
  }
  total_bukets  = i/8  ;
  // you have declared char*str[160] check if enough b_id < 160
  // asserts are helpful if nothing else assert( b_id < 160 );
  for (j=0;str[b_id]!=NULL,j<total_bukets;j++)
  {
    balance_id[j]=str[b_id];
    b_id=b_id+8;
  }
  // don't use memcmp, if ('y'==i_use_balance_ind[0]) is better
  if (!memcmp(i_use_balance_ind,"Y",1))
  {
    // atoi needs balance_id str to end with \0 has it?
    if (atoi(balance_id[0])==1)
    {
      // length assumptions and memcpy when its only one byte
      memcpy(o_balance,str[2],16);
      memcpy(o_balance_change,str[3],16);
      memcpy(o_balance_sign,str[7],1);
      for(i=0;i<160;i++)
        free(str[i]);
      return 1;
    }
    else
    {
      for(i=0;i<160;i++)
        free(str[i]);
      return 0;
    }
  }
  // if ('N'==i_use_balance_ind[0]) 
  else if (!memcmp(i_use_balance_ind,"N",1))
  {
    // here I get a headache, this looks just at first glance risky. 
    for (j=1;balance_id[j]!=NULL,j<MAX_BUCKETS;j++)
    {
      b_ind=(j*8)+2;
      bc_ind=(j*8)+3;
      bs_ind=(j*8)+7;
      if (atoi(balance_id[j])!=1 && atoi( str[bc_ind] )!=0)
      {
        // length assumptions and memcpy when its only one byte
        // here u assume strlen(str[b_ind])>15 including \0
        memcpy(o_balance,str[b_ind],16);
        // here u assume strlen(str[bc_ind])>15 including \0
        memcpy(o_balance_change,str[bc_ind],16);
        // here, besides length assumption you could use a simple assignment
        // since its one byte
        memcpy(o_balance_sign,str[bs_ind],1);
        // a common practice is to set pointers that are freed to NULL.
        // maybe not necessary here since u return
        for(i=0;i<160;i++)
          free(str[i]);
        return 1;
      }
    }
    // suggestion do one function that frees your pointers to avoid dupl
    for(i=0;i<160;i++)
      free(str[i]);
    return 0;
  }
  for(i=0;i<160;i++)
    free(str[i]);
  return 0;
}

配列内のオフセットにアクセスする場合に役立つ手法は、メモリ レイアウトをマップする構造体を作成することです。次に、ポインターを構造体のポインターにキャストし、構造体メンバーを使用して、さまざまな memcpy の代わりに情報を抽出します。

また、関数のパラメーターを一般的に再検討することをお勧めします。すべてのパラメーターを構造体に配置すると、制御が向上し、関数が読みやすくなります。

int foo( input* inbalance, output* outbalance )

（または、あなたがやろうとしていることは何でも）

于 2009-10-09T08:04:43.413 に答える