On 06/05/2014 08:57 AM, Tomoki Sekiyama wrote: > Add command to get mounted filesystems information in the guest. > The returned value contains a list of mountpoint paths and > corresponding disks info such as disk bus type, drive address, > and the disk controllers' PCI addresses, so that management layer > such as libvirt can resolve the disk backends. > > For example, when `lsblk' result is: <snip cool example>
>
> In Linux guest, the disk information is resolved from sysfs. So far,
> it only supports virtio-blk, virtio-scsi, IDE, SATA, SCSI disks on x86
> hosts, and "disk" parameter may be empty for unsupported disk types.
>
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiy...@hds.com>
> ---
> qga/commands-posix.c | 422
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> qga/commands-win32.c | 6 +
> qga/qapi-schema.json | 79 +++++++++
> 3 files changed, 506 insertions(+), 1 deletion(-)
>
> +static int dev_major_minor(const char *devpath,
> + unsigned int *devmajor, unsigned int *devminor)
> +{
> + struct stat st;
> +
> + *devmajor = 0;
> + *devminor = 0;
> +
> + if (stat(devpath, &st) < 0) {
> + slog("failed to stat device file '%s': %s", devpath,
> strerror(errno));
> + return -1;
> + }
> + if (S_ISDIR(st.st_mode)) {
> + /* It is bind mount */
> + return -2;
> + }
> + if (S_ISBLK(st.st_mode)) {
> + *devmajor = major(st.st_rdev);
> + *devminor = minor(st.st_rdev);
major() and minor() are not POSIX functions. While they work on Linux,
and appear to have BSD origins, I still wonder if you need to be more
careful on guarding their use.
> +
> +static void decode_mntname(char *name, int len)
> +{
> + int i, j = 0;
> + for (i = 0; i <= len; i++) {
> + if (name[i] != '\\') {
> + name[j++] = name[i];
> + } else if (name[i+1] == '\\') {
> + name[j++] = '\\';
> + i++;
> + } else if (name[i+1] == '0' && name[i+2] == '4' && name[i+3] == '0')
> {
Spaces around binary '+'
> + name[j++] = ' ';
> + i += 3;
> + } else if (name[i+1] == '0' && name[i+2] == '1' && name[i+3] == '1')
> {
> + name[j++] = '\t';
> + i += 3;
> + } else if (name[i+1] == '0' && name[i+2] == '1' && name[i+3] == '2')
> {
> + name[j++] = '\n';
> + i += 3;
> + } else if (name[i+1] == '1' && name[i+2] == '3' && name[i+3] == '4')
> {
> + name[j++] = '\\';
> + i += 3;
> + } else {
This loop looks a bit hard-coded, in that it only recognizes certain
escapes. Wouldn't it be more generic to handle ALL instances of \
followed by three octal digits, even if mount names tend not to encode
that many characters as an escape?
> + name[j++] = name[i];
> + }
> + }
> +}
> +
> +static void build_fs_mount_list(FsMountList *mounts, Error **errp)
> +{
> + FsMount *mount;
> + char const *mountinfo = "/proc/self/mountinfo";
The file /proc/self/mountinfo is Linux-specific, but you are in the file
commands-posix.c. Is this function properly guarded to not cause
compilation issues on BSD?
> + FILE *fp;
> + char *line = NULL;
> + size_t n;
> + char check;
> + unsigned int devmajor, devminor;
> + int ret, dir_s, dir_e, type_s, type_e, dev_s, dev_e;
> +
> + fp = fopen(mountinfo, "r");
> + if (!fp) {
> + build_fs_mount_list_from_mtab(mounts, errp);
> + return;
> + }
> +
> + while (getline(&line, &n, fp) != -1) {
> + ret = sscanf(line,
> + "%*u %*u %u:%u %*s %n%*s%n %*s %*s %*s %n%*s%n
> %n%*s%n%c",
> + &devmajor, &devminor, &dir_s, &dir_e, &type_s, &type_e,
I'm not a huge fan of sscanf("%u") - it has undefined behavior on
integer overflow. But the alternative of avoiding sscanf and
open-coding your parser is so much bulkier, that I tend to look the
other way.
> + &dev_s, &dev_e, &check);
> + if (ret < 3) {
> + continue;
> + }
> + line[dir_e] = 0;
> + line[type_e] = 0;
> + line[dev_e] = 0;
> + decode_mntname(line+dir_s, dir_e-dir_s);
> + decode_mntname(line+dev_s, dev_e-dev_s);
Space around binary '+' and '-'
> + if (devmajor == 0) {
> + /* btrfs reports major number = 0 */
> + if (strcmp("btrfs", line+type_s) != 0 ||
> + dev_major_minor(line+dev_s, &devmajor, &devminor) < 0) {
> + continue;
> + }
> + }
> +
> + mount = g_malloc0(sizeof(FsMount));
> + mount->dirname = g_strdup(line+dir_s);
> + mount->devtype = g_strdup(line+type_s);
Space around '+'
> +
> +/* Store disk device info specified by @sysfs into @fs */
> +static void __build_guest_fsinfo(char const *syspath,
> + GuestFilesystemInfo *fs, Error **errp)
Naming functions with leading __ is dangerous - it risks conflicts with
system headers. This function is static, so you don't need to munge the
name.
> +
> + driver = get_pci_driver(syspath, (p+12+pcilen)-syspath, errp);
Spaces around operators (I'll quit pointing it out).
> +static void _build_guest_fsinfo(char const *dirpath,
> + GuestFilesystemInfo *fs, Error **errp)
Having both __build_guest_fsinfo and _build_guest_fsinfo in the same
file is confusing. Can you come up with better names?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
