| Message ID | a6f46215ee85cc76a74d06ce14b628861fb9bab5.1538652143.git.tgolembi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qga: report serial number and disk node | expand |
Hi On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote: > > Handle returned NULLs properly to: > - avoid crashes in serialization. > - properly report errors to the caller > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > qga/commands-win32.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 9c959122d9..49fc747298 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -735,6 +735,12 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp) > } > fs->type = g_strdup(fs_name); > fs->disk = build_guest_disk_info(guid, errp); > + if (fs->disk == NULL) { > + g_free(fs); I'd use qapi_free_GuestFilesystemInfo() > + fs = NULL; > + goto free; > + } > + > free: > g_free(mnt_point); > return fs; > @@ -755,7 +761,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp) > do { > GuestFilesystemInfo *info = build_guest_fsinfo(guid, errp); > if (info == NULL) { > - continue; > + goto out; > } > new = g_malloc(sizeof(*ret)); > new->value = info; > @@ -767,6 +773,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp) > error_setg_win32(errp, GetLastError(), "failed to find next volume"); > } > > +out: > FindVolumeClose(vol_h); > return ret; > } > -- > 2.19.0 >
Quoting Tomáš Golembiovský (2018-10-04 06:22:29) > Handle returned NULLs properly to: > - avoid crashes in serialization. > - properly report errors to the caller > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > qga/commands-win32.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 9c959122d9..49fc747298 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -735,6 +735,12 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp) > } > fs->type = g_strdup(fs_name); > fs->disk = build_guest_disk_info(guid, errp); > + if (fs->disk == NULL) { > + g_free(fs); > + fs = NULL; > + goto free; > + } > + The QAPI schema defines fs->disk to be a list. In the current upstream code (where CONFIG_NTDDSCSI is unset) we always set fs->disk to NULL and that just results in an empty list, which works and doesn't violate the schema, so I don't understand why that's needed here. > free: > g_free(mnt_point); > return fs; > @@ -755,7 +761,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp) > do { > GuestFilesystemInfo *info = build_guest_fsinfo(guid, errp); > if (info == NULL) { > - continue; > + goto out; > } This fails the whole guest_get_fsinfo command for any case where we can't retrieve the 'disk' list for a particular volume. I would consider that a regression in functionality. Can you confirm this is for fixing the current code? Or is it just something you need for something later in this series? If the latter, I suspect this is the wrong place to address it. > new = g_malloc(sizeof(*ret)); > new->value = info; > @@ -767,6 +773,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp) > error_setg_win32(errp, GetLastError(), "failed to find next volume"); > } > > +out: > FindVolumeClose(vol_h); > return ret; > } > -- > 2.19.0 >
diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 9c959122d9..49fc747298 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -735,6 +735,12 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp) } fs->type = g_strdup(fs_name); fs->disk = build_guest_disk_info(guid, errp); + if (fs->disk == NULL) { + g_free(fs); + fs = NULL; + goto free; + } + free: g_free(mnt_point); return fs; @@ -755,7 +761,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp) do { GuestFilesystemInfo *info = build_guest_fsinfo(guid, errp); if (info == NULL) { - continue; + goto out; } new = g_malloc(sizeof(*ret)); new->value = info; @@ -767,6 +773,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp) error_setg_win32(errp, GetLastError(), "failed to find next volume"); } +out: FindVolumeClose(vol_h); return ret; }
Handle returned NULLs properly to: - avoid crashes in serialization. - properly report errors to the caller Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- qga/commands-win32.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)