Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatic reply from events on Drop #60

Open
3 tasks
Wenzel opened this issue Feb 15, 2020 · 5 comments
Open
3 tasks

Automatic reply from events on Drop #60

Wenzel opened this issue Feb 15, 2020 · 5 comments
Labels
enhancement New feature or request

Comments

@Wenzel
Copy link
Owner

Wenzel commented Feb 15, 2020

On feature that I would like on the API is when an Event is being Dropped from the scope, it would automatically send a default response, which would be EventReplyType::Continue.

tasks:

  • add reference to Introspectable in the Event, so the event can call the driver
  • impl Drop for Event
  • add boolean in Event so that you can't reply twice to the same event

related: https://github.com/Wenzel/libmicrovmi/pull/49/files

thoughts @kylerky, @rageagainsthepc ?

@kylerky
Copy link

kylerky commented Feb 15, 2020

The default response sent in drop does not make much sense to me. I would like more control of what I send.

Also, I wonder if this will complicate implementation because we may need to add quite a few logic in the drop function. The function may need to know whether a response by user has already been sent before it sends a default one.

Moreover, I will think twice before adding a reference to Introspectable, which may bring some troubles with the borrow checker.

@Wenzel
Copy link
Owner Author

Wenzel commented Feb 16, 2020

Also, I wonder if this will complicate implementation because we may need to add quite a few logic in the drop function. The function may need to know whether a response by user has already been sent before it sends a default one.

A boolean would suffice

#[repr(C)]
pub struct Event {
    pub vcpu: u16,
    pub kind: EventType,
    driver: &dyn Introspectable, // pseudo code here, this might not be Rust compatible
    reply_sent: bool,

}

impl Event {
    // this API should be called to clearly define what event reply you want
   fn reply(reply_type: ReplyEventType) -> Result<(), Box<dyn Error>> {
        self.driver.reply_event(self, reply_type)?;
       Ok(())
   }
}

impl Drop for Event {
    fn drop(&self) {
         if !self.reply_sent {
              self.reply(self, ReplyEventType::Continue);
              self.reply_sent = true;
         }
    }
}

Moreover, I will think twice before adding a reference to Introspectable, which may bring some troubles with the borrow checker.

Yes, when I tried to implement it, I have issues with the lifetimes and such, so I gave up.

@rageagainsthepc
Copy link
Collaborator

rageagainsthepc commented Feb 16, 2020

Yes, when I tried to implement it, I have issues with the lifetimes and such, so I gave up.

You could use reference counting instead. I know this is very C++-ish, but this would allow you to store non-owning references aka weak pointers in your event. You need to upgrade weak pointers if you want to use them and this will fail if the object does not exist anymore. Might be worth a shot.
https://doc.rust-lang.org/std/rc/struct.Weak.html

@rageagainsthepc
Copy link
Collaborator

I often use weak pointers in event callbacks for the vmi implementation in my company. Helps to alleviate segfaults. ;)

@rageagainsthepc
Copy link
Collaborator

rageagainsthepc commented Feb 22, 2020

Here is an idea: Why don't we have something like an event supervisor? The only way to listen for events is to register callbacks with the event supervisor. This would allow us to shape how events have to be handled. For example we could enforce an event reply by defining the reply as the return value of the callback.

@Wenzel Wenzel added the enhancement New feature or request label Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants