Re: [Qemu-stable] [Qemu-devel] [PATCH] qemu-ga: use key-value store to avoid recycling fd handles after restart

[Luiz Capitulino]

On Wed, 20 Mar 2013 13:14:21 -0500
mdroth <address@hidden> wrote:

> > > > > > > +    handle = s->pstate.fd_counter++;
> > > > > > > +    if (s->pstate.fd_counter < 0) {
> > > > > > > +        s->pstate.fd_counter = 0;
> > > > > > > +    }
> > > > > > 
> > > > > > Is this handling the overflow case? Can't fd 0 be in use already?
> > > > > 
> > > > > It could, but it's very unlikely that an overflow/counter reset would
> > > > > result in issuing still-in-use handle, since guest-file-open would 
> > > > > need
> > > > > to be called 2^63 times in the meantime.
> > > > 
> > > > Agreed, but as you do check for that case and as the right fix is simple
> > > > and I think it's worth it. I can send a patch myself.
> > > > 
> > > 
> > > A patch to switch to tracking a list of issued handles in the keystore,
> > > or to return an error on overflow?
> > 
> > To return an error on overflow. Minor, but if we do handle it let's do it
> > right. Or, we could just add an assert like:
> > 
> > assert(s->pstate.fd_counter >= 0);
> 
> Ah, well, I'm not sure I understand then. You mean dropping:
> 
>     if (s->pstate.fd_counter < 0) {
>         s->pstate.fd_counter = 0;
>     }
> 
> And replacing it with an error or assertion?

Yes, because I had understood you meant that this is very unlikely to be
triggered because we'd need guest-file-open to be called 2^63. This is
what I agreed above, although thinking more about it there's also the
possibility of a malicious client doing this on purpose.

But now I see that what you really meant is that it's unlikely for fd 0
to be in use after 2^63 guest-file-open calls. Am I right? If yes, then
I disagree because there's no way to guarantee when a certain fd will be
in use or not, unless we allow fds to be returned.

> If so, the overflow is actually expected: once we dish out handle MAX_INT64,
> we should restart at 0. I initially made fd_counter a uint64_t so
> overflow/reset would happen more naturally, but since we issue handles as
> int64_t this would've caused other complications.
> 
> Something like this might be more clear about the intent though:
> 
>     handle = s->pstate.fd_counter;
>     if (s->pstate.fd_counter == MAX_INT64) {
>         s->pstate.fd_counter = 0;
>     } else {
>         s->pstate.fd_counter++;
>     }

I disagree about restarting to zero as I have explained above. You seem to
not like returning an error, is it because we'll make guest-file-open
useless after the limit is reached?

Let's review our options:

 1. When fd_count reaches MAX_INT64 we reset it to zero

    Pros: simple and guest-file-open always work
    Cons: fd 0 might be in use by a client

 2. When fd_count reaches MAX_INT64 we return an error

    Pros: simple and we fix 'cons' from item 1
    Cons: guest-file-open will have a usage count limit

 3. Allow fds to be returned by clients on guest-file-close and do 2 on top

    Pros: solve problems discussed in items 1 and 2
    Cons: not trivial and the usage limit problem from item 2 can still
          happen if the client ends up not calling guest-file-close
          (although I do think we'll reach the OS limit here)

Do you see other options? Am I overcomplicating?