| Message ID | 1409211685-31767-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 28, 2014 at 03:41:25PM +0800, Jason Wang wrote: > Commit 1ceef9f27359cbe92ef124bf74de6f792e71f6fb (net: multiqueue > support) tries to use set_pointer() and get_pointer() to set and get > NICPeers which is not a pointer defined in DEFINE_PROP_NETDEV. This > trick works but result a unclean and fragile implementation (e.g > print_netdev and parse_netdev). > > This patch solves this issue by not using set/get_pinter() and set and > get netdev directly in set_netdev() and get_netdev(). After this the > parse_netdev() and print_netdev() were no longer used and dropped from > the source. > > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/core/qdev-properties-system.c | 71 ++++++++++++++++++++++------------------ > 1 file changed, 39 insertions(+), 32 deletions(-) Thanks, applied to my net tree: https://github.com/stefanha/qemu/commits/net Stefan
Unfortunately this patch breaks aarch64-softmmu qtests: GTESTER check-qtest-aarch64 Broken pipe GTester: last random seed: R02S6d8ab263ca56f8ae7a4b47bdf93fbc73 Please take a look at what is causing this. Dropped from the net branch. Stefan
On 4 September 2014 17:21, Stefan Hajnoczi <stefanha@gmail.com> wrote: > Unfortunately this patch breaks aarch64-softmmu qtests: > GTESTER check-qtest-aarch64 > Broken pipe > GTester: last random seed: R02S6d8ab263ca56f8ae7a4b47bdf93fbc73 > > Please take a look at what is causing this. Specifically, it breaks on all the ARM boards which have more than one built in ethernet device. Something in your patch is probably assuming there is only one (there's a somewhat suspicious "[0]" array reference, for instance). thanks -- PMM
On 09/05/2014 12:21 AM, Stefan Hajnoczi wrote: > Unfortunately this patch breaks aarch64-softmmu qtests: > GTESTER check-qtest-aarch64 > Broken pipe > GTester: last random seed: R02S6d8ab263ca56f8ae7a4b47bdf93fbc73 > > Please take a look at what is causing this. > > Dropped from the net branch. > > Stefan > Ok, will post V2 later. Thanks
On 09/05/2014 12:40 AM, Peter Maydell wrote: > On 4 September 2014 17:21, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> Unfortunately this patch breaks aarch64-softmmu qtests: >> GTESTER check-qtest-aarch64 >> Broken pipe >> GTester: last random seed: R02S6d8ab263ca56f8ae7a4b47bdf93fbc73 >> >> Please take a look at what is causing this. > Specifically, it breaks on all the ARM boards which have > more than one built in ethernet device. Something in your > patch is probably assuming there is only one (there's > a somewhat suspicious "[0]" array reference, for instance). > > thanks > -- PMM > The '[0]' dereference was introduced after multiqueue is supported. Since there may be multiple pairs of peers for each nic. The failure reason looks like the ncs[0] pointer should be validated before trying to access them. Will post V2. Thanks
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index ae0900f..b3753ce 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -176,41 +176,67 @@ PropertyInfo qdev_prop_chr = { }; /* --- netdev device --- */ +static void get_netdev(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) +{ + DeviceState *dev = DEVICE(obj); + Property *prop = opaque; + NICPeers *peers_ptr = qdev_get_prop_ptr(dev, prop); + char *p = g_strdup(peers_ptr->ncs[0]->name); -static int parse_netdev(DeviceState *dev, const char *str, void **ptr) + visit_type_str(v, &p, name, errp); + g_free(p); +} + +static void set_netdev(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) { - NICPeers *peers_ptr = (NICPeers *)ptr; + DeviceState *dev = DEVICE(obj); + Property *prop = opaque; + NICPeers *peers_ptr = qdev_get_prop_ptr(dev, prop); NetClientState **ncs = peers_ptr->ncs; NetClientState *peers[MAX_QUEUE_NUM]; - int queues, i = 0; - int ret; + Error *local_err = NULL; + int err, queues, i = 0; + char *str; + + if (dev->realized) { + qdev_prop_set_after_realize(dev, name, errp); + return; + } + + visit_type_str(v, &str, name, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } queues = qemu_find_net_clients_except(str, peers, NET_CLIENT_OPTIONS_KIND_NIC, MAX_QUEUE_NUM); if (queues == 0) { - ret = -ENOENT; + err = -ENOENT; goto err; } if (queues > MAX_QUEUE_NUM) { - ret = -E2BIG; + err = -E2BIG; goto err; } for (i = 0; i < queues; i++) { if (peers[i] == NULL) { - ret = -ENOENT; + err = -ENOENT; goto err; } if (peers[i]->peer) { - ret = -EEXIST; + err = -EEXIST; goto err; } if (ncs[i]) { - ret = -EINVAL; + err = -EINVAL; goto err; } @@ -219,31 +245,12 @@ static int parse_netdev(DeviceState *dev, const char *str, void **ptr) } peers_ptr->queues = queues; - - return 0; + g_free(str); + return; err: - return ret; -} - -static char *print_netdev(void *ptr) -{ - NetClientState *netdev = ptr; - const char *val = netdev->name ? netdev->name : ""; - - return g_strdup(val); -} - -static void get_netdev(Object *obj, Visitor *v, void *opaque, - const char *name, Error **errp) -{ - get_pointer(obj, v, opaque, print_netdev, name, errp); -} - -static void set_netdev(Object *obj, Visitor *v, void *opaque, - const char *name, Error **errp) -{ - set_pointer(obj, v, opaque, parse_netdev, name, errp); + error_set_from_qdev_prop_error(errp, err, dev, prop, str); + g_free(str); } PropertyInfo qdev_prop_netdev = {
Commit 1ceef9f27359cbe92ef124bf74de6f792e71f6fb (net: multiqueue support) tries to use set_pointer() and get_pointer() to set and get NICPeers which is not a pointer defined in DEFINE_PROP_NETDEV. This trick works but result a unclean and fragile implementation (e.g print_netdev and parse_netdev). This patch solves this issue by not using set/get_pinter() and set and get netdev directly in set_netdev() and get_netdev(). After this the parse_netdev() and print_netdev() were no longer used and dropped from the source. Cc: Markus Armbruster <armbru@redhat.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/core/qdev-properties-system.c | 71 ++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 32 deletions(-)