Add status on/off switching to filter-redirector

[PATCH 1/3] net/filter-redirector: add support for dynamic status on/off switching

Posted by Jason Wang 3 days, 1 hour ago

Currently, filter-redirector does not implement the status_changed
callback, which means the 'status' property cannot be used to
dynamically enable/disable the filter at runtime. When status is
set to 'off' via QMP/HMP, the filter still receives data from the
indev chardev because the chardev handlers remain registered.

This patch adds proper support for the 'status' property:

1. Implement filter_redirector_status_changed() callback:
   - When status changes to 'off': remove chardev read handlers
   - When status changes to 'on': re-register chardev handlers
     (only if chardev is already open)

2. Update filter_redirector_setup() to respect initial status:
   - If filter is created with status=off, do not register handlers
   - This allows creating disabled filters via command line or QMP

3. Handle chardev OPENED/CLOSED events to re-arm handlers on reconnect:
   - Keep the chr_event callback installed on CLOSE so a later OPENED
     can re-register the read handlers when nf->on
   - Use qemu_chr_fe_set_handlers_full(..., set_open=false, sync_state=false)
     instead of qemu_chr_fe_set_handlers() because the latter forces
     sync_state=true and may emit CHR_EVENT_OPENED for an already-open
     backend. Doing that from inside the chr_event callback would cause
     recursive/re-entrant OPENED handling.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/filter-mirror.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index 8e01e98f40..aa2ab452fd 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -179,9 +179,16 @@ static void redirector_chr_event(void *opaque, QEMUChrEvent event)
     MirrorState *s = FILTER_REDIRECTOR(nf);
 
     switch (event) {
+    case CHR_EVENT_OPENED:
+        if (nf->on) {
+            qemu_chr_fe_set_handlers_full(&s->chr_in, redirector_chr_can_read,
+                                          redirector_chr_read, redirector_chr_event,
+                                          NULL, nf, NULL, false, false);
+        }
+        break;
     case CHR_EVENT_CLOSED:
-        qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
-                                 NULL, NULL, NULL, true);
+        qemu_chr_fe_set_handlers_full(&s->chr_in, NULL, NULL, redirector_chr_event,
+                                      NULL, nf, NULL, false, false);
         break;
     default:
         break;
@@ -306,9 +313,11 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
             return;
         }
 
-        qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
-                                 redirector_chr_read, redirector_chr_event,
-                                 NULL, nf, NULL, true);
+        if (nf->on) {
+            qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
+                                     redirector_chr_read, redirector_chr_event,
+                                     NULL, nf, NULL, true);
+        }
     }
 
     if (s->outdev) {
@@ -324,6 +333,24 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
     }
 }
 
+static void filter_redirector_status_changed(NetFilterState *nf, Error **errp)
+{
+    MirrorState *s = FILTER_REDIRECTOR(nf);
+
+    if (!s->indev) {
+        return;
+    }
+
+    if (nf->on) {
+        qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
+                                 redirector_chr_read, redirector_chr_event,
+                                 NULL, nf, NULL, true);
+    } else {
+        qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
+                                 NULL, NULL, NULL, true);
+    }
+}
+
 static char *filter_redirector_get_indev(Object *obj, Error **errp)
 {
     MirrorState *s = FILTER_REDIRECTOR(obj);
@@ -440,6 +467,7 @@ static void filter_redirector_class_init(ObjectClass *oc, const void *data)
     nfc->setup = filter_redirector_setup;
     nfc->cleanup = filter_redirector_cleanup;
     nfc->receive_iov = filter_redirector_receive_iov;
+    nfc->status_changed = filter_redirector_status_changed;
 }
 
 static void filter_mirror_init(Object *obj)
-- 
2.34.1

Re: [PATCH 1/3] net/filter-redirector: add support for dynamic status on/off switching

Posted by Zhang Chen 1 day, 20 hours ago

On Sun, Jan 4, 2026 at 3:54 PM Jason Wang <jasowang@redhat.com> wrote:
>
> Currently, filter-redirector does not implement the status_changed
> callback, which means the 'status' property cannot be used to
> dynamically enable/disable the filter at runtime. When status is
> set to 'off' via QMP/HMP, the filter still receives data from the
> indev chardev because the chardev handlers remain registered.
>
> This patch adds proper support for the 'status' property:
>
> 1. Implement filter_redirector_status_changed() callback:
>    - When status changes to 'off': remove chardev read handlers
>    - When status changes to 'on': re-register chardev handlers
>      (only if chardev is already open)
>
> 2. Update filter_redirector_setup() to respect initial status:
>    - If filter is created with status=off, do not register handlers
>    - This allows creating disabled filters via command line or QMP
>
> 3. Handle chardev OPENED/CLOSED events to re-arm handlers on reconnect:
>    - Keep the chr_event callback installed on CLOSE so a later OPENED
>      can re-register the read handlers when nf->on
>    - Use qemu_chr_fe_set_handlers_full(..., set_open=false, sync_state=false)
>      instead of qemu_chr_fe_set_handlers() because the latter forces
>      sync_state=true and may emit CHR_EVENT_OPENED for an already-open
>      backend. Doing that from inside the chr_event callback would cause
>      recursive/re-entrant OPENED handling.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/filter-mirror.c | 38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> index 8e01e98f40..aa2ab452fd 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -179,9 +179,16 @@ static void redirector_chr_event(void *opaque, QEMUChrEvent event)
>      MirrorState *s = FILTER_REDIRECTOR(nf);
>
>      switch (event) {
> +    case CHR_EVENT_OPENED:
> +        if (nf->on) {
> +            qemu_chr_fe_set_handlers_full(&s->chr_in, redirector_chr_can_read,
> +                                          redirector_chr_read, redirector_chr_event,
> +                                          NULL, nf, NULL, false, false);
> +        }
> +        break;
>      case CHR_EVENT_CLOSED:
> -        qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
> -                                 NULL, NULL, NULL, true);
> +        qemu_chr_fe_set_handlers_full(&s->chr_in, NULL, NULL, redirector_chr_event,
> +                                      NULL, nf, NULL, false, false);
>          break;
>      default:
>          break;
> @@ -306,9 +313,11 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
>              return;
>          }
>
> -        qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
> -                                 redirector_chr_read, redirector_chr_event,
> -                                 NULL, nf, NULL, true);
> +        if (nf->on) {
> +            qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
> +                                     redirector_chr_read, redirector_chr_event,
> +                                     NULL, nf, NULL, true);
> +        }
>      }
>
>      if (s->outdev) {
> @@ -324,6 +333,24 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
>      }
>  }
>
> +static void filter_redirector_status_changed(NetFilterState *nf, Error **errp)
> +{
> +    MirrorState *s = FILTER_REDIRECTOR(nf);
> +
> +    if (!s->indev) {

It's better to add a error here, for example:

        error_setg(errp,  "filter_redirector_status_changed failed for
the s->indev cleared" );


> +        return;
> +    }
> +
> +    if (nf->on) {
> +        qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
> +                                 redirector_chr_read, redirector_chr_event,
> +                                 NULL, nf, NULL, true);
> +    } else {
> +        qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
> +                                 NULL, NULL, NULL, true);

It seems like we need to keep the "redirector_chr_event" here?

And another thing, this series focus on indev, should we need to do
the same thing for outdev?

Thanks
Chen

> +    }
> +}
> +
>  static char *filter_redirector_get_indev(Object *obj, Error **errp)
>  {
>      MirrorState *s = FILTER_REDIRECTOR(obj);
> @@ -440,6 +467,7 @@ static void filter_redirector_class_init(ObjectClass *oc, const void *data)
>      nfc->setup = filter_redirector_setup;
>      nfc->cleanup = filter_redirector_cleanup;
>      nfc->receive_iov = filter_redirector_receive_iov;
> +    nfc->status_changed = filter_redirector_status_changed;
>  }
>
>  static void filter_mirror_init(Object *obj)
> --
> 2.34.1
>

Re: [PATCH 1/3] net/filter-redirector: add support for dynamic status on/off switching

Posted by Jason Wang 1 day, 1 hour ago

On Mon, Jan 5, 2026 at 9:19 PM Zhang Chen <zhangckid@gmail.com> wrote:
>
> On Sun, Jan 4, 2026 at 3:54 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > Currently, filter-redirector does not implement the status_changed
> > callback, which means the 'status' property cannot be used to
> > dynamically enable/disable the filter at runtime. When status is
> > set to 'off' via QMP/HMP, the filter still receives data from the
> > indev chardev because the chardev handlers remain registered.
> >
> > This patch adds proper support for the 'status' property:
> >
> > 1. Implement filter_redirector_status_changed() callback:
> >    - When status changes to 'off': remove chardev read handlers
> >    - When status changes to 'on': re-register chardev handlers
> >      (only if chardev is already open)
> >
> > 2. Update filter_redirector_setup() to respect initial status:
> >    - If filter is created with status=off, do not register handlers
> >    - This allows creating disabled filters via command line or QMP
> >
> > 3. Handle chardev OPENED/CLOSED events to re-arm handlers on reconnect:
> >    - Keep the chr_event callback installed on CLOSE so a later OPENED
> >      can re-register the read handlers when nf->on
> >    - Use qemu_chr_fe_set_handlers_full(..., set_open=false, sync_state=false)
> >      instead of qemu_chr_fe_set_handlers() because the latter forces
> >      sync_state=true and may emit CHR_EVENT_OPENED for an already-open
> >      backend. Doing that from inside the chr_event callback would cause
> >      recursive/re-entrant OPENED handling.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  net/filter-mirror.c | 38 +++++++++++++++++++++++++++++++++-----
> >  1 file changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> > index 8e01e98f40..aa2ab452fd 100644
> > --- a/net/filter-mirror.c
> > +++ b/net/filter-mirror.c
> > @@ -179,9 +179,16 @@ static void redirector_chr_event(void *opaque, QEMUChrEvent event)
> >      MirrorState *s = FILTER_REDIRECTOR(nf);
> >
> >      switch (event) {
> > +    case CHR_EVENT_OPENED:
> > +        if (nf->on) {
> > +            qemu_chr_fe_set_handlers_full(&s->chr_in, redirector_chr_can_read,
> > +                                          redirector_chr_read, redirector_chr_event,
> > +                                          NULL, nf, NULL, false, false);
> > +        }
> > +        break;
> >      case CHR_EVENT_CLOSED:
> > -        qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
> > -                                 NULL, NULL, NULL, true);
> > +        qemu_chr_fe_set_handlers_full(&s->chr_in, NULL, NULL, redirector_chr_event,
> > +                                      NULL, nf, NULL, false, false);
> >          break;
> >      default:
> >          break;
> > @@ -306,9 +313,11 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
> >              return;
> >          }
> >
> > -        qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
> > -                                 redirector_chr_read, redirector_chr_event,
> > -                                 NULL, nf, NULL, true);
> > +        if (nf->on) {
> > +            qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
> > +                                     redirector_chr_read, redirector_chr_event,
> > +                                     NULL, nf, NULL, true);
> > +        }
> >      }
> >
> >      if (s->outdev) {
> > @@ -324,6 +333,24 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
> >      }
> >  }
> >
> > +static void filter_redirector_status_changed(NetFilterState *nf, Error **errp)
> > +{
> > +    MirrorState *s = FILTER_REDIRECTOR(nf);
> > +
> > +    if (!s->indev) {
>
> It's better to add a error here, for example:
>
>         error_setg(errp,  "filter_redirector_status_changed failed for
> the s->indev cleared" );

Will do.

>
>
> > +        return;
> > +    }
> > +
> > +    if (nf->on) {
> > +        qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
> > +                                 redirector_chr_read, redirector_chr_event,
> > +                                 NULL, nf, NULL, true);
> > +    } else {
> > +        qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
> > +                                 NULL, NULL, NULL, true);
>
> It seems like we need to keep the "redirector_chr_event" here?

Since the filter is disabled I think we probably don't care about the
event in that case.

>
> And another thing, this series focus on indev, should we need to do
> the same thing for outdev?

We don't poll outdev, if we get something to send to outdev it's the
bug of other filters or networking core.

Thanks

>
> Thanks
> Chen
>
> > +    }
> > +}
> > +
> >  static char *filter_redirector_get_indev(Object *obj, Error **errp)
> >  {
> >      MirrorState *s = FILTER_REDIRECTOR(obj);
> > @@ -440,6 +467,7 @@ static void filter_redirector_class_init(ObjectClass *oc, const void *data)
> >      nfc->setup = filter_redirector_setup;
> >      nfc->cleanup = filter_redirector_cleanup;
> >      nfc->receive_iov = filter_redirector_receive_iov;
> > +    nfc->status_changed = filter_redirector_status_changed;
> >  }
> >
> >  static void filter_mirror_init(Object *obj)
> > --
> > 2.34.1
> >
>

Re: [PATCH 1/3] net/filter-redirector: add support for dynamic status on/off switching

Posted by Zhang Chen 56 minutes ago

On Tue, Jan 6, 2026 at 4:10 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Jan 5, 2026 at 9:19 PM Zhang Chen <zhangckid@gmail.com> wrote:
> >
> > On Sun, Jan 4, 2026 at 3:54 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > Currently, filter-redirector does not implement the status_changed
> > > callback, which means the 'status' property cannot be used to
> > > dynamically enable/disable the filter at runtime. When status is
> > > set to 'off' via QMP/HMP, the filter still receives data from the
> > > indev chardev because the chardev handlers remain registered.
> > >
> > > This patch adds proper support for the 'status' property:
> > >
> > > 1. Implement filter_redirector_status_changed() callback:
> > >    - When status changes to 'off': remove chardev read handlers
> > >    - When status changes to 'on': re-register chardev handlers
> > >      (only if chardev is already open)
> > >
> > > 2. Update filter_redirector_setup() to respect initial status:
> > >    - If filter is created with status=off, do not register handlers
> > >    - This allows creating disabled filters via command line or QMP
> > >
> > > 3. Handle chardev OPENED/CLOSED events to re-arm handlers on reconnect:
> > >    - Keep the chr_event callback installed on CLOSE so a later OPENED
> > >      can re-register the read handlers when nf->on
> > >    - Use qemu_chr_fe_set_handlers_full(..., set_open=false, sync_state=false)
> > >      instead of qemu_chr_fe_set_handlers() because the latter forces
> > >      sync_state=true and may emit CHR_EVENT_OPENED for an already-open
> > >      backend. Doing that from inside the chr_event callback would cause
> > >      recursive/re-entrant OPENED handling.
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  net/filter-mirror.c | 38 +++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 33 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> > > index 8e01e98f40..aa2ab452fd 100644
> > > --- a/net/filter-mirror.c
> > > +++ b/net/filter-mirror.c
> > > @@ -179,9 +179,16 @@ static void redirector_chr_event(void *opaque, QEMUChrEvent event)
> > >      MirrorState *s = FILTER_REDIRECTOR(nf);
> > >
> > >      switch (event) {
> > > +    case CHR_EVENT_OPENED:
> > > +        if (nf->on) {
> > > +            qemu_chr_fe_set_handlers_full(&s->chr_in, redirector_chr_can_read,
> > > +                                          redirector_chr_read, redirector_chr_event,
> > > +                                          NULL, nf, NULL, false, false);
> > > +        }
> > > +        break;
> > >      case CHR_EVENT_CLOSED:
> > > -        qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
> > > -                                 NULL, NULL, NULL, true);
> > > +        qemu_chr_fe_set_handlers_full(&s->chr_in, NULL, NULL, redirector_chr_event,
> > > +                                      NULL, nf, NULL, false, false);
> > >          break;
> > >      default:
> > >          break;
> > > @@ -306,9 +313,11 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
> > >              return;
> > >          }
> > >
> > > -        qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
> > > -                                 redirector_chr_read, redirector_chr_event,
> > > -                                 NULL, nf, NULL, true);
> > > +        if (nf->on) {
> > > +            qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
> > > +                                     redirector_chr_read, redirector_chr_event,
> > > +                                     NULL, nf, NULL, true);
> > > +        }
> > >      }
> > >
> > >      if (s->outdev) {
> > > @@ -324,6 +333,24 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
> > >      }
> > >  }
> > >
> > > +static void filter_redirector_status_changed(NetFilterState *nf, Error **errp)
> > > +{
> > > +    MirrorState *s = FILTER_REDIRECTOR(nf);
> > > +
> > > +    if (!s->indev) {
> >
> > It's better to add a error here, for example:
> >
> >         error_setg(errp,  "filter_redirector_status_changed failed for
> > the s->indev cleared" );
>
> Will do.
>
> >
> >
> > > +        return;
> > > +    }
> > > +
> > > +    if (nf->on) {
> > > +        qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
> > > +                                 redirector_chr_read, redirector_chr_event,
> > > +                                 NULL, nf, NULL, true);
> > > +    } else {
> > > +        qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
> > > +                                 NULL, NULL, NULL, true);
> >
> > It seems like we need to keep the "redirector_chr_event" here?
>
> Since the filter is disabled I think we probably don't care about the
> event in that case.

OK.

>
> >
> > And another thing, this series focus on indev, should we need to do
> > the same thing for outdev?
>
> We don't poll outdev, if we get something to send to outdev it's the
> bug of other filters or networking core.

OK.

Thanks
Chen

>
> Thanks
>
> >
> > Thanks
> > Chen
> >
> > > +    }
> > > +}
> > > +
> > >  static char *filter_redirector_get_indev(Object *obj, Error **errp)
> > >  {
> > >      MirrorState *s = FILTER_REDIRECTOR(obj);
> > > @@ -440,6 +467,7 @@ static void filter_redirector_class_init(ObjectClass *oc, const void *data)
> > >      nfc->setup = filter_redirector_setup;
> > >      nfc->cleanup = filter_redirector_cleanup;
> > >      nfc->receive_iov = filter_redirector_receive_iov;
> > > +    nfc->status_changed = filter_redirector_status_changed;
> > >  }
> > >
> > >  static void filter_mirror_init(Object *obj)
> > > --
> > > 2.34.1
> > >
> >
>

[PATCH 1/3] net/filter-redirector: add support for dynamic status on/off switching
[PATCH 2/3] qtest: add a test to test redirector status change
[PATCH 3/3] tests/qtest: Add test for filter-redirector rx event opened