[Nut-upsdev] upsdrv_print_ups_list and afferant reflections

[Peter Selinger]

Jonathan Dion wrote:
> 
...

> > 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.

No, the parser will set productID and vendorID to 0 *if* new_entry == 1.
(line 134). In the particular sequence that I gave, new_entry would
have been 0, so this bug would have happened. 

> > 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 !

Oops, my solution also contains a bug similar to the new_entry bug: 
should have been:

             } else if (*p == '=') {  /* any line starting with '=' */
                if (have_record) {
                    print_record(vendorid, productid, ...);
                }
                have_record = 0;
		product_id = 0;
		vendor_id = 0;
		...;
             }

-- Peter