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 1 month 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 month 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 month 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 Jason Wang 3 weeks, 6 days ago

On Tue, Jan 6, 2026 at 4:09 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.
>

I think it's probably wrong to return an error here as we may only use
outdev but not indev, in this case status_changed doesn't need to do
anything.

Thanks

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

Posted by Zhang Chen 3 weeks, 6 days ago

On Tue, Jan 13, 2026 at 2:43 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Jan 6, 2026 at 4:09 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.
> >
>
> I think it's probably wrong to return an error here as we may only use
> outdev but not indev, in this case status_changed doesn't need to do
> anything.

This confuses me. The filter_redirector_status_changed code focus on
how to read the the indev, it looks not affect the outdev.
If we just return without any error, the user is unsure whether the attempt
was successful.

Thanks
Chen

>
> Thanks
>

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

Posted by Jason Wang 3 weeks, 5 days ago

On Tue, Jan 13, 2026 at 5:22 PM Zhang Chen <zhangckid@gmail.com> wrote:
>
> On Tue, Jan 13, 2026 at 2:43 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Jan 6, 2026 at 4:09 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.
> > >
> >
> > I think it's probably wrong to return an error here as we may only use
> > outdev but not indev, in this case status_changed doesn't need to do
> > anything.
>
> This confuses me. The filter_redirector_status_changed code focus on
> how to read the the indev, it looks not affect the outdev.
> If we just return without any error, the user is unsure whether the attempt
> was successful.
>

Consider the case where there's only outdev, in this case we don't
need to do anything when status is changed:

1) we don't poll outdev
2) net core will bypass the redirector if the status is off.

I meant basically we don't need to do anything in this case and it's
not an error.

Thanks

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

Posted by Zhang Chen 3 weeks, 4 days ago

On Wed, Jan 14, 2026 at 3:35 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Jan 13, 2026 at 5:22 PM Zhang Chen <zhangckid@gmail.com> wrote:
> >
> > On Tue, Jan 13, 2026 at 2:43 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Tue, Jan 6, 2026 at 4:09 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.
> > > >
> > >
> > > I think it's probably wrong to return an error here as we may only use
> > > outdev but not indev, in this case status_changed doesn't need to do
> > > anything.
> >
> > This confuses me. The filter_redirector_status_changed code focus on
> > how to read the the indev, it looks not affect the outdev.
> > If we just return without any error, the user is unsure whether the attempt
> > was successful.
> >
>
> Consider the case where there's only outdev, in this case we don't
> need to do anything when status is changed:
>
> 1) we don't poll outdev
> 2) net core will bypass the redirector if the status is off.
>
> I meant basically we don't need to do anything in this case and it's
> not an error.
>

Okay, I understand now.

Thanks
Chen

> Thanks
>

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

Posted by Zhang Chen 1 month 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