* [PATCH 0/1] qga: add command guest-get-disk @ 2020-08-06 9:03 Tomáš Golembiovský 2020-08-06 9:03 ` [PATCH 1/1] qga: add command guest-get-disks Tomáš Golembiovský 0 siblings, 1 reply; 6+ messages in thread From: Tomáš Golembiovský @ 2020-08-06 9:03 UTC (permalink / raw) To: Thomas Huth, Michael Roth, qemu-devel; +Cc: Tomáš Golembiovský Adds command to list root disks of the VM. The patch does compile on master but to work properly it requires changes to qemu-ga by Thomas Huth in series: Allow guest-get-fsinfo also for non-PCI devices. Tomáš Golembiovský (1): qga: add command guest-get-disks qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++- qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++ qga/qapi-schema.json | 13 +++++++ 3 files changed, 186 insertions(+), 1 deletion(-) -- 2.25.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] qga: add command guest-get-disks 2020-08-06 9:03 [PATCH 0/1] qga: add command guest-get-disk Tomáš Golembiovský @ 2020-08-06 9:03 ` Tomáš Golembiovský 2020-08-06 10:44 ` Philippe Mathieu-Daudé 2020-08-12 21:29 ` Michael Roth 0 siblings, 2 replies; 6+ messages in thread From: Tomáš Golembiovský @ 2020-08-06 9:03 UTC (permalink / raw) To: Thomas Huth, Michael Roth, qemu-devel; +Cc: Tomáš Golembiovský The command guest-get-fsinfo can be used to list information about disks and partitions but it is limited only to mounted disks with filesystem. This new command allows listing information about attached root disks of the VM. This is usefull for management applications for mapping virtualized devices or pass-through devices to device names in the guest OS. Output is similar to the list of partitions of guest-get-fsinfo, except that the disks are mapped instead of partitions. Example output: { "return": [ { "serial": "SAMSUNG_123456789", "bus-type": "sata", "bus": 0, "unit": 0, "pci-controller": { "bus": 0, "slot": 31, "domain": 0, "function": 2 }, "dev": "/dev/sda", "target": 0 }, ... ] } Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++- qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++ qga/qapi-schema.json | 13 +++++++ 3 files changed, 186 insertions(+), 1 deletion(-) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 744c2b5a5d..4cebec96a6 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -62,6 +62,8 @@ extern char **environ; #endif #endif +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo, qapi_free_GuestFilesystemInfo) + static void ga_wait_child(pid_t pid, int *status, Error **errp) { pid_t rpid; @@ -1177,6 +1179,92 @@ static void build_guest_fsinfo_for_device(char const *devpath, free(syspath); } +GuestDiskAddressList *qmp_guest_get_disks(Error **errp) +{ + GuestDiskAddressList *item, *ret = NULL; + DIR *dp = NULL; + struct dirent *de = NULL; + + g_debug("listing /sys/block directory"); + dp = opendir("/sys/block"); + if (dp == NULL) { + error_setg_errno(errp, errno, "Can't open directory \"/sys/block\""); + return NULL; + } + while ((de = readdir(dp)) != NULL) { + g_autofree char *disk_dir = NULL, *line = NULL, + *size_path = NULL, *slaves_dir = NULL; + g_autoptr(GuestFilesystemInfo) fs = NULL; + uint64_t slaves = 0; + struct dirent *de_slaves; + DIR *dp_slaves = NULL; + FILE *fp = NULL; + size_t n; + Error *local_err = NULL; + if (de->d_type != DT_LNK) { + g_debug(" skipping entry: %s", de->d_name); + continue; + } + + slaves_dir = g_strdup_printf("/sys/block/%s/slaves", de->d_name); + if (slaves_dir == NULL) { + g_debug(" failed to open directory %s", slaves_dir); + continue; + } + g_debug(" counting entries in: %s", slaves_dir); + dp_slaves = opendir(slaves_dir); + while ((de_slaves = readdir(dp_slaves)) != NULL) { + if ((strcmp(".", de_slaves->d_name) == 0) || + (strcmp("..", de_slaves->d_name) == 0)) { + continue; + } + slaves++; + } + closedir(dp_slaves); + g_debug(" counted %lu items", slaves); + if (slaves != 0) { + continue; + } + + g_debug(" checking disk size"); + size_path = g_strdup_printf("/sys/block/%s/size", de->d_name); + fp = fopen(size_path, "r"); + if (!fp) { + g_debug(" failed to read disk size"); + continue; + } + if (getline(&line, &n, fp) == -1) { + g_debug(" failed to read disk size"); + fclose(fp); + continue; + } + fclose(fp); + if (strcmp(line, "0\n") == 0) { + g_debug(" skipping zero-sized disk"); + continue; + } + + fs = g_malloc0(sizeof(*fs)); + g_debug(" adding %s", de->d_name); + disk_dir = g_strdup_printf("/sys/block/%s", de->d_name); + build_guest_fsinfo_for_device(disk_dir, fs, &local_err); + if (local_err != NULL) { + g_debug(" failed to get device info, ignoring error: %s", + error_get_pretty(local_err)); + error_free(local_err); + continue; + } else if (fs->disk == NULL) { + g_debug(" skipping unknown disk"); + continue; + } + item = g_steal_pointer(&fs->disk); + g_assert(item->next == NULL); /* expecting just a single disk */ + item->next = ret; + ret = item; + } + return ret; +} + /* Return a list of the disk device(s)' info which @mount lies on */ static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount, Error **errp) @@ -2809,7 +2897,8 @@ GList *ga_command_blacklist_init(GList *blacklist) const char *list[] = { "guest-get-fsinfo", "guest-fsfreeze-status", "guest-fsfreeze-freeze", "guest-fsfreeze-freeze-list", - "guest-fsfreeze-thaw", "guest-get-fsinfo", NULL}; + "guest-fsfreeze-thaw", "guest-get-fsinfo", + "guest-get-disks", NULL}; char **p = (char **)list; while (*p) { diff --git a/qga/commands-win32.c b/qga/commands-win32.c index aaa71f147b..0bafa2dc4c 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -945,6 +945,83 @@ out: return list; } +GuestDiskAddressList *qmp_guest_get_disks(Error **errp) +{ + GuestDiskAddressList *new = NULL, *ret = NULL; + HDEVINFO dev_info; + SP_DEVICE_INTERFACE_DATA dev_iface_data; + int i; + + dev_info = SetupDiGetClassDevs(&GUID_DEVINTERFACE_DISK, 0, 0, + DIGCF_PRESENT | DIGCF_DEVICEINTERFACE); + if (dev_info == INVALID_HANDLE_VALUE) { + error_setg_win32(errp, GetLastError(), "failed to get device tree"); + return NULL; + } + + g_debug("enumerating devices"); + dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA); + for (i = 0; + SetupDiEnumDeviceInterfaces(dev_info, NULL, &GUID_DEVINTERFACE_DISK, + i, &dev_iface_data); + i++) { + GuestDiskAddress *disk = NULL; + Error *local_err = NULL; + g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA + pdev_iface_detail_data = NULL; + STORAGE_DEVICE_NUMBER sdn; + HANDLE dev_file; + DWORD size = 0; + + g_debug(" getting device path"); + while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data, + pdev_iface_detail_data, + size, &size, + NULL)) { + if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) { + pdev_iface_detail_data = g_malloc(size); + pdev_iface_detail_data->cbSize = + sizeof(*pdev_iface_detail_data); + } else { + g_debug("failed to get device interface details"); + continue; + } + } + + g_debug(" device: %s", pdev_iface_detail_data->DevicePath); + dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0, + FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); + if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER, + NULL, 0, &sdn, sizeof(sdn), &size, NULL)) { + CloseHandle(dev_file); + debug_error("failed to get storage device number"); + continue; + } + CloseHandle(dev_file); + + g_debug(" number: %lu", sdn.DeviceNumber); + disk = g_malloc0(sizeof(GuestDiskAddress)); + disk->has_dev = true; + disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu", + sdn.DeviceNumber); + get_single_disk_info(sdn.DeviceNumber, disk, &local_err); + if (local_err) { + g_debug("failed to get disk info: %s", + error_get_pretty(local_err)); + error_free(local_err); + g_free(disk); + continue; + } + new = g_malloc0(sizeof(GuestDiskAddressList)); + new->value = disk; + new->next = ret; + ret = new; + } + + SetupDiDestroyDeviceInfoList(dev_info); + return ret; +} + #else static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) @@ -952,6 +1029,12 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) return NULL; } +GuestDiskAddressList *qmp_guest_get_disks(Error **errp) +{ + error_setg(errp, QERR_UNSUPPORTED); + return NULL; +} + #endif /* CONFIG_QGA_NTDDSCSI */ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp) diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index 408a662ea5..5d8fa1c283 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -862,6 +862,19 @@ 'bus': 'int', 'target': 'int', 'unit': 'int', '*serial': 'str', '*dev': 'str'} } +## +# @guest-get-disks: +# +# Returns: The list of disks in the guest. For Windows these are only the +# physical disks. On Linux these are all root block devices of +# non-zero size including e.g. removable devices, loop devices, +# NBD, etc. +# +# Since: 5.2 +## +{ 'command': 'guest-get-disks', + 'returns': ['GuestDiskAddress'] } + ## # @GuestFilesystemInfo: # -- 2.25.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] qga: add command guest-get-disks 2020-08-06 9:03 ` [PATCH 1/1] qga: add command guest-get-disks Tomáš Golembiovský @ 2020-08-06 10:44 ` Philippe Mathieu-Daudé 2020-08-12 21:30 ` Michael Roth 2020-08-12 21:29 ` Michael Roth 1 sibling, 1 reply; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2020-08-06 10:44 UTC (permalink / raw) To: Tomáš Golembiovský, Thomas Huth, Michael Roth, qemu-devel On 8/6/20 11:03 AM, Tomáš Golembiovský wrote: > The command guest-get-fsinfo can be used to list information about disks and > partitions but it is limited only to mounted disks with filesystem. This new > command allows listing information about attached root disks of the VM. This is > usefull for management applications for mapping virtualized devices or > pass-through devices to device names in the guest OS. > > Output is similar to the list of partitions of guest-get-fsinfo, except that > the disks are mapped instead of partitions. > > Example output: > > { > "return": [ > { > "serial": "SAMSUNG_123456789", > "bus-type": "sata", > "bus": 0, > "unit": 0, > "pci-controller": { > "bus": 0, > "slot": 31, > "domain": 0, > "function": 2 > }, > "dev": "/dev/sda", > "target": 0 > }, > ... > ] > } > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++- > qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++ > qga/qapi-schema.json | 13 +++++++ > 3 files changed, 186 insertions(+), 1 deletion(-) Not sure this is better, but splitting this in 3 could help different people reviewing (developers not familiar with the Windows API might feel not comfortable adding a R-b tag for the POSIX part, and reciprocally). 1/ qapi + stubs 2/ POSIX implementation 3/ Win32 implementation ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] qga: add command guest-get-disks 2020-08-06 10:44 ` Philippe Mathieu-Daudé @ 2020-08-12 21:30 ` Michael Roth 0 siblings, 0 replies; 6+ messages in thread From: Michael Roth @ 2020-08-12 21:30 UTC (permalink / raw) To: Tomáš Golembiovský, Philippe Mathieu-Daudé, Thomas Huth, qemu-devel Quoting Philippe Mathieu-Daudé (2020-08-06 05:44:44) > On 8/6/20 11:03 AM, Tomáš Golembiovský wrote: > > The command guest-get-fsinfo can be used to list information about disks and > > partitions but it is limited only to mounted disks with filesystem. This new > > command allows listing information about attached root disks of the VM. This is > > usefull for management applications for mapping virtualized devices or > > pass-through devices to device names in the guest OS. > > > > Output is similar to the list of partitions of guest-get-fsinfo, except that > > the disks are mapped instead of partitions. > > > > Example output: > > > > { > > "return": [ > > { > > "serial": "SAMSUNG_123456789", > > "bus-type": "sata", > > "bus": 0, > > "unit": 0, > > "pci-controller": { > > "bus": 0, > > "slot": 31, > > "domain": 0, > > "function": 2 > > }, > > "dev": "/dev/sda", > > "target": 0 > > }, > > ... > > ] > > } > > > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > > --- > > qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++- > > qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++ > > qga/qapi-schema.json | 13 +++++++ > > 3 files changed, 186 insertions(+), 1 deletion(-) > > Not sure this is better, but splitting this in 3 could help different > people reviewing (developers not familiar with the Windows API might > feel not comfortable adding a R-b tag for the POSIX part, and > reciprocally). > > 1/ qapi + stubs > 2/ POSIX implementation > 3/ Win32 implementation > +1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] qga: add command guest-get-disks 2020-08-06 9:03 ` [PATCH 1/1] qga: add command guest-get-disks Tomáš Golembiovský 2020-08-06 10:44 ` Philippe Mathieu-Daudé @ 2020-08-12 21:29 ` Michael Roth 2020-08-17 7:17 ` Tomáš Golembiovský 1 sibling, 1 reply; 6+ messages in thread From: Michael Roth @ 2020-08-12 21:29 UTC (permalink / raw) To: Tomáš Golembiovský, Thomas Huth, qemu-devel Cc: Tomáš Golembiovský Quoting Tomáš Golembiovský (2020-08-06 04:03:06) > The command guest-get-fsinfo can be used to list information about disks and > partitions but it is limited only to mounted disks with filesystem. This new > command allows listing information about attached root disks of the VM. This is > usefull for management applications for mapping virtualized devices or > pass-through devices to device names in the guest OS. > > Output is similar to the list of partitions of guest-get-fsinfo, except that > the disks are mapped instead of partitions. > > Example output: > > { > "return": [ > { > "serial": "SAMSUNG_123456789", > "bus-type": "sata", > "bus": 0, > "unit": 0, > "pci-controller": { > "bus": 0, > "slot": 31, > "domain": 0, > "function": 2 > }, > "dev": "/dev/sda", > "target": 0 > }, > ... > ] > } > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++- > qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++ > qga/qapi-schema.json | 13 +++++++ > 3 files changed, 186 insertions(+), 1 deletion(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 744c2b5a5d..4cebec96a6 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -62,6 +62,8 @@ extern char **environ; > #endif > #endif > > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo, qapi_free_GuestFilesystemInfo) > + > static void ga_wait_child(pid_t pid, int *status, Error **errp) > { > pid_t rpid; > @@ -1177,6 +1179,92 @@ static void build_guest_fsinfo_for_device(char const *devpath, > free(syspath); > } > > +GuestDiskAddressList *qmp_guest_get_disks(Error **errp) > +{ > + GuestDiskAddressList *item, *ret = NULL; > + DIR *dp = NULL; > + struct dirent *de = NULL; > + > + g_debug("listing /sys/block directory"); > + dp = opendir("/sys/block"); > + if (dp == NULL) { > + error_setg_errno(errp, errno, "Can't open directory \"/sys/block\""); > + return NULL; > + } > + while ((de = readdir(dp)) != NULL) { > + g_autofree char *disk_dir = NULL, *line = NULL, > + *size_path = NULL, *slaves_dir = NULL; > + g_autoptr(GuestFilesystemInfo) fs = NULL; > + uint64_t slaves = 0; > + struct dirent *de_slaves; > + DIR *dp_slaves = NULL; > + FILE *fp = NULL; > + size_t n; > + Error *local_err = NULL; > + if (de->d_type != DT_LNK) { > + g_debug(" skipping entry: %s", de->d_name); > + continue; > + } > + > + slaves_dir = g_strdup_printf("/sys/block/%s/slaves", de->d_name); > + if (slaves_dir == NULL) { > + g_debug(" failed to open directory %s", slaves_dir); > + continue; > + } > + g_debug(" counting entries in: %s", slaves_dir); > + dp_slaves = opendir(slaves_dir); > + while ((de_slaves = readdir(dp_slaves)) != NULL) { > + if ((strcmp(".", de_slaves->d_name) == 0) || > + (strcmp("..", de_slaves->d_name) == 0)) { > + continue; > + } > + slaves++; > + } > + closedir(dp_slaves); > + g_debug(" counted %lu items", slaves); > + if (slaves != 0) { > + continue; > + } For guest-get-fsinfo we skip returning any data about virtual devices (dm/md/etc.) and just return the physical disks underlying whatever hierarchy the filesystem is built on, which sort of makes sense as far as making sure we know what devices are currently in use. But for guest-get-disks we seem to care about usage beyond just mounted filesystems, which to me would suggest things like whether a disk is part of an LVM/RAID/multipath volume. But by skipping virtual devices with "/sys/block/*/slaves" entries we lose that information... So I guess I'm not sure I understand the use-cases you have in mind here. Can you provide some examples? And do you anticipate we would need to add an interface to report this hierarchy at some point? > + > + g_debug(" checking disk size"); > + size_path = g_strdup_printf("/sys/block/%s/size", de->d_name); > + fp = fopen(size_path, "r"); > + if (!fp) { > + g_debug(" failed to read disk size"); > + continue; > + } > + if (getline(&line, &n, fp) == -1) { We need to set n=0, otherwise an uninitialized value might be interpreted as the size of line's pre-allocated buffer > + g_debug(" failed to read disk size"); > + fclose(fp); > + continue; > + } > + fclose(fp); > + if (strcmp(line, "0\n") == 0) { > + g_debug(" skipping zero-sized disk"); > + continue; > + } > + > + fs = g_malloc0(sizeof(*fs)); > + g_debug(" adding %s", de->d_name); > + disk_dir = g_strdup_printf("/sys/block/%s", de->d_name); > + build_guest_fsinfo_for_device(disk_dir, fs, &local_err); > + if (local_err != NULL) { > + g_debug(" failed to get device info, ignoring error: %s", > + error_get_pretty(local_err)); > + error_free(local_err); > + continue; > + } else if (fs->disk == NULL) { > + g_debug(" skipping unknown disk"); > + continue; > + } > + item = g_steal_pointer(&fs->disk); Does this ensure that fs itself doesn't still get auto-free'd? In any case, it seems awkward to allocate a GuestFilesystemInfo just so we can re-use build_guest_fsinfo_for_device(). Can code be refactored into a separate build_guest_disk_info_for_device() or something that just takes a GuestDiskAddressList* directly? > + g_assert(item->next == NULL); /* expecting just a single disk */ > + item->next = ret; > + ret = item; > + } > + return ret; > +} > + > /* Return a list of the disk device(s)' info which @mount lies on */ > static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount, > Error **errp) > @@ -2809,7 +2897,8 @@ GList *ga_command_blacklist_init(GList *blacklist) > const char *list[] = { > "guest-get-fsinfo", "guest-fsfreeze-status", > "guest-fsfreeze-freeze", "guest-fsfreeze-freeze-list", > - "guest-fsfreeze-thaw", "guest-get-fsinfo", NULL}; > + "guest-fsfreeze-thaw", "guest-get-fsinfo", > + "guest-get-disks", NULL}; > char **p = (char **)list; > > while (*p) { > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index aaa71f147b..0bafa2dc4c 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -945,6 +945,83 @@ out: > return list; > } > > +GuestDiskAddressList *qmp_guest_get_disks(Error **errp) > +{ > + GuestDiskAddressList *new = NULL, *ret = NULL; > + HDEVINFO dev_info; > + SP_DEVICE_INTERFACE_DATA dev_iface_data; > + int i; > + > + dev_info = SetupDiGetClassDevs(&GUID_DEVINTERFACE_DISK, 0, 0, > + DIGCF_PRESENT | DIGCF_DEVICEINTERFACE); > + if (dev_info == INVALID_HANDLE_VALUE) { > + error_setg_win32(errp, GetLastError(), "failed to get device tree"); > + return NULL; > + } > + > + g_debug("enumerating devices"); > + dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA); > + for (i = 0; > + SetupDiEnumDeviceInterfaces(dev_info, NULL, &GUID_DEVINTERFACE_DISK, > + i, &dev_iface_data); > + i++) { > + GuestDiskAddress *disk = NULL; > + Error *local_err = NULL; > + g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA > + pdev_iface_detail_data = NULL; > + STORAGE_DEVICE_NUMBER sdn; > + HANDLE dev_file; > + DWORD size = 0; > + > + g_debug(" getting device path"); > + while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data, > + pdev_iface_detail_data, > + size, &size, > + NULL)) { > + if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) { > + pdev_iface_detail_data = g_malloc(size); > + pdev_iface_detail_data->cbSize = > + sizeof(*pdev_iface_detail_data); > + } else { > + g_debug("failed to get device interface details"); > + continue; > + } > + } > + > + g_debug(" device: %s", pdev_iface_detail_data->DevicePath); > + dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0, > + FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); > + if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER, > + NULL, 0, &sdn, sizeof(sdn), &size, NULL)) { > + CloseHandle(dev_file); > + debug_error("failed to get storage device number"); > + continue; > + } > + CloseHandle(dev_file); > + > + g_debug(" number: %lu", sdn.DeviceNumber); > + disk = g_malloc0(sizeof(GuestDiskAddress)); > + disk->has_dev = true; > + disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu", > + sdn.DeviceNumber); > + get_single_disk_info(sdn.DeviceNumber, disk, &local_err); > + if (local_err) { > + g_debug("failed to get disk info: %s", > + error_get_pretty(local_err)); > + error_free(local_err); > + g_free(disk); > + continue; > + } > + new = g_malloc0(sizeof(GuestDiskAddressList)); > + new->value = disk; > + new->next = ret; > + ret = new; > + } > + > + SetupDiDestroyDeviceInfoList(dev_info); > + return ret; > +} > + > #else > > static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) > @@ -952,6 +1029,12 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) > return NULL; > } > > +GuestDiskAddressList *qmp_guest_get_disks(Error **errp) > +{ > + error_setg(errp, QERR_UNSUPPORTED); > + return NULL; > +} > + > #endif /* CONFIG_QGA_NTDDSCSI */ > > static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp) > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index 408a662ea5..5d8fa1c283 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -862,6 +862,19 @@ > 'bus': 'int', 'target': 'int', 'unit': 'int', > '*serial': 'str', '*dev': 'str'} } > > +## > +# @guest-get-disks: > +# > +# Returns: The list of disks in the guest. For Windows these are only the > +# physical disks. On Linux these are all root block devices of > +# non-zero size including e.g. removable devices, loop devices, > +# NBD, etc. > +# > +# Since: 5.2 > +## > +{ 'command': 'guest-get-disks', > + 'returns': ['GuestDiskAddress'] } > + > ## > # @GuestFilesystemInfo: > # > -- > 2.25.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] qga: add command guest-get-disks 2020-08-12 21:29 ` Michael Roth @ 2020-08-17 7:17 ` Tomáš Golembiovský 0 siblings, 0 replies; 6+ messages in thread From: Tomáš Golembiovský @ 2020-08-17 7:17 UTC (permalink / raw) To: Michael Roth; +Cc: Thomas Huth, qemu-devel On Wed, 12 Aug 2020 16:29:38 -0500 Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > Quoting Tomáš Golembiovský (2020-08-06 04:03:06) > > The command guest-get-fsinfo can be used to list information about disks and > > partitions but it is limited only to mounted disks with filesystem. This new > > command allows listing information about attached root disks of the VM. This is > > usefull for management applications for mapping virtualized devices or > > pass-through devices to device names in the guest OS. > > > > Output is similar to the list of partitions of guest-get-fsinfo, except that > > the disks are mapped instead of partitions. > > > > Example output: > > > > { > > "return": [ > > { > > "serial": "SAMSUNG_123456789", > > "bus-type": "sata", > > "bus": 0, > > "unit": 0, > > "pci-controller": { > > "bus": 0, > > "slot": 31, > > "domain": 0, > > "function": 2 > > }, > > "dev": "/dev/sda", > > "target": 0 > > }, > > ... > > ] > > } > > > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > > --- > > qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++- > > qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++ > > qga/qapi-schema.json | 13 +++++++ > > 3 files changed, 186 insertions(+), 1 deletion(-) > > > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > > index 744c2b5a5d..4cebec96a6 100644 > > --- a/qga/commands-posix.c > > +++ b/qga/commands-posix.c > > @@ -62,6 +62,8 @@ extern char **environ; > > #endif > > #endif > > > > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo, qapi_free_GuestFilesystemInfo) > > + > > static void ga_wait_child(pid_t pid, int *status, Error **errp) > > { > > pid_t rpid; > > @@ -1177,6 +1179,92 @@ static void build_guest_fsinfo_for_device(char const *devpath, > > free(syspath); > > } > > > > +GuestDiskAddressList *qmp_guest_get_disks(Error **errp) > > +{ > > + GuestDiskAddressList *item, *ret = NULL; > > + DIR *dp = NULL; > > + struct dirent *de = NULL; > > + > > + g_debug("listing /sys/block directory"); > > + dp = opendir("/sys/block"); > > + if (dp == NULL) { > > + error_setg_errno(errp, errno, "Can't open directory \"/sys/block\""); > > + return NULL; > > + } > > + while ((de = readdir(dp)) != NULL) { > > + g_autofree char *disk_dir = NULL, *line = NULL, > > + *size_path = NULL, *slaves_dir = NULL; > > + g_autoptr(GuestFilesystemInfo) fs = NULL; > > + uint64_t slaves = 0; > > + struct dirent *de_slaves; > > + DIR *dp_slaves = NULL; > > + FILE *fp = NULL; > > + size_t n; > > + Error *local_err = NULL; > > + if (de->d_type != DT_LNK) { > > + g_debug(" skipping entry: %s", de->d_name); > > + continue; > > + } > > + > > + slaves_dir = g_strdup_printf("/sys/block/%s/slaves", de->d_name); > > + if (slaves_dir == NULL) { > > + g_debug(" failed to open directory %s", slaves_dir); > > + continue; > > + } > > + g_debug(" counting entries in: %s", slaves_dir); > > + dp_slaves = opendir(slaves_dir); > > + while ((de_slaves = readdir(dp_slaves)) != NULL) { > > + if ((strcmp(".", de_slaves->d_name) == 0) || > > + (strcmp("..", de_slaves->d_name) == 0)) { > > + continue; > > + } > > + slaves++; > > + } > > + closedir(dp_slaves); > > + g_debug(" counted %lu items", slaves); > > + if (slaves != 0) { > > + continue; > > + } > > For guest-get-fsinfo we skip returning any data about virtual devices > (dm/md/etc.) and just return the physical disks underlying whatever > hierarchy the filesystem is built on, which sort of makes sense as far > as making sure we know what devices are currently in use. > > But for guest-get-disks we seem to care about usage beyond just mounted > filesystems, which to me would suggest things like whether a disk is > part of an LVM/RAID/multipath volume. But by skipping virtual devices > with "/sys/block/*/slaves" entries we lose that information... > > So I guess I'm not sure I understand the use-cases you have in mind here. > Can you provide some examples? And do you anticipate we would need to add > an interface to report this hierarchy at some point? The intent is to provide information about devices that were specified when starting the VM and not about complete disk hierarchy. The reasoning behind it is for management applications to be able to show what names in guest OS correspond to the disks in the VM definition. But if you suggest that reporting the hierarchy similar to what lsblk provides would be useful I can do that for Linux. The issue is with Windows where I don't know how to do that. > > > + > > + g_debug(" checking disk size"); > > + size_path = g_strdup_printf("/sys/block/%s/size", de->d_name); > > + fp = fopen(size_path, "r"); > > + if (!fp) { > > + g_debug(" failed to read disk size"); > > + continue; > > + } > > + if (getline(&line, &n, fp) == -1) { > > We need to set n=0, otherwise an uninitialized value might be interpreted as > the size of line's pre-allocated buffer > > > + g_debug(" failed to read disk size"); > > + fclose(fp); > > + continue; > > + } > > + fclose(fp); > > + if (strcmp(line, "0\n") == 0) { > > + g_debug(" skipping zero-sized disk"); > > + continue; > > + } > > + > > + fs = g_malloc0(sizeof(*fs)); > > + g_debug(" adding %s", de->d_name); > > + disk_dir = g_strdup_printf("/sys/block/%s", de->d_name); > > + build_guest_fsinfo_for_device(disk_dir, fs, &local_err); > > + if (local_err != NULL) { > > + g_debug(" failed to get device info, ignoring error: %s", > > + error_get_pretty(local_err)); > > + error_free(local_err); > > + continue; > > + } else if (fs->disk == NULL) { > > + g_debug(" skipping unknown disk"); > > + continue; > > + } > > + item = g_steal_pointer(&fs->disk); > > Does this ensure that fs itself doesn't still get auto-free'd? Yes, it does. > In any > case, it seems awkward to allocate a GuestFilesystemInfo just so we can > re-use build_guest_fsinfo_for_device(). Can code be refactored into a > separate build_guest_disk_info_for_device() or something that just takes > a GuestDiskAddressList* directly? > It is little bit cumbersome, I agree. Unfortunately refactoring it is little bit tricky and would make the build_guestfs_info_*() functions less readable. But moving the code into separate function to hide this is certainly an option. Tomas > > + g_assert(item->next == NULL); /* expecting just a single disk */ > > + item->next = ret; > > + ret = item; > > + } > > + return ret; > > +} > > + > > /* Return a list of the disk device(s)' info which @mount lies on */ > > static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount, > > Error **errp) > > @@ -2809,7 +2897,8 @@ GList *ga_command_blacklist_init(GList *blacklist) > > const char *list[] = { > > "guest-get-fsinfo", "guest-fsfreeze-status", > > "guest-fsfreeze-freeze", "guest-fsfreeze-freeze-list", > > - "guest-fsfreeze-thaw", "guest-get-fsinfo", NULL}; > > + "guest-fsfreeze-thaw", "guest-get-fsinfo", > > + "guest-get-disks", NULL}; > > char **p = (char **)list; > > > > while (*p) { > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > > index aaa71f147b..0bafa2dc4c 100644 > > --- a/qga/commands-win32.c > > +++ b/qga/commands-win32.c > > @@ -945,6 +945,83 @@ out: > > return list; > > } > > > > +GuestDiskAddressList *qmp_guest_get_disks(Error **errp) > > +{ > > + GuestDiskAddressList *new = NULL, *ret = NULL; > > + HDEVINFO dev_info; > > + SP_DEVICE_INTERFACE_DATA dev_iface_data; > > + int i; > > + > > + dev_info = SetupDiGetClassDevs(&GUID_DEVINTERFACE_DISK, 0, 0, > > + DIGCF_PRESENT | DIGCF_DEVICEINTERFACE); > > + if (dev_info == INVALID_HANDLE_VALUE) { > > + error_setg_win32(errp, GetLastError(), "failed to get device tree"); > > + return NULL; > > + } > > + > > + g_debug("enumerating devices"); > > + dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA); > > + for (i = 0; > > + SetupDiEnumDeviceInterfaces(dev_info, NULL, &GUID_DEVINTERFACE_DISK, > > + i, &dev_iface_data); > > + i++) { > > + GuestDiskAddress *disk = NULL; > > + Error *local_err = NULL; > > + g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA > > + pdev_iface_detail_data = NULL; > > + STORAGE_DEVICE_NUMBER sdn; > > + HANDLE dev_file; > > + DWORD size = 0; > > + > > + g_debug(" getting device path"); > > + while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data, > > + pdev_iface_detail_data, > > + size, &size, > > + NULL)) { > > + if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) { > > + pdev_iface_detail_data = g_malloc(size); > > + pdev_iface_detail_data->cbSize = > > + sizeof(*pdev_iface_detail_data); > > + } else { > > + g_debug("failed to get device interface details"); > > + continue; > > + } > > + } > > + > > + g_debug(" device: %s", pdev_iface_detail_data->DevicePath); > > + dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0, > > + FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); > > + if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER, > > + NULL, 0, &sdn, sizeof(sdn), &size, NULL)) { > > + CloseHandle(dev_file); > > + debug_error("failed to get storage device number"); > > + continue; > > + } > > + CloseHandle(dev_file); > > + > > + g_debug(" number: %lu", sdn.DeviceNumber); > > + disk = g_malloc0(sizeof(GuestDiskAddress)); > > + disk->has_dev = true; > > + disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu", > > + sdn.DeviceNumber); > > + get_single_disk_info(sdn.DeviceNumber, disk, &local_err); > > + if (local_err) { > > + g_debug("failed to get disk info: %s", > > + error_get_pretty(local_err)); > > + error_free(local_err); > > + g_free(disk); > > + continue; > > + } > > + new = g_malloc0(sizeof(GuestDiskAddressList)); > > + new->value = disk; > > + new->next = ret; > > + ret = new; > > + } > > + > > + SetupDiDestroyDeviceInfoList(dev_info); > > + return ret; > > +} > > + > > #else > > > > static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) > > @@ -952,6 +1029,12 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) > > return NULL; > > } > > > > +GuestDiskAddressList *qmp_guest_get_disks(Error **errp) > > +{ > > + error_setg(errp, QERR_UNSUPPORTED); > > + return NULL; > > +} > > + > > #endif /* CONFIG_QGA_NTDDSCSI */ > > > > static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp) > > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > > index 408a662ea5..5d8fa1c283 100644 > > --- a/qga/qapi-schema.json > > +++ b/qga/qapi-schema.json > > @@ -862,6 +862,19 @@ > > 'bus': 'int', 'target': 'int', 'unit': 'int', > > '*serial': 'str', '*dev': 'str'} } > > > > +## > > +# @guest-get-disks: > > +# > > +# Returns: The list of disks in the guest. For Windows these are only the > > +# physical disks. On Linux these are all root block devices of > > +# non-zero size including e.g. removable devices, loop devices, > > +# NBD, etc. > > +# > > +# Since: 5.2 > > +## > > +{ 'command': 'guest-get-disks', > > + 'returns': ['GuestDiskAddress'] } > > + > > ## > > # @GuestFilesystemInfo: > > # > > -- > > 2.25.0 > > > -- Tomáš Golembiovský <tgolembi@redhat.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-17 7:17 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-06 9:03 [PATCH 0/1] qga: add command guest-get-disk Tomáš Golembiovský 2020-08-06 9:03 ` [PATCH 1/1] qga: add command guest-get-disks Tomáš Golembiovský 2020-08-06 10:44 ` Philippe Mathieu-Daudé 2020-08-12 21:30 ` Michael Roth 2020-08-12 21:29 ` Michael Roth 2020-08-17 7:17 ` Tomáš Golembiovský
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).