[Nut-upsdev] upsdrv_print_ups_list and afferant reflections

[Jonathan Dion]

On 8/9/06, Peter Selinger <selinger at mathstat.dal.ca> wrote:
> Hi Jonathan,
>
> I had a brief look at the parser. Here are some cases where I think
> the parser will malfunction:
>
> Name: APC Back-UPS 800         # the parser will set value="APC"

The program print-ups-list don't use other tag than VendorID and
Product ID for the moment. I would of course manage this case when I'd
need it

> VendorID:0x0001                # the parser will not see the ":"

You are right

> ProductID: 0002                # missing "0x"

The parser should add the 0x. It is a good idea

> ====                           # the parser will not recognize this line

You are right. Didn't think of this case

> Name: XYZ UPS 1000
> VendorID: 0x0003
> ProductID: 0x0004
> ===
> Name: XYZ UPS 1001
> VendorID: 0x0005               # since productID != 0 (line 152), the
>                                # parser will call print_ups(0005,0004), a bug

For once you are wrong here ^^. The "===" line set up productID and
vendorID to 0, so it won't call print_ups. I used this mechanism to be
able to have VendorID and ProductID in any order.

> ProductID: 0x0006              # since vendorID != 0 (line 165), the
>                                # parser will call print_ups(0005,0006)
>
> Is it really appropriate to use the PCONF_CTX macros for parsing a
> key-value file format? Perhaps it would be better to have a function
> strip() (destructively remove white space, i.e., ' \t\n', from front
> and end of a string), and a function key_value, perhaps as follows:
>
> /* if line represents a key-value pair, return 1 and let key and value
>    point to key and value. "line" is updated destructively, and key
>    and value may point inside line. If not a key-value-pair, return 0
>    and leave line unchanged. */
> int key_value(char *line, char **key, char **value) {
>     char *p;
>
>     p = strchr(line, ':');
>     if (!p) {
>        return 0;
>     }
>     *p = 0;
>     *key = strip(line);
>     *value = strip(value);
>     return 1;
> }
>
> Then the parse() function could be something simple and reliable, like:
>
> void parse(FILE *pipe) {
>      char line[100];
>      char *p, *key, *value;
>      int l = 0;
>      int productid = 0, vendorid = 0, ...;
>      int have_record = 0;
>
>      while (getline(line, 100, pipe) >= 0) {
>          l++;
>          p = strip(line);
>          if (*p == 0 || *p == '#') { /* comment, blank line */
>             continue;
>          } else if (key_value(p, &key, &value)) {
>             if (strcasecmp(key, "ProductID") == 0) {
>                productid = parse_hex(value);
>             } else if (strcasecmp(key, "VendorID") == 0) {
>                vendorid = parse_hex(value);
>             } else if (...) {
>                ...
>             } else {
>                upsdebugx("Warning: unknown key %s in line %d\n", key, l);
>             }
>             have_record = 1;
>             continue;
>          } else if (*p == '=') {  /* any line starting with '=' */
>             if (have_record) {
>                 print_record(vendorid, productid, ...);
>             }
>          }
>      }
>      if (have_record) {
>          print_record(vendorid, productid, ...);
>      }
> }
>
> This would treat multi-word values, and all the other special cases,
> correctly, and would also extend easily to new key-value pairs.
>

You are totally right ! I like your solution, simple and elegant !

> -- Peter
>

Thank for your comments. I'll do the modifications

Jonathan