| Message ID | 1410232205-39473-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
Jason Wang <jasowang@redhat.com> 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 <armbru@redhat.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > 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? > > -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. > 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. > 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. > - 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); }
On 10/10/2014 09:03 PM, Markus Armbruster wrote: > Jason Wang <jasowang@redhat.com> 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 <armbru@redhat.com> >> Cc: Stefan Hajnoczi <stefanha@redhat.com> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> 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
On 10/13/2014 11:31 AM, Jason Wang wrote: > On 10/10/2014 09:03 PM, Markus Armbruster wrote: >> Jason Wang <jasowang@redhat.com> 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 <armbru@redhat.com> >>> Cc: Stefan Hajnoczi <stefanha@redhat.com> >>> Cc: Peter Maydell <peter.maydell@linaro.org> >>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>> --- >>> 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 = { >>> }; >>> [...] >>> 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. Just have a check, error_set_from_qdev_prop_error() can accept -EINVAL, so I will only call error_setg() directly for -E2BIG.
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 : ""); -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> Cc: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Jason Wang <jasowang@redhat.com> --- 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(-)