[Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Amos Kong]

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);
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Amos Kong]

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.

Re: [Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[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"

(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.

-- 
			Amos.

Re: [Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Markus Armbruster]

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().

[...]

Re: [Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Andreas Färber]

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.
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Amos Kong]

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

-- 
			Amos.

Re: [Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Amos Kong]

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.

Re: [Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH for mst/pci] output nc->name in NIC_RX_FILTER_CHANGED event

[Amos Kong]

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.