On Tue, Nov 08, 2011 at 18:20:02 +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Split the existing virNetClientSend into two parts
> virNetClientSend and virNetClientSendNoReply, instead
> of having a 'bool expectReply' parameter.
>
> Add a new virNetClientSendNonBlock which returns 2 on
> full send, 1 on partial send, 0 on no send, -1 on error
>
> If a partial send occurs, then a subsequent call to any
> of the virNetClientSend* APIs will finish any outstanding
> I/O.
Do I understand it correctly that the main difference between this patch and
my patch is that my patch stores the unfinished call separated from the other
call while you store it at the beginning of the waitDispatch list?
Your solution looks cleaner but I suspect the other minor differences from my
patch are actually bugs (more on them later in this email) :-)
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index 4b7d4a9..b0ed507 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
...
> @@ -924,6 +954,12 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
> if (virNetSocketHasCachedData(client->sock))
> timeout = 0;
>
> + /* If we're a non-blocking call, then we don't
> + * want to wait for I/O readyness
> + */
> + if (thiscall->nonBlock)
> + timeout = 0;
> +
> fds[0].events = fds[0].revents = 0;
> fds[1].events = fds[1].revents = 0;
>
> @@ -975,8 +1011,34 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
> /* If we have existing SASL decoded data, pretend
> * the socket became readable so we consume it
> */
> - if (virNetSocketHasCachedData(client->sock))
> + if (virNetSocketHasCachedData(client->sock)) {
> fds[0].revents |= POLLIN;
> + } else if (ret == 0 && thiscall->nonBlock) {
> + if (thiscall->msg->bufferOffset == 0) {
Unfortunately, this condition won't work with SASL since it encodes some data
first time we call virNetClientIOWriteMessage but returns 0 and requires us to
call it repeatedly until it returns bufferLength, which means all data were
sent. Thus, thiscall->msg->bufferOffset == 0 doesn't mean we did not send any
data.
> + /* No data sent at all, remove ourselves from the list */
> + tmp = client->waitDispatch;
> + prev = NULL;
> + while (tmp) {
> + if (tmp == thiscall) {
> + if (prev) {
> + prev->next = thiscall->next;
> + } else {
> + client->waitDispatch = thiscall->next;
> + }
> + break;
> + }
> + prev = tmp;
> + tmp = tmp->next;
> + }
> + VIR_DEBUG("Giving up the buck %p %p", thiscall, client->waitDispatch);
> + virNetClientPassTheBuck(client);
> + return 0;
> + } else {
> + VIR_DEBUG("Giving up the buck %p %p", thiscall, client->waitDispatch);
> + virNetClientPassTheBuck(client);
> + return 1; /* partial send */
> + }
> + }
>
> if (fds[1].revents) {
> VIR_DEBUG("Woken up from poll by other thread");
...
> @@ -1117,20 +1196,27 @@ static int virNetClientIO(virNetClientPtr client,
> thiscall->msg->bufferLength,
> client->waitDispatch);
>
> + /* Trivially detect blocking if someone else has the buck already */
> + if (client->haveTheBuck &&
> + thiscall->nonBlock)
> + return 0;
Hmm, I don't think this is correct. We actually want nonBlock call to be send
while another thread has the buck and is waiting for its call to finish. The
call may take a long time to complete and we need to send keepalive
requests/responses during that time.
> +
> + /* Stick ourselves on the end of the wait queue */
> + tmp = client->waitDispatch;
> + while (tmp && tmp->next)
> + tmp = tmp->next;
> + if (tmp)
> + tmp->next = thiscall;
> + else
> + client->waitDispatch = thiscall;
> +
> /* Check to see if another thread is dispatching */
> - if (client->waitDispatch) {
> - /* Stick ourselves on the end of the wait queue */
> - virNetClientCallPtr tmp = client->waitDispatch;
> + if (client->haveTheBuck) {
> char ignore = 1;
> - while (tmp && tmp->next)
> - tmp = tmp->next;
> - if (tmp)
> - tmp->next = thiscall;
> - else
> - client->waitDispatch = thiscall;
>
> /* Force other thread to wakeup from poll */
> if (safewrite(client->wakeupSendFD, &ignore, sizeof(ignore)) != sizeof(ignore)) {
> + /* Something went wrong, so we need to remove that call we just added */
> if (tmp)
> tmp->next = NULL;
> else
...
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list