| Message ID | 1372055699-13501-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 24, 2013 at 02:34:59PM +0800, Amos Kong wrote: > netclient 'name' entry in event is useful for management to know > which device is changed. n->netclient_name is not always set. > This patch changes to use nc->name. If we don't assign 'id', > qemu will set a generated name to nc->name. IRC: <mst> akong, what do other events include? name or id? I just checked QMP/qmp-event.txt, they all use 'device name'. (eg: BLOCK_IO_ERROR, DEVICE_DELETED, DEVICE_TRAY_MOVED, BLOCK_JOB_*) If we assign 'id' for -device, device name will be set to id. Otherwise, a generated device name will set to some device. > Signed-off-by: Amos Kong <akong@redhat.com> > --- > hw/net/virtio-net.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index c88403a..e4d9752 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -200,14 +200,9 @@ static void rxfilter_notify(NetClientState *nc) > VirtIONet *n = qemu_get_nic_opaque(nc); > > if (nc->rxfilter_notify_enabled) { > - if (n->netclient_name) { > - event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }", > - n->netclient_name, > - object_get_canonical_path(OBJECT(n->qdev))); > - } else { > - event_data = qobject_from_jsonf("{ 'path': %s }", > - object_get_canonical_path(OBJECT(n->qdev))); > - } > + event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }", > + nc->name, > + object_get_canonical_path(OBJECT(n->qdev))); > monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data); > qobject_decref(event_data); > > -- > 1.8.1.4
Amos Kong <akong@redhat.com> writes: > netclient 'name' entry in event is useful for management to know > which device is changed. n->netclient_name is not always set. > This patch changes to use nc->name. If we don't assign 'id', > qemu will set a generated name to nc->name. > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > hw/net/virtio-net.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index c88403a..e4d9752 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -200,14 +200,9 @@ static void rxfilter_notify(NetClientState *nc) > VirtIONet *n = qemu_get_nic_opaque(nc); > > if (nc->rxfilter_notify_enabled) { > - if (n->netclient_name) { > - event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }", > - n->netclient_name, > - object_get_canonical_path(OBJECT(n->qdev))); > - } else { > - event_data = qobject_from_jsonf("{ 'path': %s }", > - object_get_canonical_path(OBJECT(n->qdev))); > - } > + event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }", > + nc->name, > + object_get_canonical_path(OBJECT(n->qdev))); > monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data); > qobject_decref(event_data); Is this on top of "[PATCH v3 0/2] mac programming over macvtap"? Yes, qdev IDs are optional, and therefore can serve as reliable identifier only when the user / management application always specifies one, and even then, you're still screwed for auto-created devices. Easily avoided for NICs, but yes, the problem is real. However, I don't agree with the solution "use NetCientState name", because that's too ad hoc, and not general. Can we please use QOM paths?
Amos Kong <akong@redhat.com> writes: > On Mon, Jun 24, 2013 at 02:34:59PM +0800, Amos Kong wrote: >> netclient 'name' entry in event is useful for management to know >> which device is changed. n->netclient_name is not always set. >> This patch changes to use nc->name. If we don't assign 'id', >> qemu will set a generated name to nc->name. > > > IRC: <mst> akong, what do other events include? name or id? > > I just checked QMP/qmp-event.txt, they all use 'device name'. > (eg: BLOCK_IO_ERROR, DEVICE_DELETED, DEVICE_TRAY_MOVED, BLOCK_JOB_*) > > If we assign 'id' for -device, device name will be set to id. > Otherwise, a generated device name will set to some device. DEVICE_DELETED uses "device" (the qdev ID) and "path" (the QOM path). For reasons I don't understand, it sets "path" only when the device has no qdev ID. That should be cleaned up.
On Wed, Jun 26, 2013 at 12:07:53PM +0200, Markus Armbruster wrote: > Amos Kong <akong@redhat.com> writes: > > > On Mon, Jun 24, 2013 at 02:34:59PM +0800, Amos Kong wrote: > >> netclient 'name' entry in event is useful for management to know > >> which device is changed. n->netclient_name is not always set. > >> This patch changes to use nc->name. If we don't assign 'id', > >> qemu will set a generated name to nc->name. > > > > > > IRC: <mst> akong, what do other events include? name or id? > > > > I just checked QMP/qmp-event.txt, they all use 'device name'. > > (eg: BLOCK_IO_ERROR, DEVICE_DELETED, DEVICE_TRAY_MOVED, BLOCK_JOB_*) > > > > If we assign 'id' for -device, device name will be set to id. > > Otherwise, a generated device name will set to some device. > > DEVICE_DELETED uses "device" (the qdev ID) and "path" (the QOM path). > > For reasons I don't understand, it sets "path" only when the device has > no qdev ID. That should be cleaned up. The path are alwasy set. example: (have id) "path": "/machine/peripheral-anon/vnet0/virtio-backend" (no id) "path": "/machine/peripheral-anon/device[0]/virtio-backend" It's enough to just use path to distinguish the changed device. So we ignore this patch.
On Wed, Jun 26, 2013 at 12:02:45PM +0200, Markus Armbruster wrote: > Amos Kong <akong@redhat.com> writes: > > > netclient 'name' entry in event is useful for management to know > > which device is changed. n->netclient_name is not always set. > > This patch changes to use nc->name. If we don't assign 'id', > > qemu will set a generated name to nc->name. > > > > Signed-off-by: Amos Kong <akong@redhat.com> > > --- > > hw/net/virtio-net.c | 11 +++-------- > > 1 file changed, 3 insertions(+), 8 deletions(-) > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index c88403a..e4d9752 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -200,14 +200,9 @@ static void rxfilter_notify(NetClientState *nc) > > VirtIONet *n = qemu_get_nic_opaque(nc); > > > > if (nc->rxfilter_notify_enabled) { > > - if (n->netclient_name) { > > - event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }", > > - n->netclient_name, > > - object_get_canonical_path(OBJECT(n->qdev))); > > - } else { > > - event_data = qobject_from_jsonf("{ 'path': %s }", > > - object_get_canonical_path(OBJECT(n->qdev))); > > - } > > + event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }", > > + nc->name, > > + object_get_canonical_path(OBJECT(n->qdev))); > > monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data); > > qobject_decref(event_data); > > Is this on top of "[PATCH v3 0/2] mac programming over macvtap"? It's based on mst's pci tree: https://git.kernel.org/cgit/virt/kvm/mst/qemu.git/log/?h=pci https://git.kernel.org/cgit/virt/kvm/mst/qemu.git/commit/?h=pci&id=1c0fa6b709d02fe4f98d4ce7b55a6cc3c925791c > Yes, qdev IDs are optional, and therefore can serve as reliable > identifier only when the user / management application always specifies > one, and even then, you're still screwed for auto-created devices. > Easily avoided for NICs, but yes, the problem is real. > > However, I don't agree with the solution "use NetCientState name", > because that's too ad hoc, and not general. Can we please use QOM > paths? path is always provided in event, libvirt can use it.
Amos Kong <akong@redhat.com> writes: > On Wed, Jun 26, 2013 at 12:07:53PM +0200, Markus Armbruster wrote: >> Amos Kong <akong@redhat.com> writes: >> >> > On Mon, Jun 24, 2013 at 02:34:59PM +0800, Amos Kong wrote: >> >> netclient 'name' entry in event is useful for management to know >> >> which device is changed. n->netclient_name is not always set. >> >> This patch changes to use nc->name. If we don't assign 'id', >> >> qemu will set a generated name to nc->name. >> > >> > >> > IRC: <mst> akong, what do other events include? name or id? >> > >> > I just checked QMP/qmp-event.txt, they all use 'device name'. >> > (eg: BLOCK_IO_ERROR, DEVICE_DELETED, DEVICE_TRAY_MOVED, BLOCK_JOB_*) >> > >> > If we assign 'id' for -device, device name will be set to id. >> > Otherwise, a generated device name will set to some device. >> >> DEVICE_DELETED uses "device" (the qdev ID) and "path" (the QOM path). >> >> For reasons I don't understand, it sets "path" only when the device has >> no qdev ID. That should be cleaned up. > > The path are alwasy set. You're right; I misread device_unparent(). [...]
On Mon, Jun 24, 2013 at 02:34:59PM +0800, Amos Kong wrote: > netclient 'name' entry in event is useful for management to know > which device is changed. n->netclient_name is not always set. > This patch changes to use nc->name. If we don't assign 'id', > qemu will set a generated name to nc->name. > > Signed-off-by: Amos Kong <akong@redhat.com> Just making sure - you don't think this patch is necessary, correct? > --- > hw/net/virtio-net.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index c88403a..e4d9752 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -200,14 +200,9 @@ static void rxfilter_notify(NetClientState *nc) > VirtIONet *n = qemu_get_nic_opaque(nc); > > if (nc->rxfilter_notify_enabled) { > - if (n->netclient_name) { > - event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }", > - n->netclient_name, > - object_get_canonical_path(OBJECT(n->qdev))); > - } else { > - event_data = qobject_from_jsonf("{ 'path': %s }", > - object_get_canonical_path(OBJECT(n->qdev))); > - } > + event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }", > + nc->name, > + object_get_canonical_path(OBJECT(n->qdev))); > monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data); > qobject_decref(event_data); > > -- > 1.8.1.4
On Thu, Aug 01, 2013 at 11:59:14AM +0300, Michael S. Tsirkin wrote: > On Mon, Jun 24, 2013 at 02:34:59PM +0800, Amos Kong wrote: > > netclient 'name' entry in event is useful for management to know > > which device is changed. n->netclient_name is not always set. > > This patch changes to use nc->name. If we don't assign 'id', > > qemu will set a generated name to nc->name. > > > > Signed-off-by: Amos Kong <akong@redhat.com> > > Just making sure - you don't think this patch is necessary, correct? Correct.
Am 01.07.2013 04:55, schrieb Amos Kong: > On Wed, Jun 26, 2013 at 12:07:53PM +0200, Markus Armbruster wrote: >> Amos Kong <akong@redhat.com> writes: >> >>> On Mon, Jun 24, 2013 at 02:34:59PM +0800, Amos Kong wrote: >>>> netclient 'name' entry in event is useful for management to know >>>> which device is changed. n->netclient_name is not always set. >>>> This patch changes to use nc->name. If we don't assign 'id', >>>> qemu will set a generated name to nc->name. >>> >>> >>> IRC: <mst> akong, what do other events include? name or id? >>> >>> I just checked QMP/qmp-event.txt, they all use 'device name'. >>> (eg: BLOCK_IO_ERROR, DEVICE_DELETED, DEVICE_TRAY_MOVED, BLOCK_JOB_*) >>> >>> If we assign 'id' for -device, device name will be set to id. >>> Otherwise, a generated device name will set to some device. >> >> DEVICE_DELETED uses "device" (the qdev ID) and "path" (the QOM path). >> >> For reasons I don't understand, it sets "path" only when the device has >> no qdev ID. That should be cleaned up. > > The path are alwasy set. > > example: > (have id) > "path": "/machine/peripheral-anon/vnet0/virtio-backend" You hopefully meant "/machine/peripheral/vnet0/virtio-backend"? Otherwise we have a bug somewhere. Andreas > > (no id) > "path": "/machine/peripheral-anon/device[0]/virtio-backend" > > It's enough to just use path to distinguish the changed device. > So we ignore this patch. >
On Thu, Aug 01, 2013 at 03:30:53PM +0200, Andreas Färber wrote: > Am 01.07.2013 04:55, schrieb Amos Kong: > > On Wed, Jun 26, 2013 at 12:07:53PM +0200, Markus Armbruster wrote: > >> Amos Kong <akong@redhat.com> writes: > >> > >>> On Mon, Jun 24, 2013 at 02:34:59PM +0800, Amos Kong wrote: > >>>> netclient 'name' entry in event is useful for management to know > >>>> which device is changed. n->netclient_name is not always set. > >>>> This patch changes to use nc->name. If we don't assign 'id', > >>>> qemu will set a generated name to nc->name. > >>> > >>> > >>> IRC: <mst> akong, what do other events include? name or id? > >>> > >>> I just checked QMP/qmp-event.txt, they all use 'device name'. > >>> (eg: BLOCK_IO_ERROR, DEVICE_DELETED, DEVICE_TRAY_MOVED, BLOCK_JOB_*) > >>> > >>> If we assign 'id' for -device, device name will be set to id. > >>> Otherwise, a generated device name will set to some device. > >> > >> DEVICE_DELETED uses "device" (the qdev ID) and "path" (the QOM path). > >> > >> For reasons I don't understand, it sets "path" only when the device has > >> no qdev ID. That should be cleaned up. > > > > The path are alwasy set. > > > > example: > > (have id) > > "path": "/machine/peripheral-anon/vnet0/virtio-backend" > > You hopefully meant "/machine/peripheral/vnet0/virtio-backend"? > Otherwise we have a bug somewhere. Just my typo in mail. // -device virtio-net-pci,netdev=h1,id=vnet0 -netdev tap,id=h1 { "timestamp": { "seconds": 1375366321, "microseconds": 595727 }, "event": "NIC_RX_FILTER_CHANGED", "data": { "name": "vnet0", "path": "/machine/peripheral/vnet0/virtio-backend" } } > Andreas > > > > > (no id) > > "path": "/machine/peripheral-anon/device[0]/virtio-backend" > > > > It's enough to just use path to distinguish the changed device. > > So we ignore this patch. > > > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index c88403a..e4d9752 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -200,14 +200,9 @@ static void rxfilter_notify(NetClientState *nc) VirtIONet *n = qemu_get_nic_opaque(nc); if (nc->rxfilter_notify_enabled) { - if (n->netclient_name) { - event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }", - n->netclient_name, - object_get_canonical_path(OBJECT(n->qdev))); - } else { - event_data = qobject_from_jsonf("{ 'path': %s }", - object_get_canonical_path(OBJECT(n->qdev))); - } + event_data = qobject_from_jsonf("{ 'name': %s, 'path': %s }", + nc->name, + object_get_canonical_path(OBJECT(n->qdev))); monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data); qobject_decref(event_data);
netclient 'name' entry in event is useful for management to know which device is changed. n->netclient_name is not always set. This patch changes to use nc->name. If we don't assign 'id', qemu will set a generated name to nc->name. Signed-off-by: Amos Kong <akong@redhat.com> --- hw/net/virtio-net.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)