| Message ID | 1409645070-8720-3-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 2, 2014 at 6:04 PM, <arei.gonglei@huawei.com> wrote: > From: Gonglei <arei.gonglei@huawei.com> > > Forcefully unrealize all children regardless of early-iteration > errors(if happened). We should keep going with cleanup operation > rather than report a error immediately. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > hw/core/qdev.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 4a1ac5b..d3ff526 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -873,15 +873,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > } else if (!value && dev->realized) { > QLIST_FOREACH(bus, &dev->child_bus, sibling) { > object_property_set_bool(OBJECT(bus), false, "realized", > - &local_err); > - if (local_err != NULL) { > - break; > - } > + NULL); Local err should still be set if at least one child fails to unrealize. Otherwise the calls gets a silent failure. Couple of ways of doing this. 1. Propagate only the first failure. 2. Raise a generic error to indicate "at least one child failed to unrealize". 3. Concatenate all the child unrealize errors together. I like option 1 out of simplicity. > } > - if (qdev_get_vmsd(dev) && local_err == NULL) { > + if (qdev_get_vmsd(dev)) { You are also forcefully un-vmsding and unrealizing the actual object too. You can generalise the command message. Regards, Peter > vmstate_unregister(dev, qdev_get_vmsd(dev), dev); > } > - if (dc->unrealize && local_err == NULL) { > + if (dc->unrealize) { > dc->unrealize(dev, &local_err); > } > dev->pending_deleted_event = true; > -- > 1.7.12.4 > > >
> > > > Forcefully unrealize all children regardless of early-iteration > > errors(if happened). We should keep going with cleanup operation > > rather than report a error immediately. > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > --- > > hw/core/qdev.c | 9 +++------ > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index 4a1ac5b..d3ff526 100644 > > --- a/hw/core/qdev.c > > +++ b/hw/core/qdev.c > > @@ -873,15 +873,12 @@ static void device_set_realized(Object *obj, bool > value, Error **errp) > > } else if (!value && dev->realized) { > > QLIST_FOREACH(bus, &dev->child_bus, sibling) { > > object_property_set_bool(OBJECT(bus), false, "realized", > > - &local_err); > > - if (local_err != NULL) { > > - break; > > - } > > + NULL); > > Local err should still be set if at least one child fails to > unrealize. Otherwise the calls gets a silent failure. Yes. > Couple of ways of doing this. > > 1. Propagate only the first failure. > 2. Raise a generic error to indicate "at least one child failed to unrealize". > 3. Concatenate all the child unrealize errors together. > > I like option 1 out of simplicity. > OK. Agreed. > > } > > - if (qdev_get_vmsd(dev) && local_err == NULL) { > > + if (qdev_get_vmsd(dev)) { > > You are also forcefully un-vmsding and unrealizing the actual object > too. You can generalise the command message. > Peter, Do you mean change the commit message to cover this change? Or don't un-vmsding and unrealizing actual object when one child unrealized failed? Thanks. Best regards, -Gonglei > Regards, > Peter > > > vmstate_unregister(dev, qdev_get_vmsd(dev), dev); > > } > > - if (dc->unrealize && local_err == NULL) { > > + if (dc->unrealize) { > > dc->unrealize(dev, &local_err); > > } > > dev->pending_deleted_event = true; > > -- > > 1.7.12.4 > > > > > >
On Tue, Sep 2, 2014 at 6:39 PM, Gonglei (Arei) <arei.gonglei@huawei.com> wrote: >> > >> > Forcefully unrealize all children regardless of early-iteration >> > errors(if happened). We should keep going with cleanup operation >> > rather than report a error immediately. >> > >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> > --- >> > hw/core/qdev.c | 9 +++------ >> > 1 file changed, 3 insertions(+), 6 deletions(-) >> > >> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> > index 4a1ac5b..d3ff526 100644 >> > --- a/hw/core/qdev.c >> > +++ b/hw/core/qdev.c >> > @@ -873,15 +873,12 @@ static void device_set_realized(Object *obj, bool >> value, Error **errp) >> > } else if (!value && dev->realized) { >> > QLIST_FOREACH(bus, &dev->child_bus, sibling) { >> > object_property_set_bool(OBJECT(bus), false, "realized", >> > - &local_err); >> > - if (local_err != NULL) { >> > - break; >> > - } >> > + NULL); >> >> Local err should still be set if at least one child fails to >> unrealize. Otherwise the calls gets a silent failure. > > Yes. > >> Couple of ways of doing this. >> >> 1. Propagate only the first failure. >> 2. Raise a generic error to indicate "at least one child failed to unrealize". >> 3. Concatenate all the child unrealize errors together. >> >> I like option 1 out of simplicity. >> > OK. Agreed. > >> > } >> > - if (qdev_get_vmsd(dev) && local_err == NULL) { >> > + if (qdev_get_vmsd(dev)) { >> >> You are also forcefully un-vmsding and unrealizing the actual object >> too. You can generalise the command message. >> > Peter, Do you mean change the commit message to cover this change? > Or don't un-vmsding and unrealizing actual object when one child > unrealized failed? Thanks. > Just commit msg. Typo by me, s/command/commit. Regards, Peter > Best regards, > -Gonglei > >> Regards, >> Peter >> >> > vmstate_unregister(dev, qdev_get_vmsd(dev), dev); >> > } >> > - if (dc->unrealize && local_err == NULL) { >> > + if (dc->unrealize) { >> > dc->unrealize(dev, &local_err); >> > } >> > dev->pending_deleted_event = true; >> > -- >> > 1.7.12.4 >> > >> > >> >
> From: peter.crosthwaite@petalogix.com > >> > > >> > Forcefully unrealize all children regardless of early-iteration > >> > errors(if happened). We should keep going with cleanup operation > >> > rather than report a error immediately. > >> > > >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > >> > --- > >> > hw/core/qdev.c | 9 +++------ > >> > 1 file changed, 3 insertions(+), 6 deletions(-) > >> > > >> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > >> > index 4a1ac5b..d3ff526 100644 > >> > --- a/hw/core/qdev.c > >> > +++ b/hw/core/qdev.c > >> > @@ -873,15 +873,12 @@ static void device_set_realized(Object *obj, > bool > >> value, Error **errp) > >> > } else if (!value && dev->realized) { > >> > QLIST_FOREACH(bus, &dev->child_bus, sibling) { > >> > object_property_set_bool(OBJECT(bus), false, "realized", > >> > - &local_err); > >> > - if (local_err != NULL) { > >> > - break; > >> > - } > >> > + NULL); > >> > >> Local err should still be set if at least one child fails to > >> unrealize. Otherwise the calls gets a silent failure. > > > > Yes. > > > >> Couple of ways of doing this. > >> > >> 1. Propagate only the first failure. > >> 2. Raise a generic error to indicate "at least one child failed to unrealize". > >> 3. Concatenate all the child unrealize errors together. > >> > >> I like option 1 out of simplicity. > >> > > OK. Agreed. > > > >> > } > >> > - if (qdev_get_vmsd(dev) && local_err == NULL) { > >> > + if (qdev_get_vmsd(dev)) { > >> > >> You are also forcefully un-vmsding and unrealizing the actual object > >> too. You can generalise the command message. > >> > > Peter, Do you mean change the commit message to cover this change? > > Or don't un-vmsding and unrealizing actual object when one child > > unrealized failed? Thanks. > > > > Just commit msg. Typo by me, s/command/commit. > OK. Thanks. Will do. Best regards, -Gonglei
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 4a1ac5b..d3ff526 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -873,15 +873,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } else if (!value && dev->realized) { QLIST_FOREACH(bus, &dev->child_bus, sibling) { object_property_set_bool(OBJECT(bus), false, "realized", - &local_err); - if (local_err != NULL) { - break; - } + NULL); } - if (qdev_get_vmsd(dev) && local_err == NULL) { + if (qdev_get_vmsd(dev)) { vmstate_unregister(dev, qdev_get_vmsd(dev), dev); } - if (dc->unrealize && local_err == NULL) { + if (dc->unrealize) { dc->unrealize(dev, &local_err); } dev->pending_deleted_event = true;