On Tue, Jun 30, 2020 at 10:17:36AM +0000, Wei Liu wrote:
[...]

quoted

quoted

If the allocation of the requestor fails during runtime, vmbus_open()
fails too and therefore,
the channel and the requestor will not be created. So, the 2 functions
(next_id, requestor_addr)
will never get called, right? The only case in which we hit this edge
case is if a driver is using this
mechanism with a size of 0 (i.e. rqstor_size is not set to a non-zero
value before calling vmbus_open()),

Right. This is what I was getting at. Setting the size to 0 effectively
makes the driver unusable. And per your design, it should be considered
a bug.

quoted

but that would be more like a coding bug. So, I think it would be
better to return VMBUS_RQST_ERROR
as a way to assert that there is a bug in the code. I don't know if
I'm missing something here.

Since we know setting size to 0 is a bug, you can actually just do the
following in the __vmbus_open function instead of going through all the
initialization with the knowledge vmbus_next_request_id & co will fail.

    /* Create and init requestor */
    if (!newchannel->rqstor_size)
          return an error to caller here

    vmbus_alloc_requestor(...);

And obviously you should check vmbus_alloc_requestor's return value
somehow. You get the idea...

Andrea pointed out that I missed one critical aspect of the design --
not all drivers are supposed to use this infrastructure. That's contrary
to my original understanding, in which all drivers are supposed to use
this infrastructure.

With that in mind, it is okay to only initialize the infra only when
->rqstor_size is not zero. Then you just handle the edge case in
vmbus_next_request_id & co.

Wei.

Wei.

quoted

Wei.

quoted

Andres.