From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43175) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XdWM4-0007nD-GB for qemu-devel@nongnu.org; Sun, 12 Oct 2014 23:31:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XdWLz-00005Y-Ie for qemu-devel@nongnu.org; Sun, 12 Oct 2014 23:31:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16735) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XdWLz-00005Q-Ai for qemu-devel@nongnu.org; Sun, 12 Oct 2014 23:31:23 -0400 Message-ID: <543B4784.4030603@redhat.com> Date: Mon, 13 Oct 2014 11:31:16 +0800 From: Jason Wang MIME-Version: 1.0 References: <1410232205-39473-1-git-send-email-jasowang@redhat.com> <87bnpkkrfn.fsf@blackfin.pond.sub.org> In-Reply-To: <87bnpkkrfn.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2] net: don't use set/get_pointer() in set/get_netdev() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Peter Maydell , afaerber@suse.de, qemu-devel@nongnu.org, Stefan Hajnoczi , mst@redhat.com On 10/10/2014 09:03 PM, Markus Armbruster wrote: > Jason Wang writes: > >> 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 >> Cc: Stefan Hajnoczi >> Cc: Peter Maydell >> Signed-off-by: Jason Wang >> --- >> Changes from V1: >> - validate ncs pointer before accessing them, this fixes the qtest failure >> on arm. >> --- >> hw/core/qdev-properties-system.c | 71 ++++++++++++++++++++++------------------ >> 1 file changed, 39 insertions(+), 32 deletions(-) >> >> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c >> index ae0900f..6939ea5 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] ? peers_ptr->ncs[0]->name : ""); > Can ->ncs[0]->name ever be null? Seems not, id is mandatory for netdev. > >> >> -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; > error_set_from_qdev_prop_error() does not accept -E2BIG. > > You could call error_setg(...) directly instead. Ok. >> 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; > error_set_from_qdev_prop_error() does not accept -EINVAL, either. Ok. >> 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: > Label err clashes with local variable err. Harmless, but maybe you'd > like to rename one of them. It was used in "error_set_from_qdev_prop_error(errp, err, dev, prop, str);" >> - 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 = { > Instead of > > g_free(str); > return; > > err: > error_set_from_qdev_prop_error(errp, err, dev, prop, str); > g_free(str); > } > > You could exploit that error_set_from_qdev_prop_error() does nothing > when err is 0: > > out: > g_free(str); > error_set_from_qdev_prop_error(errp, err, dev, prop, str); > g_free(str); > } This looks more clean, will do. Thanks