On Tue, Jan 6, 2026 at 4:10 PM Jason Wang <[email protected]> wrote:
>
> On Mon, Jan 5, 2026 at 9:19 PM Zhang Chen <[email protected]> wrote:
> >
> > On Sun, Jan 4, 2026 at 3:54 PM Jason Wang <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> > >
> >
>
