| Message ID | 0ad2a56e767a04f8fd2c8cb32ed6c1bc0f1c2497.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: > > The guest-get-fsinfo command collects also information about PCI > controller where the disk is attached. When this fails for some reasons > it tries to return just the partial information. However in certain > cases the pointer to the structure was not initialized and was set to > NULL. This breaks the serializer and leads to a crash of the guest agent. > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > qga/commands-win32.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 98d9735389..9c959122d9 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -633,15 +633,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) > * https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */ > if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad, > sizeof(SCSI_ADDRESS), &len, NULL)) { > + Error *local_err = NULL; > disk->unit = addr.Lun; > disk->target = addr.TargetId; > disk->bus = addr.PathId; > - disk->pci_controller = get_pci_info(name, errp); > + g_debug("unit=%lld target=%lld bus=%lld", > + disk->unit, disk->target, disk->bus); > + disk->pci_controller = get_pci_info(name, &local_err); > + > + if (local_err) { > + g_debug("failed to get PCI controller info: %s", > + error_get_pretty(local_err)); > + error_free(local_err); > + } else if (disk->pci_controller != NULL) { > + g_debug("pci: domain=%lld bus=%lld slot=%lld function=%lld", > + disk->pci_controller->domain, > + disk->pci_controller->bus, > + disk->pci_controller->slot, > + disk->pci_controller->function); > + } > } > - /* We do not set error in this case, because we still have enough > - * information about volume. */ > - } else { > - disk->pci_controller = NULL; > + } > + /* We do not set error in case pci_controller is NULL, because we still > + * have enough information about volume. */ > + if (disk->pci_controller == NULL) { > + g_debug("no PCI controller info"); > + disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress)); > } Shouldn't pci-controller be made optional in the schema instead? > > list = g_malloc0(sizeof(*list)); > -- > 2.19.0 >
On Thu, 4 Oct 2018 17:21:32 +0400 Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > Hi > On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote: > > > > The guest-get-fsinfo command collects also information about PCI > > controller where the disk is attached. When this fails for some reasons > > it tries to return just the partial information. However in certain > > cases the pointer to the structure was not initialized and was set to > > NULL. This breaks the serializer and leads to a crash of the guest agent. > > > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > > --- > > qga/commands-win32.c | 27 ++++++++++++++++++++++----- > > 1 file changed, 22 insertions(+), 5 deletions(-) > > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > > index 98d9735389..9c959122d9 100644 > > --- a/qga/commands-win32.c > > +++ b/qga/commands-win32.c > > @@ -633,15 +633,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) > > * https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */ > > if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad, > > sizeof(SCSI_ADDRESS), &len, NULL)) { > > + Error *local_err = NULL; > > disk->unit = addr.Lun; > > disk->target = addr.TargetId; > > disk->bus = addr.PathId; > > - disk->pci_controller = get_pci_info(name, errp); > > + g_debug("unit=%lld target=%lld bus=%lld", > > + disk->unit, disk->target, disk->bus); > > + disk->pci_controller = get_pci_info(name, &local_err); > > + > > + if (local_err) { > > + g_debug("failed to get PCI controller info: %s", > > + error_get_pretty(local_err)); > > + error_free(local_err); > > + } else if (disk->pci_controller != NULL) { > > + g_debug("pci: domain=%lld bus=%lld slot=%lld function=%lld", > > + disk->pci_controller->domain, > > + disk->pci_controller->bus, > > + disk->pci_controller->slot, > > + disk->pci_controller->function); > > + } > > } > > - /* We do not set error in this case, because we still have enough > > - * information about volume. */ > > - } else { > > - disk->pci_controller = NULL; > > + } > > + /* We do not set error in case pci_controller is NULL, because we still > > + * have enough information about volume. */ > > + if (disk->pci_controller == NULL) { > > + g_debug("no PCI controller info"); > > + disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress)); > > } > > Shouldn't pci-controller be made optional in the schema instead? It should, but that requires API change. Eric suggested that previously too: https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg08599.html I will do that in next version. Tomas > > > > > list = g_malloc0(sizeof(*list)); > > -- > > 2.19.0 > >
Quoting Tomáš Golembiovský (2018-10-04 06:22:28) > The guest-get-fsinfo command collects also information about PCI > controller where the disk is attached. When this fails for some reasons > it tries to return just the partial information. However in certain > cases the pointer to the structure was not initialized and was set to > NULL. This breaks the serializer and leads to a crash of the guest agent. > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > qga/commands-win32.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 98d9735389..9c959122d9 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -633,15 +633,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) > * https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */ > if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad, > sizeof(SCSI_ADDRESS), &len, NULL)) { > + Error *local_err = NULL; > disk->unit = addr.Lun; > disk->target = addr.TargetId; > disk->bus = addr.PathId; > - disk->pci_controller = get_pci_info(name, errp); > + g_debug("unit=%lld target=%lld bus=%lld", > + disk->unit, disk->target, disk->bus); > + disk->pci_controller = get_pci_info(name, &local_err); > + > + if (local_err) { > + g_debug("failed to get PCI controller info: %s", > + error_get_pretty(local_err)); > + error_free(local_err); > + } else if (disk->pci_controller != NULL) { > + g_debug("pci: domain=%lld bus=%lld slot=%lld function=%lld", > + disk->pci_controller->domain, > + disk->pci_controller->bus, > + disk->pci_controller->slot, > + disk->pci_controller->function); > + } > } > - /* We do not set error in this case, because we still have enough > - * information about volume. */ > - } else { > - disk->pci_controller = NULL; > + } > + /* We do not set error in case pci_controller is NULL, because we still > + * have enough information about volume. */ > + if (disk->pci_controller == NULL) { > + g_debug("no PCI controller info"); > + disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress)); Initializing to 0 would be wrong. I pointed out a patch from Sameeh in v3 that initializes to -1. I'd recommend either picking up his patch, or perhaps the schema change. But if we do go to the extent of a non-backward-compatible schema change, I think we should also consider just deprecating the current GuestDiskAddress list completely: { 'struct': 'GuestDiskAddress', 'data': {'pci-controller': 'GuestPCIAddress', 'bus-type': 'GuestDiskBusType', 'bus': 'int', 'target': 'int', 'unit': 'int'} } and defining something more modular. Some these there don't make a lot of sense, like how GuestDiskBusType varies between scsi, ide, usb, etc, but we still have the same bus/target/unit fields. I think each bus type should have it's own addressing units associated with it. The original code made use of the fact that IDE/SATA/SCSI/SAS/etc could all be retrieved via IOCTL_SCSI_GET_ADDRESS with those units but making sense of them is sort of Windows magic that isn't good from an API perspective and then there's all the other bus types where those units may or may not be sensible. And on POSIX you basically have to look at the code to figure out where each unit is/isn't being plucked from... So for now I'd recommend just hard-setting the PCI fields to -1 like in Sameeh's patch, and I'll do some testing and send a follow-up patch to do the same for bus-type if that seems needed. We can explore better options after 3.1. > } > > list = g_malloc0(sizeof(*list)); > -- > 2.19.0 >
On 10/4/18 6:22 AM, Tomáš Golembiovský wrote: In the subject: s/retrived/retrieved/ > The guest-get-fsinfo command collects also information about PCI > controller where the disk is attached. When this fails for some reasons > it tries to return just the partial information. However in certain > cases the pointer to the structure was not initialized and was set to > NULL. This breaks the serializer and leads to a crash of the guest agent. > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > qga/commands-win32.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) >
diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 98d9735389..9c959122d9 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -633,15 +633,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) * https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */ if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad, sizeof(SCSI_ADDRESS), &len, NULL)) { + Error *local_err = NULL; disk->unit = addr.Lun; disk->target = addr.TargetId; disk->bus = addr.PathId; - disk->pci_controller = get_pci_info(name, errp); + g_debug("unit=%lld target=%lld bus=%lld", + disk->unit, disk->target, disk->bus); + disk->pci_controller = get_pci_info(name, &local_err); + + if (local_err) { + g_debug("failed to get PCI controller info: %s", + error_get_pretty(local_err)); + error_free(local_err); + } else if (disk->pci_controller != NULL) { + g_debug("pci: domain=%lld bus=%lld slot=%lld function=%lld", + disk->pci_controller->domain, + disk->pci_controller->bus, + disk->pci_controller->slot, + disk->pci_controller->function); + } } - /* We do not set error in this case, because we still have enough - * information about volume. */ - } else { - disk->pci_controller = NULL; + } + /* We do not set error in case pci_controller is NULL, because we still + * have enough information about volume. */ + if (disk->pci_controller == NULL) { + g_debug("no PCI controller info"); + disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress)); } list = g_malloc0(sizeof(*list));
The guest-get-fsinfo command collects also information about PCI controller where the disk is attached. When this fails for some reasons it tries to return just the partial information. However in certain cases the pointer to the structure was not initialized and was set to NULL. This breaks the serializer and leads to a crash of the guest agent. Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- qga/commands-win32.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)