[Nut-upsdev] [nut-commits] svn commit r1074 - in trunk: .

[Peter Selinger]

Hi Arjen,

I see now what our misunderstanding might be. 

Arjen de Korte wrote:
> 
> 
> 
> >> Yes. What is open to discussion, is what the default should be. I also
> >> pointed that out.
> >
> > Okay, perhaps I am misunderstanding. There are three possible
> > behaviors on reconnection:
> >
> > (1) *always* match strictly the same device, and give an error otherwise,
> > (2) *always* match laxly; do not even attempt strict matching, even if
> >     it would succeed.
> > (3) try strict matching first, and use lax matching as a fallback.
> >
> > Option (3) used to be the default, and in fact it used to be the only
> > possible behavior.
> 
> No, we never had that, until I started messing with this code a few days
> ago. We would always use 'regex' matching on the first connect and
> 'strict' matching on reconnects.

You are right, that's what used to happen. I assume that the current
discussion is about what happens on "reconnect"; there is no question
that for a first connection, (2) is always the desired behavior (even
for servers). Namely, a regular expression can be "strict" (if it
contains no wildcards), or as non-strict as the user might find
useful. 

On *re*-connection, you are right that we used to implement
(1). However, we discussed that (3) would be more useful (and I
thought that you had tried implementing (3) a few days ago). I don't
see the usefulness of option (2), as it seems that (3) is always
strictly better.

> > If I understand your log message correctly, you have now eliminated
> > option (3) altogether, giving the user a choice only between (1) and (2).
> 
> It never existed, although by adding the 'Bus' to the exact matcher, I
> made the exact matcher a little more strict.
> 
> > This would be a very bad idea in my opinion. I cannot think of any
> > situation where behavior (2) would be preferable to behavior (3).
> 
> You suggested it yourself, a couple of days ago, when you said that we
> should probably always reconnect in MODE_OPEN, while I bargained to keep
> the MODE_REOPEN in. Effectively, that is the same as option 2 if you don't
> change the reopen matcher (which we didn't discuss at that time however).

Ah, this seems to be where our main misunderstanding is. There are
*two* relevant parameters to HIDOpenDevice:

* "mode", which can be MODE_OPEN or MODE_REOPEN. This controls whether
  the report descriptor is re-read and re-parsed. As I said a couple
  of days ago, the MODE_REOPEN option is perhaps not entirely safe,
  and the time savings from not re-parsing the report descriptor are
  negligible. So I suggested to use MODE_OPEN, to re-parse the report
  descriptor each time, even if we think we matched the same device. 
  However, you had some objection to this based on dynamic allocation
  of memory. 

  Actually, there is a third value for this parameter, which is
  MODE_NOHID. It is documented only in libusb.c, as it is only used in
  non-HID devices (i.e., it is not a valid option to HIDOpenDevice). 

* "matcher". This determines which device will be opened. 

Now when I said that we should probably always reconnect in MODE_OPEN,
I was talking about the "mode" parameter, and not about the "matcher"
parameter. So I was talking about *how* we should open the device, not
*which* device to open. 

> > Clearly, if the exact matching device is available, we should try to
> > connect to it. The question should only be what to do if the exact
> > matching device is not available. Therefore, the user should be given
> > a choice between (1) and (3), and not between (1) and (2).
> 
> I really don't see a point here. The device has matched on the first
> attempt the 'regex' matcher, I don't see why it wouldn't a second time.

It would. But it may not be the only device matching. There may be two
devices that match the same regular expression. In fact, one extreme
case of this is where the user didn't specify any regular expression
at all (perhaps she had only one UPS at the time). She might add a
second UPS for testing purposes. If the first one gets disconnected
due to wire congestion or whatever, NUT should try to reconnect to the
same device that just got disconnected, if possible. This was the
purpose of the exact_matcher. 

On the other hand, if our user plugs in UPS2, then accidentally
unplugs UPS1, and then plugs UPS1 into a different USB port, I think
the driver should still try to connect to UPS1, and not UPS2. This is
why I don't think "bus" belongs in the exact_matcher. It was intended
to match a device, not a location. 

> What I want to achieve here, is predictable behaviour. In 'exact' mode,
> the driver would always reconnect to the device it connected to on
> startup, while in 'regex' mode it would behave as if you would restart the
> whole driver.

Yes, but reconnection is not a user-initiated event. With some
devices, it happens once every half-hour. So behaving exactly as if
the whole driver was restarted was perhaps not the most desirable
behavior. 

But I agree that it is a murky issue. Perhaps we should go back to
behavior (1) as the only alternative on reconnect (and behavior (2) on
a first connection of course). 

Even in this case, I would *still* use MODE_OPEN, because even the
exact matcher is not 100% sure to open the exact same device. Many
vendors don't include a serial number, for example. 
 
> The problem with the 'regex' matcher we have now, is that it may also
> require the subdriver to change, so behaviour (2) and (3) are not possible
> yet. Currently, usbhid-ups only checks this on startup. So you may be able
> to open a new device through the 'regex' matcher, only to find out that
> when you try to read anthing from the device, you have the wrong hid2nut
> tables, because we never tried to see if we still use the same subdriver.
> Bummer.

That is true. Perhaps we should stick to behavior (1) for this
reason. At least the exact_matcher will guarantee that it's the same
vendorid/productid.
 
> [...]
> 
> > OK, I read that message after the other one. However, you message only
> > makes sense if option (3) has already been eliminated. For the reasons
> > above, I think option (3) should not be eliminated.
> 
> Since it never existed, I don't think we should worry about that. Before
> we used the
> 
> 	reopen_matcher->regex_matcher->subdriver_matcher
> 
> Which means that the only behaviour we had for reconnects was a slightly
> less strict (without matching the Bus) matching than what we have now.
> 
> > Strict bus matching may (or may not) be desirable for behavior (1). In
> > fact, I think it is not desirable, because even for a server, being
> > able to move the UPS to different physical locations can be useful.
> 
> Yes, but no operator in his right mind would do this without seeing if
> this worked while on site.

Actually, I disagree. Most people assume that USB is a transparent bus
system. We should not violate this assumption - in fact it is a very
useful assumption. When a device works on one bus, but not another,
this is bound to cause someone a major debugging headache.

It's an entirely different story if the user actually specified the -x
bus option. In this case, they have *requested* this behavior, so they
will not be surprised by it. 

> > If the user wants a specific bus, they can use the existing -x bus
> > option, as has always been the case.
> 
> That would be cumbersome, since in that case you would first have to find
> out where the UPS was connected before starting the driver. If you change
> it, you need to both modify the -x bus option (more likely, 'ups.conf')
> and then restart the driver. With the strict matching we have now, moving
> the UPS to another port would just require a restart of the driver.

OK, so the -x bus option will be almost never used. In 99% of
situations, there is a single UPS on the system, and no -x option will
be needed. In 0.99% of situations, there are multiple UPS, but they
are not identical (e.g., they have different serial numbers). I think
there is only 0.01% of situations where a server has two UPS connected
that are absolutely identical, but power two different loads. In this
case (and only this case), the -x bus option is necessary and useful. 

On the other hand, by putting "bus" in the strict_matcher, you are
bound to already confuse the first 99% of users, who may simply move
their UPS to a different port because they can't reach the crawl space
behind their computer.
 
> > Strict bus matching is irrelevant for behavior (2).
> 
> It can be used in the 'regex' expressions.

Yes, but the question of whether the strict-matcher includes "bus" is
irrelevant for behavior (2), because it does not use the
strict-matcher. 
 
> > Strict bus matching is a really *bad* idea for behavior (3). It
> > basically means that no users, including desktop users, have the
> > option to move their UPS to a different port any more.
> 
> It's not meant for desktop users, I think they would expect the driver to
> find their UPS on any USB port they connect it to.

Yes, and they still may want reconnect to behave in a predictable way,
i.e., connect to the same UPS (if they have more than one), no matter
which USB port they connect it to. Strict matching (excluding bus
matching) should be available to them. 

-- Peter