| Message ID | 1bf9ede3b9273613319ff8ff99b40c385439cfa8.1599470071.git.tgolembi@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qga: add command guest-get-disk | expand |
Hi On Mon, Sep 7, 2020 at 1:17 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote: > The command lists all disks (real and virtual) as well as disk > partitions. For each disk the list of slave disks is also listed and > /dev path is used as a handle so it can be matched with "name" filed of > field other returned disk entries. For disk partitions the "slave" list is > populated with the the parent device for easier tracking of hierarchy. > > Example output: > { > "return": [ > ... > { > "name": "/dev/dm-0", > "partition": false, > "slaves": [ > "/dev/sda2" > ], > "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7" > }, > { > "name": "/dev/sda2", > "partition": true, > "slaves": [ > "/dev/sda" > ] > }, > { > "name": "/dev/sda", > "partition": false, > "address": { > "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493", > "bus-type": "sata", > ... > "dev": "/dev/sda", > "target": 0 > }, > "slaves": [] > }, > ... > ] > } > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > qga/commands-posix.c | 247 +++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 240 insertions(+), 7 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index f99731af51..3babc25c09 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -62,6 +62,9 @@ extern char **environ; > #endif > #endif > > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo, > + qapi_free_GuestFilesystemInfo) > + > This will now conflict with qapi-gen generated headers. static void ga_wait_child(pid_t pid, int *status, Error **errp) > { > pid_t rpid; > @@ -1150,6 +1153,21 @@ static void > build_guest_fsinfo_for_virtual_device(char const *syspath, > closedir(dir); > } > > +static bool is_disk_virtual(const char *devpath, Error **errp) > +{ > + g_autofree char *syspath = realpath(devpath, NULL); > + > + if (!syspath) { > + error_setg_errno(errp, errno, "realpath(\"%s\")", devpath); > + return false; > + } > + if (strstr(syspath, "/devices/virtual/block/")) { > + return true; > + } else { > + return false; > + } > simply to "return strstr(syspath, "/devices/virtual/block/") != NULL;" ? (Or strstr(syspath, "/devices/virtual/block/") ? true : false ) +} > + > /* Dispatch to functions for virtual/real device */ > static void build_guest_fsinfo_for_device(char const *devpath, > GuestFilesystemInfo *fs, > @@ -1168,6 +1186,7 @@ static void build_guest_fsinfo_for_device(char const > *devpath, > > g_debug(" parse sysfs path '%s'", syspath); > > + /* TODO: use is_disk_virtual() */ > just do it, no? if (strstr(syspath, "/devices/virtual/block/")) { > build_guest_fsinfo_for_virtual_device(syspath, fs, errp); > } else { > @@ -1177,6 +1196,225 @@ static void build_guest_fsinfo_for_device(char > const *devpath, > free(syspath); > } > > +#ifdef CONFIG_LIBUDEV > + > +/* > + * Wrapper around build_guest_fsinfo_for_device() for getting just > + * the disk address. > + */ > +static GuestDiskAddress *get_disk_address(const char *syspath, Error > **errp) > +{ > + g_autoptr(GuestFilesystemInfo) fs = NULL; > + > + fs = g_new0(GuestFilesystemInfo, 1); > Heap allocation / auto wasn't really necessary here, but ok. > + build_guest_fsinfo_for_device(syspath, fs, errp); > + if (fs->disk != NULL) { > + return g_steal_pointer(&fs->disk->value); > + } > + return NULL; > Could also be a onliner, but perhaps less readable. +} > + > +static char *get_alias_for_syspath(const char *syspath) > +{ > + struct udev *udev = NULL; > + struct udev_device *udevice = NULL; > + char *ret = NULL; > + > + udev = udev_new(); > I would have g_return_val_if_fail(udev != NULL, NULL); here as, + udevice = udev_device_new_from_syspath(udev, syspath); > udev_device_new_from_syspath() might crash otherwise. > + if (udev == NULL || udevice == NULL) { > + g_debug("failed to query udev"); > + } else { > + const char *alias = udev_device_get_property_value( > + udevice, "DM_NAME"); > + if (alias != NULL && *alias != 0) { > + ret = g_strdup(alias); > g_strdup() works fine with NULL. Is there "" empty aliases? Why not report them too? + } > + } > + > + udev_unref(udev); > + udev_device_unref(udevice); > + return ret; > +} > + > +static char *get_device_for_syspath(const char *syspath) > +{ > + struct udev *udev = NULL; > + struct udev_device *udevice = NULL; > + char *ret = NULL; > + > + udev = udev_new(); > + udevice = udev_device_new_from_syspath(udev, syspath); > + if (udev == NULL || udevice == NULL) { > Same as above + g_debug("failed to query udev"); > + } else { > + ret = g_strdup(udev_device_get_devnode(udevice)); > + } > + udev_unref(udev); > + udev_device_unref(udevice); > + return ret; > +} > + > +GuestDiskInfoList *qmp_guest_get_disks(Error **errp) > +{ > + GuestDiskInfoList *item, *ret = NULL; > + GuestDiskInfo *disk, *partition; > + 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_dir = NULL, *slaves_dir = NULL; > + struct dirent *de_disk, *de_slaves; > + DIR *dp_disk = NULL, *dp_slaves = NULL; > + FILE *fp = NULL; > + size_t n = 0; > + Error *local_err = NULL; > + if (de->d_type != DT_LNK) { > + g_debug(" skipping entry: %s", de->d_name); > + continue; > + } > + > + /* Check size and skip zero-sized disks */ > + g_debug(" checking disk size"); > + size_dir = g_strdup_printf("/sys/block/%s/size", de->d_name); > + fp = fopen(size_dir, "r"); > + if (!fp) { > + g_debug(" failed to read disk size"); > + continue; > + } > + if (getline(&line, &n, fp) == -1) { > + g_debug(" failed to read disk size"); > line: getline(3) "This buffer should be freed by the user program even if getline() failed." + fclose(fp); > + continue; > + } > + fclose(fp); > + if (strcmp(line, "0\n") == 0) { > It would be slightly better to defend against NULL crash here, with g_strcmp0() + g_debug(" skipping zero-sized disk"); > + continue; > + } > + > line is never freed? + g_debug(" adding %s", de->d_name); > + disk_dir = g_strdup_printf("/sys/block/%s", de->d_name); > + disk = g_new0(GuestDiskInfo, 1); > + disk->name = get_device_for_syspath(disk_dir); > + disk->partition = false; > + disk->alias = get_alias_for_syspath(disk_dir); > + if (disk->alias != NULL) { > + disk->has_alias = true; > + } > Could be a single line too + item = g_new0(GuestDiskInfoList, 1); > + item->value = disk; > + item->next = ret; > + ret = item; > + > + /* Get address for non-virtual devices */ > + bool is_virtual = is_disk_virtual(disk_dir, &local_err); > + if (local_err != NULL) { > + g_debug(" failed to check disk path, ignoring error: %s", > + error_get_pretty(local_err)); > + error_free(local_err); > + local_err = NULL; > + /* Don't try to get the address */ > + is_virtual = true; > + } > + if (!is_virtual) { > + disk->address = get_disk_address(disk_dir, &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); > + local_err = NULL; > + } else if (disk->address != NULL) { > + disk->has_address = true; > + } > + } > + > + /* List slave disks */ > + slaves_dir = g_strdup_printf("%s/slaves", disk_dir); > + g_debug(" listing entries in: %s", slaves_dir); > + dp_slaves = opendir(slaves_dir); > + while ((de_slaves = readdir(dp_slaves)) != NULL) { > + g_autofree char *slave_dir = NULL; > + char *slave_dev; > + strList *slave_item = NULL; > + > + if ((strcmp(".", de_slaves->d_name) == 0) || > + (strcmp("..", de_slaves->d_name) == 0)) { > + continue; > + } > + > + /* Add slave disks */ > + slave_dir = g_strdup_printf("%s/%s", > + slaves_dir, de_slaves->d_name); > + slave_dev = get_device_for_syspath(slave_dir); > + if (slave_dev != NULL) { > + g_debug(" adding slave device: %s", slave_dev); > + slave_item = g_new0(strList, 1); > + slave_item->value = slave_dev; > + slave_item->next = disk->slaves; > + disk->slaves = slave_item; > + } > + } > + closedir(dp_slaves); > + > + /* > + * Detect partitions subdirectory name is "<parent><number>" or > + * "<parent>p<number>" > + */ > + dp_disk = opendir(disk_dir); > + while ((de_disk = readdir(dp_disk)) != NULL) { > + size_t len; > + g_autofree char *partition_dir = NULL; > + > + if (!(de_disk->d_type & DT_DIR)) { > + continue; > + } > + > + len = strlen(de->d_name); > + if (!(strncmp(de->d_name, de_disk->d_name, len) == 0 && > + ((*(de_disk->d_name + len) == 'p' && > + isdigit(*(de_disk->d_name + len + 1))) || > + isdigit(*(de_disk->d_name + len))))) { > + continue; > + } > + > + partition_dir = g_strdup_printf("%s/%s", > + disk_dir, de_disk->d_name); > + partition = g_new0(GuestDiskInfo, 1); > + partition->name = get_device_for_syspath(partition_dir); > + partition->partition = true; > + /* Add parent disk as slave for easier tracking of hierarchy > */ > + partition->slaves = g_new0(strList, 1); > + partition->slaves->value = g_strdup(disk->name); > + > + item = g_new0(GuestDiskInfoList, 1); > + item->value = partition; > + item->next = ret; > + ret = item; > + > + } > + closedir(dp_disk); > + } > + return ret; > +} > + > +#else > + > +GuestDiskInfoList *qmp_guest_get_disks(Error **errp) > +{ > + error_setg(errp, QERR_UNSUPPORTED); > + return NULL; > +} > + > +#endif > + > /* 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 +3047,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) { > @@ -3042,9 +3281,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp) > > return info; > } > - > -GuestDiskInfoList *qmp_guest_get_disks(Error **errp) > -{ > - error_setg(errp, QERR_UNSUPPORTED); > - return NULL; > -} > -- > 2.25.0 > > > -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 7, 2020 at 1:17 PM Tomáš Golembiovský <<a href="mailto:tgolembi@redhat.com">tgolembi@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The command lists all disks (real and virtual) as well as disk<br> partitions. For each disk the list of slave disks is also listed and<br> /dev path is used as a handle so it can be matched with "name" filed of<br></blockquote><div><br></div><div>field <br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> other returned disk entries. For disk partitions the "slave" list is<br> populated with the the parent device for easier tracking of hierarchy.<br> <br> Example output:<br> {<br> "return": [<br> ...<br> {<br> "name": "/dev/dm-0",<br> "partition": false,<br> "slaves": [<br> "/dev/sda2"<br> ],<br> "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7"<br> },<br> {<br> "name": "/dev/sda2",<br> "partition": true,<br> "slaves": [<br> "/dev/sda"<br> ]<br> },<br> {<br> "name": "/dev/sda",<br> "partition": false,<br> "address": {<br> "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",<br> "bus-type": "sata",<br> ...<br> "dev": "/dev/sda",<br> "target": 0<br> },<br> "slaves": []<br> },<br> ...<br> ]<br> }<br> <br> Signed-off-by: Tomáš Golembiovský <<a href="mailto:tgolembi@redhat.com" target="_blank">tgolembi@redhat.com</a>><br> ---<br> qga/commands-posix.c | 247 +++++++++++++++++++++++++++++++++++++++++--<br> 1 file changed, 240 insertions(+), 7 deletions(-)<br> <br> diff --git a/qga/commands-posix.c b/qga/commands-posix.c<br> index f99731af51..3babc25c09 100644<br> --- a/qga/commands-posix.c<br> +++ b/qga/commands-posix.c<br> @@ -62,6 +62,9 @@ extern char **environ;<br> #endif<br> #endif<br> <br> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo,<br> + qapi_free_GuestFilesystemInfo)<br> +<br></blockquote><div><br></div><div>This will now conflict with qapi-gen generated headers.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> static void ga_wait_child(pid_t pid, int *status, Error **errp)<br> {<br> pid_t rpid;<br> @@ -1150,6 +1153,21 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,<br> closedir(dir);<br> }<br> <br> +static bool is_disk_virtual(const char *devpath, Error **errp)<br> +{<br> + g_autofree char *syspath = realpath(devpath, NULL);<br> +<br> + if (!syspath) {<br> + error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + return false;<br> + }<br> + if (strstr(syspath, "/devices/virtual/block/")) {<br> + return true;<br> + } else {<br> + return false;<br> + }<br></blockquote><div><br></div><div> simply to "return strstr(syspath, "/devices/virtual/block/") != NULL;" ? (Or strstr(syspath, "/devices/virtual/block/") ? true : false )<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +}<br> +<br> /* Dispatch to functions for virtual/real device */<br> static void build_guest_fsinfo_for_device(char const *devpath,<br> GuestFilesystemInfo *fs,<br> @@ -1168,6 +1186,7 @@ static void build_guest_fsinfo_for_device(char const *devpath,<br> <br> g_debug(" parse sysfs path '%s'", syspath);<br> <br> + /* TODO: use is_disk_virtual() */<br></blockquote><div><br></div><div>just do it, no?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> if (strstr(syspath, "/devices/virtual/block/")) {<br> build_guest_fsinfo_for_virtual_device(syspath, fs, errp);<br> } else {<br> @@ -1177,6 +1196,225 @@ static void build_guest_fsinfo_for_device(char const *devpath,<br> free(syspath);<br> }<br> <br> +#ifdef CONFIG_LIBUDEV<br> +<br> +/*<br> + * Wrapper around build_guest_fsinfo_for_device() for getting just<br> + * the disk address.<br> + */<br> +static GuestDiskAddress *get_disk_address(const char *syspath, Error **errp)<br> +{<br> + g_autoptr(GuestFilesystemInfo) fs = NULL;<br> +<br> + fs = g_new0(GuestFilesystemInfo, 1);<br></blockquote><div><br></div><div>Heap allocation / auto wasn't really necessary here, but ok.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + build_guest_fsinfo_for_device(syspath, fs, errp);<br> + if (fs->disk != NULL) {<br> + return g_steal_pointer(&fs->disk->value);<br> + }<br> + return NULL;<br></blockquote><div><br></div><div>Could also be a onliner, but perhaps less readable.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +}<br> +<br> +static char *get_alias_for_syspath(const char *syspath)<br> +{<br> + struct udev *udev = NULL;<br> + struct udev_device *udevice = NULL;<br> + char *ret = NULL;<br> +<br> + udev = udev_new();<br></blockquote><div><br></div><div>I would have g_return_val_if_fail(udev != NULL, NULL); here as, <br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + udevice = udev_device_new_from_syspath(udev, syspath);<br></blockquote><div><br></div><div>udev_device_new_from_syspath() might crash otherwise.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if (udev == NULL || udevice == NULL) {<br> + g_debug("failed to query udev");<br> + } else {<br> + const char *alias = udev_device_get_property_value(<br> + udevice, "DM_NAME");<br> + if (alias != NULL && *alias != 0) {<br> + ret = g_strdup(alias);<br></blockquote><div><br></div><div>g_strdup() works fine with NULL. Is there "" empty aliases? Why not report them too?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + }<br> + }<br> +<br> + udev_unref(udev);<br> + udev_device_unref(udevice);<br> + return ret;<br> +}<br> +<br> +static char *get_device_for_syspath(const char *syspath)<br> +{<br> + struct udev *udev = NULL;<br> + struct udev_device *udevice = NULL;<br> + char *ret = NULL;<br> +<br> + udev = udev_new();<br> + udevice = udev_device_new_from_syspath(udev, syspath);<br> + if (udev == NULL || udevice == NULL) {<br></blockquote><div><br></div><div>Same as above</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + g_debug("failed to query udev");<br> + } else {<br> + ret = g_strdup(udev_device_get_devnode(udevice));<br> + }<br> + udev_unref(udev);<br> + udev_device_unref(udevice);<br> + return ret;<br> +}<br> +<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)<br> +{<br> + GuestDiskInfoList *item, *ret = NULL;<br> + GuestDiskInfo *disk, *partition;<br> + DIR *dp = NULL;<br> + struct dirent *de = NULL;<br> +<br> + g_debug("listing /sys/block directory");<br> + dp = opendir("/sys/block");<br> + if (dp == NULL) {<br> + error_setg_errno(errp, errno, "Can't open directory \"/sys/block\"");<br> + return NULL;<br> + }<br> + while ((de = readdir(dp)) != NULL) {<br> + g_autofree char *disk_dir = NULL, *line = NULL,<br> + *size_dir = NULL, *slaves_dir = NULL;<br> + struct dirent *de_disk, *de_slaves;<br> + DIR *dp_disk = NULL, *dp_slaves = NULL;<br> + FILE *fp = NULL;<br> + size_t n = 0;<br> + Error *local_err = NULL;<br> + if (de->d_type != DT_LNK) {<br> + g_debug(" skipping entry: %s", de->d_name);<br> + continue;<br> + }<br> +<br> + /* Check size and skip zero-sized disks */<br> + g_debug(" checking disk size");<br> + size_dir = g_strdup_printf("/sys/block/%s/size", de->d_name);<br> + fp = fopen(size_dir, "r");<br> + if (!fp) {<br> + g_debug(" failed to read disk size");<br> + continue;<br> + }<br> + if (getline(&line, &n, fp) == -1) {<br> + g_debug(" failed to read disk size");<br></blockquote><div><br></div><div>line: getline(3) "This buffer should be freed by the user program even if getline() failed."</div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + fclose(fp);<br> + continue;<br> + }<br> + fclose(fp);<br> + if (strcmp(line, "0\n") == 0) {<br></blockquote><div><br></div><div>It would be slightly better to defend against NULL crash here, with g_strcmp0()<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + g_debug(" skipping zero-sized disk");<br> + continue;<br> + }<br> +<br></blockquote><div><br></div><div>line is never freed?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + g_debug(" adding %s", de->d_name);<br> + disk_dir = g_strdup_printf("/sys/block/%s", de->d_name);<br> + disk = g_new0(GuestDiskInfo, 1);<br> + disk->name = get_device_for_syspath(disk_dir);<br> + disk->partition = false;<br> + disk->alias = get_alias_for_syspath(disk_dir);<br> + if (disk->alias != NULL) {<br> + disk->has_alias = true;<br> + }<br></blockquote><div><br></div><div>Could be a single line too</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + item = g_new0(GuestDiskInfoList, 1);<br> + item->value = disk;<br> + item->next = ret;<br> + ret = item;<br> +<br> + /* Get address for non-virtual devices */<br> + bool is_virtual = is_disk_virtual(disk_dir, &local_err);<br> + if (local_err != NULL) {<br> + g_debug(" failed to check disk path, ignoring error: %s",<br> + error_get_pretty(local_err));<br> + error_free(local_err);<br> + local_err = NULL;<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + /* Don't try to get the address */<br> + is_virtual = true;<br> + }<br> + if (!is_virtual) {<br> + disk->address = get_disk_address(disk_dir, &local_err);<br> + if (local_err != NULL) {<br> + g_debug(" failed to get device info, ignoring error: %s",<br> + error_get_pretty(local_err));<br> + error_free(local_err);<br> + local_err = NULL;<br> + } else if (disk->address != NULL) {<br> + disk->has_address = true;<br> + }<br> + }<br> +<br> + /* List slave disks */<br> + slaves_dir = g_strdup_printf("%s/slaves", disk_dir);<br> + g_debug(" listing entries in: %s", slaves_dir);<br> + dp_slaves = opendir(slaves_dir);<br> + while ((de_slaves = readdir(dp_slaves)) != NULL) {<br> + g_autofree char *slave_dir = NULL;<br> + char *slave_dev;<br> + strList *slave_item = NULL;<br> +<br> + if ((strcmp(".", de_slaves->d_name) == 0) ||<br> + (strcmp("..", de_slaves->d_name) == 0)) {<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + continue;<br> + }<br> +<br> + /* Add slave disks */<br> + slave_dir = g_strdup_printf("%s/%s",<br> + slaves_dir, de_slaves->d_name);<br> + slave_dev = get_device_for_syspath(slave_dir);<br> + if (slave_dev != NULL) {<br> + g_debug(" adding slave device: %s", slave_dev);<br> + slave_item = g_new0(strList, 1);<br> + slave_item->value = slave_dev;<br> + slave_item->next = disk->slaves;<br> + disk->slaves = slave_item;<br> + }<br> + }<br> + closedir(dp_slaves);<br> +<br> + /*<br> + * Detect partitions subdirectory name is "<parent><number>" or<br> + * "<parent>p<number>"<br> + */<br> + dp_disk = opendir(disk_dir);<br> + while ((de_disk = readdir(dp_disk)) != NULL) {<br> + size_t len;<br> + g_autofree char *partition_dir = NULL;<br> +<br> + if (!(de_disk->d_type & DT_DIR)) {<br> + continue;<br> + }<br> +<br> + len = strlen(de->d_name);<br> + if (!(strncmp(de->d_name, de_disk->d_name, len) == 0 &&<br> + ((*(de_disk->d_name + len) == 'p' &&<br> + isdigit(*(de_disk->d_name + len + 1))) ||<br> + isdigit(*(de_disk->d_name + len))))) {<br> + continue;<br> + }<br> +<br> + partition_dir = g_strdup_printf("%s/%s",<br> + disk_dir, de_disk->d_name);<br> + partition = g_new0(GuestDiskInfo, 1);<br> + partition->name = get_device_for_syspath(partition_dir);<br> + partition->partition = true;<br> + /* Add parent disk as slave for easier tracking of hierarchy */<br> + partition->slaves = g_new0(strList, 1);<br> + partition->slaves->value = g_strdup(disk->name);<br> +<br> + item = g_new0(GuestDiskInfoList, 1);<br> + item->value = partition;<br> + item->next = ret;<br> + ret = item;<br> +<br> + }<br> + closedir(dp_disk);<br> + }<br> + return ret;<br> +}<br> +<br> +#else<br> +<br> +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)<br> +{<br> + error_setg(errp, QERR_UNSUPPORTED);<br> + return NULL;<br> +}<br> +<br> +#endif<br> +<br> /* Return a list of the disk device(s)' info which @mount lies on */<br> static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,<br> Error **errp)<br> @@ -2809,7 +3047,8 @@ GList *ga_command_blacklist_init(GList *blacklist)<br> const char *list[] = {<br> "guest-get-fsinfo", "guest-fsfreeze-status",<br> "guest-fsfreeze-freeze", "guest-fsfreeze-freeze-list",<br> - "guest-fsfreeze-thaw", "guest-get-fsinfo", NULL};<br> + "guest-fsfreeze-thaw", "guest-get-fsinfo",<br> + "guest-get-disks", NULL};<br> char **p = (char **)list;<br> <br> while (*p) {<br> @@ -3042,9 +3281,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)<br> <br> return info;<br> }<br> -<br> -GuestDiskInfoList *qmp_guest_get_disks(Error **errp)<br> -{<br> - error_setg(errp, QERR_UNSUPPORTED);<br> - return NULL;<br> -}<br> -- <br> 2.25.0<br> <br> <br> </blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
On Tue, Sep 29, 2020 at 07:22:00PM +0400, Marc-André Lureau wrote: > Hi > > On Mon, Sep 7, 2020 at 1:17 PM Tomáš Golembiovský <tgolembi@redhat.com> > wrote: > > > The command lists all disks (real and virtual) as well as disk > > partitions. For each disk the list of slave disks is also listed and > > /dev path is used as a handle so it can be matched with "name" filed of > > > > field > > other returned disk entries. For disk partitions the "slave" list is > > populated with the the parent device for easier tracking of hierarchy. > > > > Example output: > > { > > "return": [ > > ... > > { > > "name": "/dev/dm-0", > > "partition": false, > > "slaves": [ > > "/dev/sda2" > > ], > > "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7" > > }, > > { > > "name": "/dev/sda2", > > "partition": true, > > "slaves": [ > > "/dev/sda" > > ] > > }, > > { > > "name": "/dev/sda", > > "partition": false, > > "address": { > > "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493", > > "bus-type": "sata", > > ... > > "dev": "/dev/sda", > > "target": 0 > > }, > > "slaves": [] > > }, > > ... > > ] > > } > > > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > > --- > > qga/commands-posix.c | 247 +++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 240 insertions(+), 7 deletions(-) > > > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > > index f99731af51..3babc25c09 100644 > > --- a/qga/commands-posix.c > > +++ b/qga/commands-posix.c > > @@ -62,6 +62,9 @@ extern char **environ; > > #endif > > #endif > > > > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo, > > + qapi_free_GuestFilesystemInfo) > > + > > > > This will now conflict with qapi-gen generated headers. > > static void ga_wait_child(pid_t pid, int *status, Error **errp) > > { > > pid_t rpid; > > @@ -1150,6 +1153,21 @@ static void > > build_guest_fsinfo_for_virtual_device(char const *syspath, > > closedir(dir); > > } > > > > +static bool is_disk_virtual(const char *devpath, Error **errp) > > +{ > > + g_autofree char *syspath = realpath(devpath, NULL); > > + > > + if (!syspath) { > > + error_setg_errno(errp, errno, "realpath(\"%s\")", devpath); > > > + return false; > > + } > > + if (strstr(syspath, "/devices/virtual/block/")) { > > + return true; > > + } else { > > + return false; > > + } > > > > simply to "return strstr(syspath, "/devices/virtual/block/") != NULL;" ? > (Or strstr(syspath, "/devices/virtual/block/") ? true : false ) > > +} > > + > > /* Dispatch to functions for virtual/real device */ > > static void build_guest_fsinfo_for_device(char const *devpath, > > GuestFilesystemInfo *fs, > > @@ -1168,6 +1186,7 @@ static void build_guest_fsinfo_for_device(char const > > *devpath, > > > > g_debug(" parse sysfs path '%s'", syspath); > > > > + /* TODO: use is_disk_virtual() */ > > > > just do it, no? It's great that I put a note there otherwise I might have forgotten to do it. ;) > > if (strstr(syspath, "/devices/virtual/block/")) { > > build_guest_fsinfo_for_virtual_device(syspath, fs, errp); > > } else { > > @@ -1177,6 +1196,225 @@ static void build_guest_fsinfo_for_device(char > > const *devpath, > > free(syspath); > > } > > > > +#ifdef CONFIG_LIBUDEV > > + > > +/* > > + * Wrapper around build_guest_fsinfo_for_device() for getting just > > + * the disk address. > > + */ > > +static GuestDiskAddress *get_disk_address(const char *syspath, Error > > **errp) > > +{ > > + g_autoptr(GuestFilesystemInfo) fs = NULL; > > + > > + fs = g_new0(GuestFilesystemInfo, 1); > > > > Heap allocation / auto wasn't really necessary here, but ok. I used it so that qapi_free_GuestFilesystemInfo() is called on function exit in all cases. I am not sure if I could do that if `fs` were on the stack. > > > > + build_guest_fsinfo_for_device(syspath, fs, errp); > > + if (fs->disk != NULL) { > > + return g_steal_pointer(&fs->disk->value); > > + } > > + return NULL; > > > > Could also be a onliner, but perhaps less readable. Yeah, I prefer it this way. > > +} > > + > > +static char *get_alias_for_syspath(const char *syspath) > > +{ > > + struct udev *udev = NULL; > > + struct udev_device *udevice = NULL; > > + char *ret = NULL; > > + > > + udev = udev_new(); > > > > I would have g_return_val_if_fail(udev != NULL, NULL); here as, > > + udevice = udev_device_new_from_syspath(udev, syspath); > > > > udev_device_new_from_syspath() might crash otherwise. That is probably true. This may require fixes at other places too. > > > > + if (udev == NULL || udevice == NULL) { > > + g_debug("failed to query udev"); > > + } else { > > + const char *alias = udev_device_get_property_value( > > + udevice, "DM_NAME"); > > + if (alias != NULL && *alias != 0) { > > + ret = g_strdup(alias); > > > > g_strdup() works fine with NULL. Is there "" empty aliases? Why not report > them too? NULL means an error and empty alias means there is no alias. I will put that in a comment there. In QAPI we have alias as optional which seems preferable rather than always returning empty string. > > + } > > + } > > + > > + udev_unref(udev); > > + udev_device_unref(udevice); > > + return ret; > > +} > > + > > +static char *get_device_for_syspath(const char *syspath) > > +{ > > + struct udev *udev = NULL; > > + struct udev_device *udevice = NULL; > > + char *ret = NULL; > > + > > + udev = udev_new(); > > + udevice = udev_device_new_from_syspath(udev, syspath); > > + if (udev == NULL || udevice == NULL) { > > > > Same as above > > + g_debug("failed to query udev"); > > + } else { > > + ret = g_strdup(udev_device_get_devnode(udevice)); > > + } > > + udev_unref(udev); > > + udev_device_unref(udevice); > > + return ret; > > +} > > + > > > +GuestDiskInfoList *qmp_guest_get_disks(Error **errp) > > +{ > > + GuestDiskInfoList *item, *ret = NULL; > > + GuestDiskInfo *disk, *partition; > > + 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_dir = NULL, *slaves_dir = NULL; > > + struct dirent *de_disk, *de_slaves; > > + DIR *dp_disk = NULL, *dp_slaves = NULL; > > + FILE *fp = NULL; > > + size_t n = 0; > > + Error *local_err = NULL; > > + if (de->d_type != DT_LNK) { > > + g_debug(" skipping entry: %s", de->d_name); > > + continue; > > + } > > + > > + /* Check size and skip zero-sized disks */ > > + g_debug(" checking disk size"); > > + size_dir = g_strdup_printf("/sys/block/%s/size", de->d_name); > > + fp = fopen(size_dir, "r"); > > + if (!fp) { > > + g_debug(" failed to read disk size"); > > + continue; > > + } > > + if (getline(&line, &n, fp) == -1) { > > + g_debug(" failed to read disk size"); > > > > line: getline(3) "This buffer should be freed by the user program even if > getline() failed." That is handled by the g_autofree, or am I missing something? `line` will get out of scope after every interation (even with continue). Or do you prefer to have it explicit and free as soon as we don't need it? > > + fclose(fp); > > + continue; > > + } > > + fclose(fp); > > + if (strcmp(line, "0\n") == 0) { > > > > It would be slightly better to defend against NULL crash here, with > g_strcmp0() > > + g_debug(" skipping zero-sized disk"); > > + continue; > > + } > > + > > > > line is never freed? > > + g_debug(" adding %s", de->d_name); > > + disk_dir = g_strdup_printf("/sys/block/%s", de->d_name); > > + disk = g_new0(GuestDiskInfo, 1); > > + disk->name = get_device_for_syspath(disk_dir); > > + disk->partition = false; > > + disk->alias = get_alias_for_syspath(disk_dir); > > + if (disk->alias != NULL) { > > + disk->has_alias = true; > > + } > > > > Could be a single line too > > + item = g_new0(GuestDiskInfoList, 1); > > + item->value = disk; > > + item->next = ret; > > + ret = item; > > + > > + /* Get address for non-virtual devices */ > > + bool is_virtual = is_disk_virtual(disk_dir, &local_err); > > + if (local_err != NULL) { > > + g_debug(" failed to check disk path, ignoring error: %s", > > + error_get_pretty(local_err)); > > + error_free(local_err); > > + local_err = NULL; > > > + /* Don't try to get the address */ > > + is_virtual = true; > > + } > > + if (!is_virtual) { > > + disk->address = get_disk_address(disk_dir, &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); > > + local_err = NULL; > > + } else if (disk->address != NULL) { > > + disk->has_address = true; > > + } > > + } > > + > > + /* List slave disks */ > > + slaves_dir = g_strdup_printf("%s/slaves", disk_dir); > > + g_debug(" listing entries in: %s", slaves_dir); > > + dp_slaves = opendir(slaves_dir); > > + while ((de_slaves = readdir(dp_slaves)) != NULL) { > > + g_autofree char *slave_dir = NULL; > > + char *slave_dev; > > + strList *slave_item = NULL; > > + > > + if ((strcmp(".", de_slaves->d_name) == 0) || > > + (strcmp("..", de_slaves->d_name) == 0)) { > > > + continue; > > + } > > + > > + /* Add slave disks */ > > + slave_dir = g_strdup_printf("%s/%s", > > + slaves_dir, de_slaves->d_name); > > + slave_dev = get_device_for_syspath(slave_dir); > > + if (slave_dev != NULL) { > > + g_debug(" adding slave device: %s", slave_dev); > > + slave_item = g_new0(strList, 1); > > + slave_item->value = slave_dev; > > + slave_item->next = disk->slaves; > > + disk->slaves = slave_item; > > + } > > + } > > + closedir(dp_slaves); > > + > > + /* > > + * Detect partitions subdirectory name is "<parent><number>" or > > + * "<parent>p<number>" > > + */ > > + dp_disk = opendir(disk_dir); > > + while ((de_disk = readdir(dp_disk)) != NULL) { > > + size_t len; > > + g_autofree char *partition_dir = NULL; > > + > > + if (!(de_disk->d_type & DT_DIR)) { > > + continue; > > + } > > + > > + len = strlen(de->d_name); > > + if (!(strncmp(de->d_name, de_disk->d_name, len) == 0 && > > + ((*(de_disk->d_name + len) == 'p' && > > + isdigit(*(de_disk->d_name + len + 1))) || > > + isdigit(*(de_disk->d_name + len))))) { > > + continue; > > + } > > + > > + partition_dir = g_strdup_printf("%s/%s", > > + disk_dir, de_disk->d_name); > > + partition = g_new0(GuestDiskInfo, 1); > > + partition->name = get_device_for_syspath(partition_dir); > > + partition->partition = true; > > + /* Add parent disk as slave for easier tracking of hierarchy > > */ > > + partition->slaves = g_new0(strList, 1); > > + partition->slaves->value = g_strdup(disk->name); > > + > > + item = g_new0(GuestDiskInfoList, 1); > > + item->value = partition; > > + item->next = ret; > > + ret = item; > > + > > + } > > + closedir(dp_disk); > > + } > > + return ret; > > +} > > + > > +#else > > + > > +GuestDiskInfoList *qmp_guest_get_disks(Error **errp) > > +{ > > + error_setg(errp, QERR_UNSUPPORTED); > > + return NULL; > > +} > > + > > +#endif > > + > > /* 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 +3047,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) { > > @@ -3042,9 +3281,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp) > > > > return info; > > } > > - > > -GuestDiskInfoList *qmp_guest_get_disks(Error **errp) > > -{ > > - error_setg(errp, QERR_UNSUPPORTED); > > - return NULL; > > -} > > -- > > 2.25.0 > > > > > > > > -- > Marc-André Lureau -- Tomáš Golembiovský <tgolembi@redhat.com>
Hi On Tue, Oct 6, 2020 at 12:31 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote: > On Tue, Sep 29, 2020 at 07:22:00PM +0400, Marc-André Lureau wrote: > > > > + if (getline(&line, &n, fp) == -1) { > > > + g_debug(" failed to read disk size"); > > > > > > > line: getline(3) "This buffer should be freed by the user program even if > > getline() failed." > > That is handled by the g_autofree, or am I missing something? `line` > will get out of scope after every interation (even with continue). Or do > > Ah right, I got confused. thanks -- Marc-André Lureau <div dir="ltr"><div>Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 6, 2020 at 12:31 PM Tomáš Golembiovský <<a href="mailto:tgolembi@redhat.com">tgolembi@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Sep 29, 2020 at 07:22:00PM +0400, Marc-André Lureau wrote:<br><br> > > + if (getline(&line, &n, fp) == -1) {<br> > > + g_debug(" failed to read disk size");<br> > ><br> > <br> > line: getline(3) "This buffer should be freed by the user program even if<br> > getline() failed."<br> <br> That is handled by the g_autofree, or am I missing something? `line`<br> will get out of scope after every interation (even with continue). Or do<br><br></blockquote><div> </div><div>Ah right, I got confused.<br></div><div><br></div>thanks<br clear="all"></div><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
On Mon, Sep 07, 2020 at 11:14:41AM +0200, Tomáš Golembiovský wrote: > The command lists all disks (real and virtual) as well as disk > partitions. For each disk the list of slave disks is also listed and > /dev path is used as a handle so it can be matched with "name" filed of > other returned disk entries. For disk partitions the "slave" list is > populated with the the parent device for easier tracking of hierarchy. > > Example output: > { > "return": [ > ... > { > "name": "/dev/dm-0", > "partition": false, > "slaves": [ > "/dev/sda2" > ], > "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7" > }, > { > "name": "/dev/sda2", > "partition": true, > "slaves": [ > "/dev/sda" > ] > }, > { > "name": "/dev/sda", > "partition": false, > "address": { > "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493", > "bus-type": "sata", > ... > "dev": "/dev/sda", > "target": 0 > }, > "slaves": [] > }, > ... > ] > } > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > qga/commands-posix.c | 247 +++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 240 insertions(+), 7 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index f99731af51..3babc25c09 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -62,6 +62,9 @@ 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; > @@ -1150,6 +1153,21 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath, > closedir(dir); > } > > +static bool is_disk_virtual(const char *devpath, Error **errp) > +{ > + g_autofree char *syspath = realpath(devpath, NULL); > + > + if (!syspath) { > + error_setg_errno(errp, errno, "realpath(\"%s\")", devpath); > + return false; > + } > + if (strstr(syspath, "/devices/virtual/block/")) { > + return true; > + } else { > + return false; > + } > +} > + > /* Dispatch to functions for virtual/real device */ > static void build_guest_fsinfo_for_device(char const *devpath, > GuestFilesystemInfo *fs, > @@ -1168,6 +1186,7 @@ static void build_guest_fsinfo_for_device(char const *devpath, > > g_debug(" parse sysfs path '%s'", syspath); > > + /* TODO: use is_disk_virtual() */ > if (strstr(syspath, "/devices/virtual/block/")) { > build_guest_fsinfo_for_virtual_device(syspath, fs, errp); > } else { > @@ -1177,6 +1196,225 @@ static void build_guest_fsinfo_for_device(char const *devpath, > free(syspath); > } > > +#ifdef CONFIG_LIBUDEV > + > +/* > + * Wrapper around build_guest_fsinfo_for_device() for getting just > + * the disk address. > + */ > +static GuestDiskAddress *get_disk_address(const char *syspath, Error **errp) > +{ > + g_autoptr(GuestFilesystemInfo) fs = NULL; > + > + fs = g_new0(GuestFilesystemInfo, 1); > + build_guest_fsinfo_for_device(syspath, fs, errp); > + if (fs->disk != NULL) { > + return g_steal_pointer(&fs->disk->value); > + } > + return NULL; > +} > + > +static char *get_alias_for_syspath(const char *syspath) > +{ > + struct udev *udev = NULL; > + struct udev_device *udevice = NULL; > + char *ret = NULL; > + > + udev = udev_new(); > + udevice = udev_device_new_from_syspath(udev, syspath); > + if (udev == NULL || udevice == NULL) { > + g_debug("failed to query udev"); > + } else { > + const char *alias = udev_device_get_property_value( > + udevice, "DM_NAME"); > + if (alias != NULL && *alias != 0) { > + ret = g_strdup(alias); > + } > + } > + > + udev_unref(udev); > + udev_device_unref(udevice); > + return ret; > +} > + > +static char *get_device_for_syspath(const char *syspath) > +{ > + struct udev *udev = NULL; > + struct udev_device *udevice = NULL; > + char *ret = NULL; > + > + udev = udev_new(); > + udevice = udev_device_new_from_syspath(udev, syspath); > + if (udev == NULL || udevice == NULL) { > + g_debug("failed to query udev"); > + } else { > + ret = g_strdup(udev_device_get_devnode(udevice)); > + } > + udev_unref(udev); > + udev_device_unref(udevice); > + return ret; > +} > + > +GuestDiskInfoList *qmp_guest_get_disks(Error **errp) > +{ > + GuestDiskInfoList *item, *ret = NULL; > + GuestDiskInfo *disk, *partition; > + 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_dir = NULL, *slaves_dir = NULL; > + struct dirent *de_disk, *de_slaves; > + DIR *dp_disk = NULL, *dp_slaves = NULL; > + FILE *fp = NULL; > + size_t n = 0; > + Error *local_err = NULL; > + if (de->d_type != DT_LNK) { > + g_debug(" skipping entry: %s", de->d_name); > + continue; > + } > + > + /* Check size and skip zero-sized disks */ > + g_debug(" checking disk size"); > + size_dir = g_strdup_printf("/sys/block/%s/size", de->d_name); > + fp = fopen(size_dir, "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); These 10 lines can be reduced to just g_file_get_contents(size_dir, &line, NULL, NULL) > + if (strcmp(line, "0\n") == 0) { > + g_debug(" skipping zero-sized disk"); > + continue; > + } > + > + g_debug(" adding %s", de->d_name); > + disk_dir = g_strdup_printf("/sys/block/%s", de->d_name); > + disk = g_new0(GuestDiskInfo, 1); > + disk->name = get_device_for_syspath(disk_dir); > + disk->partition = false; > + disk->alias = get_alias_for_syspath(disk_dir); > + if (disk->alias != NULL) { > + disk->has_alias = true; > + } > + item = g_new0(GuestDiskInfoList, 1); > + item->value = disk; > + item->next = ret; > + ret = item; > + > + /* Get address for non-virtual devices */ > + bool is_virtual = is_disk_virtual(disk_dir, &local_err); > + if (local_err != NULL) { > + g_debug(" failed to check disk path, ignoring error: %s", > + error_get_pretty(local_err)); > + error_free(local_err); > + local_err = NULL; > + /* Don't try to get the address */ > + is_virtual = true; > + } > + if (!is_virtual) { > + disk->address = get_disk_address(disk_dir, &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); > + local_err = NULL; > + } else if (disk->address != NULL) { > + disk->has_address = true; > + } > + } > + > + /* List slave disks */ > + slaves_dir = g_strdup_printf("%s/slaves", disk_dir); > + g_debug(" listing entries in: %s", slaves_dir); > + dp_slaves = opendir(slaves_dir); > + while ((de_slaves = readdir(dp_slaves)) != NULL) { > + g_autofree char *slave_dir = NULL; > + char *slave_dev; > + strList *slave_item = NULL; > + > + if ((strcmp(".", de_slaves->d_name) == 0) || > + (strcmp("..", de_slaves->d_name) == 0)) { > + continue; > + } > + > + /* Add slave disks */ > + slave_dir = g_strdup_printf("%s/%s", > + slaves_dir, de_slaves->d_name); > + slave_dev = get_device_for_syspath(slave_dir); > + if (slave_dev != NULL) { > + g_debug(" adding slave device: %s", slave_dev); > + slave_item = g_new0(strList, 1); > + slave_item->value = slave_dev; > + slave_item->next = disk->slaves; > + disk->slaves = slave_item; > + } > + } > + closedir(dp_slaves); Since you are only using d_name, you can use g_dir_open and g_dir_read_name and g_dir_close. This always skips . and .. for you. > + > + /* > + * Detect partitions subdirectory name is "<parent><number>" or > + * "<parent>p<number>" > + */ > + dp_disk = opendir(disk_dir); > + while ((de_disk = readdir(dp_disk)) != NULL) { > + size_t len; > + g_autofree char *partition_dir = NULL; > + > + if (!(de_disk->d_type & DT_DIR)) { > + continue; > + } > + > + len = strlen(de->d_name); > + if (!(strncmp(de->d_name, de_disk->d_name, len) == 0 && > + ((*(de_disk->d_name + len) == 'p' && > + isdigit(*(de_disk->d_name + len + 1))) || > + isdigit(*(de_disk->d_name + len))))) { > + continue; > + } > + > + partition_dir = g_strdup_printf("%s/%s", > + disk_dir, de_disk->d_name); > + partition = g_new0(GuestDiskInfo, 1); > + partition->name = get_device_for_syspath(partition_dir); > + partition->partition = true; > + /* Add parent disk as slave for easier tracking of hierarchy */ > + partition->slaves = g_new0(strList, 1); > + partition->slaves->value = g_strdup(disk->name); > + > + item = g_new0(GuestDiskInfoList, 1); > + item->value = partition; > + item->next = ret; > + ret = item; > + > + } > + closedir(dp_disk); > + } > + return ret; > +} > + > +#else > + > +GuestDiskInfoList *qmp_guest_get_disks(Error **errp) > +{ > + error_setg(errp, QERR_UNSUPPORTED); > + return NULL; > +} > + > +#endif > + > /* 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 +3047,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) { > @@ -3042,9 +3281,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp) > > return info; > } > - > -GuestDiskInfoList *qmp_guest_get_disks(Error **errp) > -{ > - error_setg(errp, QERR_UNSUPPORTED); > - return NULL; > -} > -- > 2.25.0 > > Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
diff --git a/qga/commands-posix.c b/qga/commands-posix.c index f99731af51..3babc25c09 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -62,6 +62,9 @@ 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; @@ -1150,6 +1153,21 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath, closedir(dir); } +static bool is_disk_virtual(const char *devpath, Error **errp) +{ + g_autofree char *syspath = realpath(devpath, NULL); + + if (!syspath) { + error_setg_errno(errp, errno, "realpath(\"%s\")", devpath); + return false; + } + if (strstr(syspath, "/devices/virtual/block/")) { + return true; + } else { + return false; + } +} + /* Dispatch to functions for virtual/real device */ static void build_guest_fsinfo_for_device(char const *devpath, GuestFilesystemInfo *fs, @@ -1168,6 +1186,7 @@ static void build_guest_fsinfo_for_device(char const *devpath, g_debug(" parse sysfs path '%s'", syspath); + /* TODO: use is_disk_virtual() */ if (strstr(syspath, "/devices/virtual/block/")) { build_guest_fsinfo_for_virtual_device(syspath, fs, errp); } else { @@ -1177,6 +1196,225 @@ static void build_guest_fsinfo_for_device(char const *devpath, free(syspath); } +#ifdef CONFIG_LIBUDEV + +/* + * Wrapper around build_guest_fsinfo_for_device() for getting just + * the disk address. + */ +static GuestDiskAddress *get_disk_address(const char *syspath, Error **errp) +{ + g_autoptr(GuestFilesystemInfo) fs = NULL; + + fs = g_new0(GuestFilesystemInfo, 1); + build_guest_fsinfo_for_device(syspath, fs, errp); + if (fs->disk != NULL) { + return g_steal_pointer(&fs->disk->value); + } + return NULL; +} + +static char *get_alias_for_syspath(const char *syspath) +{ + struct udev *udev = NULL; + struct udev_device *udevice = NULL; + char *ret = NULL; + + udev = udev_new(); + udevice = udev_device_new_from_syspath(udev, syspath); + if (udev == NULL || udevice == NULL) { + g_debug("failed to query udev"); + } else { + const char *alias = udev_device_get_property_value( + udevice, "DM_NAME"); + if (alias != NULL && *alias != 0) { + ret = g_strdup(alias); + } + } + + udev_unref(udev); + udev_device_unref(udevice); + return ret; +} + +static char *get_device_for_syspath(const char *syspath) +{ + struct udev *udev = NULL; + struct udev_device *udevice = NULL; + char *ret = NULL; + + udev = udev_new(); + udevice = udev_device_new_from_syspath(udev, syspath); + if (udev == NULL || udevice == NULL) { + g_debug("failed to query udev"); + } else { + ret = g_strdup(udev_device_get_devnode(udevice)); + } + udev_unref(udev); + udev_device_unref(udevice); + return ret; +} + +GuestDiskInfoList *qmp_guest_get_disks(Error **errp) +{ + GuestDiskInfoList *item, *ret = NULL; + GuestDiskInfo *disk, *partition; + 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_dir = NULL, *slaves_dir = NULL; + struct dirent *de_disk, *de_slaves; + DIR *dp_disk = NULL, *dp_slaves = NULL; + FILE *fp = NULL; + size_t n = 0; + Error *local_err = NULL; + if (de->d_type != DT_LNK) { + g_debug(" skipping entry: %s", de->d_name); + continue; + } + + /* Check size and skip zero-sized disks */ + g_debug(" checking disk size"); + size_dir = g_strdup_printf("/sys/block/%s/size", de->d_name); + fp = fopen(size_dir, "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; + } + + g_debug(" adding %s", de->d_name); + disk_dir = g_strdup_printf("/sys/block/%s", de->d_name); + disk = g_new0(GuestDiskInfo, 1); + disk->name = get_device_for_syspath(disk_dir); + disk->partition = false; + disk->alias = get_alias_for_syspath(disk_dir); + if (disk->alias != NULL) { + disk->has_alias = true; + } + item = g_new0(GuestDiskInfoList, 1); + item->value = disk; + item->next = ret; + ret = item; + + /* Get address for non-virtual devices */ + bool is_virtual = is_disk_virtual(disk_dir, &local_err); + if (local_err != NULL) { + g_debug(" failed to check disk path, ignoring error: %s", + error_get_pretty(local_err)); + error_free(local_err); + local_err = NULL; + /* Don't try to get the address */ + is_virtual = true; + } + if (!is_virtual) { + disk->address = get_disk_address(disk_dir, &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); + local_err = NULL; + } else if (disk->address != NULL) { + disk->has_address = true; + } + } + + /* List slave disks */ + slaves_dir = g_strdup_printf("%s/slaves", disk_dir); + g_debug(" listing entries in: %s", slaves_dir); + dp_slaves = opendir(slaves_dir); + while ((de_slaves = readdir(dp_slaves)) != NULL) { + g_autofree char *slave_dir = NULL; + char *slave_dev; + strList *slave_item = NULL; + + if ((strcmp(".", de_slaves->d_name) == 0) || + (strcmp("..", de_slaves->d_name) == 0)) { + continue; + } + + /* Add slave disks */ + slave_dir = g_strdup_printf("%s/%s", + slaves_dir, de_slaves->d_name); + slave_dev = get_device_for_syspath(slave_dir); + if (slave_dev != NULL) { + g_debug(" adding slave device: %s", slave_dev); + slave_item = g_new0(strList, 1); + slave_item->value = slave_dev; + slave_item->next = disk->slaves; + disk->slaves = slave_item; + } + } + closedir(dp_slaves); + + /* + * Detect partitions subdirectory name is "<parent><number>" or + * "<parent>p<number>" + */ + dp_disk = opendir(disk_dir); + while ((de_disk = readdir(dp_disk)) != NULL) { + size_t len; + g_autofree char *partition_dir = NULL; + + if (!(de_disk->d_type & DT_DIR)) { + continue; + } + + len = strlen(de->d_name); + if (!(strncmp(de->d_name, de_disk->d_name, len) == 0 && + ((*(de_disk->d_name + len) == 'p' && + isdigit(*(de_disk->d_name + len + 1))) || + isdigit(*(de_disk->d_name + len))))) { + continue; + } + + partition_dir = g_strdup_printf("%s/%s", + disk_dir, de_disk->d_name); + partition = g_new0(GuestDiskInfo, 1); + partition->name = get_device_for_syspath(partition_dir); + partition->partition = true; + /* Add parent disk as slave for easier tracking of hierarchy */ + partition->slaves = g_new0(strList, 1); + partition->slaves->value = g_strdup(disk->name); + + item = g_new0(GuestDiskInfoList, 1); + item->value = partition; + item->next = ret; + ret = item; + + } + closedir(dp_disk); + } + return ret; +} + +#else + +GuestDiskInfoList *qmp_guest_get_disks(Error **errp) +{ + error_setg(errp, QERR_UNSUPPORTED); + return NULL; +} + +#endif + /* 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 +3047,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) { @@ -3042,9 +3281,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp) return info; } - -GuestDiskInfoList *qmp_guest_get_disks(Error **errp) -{ - error_setg(errp, QERR_UNSUPPORTED); - return NULL; -}
The command lists all disks (real and virtual) as well as disk partitions. For each disk the list of slave disks is also listed and /dev path is used as a handle so it can be matched with "name" filed of other returned disk entries. For disk partitions the "slave" list is populated with the the parent device for easier tracking of hierarchy. Example output: { "return": [ ... { "name": "/dev/dm-0", "partition": false, "slaves": [ "/dev/sda2" ], "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7" }, { "name": "/dev/sda2", "partition": true, "slaves": [ "/dev/sda" ] }, { "name": "/dev/sda", "partition": false, "address": { "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493", "bus-type": "sata", ... "dev": "/dev/sda", "target": 0 }, "slaves": [] }, ... ] } Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- qga/commands-posix.c | 247 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 240 insertions(+), 7 deletions(-)