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