| Message ID | 1369300080-31377-2-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, May 23, 2013 at 05:07:59PM +0800, Amos Kong wrote: > Introduce this new QMP event to notify management after guest changes > rx-filter configuration. > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > QMP/qmp-events.txt | 14 ++++++++++++++ > include/monitor/monitor.h | 1 + > monitor.c | 1 + > 3 files changed, 16 insertions(+) > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > index 92fe5fb..ad6612b 100644 > --- a/QMP/qmp-events.txt > +++ b/QMP/qmp-events.txt > @@ -154,6 +154,20 @@ Data: > "path": "/machine/peripheral/virtio-net-pci-0" }, > "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > > +RX_FILTER_CHANGED > +----------------- > + > +Emitted when rx-filter configuration is changed by the guest. Please stress this is only for the NIC. It does not apply to non-NIC netclients. > + > +Data: > + > +- "name": net client name (json-string) Maybe a device path here as well? > + > +{ "event": "RX_FILTER_CHANGED", > + "data": { "name": "vnet0" }, > + "timestamp": { "seconds": 1368697518, "microseconds": 326866 }} > +} > + > DEVICE_TRAY_MOVED > ----------------- > > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h > index 1a6cfcf..c495a67 100644 > --- a/include/monitor/monitor.h > +++ b/include/monitor/monitor.h > @@ -40,6 +40,7 @@ typedef enum MonitorEvent { > QEVENT_BLOCK_JOB_ERROR, > QEVENT_BLOCK_JOB_READY, > QEVENT_DEVICE_DELETED, > + QEVENT_RX_FILTER_CHANGED, > QEVENT_DEVICE_TRAY_MOVED, > QEVENT_SUSPEND, > QEVENT_SUSPEND_DISK, > diff --git a/monitor.c b/monitor.c > index 6ce2a4e..4f7bd48 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -489,6 +489,7 @@ static const char *monitor_event_names[] = { > [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR", > [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY", > [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED", > + [QEVENT_RX_FILTER_CHANGED] = "RX_FILTER_CHANGED", > [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED", > [QEVENT_SUSPEND] = "SUSPEND", > [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK", > -- > 1.8.1.4
On 05/23/2013 03:07 AM, Amos Kong wrote: > Introduce this new QMP event to notify management after guest changes > rx-filter configuration. > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > QMP/qmp-events.txt | 14 ++++++++++++++ > include/monitor/monitor.h | 1 + > monitor.c | 1 + > 3 files changed, 16 insertions(+) > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > index 92fe5fb..ad6612b 100644 > --- a/QMP/qmp-events.txt > +++ b/QMP/qmp-events.txt > @@ -154,6 +154,20 @@ Data: > "path": "/machine/peripheral/virtio-net-pci-0" }, > "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > > +RX_FILTER_CHANGED > +----------------- > + > DEVICE_TRAY_MOVED > ----------------- Isn't this file supposed to be kept in sorted order, to minimize merge conflicts when backporting events?
On Thu, 23 May 2013 13:24:59 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, May 23, 2013 at 05:07:59PM +0800, Amos Kong wrote: > > Introduce this new QMP event to notify management after guest changes > > rx-filter configuration. > > > > Signed-off-by: Amos Kong <akong@redhat.com> > > --- > > QMP/qmp-events.txt | 14 ++++++++++++++ > > include/monitor/monitor.h | 1 + > > monitor.c | 1 + > > 3 files changed, 16 insertions(+) > > > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > > index 92fe5fb..ad6612b 100644 > > --- a/QMP/qmp-events.txt > > +++ b/QMP/qmp-events.txt > > @@ -154,6 +154,20 @@ Data: > > "path": "/machine/peripheral/virtio-net-pci-0" }, > > "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > > > > +RX_FILTER_CHANGED > > +----------------- > > + > > +Emitted when rx-filter configuration is changed by the guest. > > Please stress this is only for the NIC. It does not apply > to non-NIC netclients. Stress it in the event name too, please. I find RX_FILTER_CHANGED a bit generic. Also, although I haven't reviewed the next patch yet, I think you should move the event trigger to this patch. > > > + > > +Data: > > + > > +- "name": net client name (json-string) > > Maybe a device path here as well? > > > + > > +{ "event": "RX_FILTER_CHANGED", > > + "data": { "name": "vnet0" }, > > + "timestamp": { "seconds": 1368697518, "microseconds": 326866 }} > > +} > > + > > DEVICE_TRAY_MOVED > > ----------------- > > > > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h > > index 1a6cfcf..c495a67 100644 > > --- a/include/monitor/monitor.h > > +++ b/include/monitor/monitor.h > > @@ -40,6 +40,7 @@ typedef enum MonitorEvent { > > QEVENT_BLOCK_JOB_ERROR, > > QEVENT_BLOCK_JOB_READY, > > QEVENT_DEVICE_DELETED, > > + QEVENT_RX_FILTER_CHANGED, > > QEVENT_DEVICE_TRAY_MOVED, > > QEVENT_SUSPEND, > > QEVENT_SUSPEND_DISK, > > diff --git a/monitor.c b/monitor.c > > index 6ce2a4e..4f7bd48 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -489,6 +489,7 @@ static const char *monitor_event_names[] = { > > [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR", > > [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY", > > [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED", > > + [QEVENT_RX_FILTER_CHANGED] = "RX_FILTER_CHANGED", > > [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED", > > [QEVENT_SUSPEND] = "SUSPEND", > > [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK", > > -- > > 1.8.1.4 >
On Thu, May 23, 2013 at 10:28:42AM -0400, Luiz Capitulino wrote: > On Thu, 23 May 2013 13:24:59 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, May 23, 2013 at 05:07:59PM +0800, Amos Kong wrote: > > > Introduce this new QMP event to notify management after guest changes > > > rx-filter configuration. > > > > > > Signed-off-by: Amos Kong <akong@redhat.com> > > > --- > > > QMP/qmp-events.txt | 14 ++++++++++++++ > > > include/monitor/monitor.h | 1 + > > > monitor.c | 1 + > > > 3 files changed, 16 insertions(+) > > > > > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > > > index 92fe5fb..ad6612b 100644 > > > --- a/QMP/qmp-events.txt > > > +++ b/QMP/qmp-events.txt > > > @@ -154,6 +154,20 @@ Data: > > > "path": "/machine/peripheral/virtio-net-pci-0" }, > > > "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > > > > > > +RX_FILTER_CHANGED > > > +----------------- > > > + > > > +Emitted when rx-filter configuration is changed by the guest. > > > > Please stress this is only for the NIC. It does not apply > > to non-NIC netclients. > > Stress it in the event name too, please. I find RX_FILTER_CHANGED a bit > generic. What do you suggest? NIC_RX_FILTER_CHANGED ? > Also, although I haven't reviewed the next patch yet, I think you > should move the event trigger to this patch. That's hard because of the flag that's shared with the query command. > > > > > + > > > +Data: > > > + > > > +- "name": net client name (json-string) > > > > Maybe a device path here as well? > > > > > + > > > +{ "event": "RX_FILTER_CHANGED", > > > + "data": { "name": "vnet0" }, > > > + "timestamp": { "seconds": 1368697518, "microseconds": 326866 }} > > > +} > > > + > > > DEVICE_TRAY_MOVED > > > ----------------- > > > > > > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h > > > index 1a6cfcf..c495a67 100644 > > > --- a/include/monitor/monitor.h > > > +++ b/include/monitor/monitor.h > > > @@ -40,6 +40,7 @@ typedef enum MonitorEvent { > > > QEVENT_BLOCK_JOB_ERROR, > > > QEVENT_BLOCK_JOB_READY, > > > QEVENT_DEVICE_DELETED, > > > + QEVENT_RX_FILTER_CHANGED, > > > QEVENT_DEVICE_TRAY_MOVED, > > > QEVENT_SUSPEND, > > > QEVENT_SUSPEND_DISK, > > > diff --git a/monitor.c b/monitor.c > > > index 6ce2a4e..4f7bd48 100644 > > > --- a/monitor.c > > > +++ b/monitor.c > > > @@ -489,6 +489,7 @@ static const char *monitor_event_names[] = { > > > [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR", > > > [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY", > > > [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED", > > > + [QEVENT_RX_FILTER_CHANGED] = "RX_FILTER_CHANGED", > > > [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED", > > > [QEVENT_SUSPEND] = "SUSPEND", > > > [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK", > > > -- > > > 1.8.1.4 > >
On 05/23/2013 08:43 AM, Michael S. Tsirkin wrote: >>> Please stress this is only for the NIC. It does not apply >>> to non-NIC netclients. >> >> Stress it in the event name too, please. I find RX_FILTER_CHANGED a bit >> generic. > > What do you suggest? > NIC_RX_FILTER_CHANGED ? Yes, that might work. (And whatever name we bikeshed, insert it in the correct sorted order in the file) > >> Also, although I haven't reviewed the next patch yet, I think you >> should move the event trigger to this patch. > > That's hard because of the flag that's shared with the query > command. It's fine if this patch declares and sets the flag, but the event is one-shot until the next patch adds the query to clear the flag. And again, the flag should be per-device, not global.
On Thu, May 23, 2013 at 08:52:16AM -0600, Eric Blake wrote: > On 05/23/2013 08:43 AM, Michael S. Tsirkin wrote: > >>> Please stress this is only for the NIC. It does not apply > >>> to non-NIC netclients. > >> > >> Stress it in the event name too, please. I find RX_FILTER_CHANGED a bit > >> generic. > > > > What do you suggest? > > NIC_RX_FILTER_CHANGED ? > > Yes, that might work. (And whatever name we bikeshed, insert it in the > correct sorted order in the file) > > > > >> Also, although I haven't reviewed the next patch yet, I think you > >> should move the event trigger to this patch. > > > > That's hard because of the flag that's shared with the query > > command. > > It's fine if this patch declares and sets the flag, but the event is > one-shot until the next patch adds the query to clear the flag. And > again, the flag should be per-device, not global. Maybe just merge both patches together. They are pretty small anyway. > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On Thu, 23 May 2013 17:57:56 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, May 23, 2013 at 08:52:16AM -0600, Eric Blake wrote: > > On 05/23/2013 08:43 AM, Michael S. Tsirkin wrote: > > >>> Please stress this is only for the NIC. It does not apply > > >>> to non-NIC netclients. > > >> > > >> Stress it in the event name too, please. I find RX_FILTER_CHANGED a bit > > >> generic. > > > > > > What do you suggest? > > > NIC_RX_FILTER_CHANGED ? > > > > Yes, that might work. (And whatever name we bikeshed, insert it in the > > correct sorted order in the file) > > > > > > > >> Also, although I haven't reviewed the next patch yet, I think you > > >> should move the event trigger to this patch. > > > > > > That's hard because of the flag that's shared with the query > > > command. > > > > It's fine if this patch declares and sets the flag, but the event is > > one-shot until the next patch adds the query to clear the flag. And > > again, the flag should be per-device, not global. > > Maybe just merge both patches together. They are pretty small anyway. That's what I was going to suggest if it's really hard to move the event to this patch.
On Thu, May 23, 2013 at 11:33:37AM -0400, Luiz Capitulino wrote: > On Thu, 23 May 2013 17:57:56 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, May 23, 2013 at 08:52:16AM -0600, Eric Blake wrote: > > > On 05/23/2013 08:43 AM, Michael S. Tsirkin wrote: > > > >>> Please stress this is only for the NIC. It does not apply > > > >>> to non-NIC netclients. > > > >> > > > >> Stress it in the event name too, please. I find RX_FILTER_CHANGED a bit > > > >> generic. > > > > > > > > What do you suggest? > > > > NIC_RX_FILTER_CHANGED ? > > > > > > Yes, that might work. (And whatever name we bikeshed, insert it in the > > > correct sorted order in the file) Reasonable. > > > >> Also, although I haven't reviewed the next patch yet, I think you > > > >> should move the event trigger to this patch. > > > > > > > > That's hard because of the flag that's shared with the query > > > > command. > > > > > > It's fine if this patch declares and sets the flag, but the event is > > > one-shot until the next patch adds the query to clear the flag. And > > > again, the flag should be per-device, not global. It works. > > Maybe just merge both patches together. They are pretty small anyway. > > That's what I was going to suggest if it's really hard to move the > event to this patch. Ok, I will merge those two patches together, and give a detail/whole description in one patch.
Eric Blake <eblake@redhat.com> writes: > On 05/23/2013 03:07 AM, Amos Kong wrote: >> Introduce this new QMP event to notify management after guest changes >> rx-filter configuration. >> >> Signed-off-by: Amos Kong <akong@redhat.com> >> --- >> QMP/qmp-events.txt | 14 ++++++++++++++ >> include/monitor/monitor.h | 1 + >> monitor.c | 1 + >> 3 files changed, 16 insertions(+) >> >> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt >> index 92fe5fb..ad6612b 100644 >> --- a/QMP/qmp-events.txt >> +++ b/QMP/qmp-events.txt >> @@ -154,6 +154,20 @@ Data: >> "path": "/machine/peripheral/virtio-net-pci-0" }, >> "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } >> >> +RX_FILTER_CHANGED >> +----------------- > >> + >> DEVICE_TRAY_MOVED >> ----------------- > > Isn't this file supposed to be kept in sorted order, to minimize merge > conflicts when backporting events? Yes, please. Also helps humans navigate. GUEST_PANICKED is out of order, needs fixing.
On Wed, 26 Jun 2013 08:54:46 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Eric Blake <eblake@redhat.com> writes: > > > On 05/23/2013 03:07 AM, Amos Kong wrote: > >> Introduce this new QMP event to notify management after guest changes > >> rx-filter configuration. > >> > >> Signed-off-by: Amos Kong <akong@redhat.com> > >> --- > >> QMP/qmp-events.txt | 14 ++++++++++++++ > >> include/monitor/monitor.h | 1 + > >> monitor.c | 1 + > >> 3 files changed, 16 insertions(+) > >> > >> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > >> index 92fe5fb..ad6612b 100644 > >> --- a/QMP/qmp-events.txt > >> +++ b/QMP/qmp-events.txt > >> @@ -154,6 +154,20 @@ Data: > >> "path": "/machine/peripheral/virtio-net-pci-0" }, > >> "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > >> > >> +RX_FILTER_CHANGED > >> +----------------- > > > >> + > >> DEVICE_TRAY_MOVED > >> ----------------- > > > > Isn't this file supposed to be kept in sorted order, to minimize merge > > conflicts when backporting events? > > Yes, please. Also helps humans navigate. > > GUEST_PANICKED is out of order, needs fixing. The ultimate fix would be to add event support to the qapi.
diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index 92fe5fb..ad6612b 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -154,6 +154,20 @@ Data: "path": "/machine/peripheral/virtio-net-pci-0" }, "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } +RX_FILTER_CHANGED +----------------- + +Emitted when rx-filter configuration is changed by the guest. + +Data: + +- "name": net client name (json-string) + +{ "event": "RX_FILTER_CHANGED", + "data": { "name": "vnet0" }, + "timestamp": { "seconds": 1368697518, "microseconds": 326866 }} +} + DEVICE_TRAY_MOVED ----------------- diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 1a6cfcf..c495a67 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -40,6 +40,7 @@ typedef enum MonitorEvent { QEVENT_BLOCK_JOB_ERROR, QEVENT_BLOCK_JOB_READY, QEVENT_DEVICE_DELETED, + QEVENT_RX_FILTER_CHANGED, QEVENT_DEVICE_TRAY_MOVED, QEVENT_SUSPEND, QEVENT_SUSPEND_DISK, diff --git a/monitor.c b/monitor.c index 6ce2a4e..4f7bd48 100644 --- a/monitor.c +++ b/monitor.c @@ -489,6 +489,7 @@ static const char *monitor_event_names[] = { [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR", [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY", [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED", + [QEVENT_RX_FILTER_CHANGED] = "RX_FILTER_CHANGED", [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED", [QEVENT_SUSPEND] = "SUSPEND", [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK",
Introduce this new QMP event to notify management after guest changes rx-filter configuration. Signed-off-by: Amos Kong <akong@redhat.com> --- QMP/qmp-events.txt | 14 ++++++++++++++ include/monitor/monitor.h | 1 + monitor.c | 1 + 3 files changed, 16 insertions(+)