Peter Selinger
2007-Aug-23 21:49 UTC
[Nut-upsdev] [nut-commits] svn commit r1073 - in trunk: . drivers
I think having this logic buried within libhid/libusb
(libusb:libusb_open(), line 179 to 206) is ultimately a mistake,
albeit one that I am probably responsible for. Would it make sense to
confine libhid to low-level operations, and leave the decision of
trying to reopen vs. retrying to open to the high-level driver, in
this case usbhid-ups?
I envision that the code in usbhid-ups:reconnect_ups() could be
changed to something like:
hd = HIDOpenDevice(&udev, &curDevice, reopen_matcher, MODE_REOPEN);
if (hd == NULL) {
/* reopening failed, try opening new device */
hd = HIDOpenDevice(&udev, &curDevice, regex_matcher, MODE_OPEN);
}
if (hd == NULL) {
return 0;
}
Note "regex_matcher", not "reopen_matcher", in the second
call, and
also the changed MODE flag.
Then all the complicated stuff could be removed from libhid/libusb
(different non-zero return codes for matchers etc).
I am not sure the code in libusb_open() is correct anyway, as matching
a chain of matchers should mean matching *all* the matchers contained
therein, not just the first possible one. For example, after a positive
reopen_match, one should still check regex_match also.
In other words, we are dealing with a loop like this:
For each device curDevice: {
Try exact match, stop if it succeeds.
Try regex match, stop if it succeeds.
}
This means we match the first device for which an exact match *or* a
regex match is found. In other words, if the device that got
disconnected is on bus 5, and some other loosely matching device is on
bus 3, then we'll open the new device on bus 3, rather than reopening
the disconnected device on bus 5.
What we should be doing instead is:
For each device curDevice: {
Try exact match, stop if it succeeds.
}
For each device curDevice: {
Try regex match, stop if it succeeds.
}
I think moving this logic up into usbhid-ups would simplify things
greatly.
Arjen de Korte wrote:>
> Author: adkorte-guest
> Date: Thu Aug 23 19:43:41 2007
> New Revision: 1073
>
> Log:
> * drivers/libhid.[ch],libshut.c,libusb.c:
> - The 'mode' variable for HIDOpenDevice is now passed as pointer.
If a device can't be re-opened (because we can't get an exact match),
this will be modified to MODE_OPEN and treated as such.
> - Cleaned up matcher functions.
>
> * drivers/usbhid-ups.[ch]:
> - When re-connecting to a device, re-read all variables and commands from
it. If it turns out that we couldn't re-open it (for the reasons mentioned
above) we will treat it as a new device and will flush everything (except
"ups.status") and start all over again.
>
> * drivers/dstate-hal.c,megatec_usb.c,tripplite_usb.c:
> - Align with the above.
>
>
> Modified:
> trunk/ChangeLog
> trunk/drivers/dstate-hal.c
> trunk/drivers/libhid.c
> trunk/drivers/libhid.h
> trunk/drivers/libshut.c
> trunk/drivers/libusb.c
> trunk/drivers/megatec_usb.c
> trunk/drivers/tripplite_usb.c
> trunk/drivers/usbhid-ups.c
> trunk/drivers/usbhid-ups.h
>
> Modified: trunk/ChangeLog
>
=============================================================================>
--- trunk/ChangeLog (original)
> +++ trunk/ChangeLog Thu Aug 23 19:43:41 2007
> @@ -1,3 +1,19 @@
> +Thu Aug 23 19:30:28 UTC 2007 / Arjen de Korte <arjen at
de-korte.org>
> +
> + * drivers/libhid.[ch],libshut.c,libusb.c:
> + - The 'mode' variable for HIDOpenDevice is now passed as
pointer.
> + If a device can't be re-opened (because we can't get an exact
> + match), this will be modified to MODE_OPEN and treated as such.
> + - Cleaned up matcher functions.
> + * drivers/usbhid-ups.[ch]:
> + - When re-connecting to a device, re-read all variables and
> + commands from it. If it turns out that we couldn't re-open it
> + (for the reasons mentioned above) we will treat it as a new
> + device and will flush everything (except "ups.status") and
> + start all over again.
> + * drivers/dstate-hal.c,megatec_usb.c,tripplite_usb.c:
> + - Align with the above.
> +
> Wed Aug 22 18:12:50 UTC 2007 / Arjen de Korte <arjen at
de-korte.org>
>
> * drivers/libhid.[ch]:
>
> Modified: trunk/drivers/dstate-hal.c
>
=============================================================================>
--- trunk/drivers/dstate-hal.c (original)
> +++ trunk/drivers/dstate-hal.c Thu Aug 23 19:43:41 2007
> @@ -296,6 +296,11 @@
> return 0;
> }
>
> +int dstate_delcmd(const char *var)
> +{
> + return 0;
> +}
> +
> void dstate_setflags(const char *var, int flags)
> {
> return;
>
> Modified: trunk/drivers/libhid.c
>
=============================================================================>
--- trunk/drivers/libhid.c (original)
> +++ trunk/drivers/libhid.c Thu Aug 23 19:43:41 2007
> @@ -260,109 +260,131 @@
>
> /* helper function: version of strcmp that tolerates NULL
> * pointers. NULL is considered to come before all other strings
> - * alphabetically. */
> -static inline int strcmp_null(char *s1, char *s2)
> + * alphabetically.
> + */
> +static int strcmp_null(char *s1, char *s2)
> {
> if (s1 == NULL && s2 == NULL) {
> return 0;
> }
> +
> if (s1 == NULL) {
> return -1;
> }
> +
> if (s2 == NULL) {
> return 1;
> }
> +
> return strcmp(s1, s2);
> }
>
> -/* private callback function for exact matches */
> -static int match_function_exact(HIDDevice_t *d, void *privdata)
> +/* private callback function for exact matches
> + * note: the exact matcher ignores the "Bus" field,
> + * because it can change during a reconnect.
> + */
> +static int match_function_exact(HIDDevice_t *hd, void *privdata)
> {
> - HIDDevice_t *data = (HIDDevice_t *)privdata;
> + HIDDevice_t *data = (HIDDevice_t *)privdata;
>
> - if (d->VendorID != data->VendorID) {
> + if (hd->VendorID != data->VendorID) {
> return 0;
> }
> - if (d->ProductID != data->ProductID) {
> +
> + if (hd->ProductID != data->ProductID) {
> return 0;
> }
> - if (strcmp_null(d->Vendor, data->Vendor) != 0) {
> +
> + if (strcmp_null(hd->Vendor, data->Vendor) != 0) {
> return 0;
> }
> - if (strcmp_null(d->Product, data->Product) != 0) {
> +
> + if (strcmp_null(hd->Product, data->Product) != 0) {
> return 0;
> }
> - if (strcmp_null(d->Serial, data->Serial) != 0) {
> +
> + if (strcmp_null(hd->Serial, data->Serial) != 0) {
> return 0;
> }
> - /* note: the exact matcher ignores the "Bus" field, because
> - it can change during a reconnect. */
> - return 1;
> +
> + return 2;
> }
>
> -/* constructor: return a new matcher that matches the exact HIDDevice_t
> - * d. Return NULL with errno set on error. */
> -HIDDeviceMatcher_t *new_exact_matcher(HIDDevice_t *d)
> +/* constructor: create an exact matcher that matches the device.
> + * On success, return 0 and store the matcher in *matcher. On
> + * error, return -1 with errno set
> + */
> +int HIDNewExactMatcher(HIDDeviceMatcher_t **matcher, HIDDevice_t *hd)
> {
> - HIDDeviceMatcher_t *m;
> - HIDDevice_t *data;
> + HIDDeviceMatcher_t *m;
> + HIDDevice_t *data;
>
> - m = (HIDDeviceMatcher_t *)malloc(sizeof(HIDDeviceMatcher_t));
> - if (!m) {
> - return NULL;
> + m = malloc(sizeof(*m));
> + if (!matcher) {
> + return -1;
> }
> - data = (HIDDevice_t *)malloc(sizeof(HIDDevice_t));
> +
> + data = malloc(sizeof(*data));
> if (!data) {
> free(m);
> - return NULL;
> + return -1;
> }
> - data->VendorID = d->VendorID;
> - data->ProductID = d->ProductID;
> - data->Vendor = d->Vendor ? strdup(d->Vendor) : NULL;
> - data->Product = d->Product ? strdup(d->Product) : NULL;
> - data->Serial = d->Serial ? strdup(d->Serial) : NULL;
> +
> + data->VendorID = hd->VendorID;
> + data->ProductID = hd->ProductID;
> + data->Vendor = hd->Vendor ? strdup(hd->Vendor) : NULL;
> + data->Product = hd->Product ? strdup(hd->Product) : NULL;
> + data->Serial = hd->Serial ? strdup(hd->Serial) : NULL;
>
> m->match_function = &match_function_exact;
> m->privdata = (void *)data;
> m->next = NULL;
> - return m;
> +
> + *matcher = m;
> +
> + return 0;
> }
>
> -/* destructor: free matcher previously created with new_exact_matcher */
> -void free_exact_matcher(HIDDeviceMatcher_t *matcher)
> +/* destructor: free matcher previously created with HIDNewExactMatcher */
> +void HIDFreeExactMatcher(HIDDeviceMatcher_t *matcher)
> {
> - HIDDevice_t *data;
> + HIDDevice_t *data;
>
> - if (matcher) {
> - data = (HIDDevice_t *)matcher->privdata;
> -
> - free(data->Vendor);
> - free(data->Product);
> - free(data->Serial);
> - free(data);
> - free(matcher);
> + if (!matcher) {
> + return;
> }
> +
> + data = (HIDDevice_t *)matcher->privdata;
> +
> + free(data->Vendor);
> + free(data->Product);
> + free(data->Serial);
> + free(data);
> +
> + free(matcher);
> }
>
> /* Private function for compiling a regular expression. On success,
> - store the compiled regular expression (or NULL) in *compiled, and
> - return 0. On error with errno set, return -1. If the supplied
> - regular expression is unparseable, return -2 (an error message can
> - then be retrieved with regerror(3)). Note that *compiled will be an
> - allocated value, and must be freed with regfree(), then free(), see
> - regex(3). As a special case, if regex==NULL, then set
> - *compiled=NULL (regular expression NULL is intended to match
> - anything). */
> -static inline int compile_regex(regex_t **compiled, char *regex, int
cflags)
> + * store the compiled regular expression (or NULL) in *compiled, and
> + * return 0. On error with errno set, return -1. If the supplied
> + * regular expression is unparseable, return -2 (an error message can
> + * then be retrieved with regerror(3)). Note that *compiled will be an
> + * allocated value, and must be freed with regfree(), then free(), see
> + * regex(3). As a special case, if regex==NULL, then set
> + * *compiled=NULL (regular expression NULL is intended to match
> + * anything).
> + */
> +static int compile_regex(regex_t **compiled, char *regex, int cflags)
> {
> - int r;
> - regex_t *preg;
> + int r;
> + regex_t *preg;
>
> if (regex == NULL) {
> *compiled = NULL;
> return 0;
> }
> - preg = (regex_t *)malloc(sizeof(regex_t));
> +
> + preg = malloc(sizeof(*preg));
> if (!preg) {
> return -1;
> }
> @@ -371,136 +393,157 @@
> if (r) {
> return -2;
> }
> +
> *compiled = preg;
> +
> return 0;
> }
>
> /* Private function for regular expression matching. Check if the
> - entire string str (minus any initial and trailing whitespace)
> - matches the compiled regular expression preg. Return 1 if it
> - matches, 0 if not. Return -1 on error with errno set. Special
> - cases: if preg==NULL, it matches everything (no contraint). If
> - str==NULL, then it is treated as "". */
> -static int match_regex(regex_t *preg, char *str) {
> - int r;
> - regmatch_t pmatch[1];
> - char *p, *q;
> - int len;
> -
> - if (preg == NULL) {
> - return 1;
> - }
> - if (str == NULL) {
> - str = "";
> - }
> -
> - /* make a copy of str with whitespace stripped */
> - for (q=str; *q==' ' || *q=='\t' || *q=='\n';
q++) {
> - /* empty */
> - }
> - len = strlen(q);
> - p = (char *)malloc(len+1);
> - if (!p) {
> - return -1;
> - }
> - memcpy(p, q, len+1);
> - while (len>0 && (p[len-1]==' ' ||
p[len-1]=='\t' || p[len-1]=='\n')) {
> - len--;
> - }
> - p[len] = 0;
> -
> - /* test the regular expression */
> - r = regexec(preg, p, 1, pmatch, 0);
> - free(p);
> - if (r) {
> - return 0;
> - }
> - /* check that the match is the entire string */
> - if (pmatch[0].rm_so != 0 || pmatch[0].rm_eo != len) {
> - return 0;
> - }
> - return 1;
> + * entire string str (minus any initial and trailing whitespace)
> + * matches the compiled regular expression preg. Return 1 if it
> + * matches, 0 if not. Return -1 on error with errno set. Special
> + * cases: if preg==NULL, it matches everything (no contraint). If
> + * str==NULL, then it is treated as "".
> + */
> +static int match_regex(regex_t *preg, char *str)
> +{
> + int r, len;
> + char *string;
> + regmatch_t pmatch[1];
> +
> + if (preg == NULL) {
> + return 1;
> + }
> +
> + if (str == NULL) {
> + str = "";
> + } else {
> + str += strcspn(str, " \t\n"); /* remove leading whitespace */
> + }
> +
> + string = strdup(str);
> + if (!string) {
> + return -1;
> + }
> +
> + /* remove trailing whitespace */
> + for (len = strlen(string); len > 0; len--) {
> +
> + if (strchr(" \t\n", string[len-1])) {
> +
> + string[len-1] = '\0';
> + continue;
> + }
> + }
> +
> +
> + /* test the regular expression */
> + r = regexec(preg, string, 1, pmatch, 0);
> + free(string);
> + if (r) {
> + return 0;
> + }
> +
> + /* check that the match is the entire string */
> + if ((pmatch[0].rm_so != 0) || (pmatch[0].rm_eo != len)) {
> + return 0;
> + }
> +
> + return 1;
> }
>
> /* Private function, similar to match_regex, but the argument being
> * matched is a (hexadecimal) number, rather than a string. It is
> * converted to a 4-digit hexadecimal string. */
> -static inline int match_regex_hex(regex_t *preg, int n)
> +static int match_regex_hex(regex_t *preg, int n)
> {
> - char buf[10];
> + char buf[10];
> +
> snprintf(buf, sizeof(buf), "%04x", n);
> +
> return match_regex(preg, buf);
> }
>
> /* private data type: hold a set of compiled regular expressions. */
> struct regex_matcher_data_s {
> - regex_t *regex[6];
> + regex_t *regex[6];
> };
> +
> typedef struct regex_matcher_data_s regex_matcher_data_t;
>
> /* private callback function for regex matches */
> -static int match_function_regex(HIDDevice_t *d, void *privdata)
> +static int match_function_regex(HIDDevice_t *hd, void *privdata)
> {
> - regex_matcher_data_t *data = (regex_matcher_data_t *)privdata;
> + regex_matcher_data_t *data = (regex_matcher_data_t *)privdata;
> int r;
>
> - r = match_regex_hex(data->regex[0], d->VendorID);
> + r = match_regex_hex(data->regex[0], hd->VendorID);
> if (r != 1) {
> return r;
> }
> - r = match_regex_hex(data->regex[1], d->ProductID);
> +
> + r = match_regex_hex(data->regex[1], hd->ProductID);
> if (r != 1) {
> return r;
> }
> - r = match_regex(data->regex[2], d->Vendor);
> +
> + r = match_regex(data->regex[2], hd->Vendor);
> if (r != 1) {
> return r;
> }
> - r = match_regex(data->regex[3], d->Product);
> +
> + r = match_regex(data->regex[3], hd->Product);
> if (r != 1) {
> return r;
> }
> - r = match_regex(data->regex[4], d->Serial);
> +
> + r = match_regex(data->regex[4], hd->Serial);
> if (r != 1) {
> return r;
> }
> - r = match_regex(data->regex[5], d->Bus);
> +
> + r = match_regex(data->regex[5], hd->Bus);
> if (r != 1) {
> return r;
> }
> +
> return 1;
> }
>
> /* constructor: create a regular expression matcher. This matcher is
> - based on six regular expression strings in regex_array[0..5],
> - corresponding to: vendorid, productid, vendor, product, serial,
> - bus. Any of these strings can be NULL, which matches
> - everything. Cflags are as in regcomp(3). Typical values for cflags
> - are REG_ICASE (case insensitive matching) and REG_EXTENDED (use
> - extended regular expressions). On success, return 0 and store the
> - matcher in *matcher. On error, return -1 with errno set, or return
> - i=1--5 to indicate that the regular expression regex_array[i] was
> - ill-formed (an error message can then be retrieved with
> - regerror(3)). */
> -int new_regex_matcher(HIDDeviceMatcher_t **matcher, char *regex_array[6],
int cflags)
> -{
> - HIDDeviceMatcher_t *m = NULL;
> - regex_matcher_data_t *data = NULL;
> - int r, i;
> + * based on six regular expression strings in regex_array[0..5],
> + * corresponding to: vendorid, productid, vendor, product, serial,
> + * bus. Any of these strings can be NULL, which matches
> + * everything. Cflags are as in regcomp(3). Typical values for cflags
> + * are REG_ICASE (case insensitive matching) and REG_EXTENDED (use
> + * extended regular expressions). On success, return 0 and store the
> + * matcher in *matcher. On error, return -1 with errno set, or return
> + * i=1--6 to indicate that the regular expression regex_array[i-1] was
> + * ill-formed (an error message can then be retrieved with
> + * regerror(3)).
> + */
> +int HIDNewRegexMatcher(HIDDeviceMatcher_t **matcher, char **regex, int
cflags)
> +{
> + int r, i;
> + HIDDeviceMatcher_t *m;
> + regex_matcher_data_t *data;
>
> - m = (HIDDeviceMatcher_t *)malloc(sizeof(HIDDeviceMatcher_t));
> + m = malloc(sizeof(*m));
> if (!m) {
> return -1;
> }
> - data = (regex_matcher_data_t *)malloc(sizeof(regex_matcher_data_t));
> +
> + data = calloc(1, sizeof(*data));
> if (!data) {
> free(m);
> return -1;
> }
> +
> for (i=0; i<6; i++) {
> - r = compile_regex(&data->regex[i], regex_array[i], cflags);
> - if (r==-2) {
> - r = i;
> + r = compile_regex(&data->regex[i], regex[i], cflags);
> + if (r == -2) {
> + r = i+1;
> }
> if (r) {
> free(m);
> @@ -512,26 +555,34 @@
> m->match_function = &match_function_regex;
> m->privdata = (void *)data;
> m->next = NULL;
> +
> *matcher = m;
> +
> return 0;
> }
>
> -void free_regex_matcher(HIDDeviceMatcher_t *matcher)
> +void HIDFreeRegexMatcher(HIDDeviceMatcher_t *matcher)
> {
> - int i;
> - regex_matcher_data_t *data;
> + int i;
> + regex_matcher_data_t *data;
>
> - if (matcher) {
> - data = (regex_matcher_data_t *)matcher->privdata;
> - for (i=0; i<6; i++) {
> - if (data->regex[i]) {
> - regfree(data->regex[i]);
> - free(data->regex[i]);
> - }
> + if (!matcher) {
> + return;
> + }
> +
> + data = (regex_matcher_data_t *)matcher->privdata;
> +
> + for (i = 0; i < 6; i++) {
> + if (!data->regex[i]) {
> + continue;
> }
> - free(data);
> - free(matcher);
> +
> + regfree(data->regex[i]);
> + free(data->regex[i]);
> }
> +
> + free(data);
> + free(matcher);
> }
>
> /* ----------------------------------------------------------------------
*/
> @@ -600,12 +651,12 @@
> /* Matcher is a linked list of matchers (see libhid.h), and the opened
> device must match all of them. On success, set *udevp and *hd and
> return hd. On failure, return NULL. Mode is MODE_OPEN or MODE_REOPEN.
*/
> -HIDDevice_t *HIDOpenDevice(hid_dev_handle_t **udevp, HIDDevice_t *hd,
HIDDeviceMatcher_t *matcher, int mode)
> +HIDDevice_t *HIDOpenDevice(hid_dev_handle_t **udevp, HIDDevice_t *hd,
HIDDeviceMatcher_t *matcher, int *mode)
> {
> int ReportSize;
> unsigned char ReportDesc[4096];
>
> - if (mode == MODE_REOPEN) {
> + if (*mode == MODE_REOPEN) {
> #if defined(SHUT_MODE) || defined(SUN_LIBUSB)
> /* Cause a double free corruption in USB mode on linux! */
> if (*udevp != NULL) {
> @@ -623,7 +674,7 @@
> if (ReportSize < 0)
> return NULL;
>
> - if (mode == MODE_REOPEN) {
> + if (*mode == MODE_REOPEN) {
> upsdebugx(4, "Device reopened successfully");
> return hd;
> }
> @@ -632,6 +683,7 @@
> upsdebug_hex(3, "Report Descriptor", ReportDesc, ReportSize);
>
> /* Parse Report Descriptor */
> + Free_ReportDesc(pDesc);
> pDesc = Parse_ReportDesc(ReportDesc, ReportSize);
> if (!pDesc) {
> HIDCloseDevice(*udevp);
> @@ -639,6 +691,7 @@
> }
>
> /* prepare report buffer */
> + free_report_buffer(rbuf);
> rbuf = new_report_buffer(pDesc);
> if (!rbuf) {
> Free_ReportDesc(pDesc);
>
> Modified: trunk/drivers/libhid.h
>
=============================================================================>
--- trunk/drivers/libhid.h (original)
> +++ trunk/drivers/libhid.h Thu Aug 23 19:43:41 2007
> @@ -133,14 +133,14 @@
> /* constructors and destructors for specific types of matchers. An
> exact matcher matches a specific HIDDevice_t structure (except for
> the Bus component, which is ignored). A regex matcher matches
> - devices based on a set of regular expressions. The new_* functions
> - return a matcher on success, or NULL on error with errno set. Note
> - that the "free_*" functions only free the current matcher,
not any
> - others that are linked via "next" fields. */
> -HIDDeviceMatcher_t *new_exact_matcher(HIDDevice_t *d);
> -int new_regex_matcher(HIDDeviceMatcher_t **matcher, char *regex_array[6],
int cflags);
> -void free_exact_matcher(HIDDeviceMatcher_t *matcher);
> -void free_regex_matcher(HIDDeviceMatcher_t *matcher);
> + devices based on a set of regular expressions. The HIDNew* functions
> + return a matcher on success, or -1 on error with errno set. Note
> + that the "HIDFree*" functions only free the current matcher,
not
> + any others that are linked via "next" fields. */
> +int HIDNewExactMatcher(HIDDeviceMatcher_t **matcher, HIDDevice_t *hd);
> +int HIDNewRegexMatcher(HIDDeviceMatcher_t **matcher, char **regex, int
cflags);
> +void HIDFreeExactMatcher(HIDDeviceMatcher_t *matcher);
> +void HIDFreeRegexMatcher(HIDDeviceMatcher_t *matcher);
>
> /* Describe a set of values to match for finding a special HID device.
> * This is given by a set of (compiled) regular expressions. If any
> @@ -169,7 +169,7 @@
> HIDDevice_t *curDevice, /* device matching HIDDeviceMatcher_t */
> HIDDeviceMatcher_t *matcher,
> unsigned char *ReportDesc,
> - int mode);
> + int *mode);
> void (*close)(hid_dev_handle_t *sdev);
> int (*get_report)(hid_dev_handle_t *sdev, int ReportId,
> unsigned char *raw_buf, int ReportSize );
> @@ -188,7 +188,7 @@
> /*
> * HIDOpenDevice
> *
-------------------------------------------------------------------------- */
> -HIDDevice_t *HIDOpenDevice(hid_dev_handle_t **udevp, HIDDevice_t *hd,
HIDDeviceMatcher_t *matcher, int mode);
> +HIDDevice_t *HIDOpenDevice(hid_dev_handle_t **udevp, HIDDevice_t *hd,
HIDDeviceMatcher_t *matcher, int *mode);
>
> /*
> * HIDGetItemValue
>
> Modified: trunk/drivers/libshut.c
>
=============================================================================>
--- trunk/drivers/libshut.c (original)
> +++ trunk/drivers/libshut.c Thu Aug 23 19:43:41 2007
> @@ -284,7 +284,7 @@
> a linked list of matchers (see libhid.h), and the opened device
> must match all of them. */
> int libshut_open(shut_dev_handle_t **sdevp, HIDDevice_t *curDevice,
> - HIDDeviceMatcher_t *matcher, unsigned char *ReportDesc, int mode)
> + HIDDeviceMatcher_t *matcher, unsigned char *ReportDesc, int *mode)
> {
> int ret, res;
> unsigned char buf[20];
> @@ -311,7 +311,7 @@
> upsdebugx(2, "Communication with UPS established");
>
> /* we can skip the rest due to serial bus specifics! */
> - if (mode == MODE_REOPEN)
> + if (*mode == MODE_REOPEN)
> return 1;
>
> /* Get DEVICE descriptor */
> @@ -368,9 +368,6 @@
> upsdebugx(2, "- Bus: %s", curDevice->Bus ? curDevice->Bus
: "unknown");
> upsdebugx(2, "Device matches");
>
> - if (mode == MODE_REOPEN)
> - return 1;
> -
> /* Get HID descriptor */
> desc = (struct my_hid_descriptor *)buf;
> res = shut_get_descriptor(devp, USB_DT_HID, 0, buf, 0x9);
>
> Modified: trunk/drivers/libusb.c
>
=============================================================================>
--- trunk/drivers/libusb.c (original)
> +++ trunk/drivers/libusb.c Thu Aug 23 19:43:41 2007
> @@ -90,7 +90,8 @@
> opened device must match all of them. Also note: the string
> components of curDevice are filled with allocated strings that
> must later be freed. */
> -static int libusb_open(usb_dev_handle **udevp, HIDDevice_t *curDevice,
HIDDeviceMatcher_t *matcher, unsigned char *ReportDesc, int mode)
> +static int libusb_open(usb_dev_handle **udevp, HIDDevice_t *curDevice,
HIDDeviceMatcher_t *matcher,
> + unsigned char *ReportDesc, int *mode)
> {
> int found = 0;
> #if LIBUSB_HAS_DETACH_KRNL_DRV
> @@ -117,7 +118,8 @@
>
> for (bus = usb_busses; bus && !found; bus = bus->next) {
> for (dev = bus->devices; dev && !found; dev = dev->next) {
> - upsdebugx(2, "Checking device (%04X/%04X) (%s/%s)",
dev->descriptor.idVendor, dev->descriptor.idProduct, bus->dirname,
dev->filename);
> + upsdebugx(2, "Checking device (%04X/%04X) (%s/%s)",
dev->descriptor.idVendor,
> + dev->descriptor.idProduct, bus->dirname, dev->filename);
>
> /* supported vendors are now checked by the
> supplied matcher */
> @@ -140,26 +142,29 @@
> curDevice->Vendor = NULL;
> curDevice->Product = NULL;
> curDevice->Serial = NULL;
> - curDevice->Bus = strdup(bus->dirname);
> + curDevice->Bus = xstrdup(bus->dirname);
>
> if (dev->descriptor.iManufacturer) {
> - ret = usb_get_string_simple(udev, dev->descriptor.iManufacturer,
string, sizeof(string));
> + ret = usb_get_string_simple(udev, dev->descriptor.iManufacturer,
> + string, sizeof(string));
> if (ret > 0) {
> - curDevice->Vendor = strdup(string);
> + curDevice->Vendor = xstrdup(string);
> }
> }
>
> if (dev->descriptor.iProduct) {
> - ret = usb_get_string_simple(udev, dev->descriptor.iProduct, string,
sizeof(string));
> + ret = usb_get_string_simple(udev, dev->descriptor.iProduct,
> + string, sizeof(string));
> if (ret > 0) {
> - curDevice->Product = strdup(string);
> + curDevice->Product = xstrdup(string);
> }
> }
>
> if (dev->descriptor.iSerialNumber) {
> - ret = usb_get_string_simple(udev, dev->descriptor.iSerialNumber,
string, sizeof(string));
> + ret = usb_get_string_simple(udev, dev->descriptor.iSerialNumber,
> + string, sizeof(string));
> if (ret > 0) {
> - curDevice->Serial = strdup(string);
> + curDevice->Serial = xstrdup(string);
> }
> }
>
> @@ -171,20 +176,33 @@
> upsdebugx(2, "- Bus: %s", curDevice->Bus ?
curDevice->Bus : "unknown");
>
> upsdebugx(2, "Trying to match device");
> - for (m = matcher; m; m=m->next) {
> + for (m = matcher; m != NULL; m = m->next) {
> +
> ret = matches(m, curDevice);
> - if (ret==0) {
> + switch(ret)
> + {
> + case 2:
> + upsdebugx(2, "Device exact match");
> + break;
> + case 1:
> + upsdebugx(2, "Device regex match");
> + if (*mode == MODE_REOPEN) {
> + /* Asked for reopening the device, but we couldn't
> + * get an exact match, so opening instead. */
> + *mode = MODE_OPEN;
> + }
> + break;
> + case 0:
> upsdebugx(2, "Device does not match - skipping");
> goto next_device;
> - } else if (ret==-1) {
> - fatalx(EXIT_FAILURE, "matcher: %s", strerror(errno));
> - goto next_device;
> - } else if (ret==-2) {
> + case -1:
> + fatal_with_errno(EXIT_FAILURE, "matcher");
> + default:
> upsdebugx(2, "matcher: unspecified error");
> goto next_device;
> }
> + break;
> }
> - upsdebugx(2, "Device matches");
>
> /* Now we have matched the device we wanted. Claim it. */
>
> @@ -212,7 +230,7 @@
> /* set default interface */
> usb_set_altinterface(udev, 0);
>
> - if (mode == MODE_REOPEN || mode == MODE_NOHID) {
> + if (*mode == MODE_REOPEN || *mode == MODE_NOHID) {
> return 1;
> }
>
> @@ -257,7 +275,8 @@
> altsetting 0, as above. */
> iface = &dev->config[0].interface[0].altsetting[0];
> for (i=0; i<iface->extralen; i+=iface->extra[i]) {
> - upsdebugx(4, "i=%d, extra[i]=%02x, extra[i+1]=%02x", i,
iface->extra[i], iface->extra[i+1]);
> + upsdebugx(4, "i=%d, extra[i]=%02x, extra[i+1]=%02x", i,
> + iface->extra[i], iface->extra[i+1]);
> if (i+9 <= iface->extralen && iface->extra[i] >= 9
&& iface->extra[i+1] == 0x21) {
> p = &iface->extra[i];
> upsdebug_hex(3, "HID descriptor, method 2", p, 9);
>
> Modified: trunk/drivers/megatec_usb.c
>
=============================================================================>
--- trunk/drivers/megatec_usb.c (original)
> +++ trunk/drivers/megatec_usb.c Thu Aug 23 19:43:41 2007
> @@ -153,6 +153,7 @@
> HIDDeviceMatcher_t subdriver_matcher;
> int ret, i;
> char flush_buf[256];
> + int mode = MODE_NOHID;
>
> HIDDeviceMatcher_t *regex_matcher = NULL;
> int r;
> @@ -200,21 +201,21 @@
> regex_array[4] = NULL; /* getval("serial"); */
> regex_array[5] = getval("bus");
>
> - r = new_regex_matcher(®ex_matcher, regex_array, REG_ICASE |
REG_EXTENDED);
> + r = HIDNewRegexMatcher(®ex_matcher, regex_array, REG_ICASE |
REG_EXTENDED);
> if (r==-1) {
> - fatalx(EXIT_FAILURE, "new_regex_matcher: %s",
strerror(errno));
> + fatal_with_errno(EXIT_FAILURE, "HIDNewRegexMatcher");
> } else if (r) {
> fatalx(EXIT_FAILURE, "invalid regular expression: %s",
regex_array[r]);
> }
> /* link the matchers */
> regex_matcher->next = &subdriver_matcher;
>
> - ret = usb->open(&udev, &hiddevice, regex_matcher, NULL,
MODE_NOHID);
> + ret = usb->open(&udev, &hiddevice, regex_matcher, NULL,
&mode);
> if (ret < 0)
> usb_open_error(port);
>
> /* TODO: Add make exact matcher for reconnecting feature support */
> - free_regex_matcher(regex_matcher);
> + HIDFreeRegexMatcher(regex_matcher);
>
> /* flush input buffers */
> for (i = 0; i < 10; i++) {
>
> Modified: trunk/drivers/tripplite_usb.c
>
=============================================================================>
--- trunk/drivers/tripplite_usb.c (original)
> +++ trunk/drivers/tripplite_usb.c Thu Aug 23 19:43:41 2007
> @@ -348,6 +348,7 @@
> static int reconnect_ups(void)
> {
> int ret = 1;
> + int mode = MODE_REOPEN;
>
> if (hd == NULL)
> {
> @@ -357,7 +358,7 @@
>
> upsdrv_cleanup();
>
> - if ((hd = HIDOpenDevice(&udev, &curDevice, reopen_matcher,
MODE_REOPEN)) == NULL) {
> + if ((hd = HIDOpenDevice(&udev, &curDevice, reopen_matcher,
&mode)) == NULL) {
> upslogx(LOG_INFO, "Reconnecting to UPS failed; will retry
later...");
> dstate_datastale();
> ret = 0;
> @@ -1369,9 +1370,9 @@
> regex_array[4] = getval("serial"); /* probably won't see
this */
> regex_array[5] = getval("bus");
>
> - r = new_regex_matcher(®ex_matcher, regex_array, REG_ICASE |
REG_EXTENDED);
> + r = HIDNewRegexMatcher(®ex_matcher, regex_array, REG_ICASE |
REG_EXTENDED);
> if (r==-1) {
> - fatalx(EXIT_FAILURE, "new_regex_matcher: %s",
strerror(errno));
> + fatal_with_errno(EXIT_FAILURE, "HIDNewRegexMatcher");
> } else if (r) {
> fatalx(EXIT_FAILURE, "invalid regular expression: %s",
regex_array[r]);
> }
> @@ -1384,9 +1385,9 @@
> upslogx(1, "Detected a UPS: %s/%s", hd->Vendor ?
hd->Vendor : "unknown", hd->Product ? hd->Product :
"unknown");
>
> /* create a new matcher for later reopening */
> - reopen_matcher = new_exact_matcher(hd);
> - if (!reopen_matcher) {
> - upsdebugx(2, "new_exact_matcher: %s", strerror(errno));
> + r = HIDNewExactMatcher(&reopen_matcher, hd);
> + if (r) {
> + upsdebug_with_errno(2, "HIDNewExactMatcher");
> }
> /* link the two matchers */
> reopen_matcher->next = regex_matcher;
> @@ -1403,8 +1404,7 @@
>
> void upsdrv_cleanup(void)
> {
> - if (hd != NULL) {
> - HIDCloseDevice(udev);
> - udev = NULL;
> - }
> + HIDCloseDevice(udev);
> + HIDFreeExactMatcher(reopen_matcher);
> + HIDFreeRegexMatcher(regex_matcher);
> }
>
> Modified: trunk/drivers/usbhid-ups.c
>
=============================================================================>
--- trunk/drivers/usbhid-ups.c (original)
> +++ trunk/drivers/usbhid-ups.c Thu Aug 23 19:43:41 2007
> @@ -703,6 +703,11 @@
> dstate_datastale();
> return;
> }
> +
> + if (hid_ups_walk(HU_WALKMODE_INIT) == FALSE) {
> + hd = NULL;
> + return;
> + }
> }
> #ifdef DEBUG
> interval();
> @@ -716,7 +721,7 @@
> /* Process pending events (HID notifications on Interrupt pipe) */
> for (i = 0; i < evtCount; i++) {
>
> - if (HIDGetDataValue(udev, event[i], &value, 2) != 1)
> + if (HIDGetDataValue(udev, event[i], &value, poll_interval) != 1)
> continue;
>
> if (nut_debug_level >= 1) {
> @@ -807,7 +812,7 @@
>
> void upsdrv_initups(void)
> {
> - int i;
> + int i, mode = MODE_OPEN;
> #ifndef SHUT_MODE
> /*!
> * SHUT is only supported by MGE UPS SYSTEMS units
> @@ -831,13 +836,13 @@
> regex_array[4] = getval("serial");
> regex_array[5] = getval("bus");
>
> - r = new_regex_matcher(®ex_matcher, regex_array, REG_ICASE |
REG_EXTENDED);
> + r = HIDNewRegexMatcher(®ex_matcher, regex_array, REG_ICASE |
REG_EXTENDED);
> switch(r)
> {
> - case -1:
> - fatal_with_errno(EXIT_FAILURE, "new_regex_matcher()");
> case 0:
> break;
> + case -1:
> + fatal_with_errno(EXIT_FAILURE, "HIDNewRegexMatcher()");
> default:
> fatalx(EXIT_FAILURE, "invalid regular expression: %s",
regex_array[r]);
> }
> @@ -859,15 +864,15 @@
>
> /* Search for the first supported UPS matching the regular
> expression (not for SHUT_MODE) */
> - if ((hd = HIDOpenDevice(&udev, &curDevice, regex_matcher,
MODE_OPEN)) == NULL)
> + if ((hd = HIDOpenDevice(&udev, &curDevice, regex_matcher,
&mode)) == NULL)
> fatalx(EXIT_FAILURE, "No matching HID UPS found");
>
> upslogx(1, "Detected a UPS: %s/%s", hd->Vendor ?
hd->Vendor : "unknown", hd->Product ? hd->Product :
"unknown");
> #ifndef SHUT_MODE
> /* create a new matcher for later reopening */
> - reopen_matcher = new_exact_matcher(hd);
> - if (!reopen_matcher) {
> - fatal_with_errno(EXIT_FAILURE, "new_exact_matcher()");
> + r = HIDNewExactMatcher(&reopen_matcher, hd);
> + if (r) {
> + fatal_with_errno(EXIT_FAILURE, "HIDNewExactMatcher()");
> }
> /* link the two matchers */
> reopen_matcher->next = regex_matcher;
> @@ -895,6 +900,8 @@
> upsdebugx(1, "upsdrv_cleanup...");
>
> HIDCloseDevice(udev);
> + HIDFreeExactMatcher(reopen_matcher);
> + HIDFreeRegexMatcher(regex_matcher);
> }
>
> /**********************************************************************
> @@ -1004,6 +1011,23 @@
> switch (mode)
> {
> /* Device capabilities enumeration */
> + case HU_WALKMODE_RESET:
> + if (item->hiddata == NULL)
> + continue;
> +
> + /* Reset the NUT-to-HID translation */
> + item->hiddata = NULL;
> +
> + /* If this was a command, remove it... */
> + if (item->hidflags & HU_TYPE_CMD) {
> + dstate_delcmd(item->info_type);
> + continue;
> + }
> +
> + /* ...otherwise, remove variable */
> + dstate_delinfo(item->info_type);
> + continue;
> +
> case HU_WALKMODE_INIT:
> /* Special case for handling server side variables */
> if (item->hidflags & HU_FLAG_ABSENT) {
> @@ -1018,23 +1042,30 @@
> continue;
> }
>
> + /* Apparently, we are reconnecting, so
> + * NUT-to-HID translation is still good */
> + if (item->hiddata != NULL)
> + break;
> +
> /* Get pointer to the corresponding HIDData_t item */
> item->hiddata = HIDGetItemData(udev, item->hidpath,
subdriver->utab);
> -
> if (item->hiddata == NULL)
> continue;
>
> - /* Avoid redundancy when multiple defines (RO/RW)
> - * ups.status and ups.alarm is not set during
> - HU_WALKMODE_INIT, so these don't need to be
> - caught here. */
> - if (dstate_getinfo(item->info_type) != NULL) {
> + /* Allow duplicates for these NUT variables... */
> + if (!strncmp(item->info_type, "input.transfer.reason", 21)
||
> + !strncmp(item->info_type, "ups.alarm", 9)) {
> + break;
> + }
>
> - item->hiddata = NULL;
> - continue;
> + /* ...this one doesn't exist yet... */
> + if (dstate_getinfo(item->info_type) == NULL) {
> + break;
> }
>
> - break;
> + /* ...but this one does, so don't use it! */
> + item->hiddata = NULL;
> + continue;
>
> case HU_WALKMODE_QUICK_UPDATE:
> /* Quick update only deals with status and alarms! */
> @@ -1058,7 +1089,7 @@
> fatalx(EXIT_FAILURE, "hid_ups_walk: unknown update mode!");
> }
>
> - retcode = HIDGetDataValue(udev, item->hiddata, &value, 2);
> + retcode = HIDGetDataValue(udev, item->hiddata, &value,
poll_interval);
>
> switch (retcode)
> {
> @@ -1069,7 +1100,7 @@
> case -EIO:
> case -ENOENT:
> /* Uh oh, got to reconnect! */
> - reconnect_ups();
> + hd = NULL;
> return FALSE;
>
> case 1:
> @@ -1114,14 +1145,23 @@
>
> static int reconnect_ups(void)
> {
> + int mode = MODE_REOPEN;
> +
> upsdebugx(4,
"==================================================");
> upsdebugx(4, "= device has been disconnected, try to reconnect
=");
> upsdebugx(4,
"==================================================");
>
> - if ((hd = HIDOpenDevice(&udev, &curDevice, reopen_matcher,
MODE_REOPEN)) == NULL) {
> + if ((hd = HIDOpenDevice(&udev, &curDevice, reopen_matcher,
&mode)) == NULL) {
> return 0;
> }
>
> + /* Apparently, HIDOpenDevice() couldn't get an exact
> + * match, so we must treat this as a new connection */
> + if (mode != MODE_REOPEN) {
> + upsdebugx(4, "reconnect_ups: couldn't get an exact match,
resetting");
> + hid_ups_walk(HU_WALKMODE_RESET);
> + }
> +
> return 1;
> }
>
>
> Modified: trunk/drivers/usbhid-ups.h
>
=============================================================================>
--- trunk/drivers/usbhid-ups.h (original)
> +++ trunk/drivers/usbhid-ups.h Thu Aug 23 19:43:41 2007
> @@ -151,6 +151,7 @@
> #define HU_WALKMODE_INIT 1
> #define HU_WALKMODE_QUICK_UPDATE 2
> #define HU_WALKMODE_FULL_UPDATE 3
> +#define HU_WALKMODE_RESET 4
>
> /* TODO: rework flags */
> #define HU_FLAG_OK 1 /* show element to upsd. */
>
> _______________________________________________
> nut-commits mailing list
> nut-commits at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/nut-commits
>
Arjen de Korte
2007-Aug-24 09:45 UTC
[Nut-upsdev] [nut-commits] svn commit r1073 - in trunk: . drivers
> I think having this logic buried within libhid/libusb > (libusb:libusb_open(), line 179 to 206) is ultimately a mistake, > albeit one that I am probably responsible for.You introduced this in r197... :-)> Would it make sense to > confine libhid to low-level operations, and leave the decision of > trying to reopen vs. retrying to open to the high-level driver, in > this case usbhid-ups?That is probably a good idea.> I envision that the code in usbhid-ups:reconnect_ups() could be > changed to something like: > > hd = HIDOpenDevice(&udev, &curDevice, reopen_matcher, MODE_REOPEN); > if (hd == NULL) { > /* reopening failed, try opening new device */ > hd = HIDOpenDevice(&udev, &curDevice, regex_matcher, MODE_OPEN); > } > if (hd == NULL) { > return 0; > }Good point.> Note "regex_matcher", not "reopen_matcher", in the second call, and > also the changed MODE flag.In order for this to work, we would need to split these into reopen_matcher and regex_matcher->subdriver_matcher So the linking between these in upsdrv_initups() should be removed.> Then all the complicated stuff could be removed from libhid/libusb > (different non-zero return codes for matchers etc).Makes a lot of sense.> I am not sure the code in libusb_open() is correct anyway, as matching > a chain of matchers should mean matching *all* the matchers contained > therein, not just the first possible one. For example, after a positive > reopen_match, one should still check regex_match also.The latter makes no sense to me. An exact match is a subset of (hopefully one) of the devices that match the regex matcher. If you already have an exact match, you therefor are guaranteed to have a match on the regex matcher, since that is how you got there in the first place through the initial call to HIDOpenDevice() with MODE_OPEN. The added regex match would effectively be a no-op then.> In other words, we are dealing with a loop like this: > > For each device curDevice: { > Try exact match, stop if it succeeds. > Try regex match, stop if it succeeds. > }This is the present (and wrong) way.> This means we match the first device for which an exact match *or* a > regex match is found. In other words, if the device that got > disconnected is on bus 5, and some other loosely matching device is on > bus 3, then we'll open the new device on bus 3, rather than reopening > the disconnected device on bus 5.Very true, I will fix this later today. I'll need fix a memory leak in libusb_open anyway. We leak curDevice->Vendor, curDevice->Product, curDevice->Serial and curDevice->Bus each time we run HIDOpenDevice. At the default rate of one attempt every two seconds (which will change to two attempts after the change below), this won't take long to eat up all memory if a device is detached. Maybe we should cap the number of (consecutive) times we attempt to reconnect and bail out if we see too many failures? Or would that be a bad idea?> What we should be doing instead is: > For each device curDevice: { > Try exact match, stop if it succeeds. > } > For each device curDevice: { > Try regex match, stop if it succeeds. > } > > I think moving this logic up into usbhid-ups would simplify things > greatly.Indeed. Best regards, Arjen -- Eindhoven - The Netherlands Key fingerprint - 66 4E 03 2C 9D B5 CB 9B 7A FE 7E C1 EE 88 BC 57