| Message ID | 1398091304-10677-8-git-send-email-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
* Juan Quintela (quintela@redhat.com) wrote: > If there is an error while loading a field, we should stop reading and > not continue with the rest of fields. And we should also set an error > in qemu_file. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > vmstate.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/vmstate.c b/vmstate.c > index bfa34cc..bcf1cde 100644 > --- a/vmstate.c > +++ b/vmstate.c > @@ -74,7 +74,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > ret = field->info->get(f, addr, size); > > } > + if (ret >= 0) { > + ret = qemu_file_get_error(f); > + } > if (ret < 0) { > + if (!qemu_file_get_error(f)) { > + qemu_file_set_error(f, ret); > + } qemu_file_set_error already checks for an existing error, so you don't need that check. Dve > trace_vmstate_load_field_error(field->name, ret); > return ret; > } > -- > 1.9.0 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> If there is an error while loading a field, we should stop reading and >> not continue with the rest of fields. And we should also set an error >> in qemu_file. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> vmstate.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/vmstate.c b/vmstate.c >> index bfa34cc..bcf1cde 100644 >> --- a/vmstate.c >> +++ b/vmstate.c >> @@ -74,7 +74,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, >> ret = field->info->get(f, addr, size); >> >> } >> + if (ret >= 0) { >> + ret = qemu_file_get_error(f); >> + } >> if (ret < 0) { >> + if (!qemu_file_get_error(f)) { >> + qemu_file_set_error(f, ret); >> + } > > qemu_file_set_error already checks for an existing error, so > you don't need that check. ret could already be less than zero and qemu_file error not being set yet. Problem found during testing. That is the reason that I have to "improve" the previous patch. Later, Juan. > > Dve > >> trace_vmstate_load_field_error(field->name, ret); >> return ret; >> } >> -- >> 1.9.0 >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > * Juan Quintela (quintela@redhat.com) wrote: > >> If there is an error while loading a field, we should stop reading and > >> not continue with the rest of fields. And we should also set an error > >> in qemu_file. > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> vmstate.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/vmstate.c b/vmstate.c > >> index bfa34cc..bcf1cde 100644 > >> --- a/vmstate.c > >> +++ b/vmstate.c > >> @@ -74,7 +74,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > >> ret = field->info->get(f, addr, size); > >> > >> } > >> + if (ret >= 0) { > >> + ret = qemu_file_get_error(f); > >> + } > >> if (ret < 0) { > >> + if (!qemu_file_get_error(f)) { > >> + qemu_file_set_error(f, ret); > >> + } > > > > qemu_file_set_error already checks for an existing error, so > > you don't need that check. > > ret could already be less than zero and qemu_file error not being set > yet. Problem found during testing. That is the reason that I have to > "improve" the previous patch. but you can do: if (ret < 0) { qemu_file_set_error(f, ret); There is no need for the extra qemu_file_get_error, since qemu_file_set_error does that internally. Dave > > Later, Juan. > > > > > > Dve > > > >> trace_vmstate_load_field_error(field->name, ret); > >> return ret; > >> } > >> -- > >> 1.9.0 > >> > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> > * Juan Quintela (quintela@redhat.com) wrote: >> >> If there is an error while loading a field, we should stop reading and >> >> not continue with the rest of fields. And we should also set an error >> >> in qemu_file. >> >> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> >> --- >> >> vmstate.c | 6 ++++++ >> >> 1 file changed, 6 insertions(+) >> >> >> >> diff --git a/vmstate.c b/vmstate.c >> >> index bfa34cc..bcf1cde 100644 >> >> --- a/vmstate.c >> >> +++ b/vmstate.c >> >> @@ -74,7 +74,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, >> >> ret = field->info->get(f, addr, size); >> >> >> >> } >> >> + if (ret >= 0) { >> >> + ret = qemu_file_get_error(f); >> >> + } >> >> if (ret < 0) { >> >> + if (!qemu_file_get_error(f)) { >> >> + qemu_file_set_error(f, ret); >> >> + } >> > >> > qemu_file_set_error already checks for an existing error, so >> > you don't need that check. >> >> ret could already be less than zero and qemu_file error not being set >> yet. Problem found during testing. That is the reason that I have to >> "improve" the previous patch. > > but you can do: > > if (ret < 0) { > qemu_file_set_error(f, ret); > > There is no need for the extra qemu_file_get_error, since > qemu_file_set_error does > that internally. Then we lost the previous error if there is one. cases: ret >=0 && qemu_file_get_error() == 0 success ret >=0 && qeum_file_get_error() != 0 we got error on the 1st branch and now ret < 0 & qemu_file_get_error() < 0 ret < 0 && qemu_file_get_error() < 0 In this case, we don't want to set qemu_file error with ret By convention, 1st user that sets qemu_file_set_error() wins until there is a reset. If we set this unconditionally, we do this case wrong ret < 0 && qemu_file_get_error() == 0 We want ret <0 & set qemu_file error I think that the patch that I posted got the 4 cases right. Your solution gets the 3rd case wrong (for definition of wrong that means current usage). And people wonders why I don't like the qemu_file_get_error() bussiness. To not having to check/propagate errors in some cases, we end having code like if (qemu_file_get_error()) { return error; } ret = qemu_file_foo(); if (ret >= 0) { ret = qemu_file_get_error(f); } if (ret < 0) { if (!qemu_file_get_error(f)) { qemu_file_set_error(f, ret); } This is the proper usage of calling a funtion that works with qemu_file() (much of them just don't return errors at all.) Later, Juan.
Juan Quintela <quintela@redhat.com> wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> * Juan Quintela (quintela@redhat.com) wrote: >>> If there is an error while loading a field, we should stop reading and >>> not continue with the rest of fields. And we should also set an error >>> in qemu_file. >>> >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> --- >>> vmstate.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/vmstate.c b/vmstate.c >>> index bfa34cc..bcf1cde 100644 >>> --- a/vmstate.c >>> +++ b/vmstate.c >>> @@ -74,7 +74,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, >>> ret = field->info->get(f, addr, size); >>> >>> } >>> + if (ret >= 0) { >>> + ret = qemu_file_get_error(f); >>> + } >>> if (ret < 0) { >>> + if (!qemu_file_get_error(f)) { >>> + qemu_file_set_error(f, ret); >>> + } >> >> qemu_file_set_error already checks for an existing error, so >> you don't need that check. > > ret could already be less than zero and qemu_file error not being set > yet. Problem found during testing. That is the reason that I have to > "improve" the previous patch. > > Later, Juan. qemu_file_set_error() already do the check. I stand corrected. if ((ret < 0) || qemu_file_get_error(f) { qemu_file_set_error(f, ret); return ret; } sounds better? Thanks, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > Juan Quintela <quintela@redhat.com> wrote: > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > >> * Juan Quintela (quintela@redhat.com) wrote: > >>> If there is an error while loading a field, we should stop reading and > >>> not continue with the rest of fields. And we should also set an error > >>> in qemu_file. > >>> > >>> Signed-off-by: Juan Quintela <quintela@redhat.com> > >>> --- > >>> vmstate.c | 6 ++++++ > >>> 1 file changed, 6 insertions(+) > >>> > >>> diff --git a/vmstate.c b/vmstate.c > >>> index bfa34cc..bcf1cde 100644 > >>> --- a/vmstate.c > >>> +++ b/vmstate.c > >>> @@ -74,7 +74,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > >>> ret = field->info->get(f, addr, size); > >>> > >>> } > >>> + if (ret >= 0) { > >>> + ret = qemu_file_get_error(f); > >>> + } > >>> if (ret < 0) { > >>> + if (!qemu_file_get_error(f)) { > >>> + qemu_file_set_error(f, ret); > >>> + } > >> > >> qemu_file_set_error already checks for an existing error, so > >> you don't need that check. > > > > ret could already be less than zero and qemu_file error not being set > > yet. Problem found during testing. That is the reason that I have to > > "improve" the previous patch. > > > > Later, Juan. > > qemu_file_set_error() already do the check. > > I stand corrected. > > if ((ret < 0) || qemu_file_get_error(f) { > qemu_file_set_error(f, ret); > return ret; > } > > sounds better? If ret >=0 but qemu_file_get_error has an existing error then that would return the good value from ret - is that your intent? Or do you want: if (ret >= 0) { ret = qemu_file_get_error(f); } if (ret < 0) { qemu_file_set_error(f, ret); trace_vmstate_load_field_error(field->name, ret); return ret; } Dave > > Thanks, Juan. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/vmstate.c b/vmstate.c index bfa34cc..bcf1cde 100644 --- a/vmstate.c +++ b/vmstate.c @@ -74,7 +74,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, ret = field->info->get(f, addr, size); } + if (ret >= 0) { + ret = qemu_file_get_error(f); + } if (ret < 0) { + if (!qemu_file_get_error(f)) { + qemu_file_set_error(f, ret); + } trace_vmstate_load_field_error(field->name, ret); return ret; }
If there is an error while loading a field, we should stop reading and not continue with the rest of fields. And we should also set an error in qemu_file. Signed-off-by: Juan Quintela <quintela@redhat.com> --- vmstate.c | 6 ++++++ 1 file changed, 6 insertions(+)