From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42337) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wxjo9-0001XS-So for qemu-devel@nongnu.org; Thu, 19 Jun 2014 17:23:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wxjo5-00015F-H4 for qemu-devel@nongnu.org; Thu, 19 Jun 2014 17:23:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51062) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wxjo5-000158-8n for qemu-devel@nongnu.org; Thu, 19 Jun 2014 17:23:41 -0400 Message-ID: <53A354D5.1030506@redhat.com> Date: Thu, 19 Jun 2014 15:23:33 -0600 From: Eric Blake MIME-Version: 1.0 References: <20140605145714.10787.97053.stgit@hds.com> <20140605145734.10787.28959.stgit@hds.com> In-Reply-To: <20140605145734.10787.28959.stgit@hds.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JohGNciXh9FHjq96pIMrmxVOIVceshd1H" Subject: Re: [Qemu-devel] [PATCH v4 2/2] qga: Add guest-get-fsinfo command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Tomoki Sekiyama , qemu-devel@nongnu.org Cc: mitsuhiro.tanino@hds.com, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --JohGNciXh9FHjq96pIMrmxVOIVceshd1H Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > For example, when `lsblk' result is: >=20 > 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. >=20 > Signed-off-by: Tomoki Sekiyama > --- > qga/commands-posix.c | 422 ++++++++++++++++++++++++++++++++++++++++++= ++++++++ > qga/commands-win32.c | 6 + > qga/qapi-schema.json | 79 +++++++++ > 3 files changed, 506 insertions(+), 1 deletion(-) >=20 > +static int dev_major_minor(const char *devpath, > + unsigned int *devmajor, unsigned int *devmi= nor) > +{ > + struct stat st; > + > + *devmajor =3D 0; > + *devminor =3D 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 =3D major(st.st_rdev); > + *devminor =3D 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 =3D 0; > + for (i =3D 0; i <=3D len; i++) { > + if (name[i] !=3D '\\') { > + name[j++] =3D name[i]; > + } else if (name[i+1] =3D=3D '\\') { > + name[j++] =3D '\\'; > + i++; > + } else if (name[i+1] =3D=3D '0' && name[i+2] =3D=3D '4' && nam= e[i+3] =3D=3D '0') { Spaces around binary '+' > + name[j++] =3D ' '; > + i +=3D 3; > + } else if (name[i+1] =3D=3D '0' && name[i+2] =3D=3D '1' && nam= e[i+3] =3D=3D '1') { > + name[j++] =3D '\t'; > + i +=3D 3; > + } else if (name[i+1] =3D=3D '0' && name[i+2] =3D=3D '1' && nam= e[i+3] =3D=3D '2') { > + name[j++] =3D '\n'; > + i +=3D 3; > + } else if (name[i+1] =3D=3D '1' && name[i+2] =3D=3D '3' && nam= e[i+3] =3D=3D '4') { > + name[j++] =3D '\\'; > + i +=3D 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++] =3D name[i]; > + } > + } > +} > + > +static void build_fs_mount_list(FsMountList *mounts, Error **errp) > +{ > + FsMount *mount; > + char const *mountinfo =3D "/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 =3D 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 =3D fopen(mountinfo, "r"); > + if (!fp) { > + build_fs_mount_list_from_mtab(mounts, errp); > + return; > + } > + > + while (getline(&line, &n, fp) !=3D -1) { > + ret =3D 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, &t= ype_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] =3D 0; > + line[type_e] =3D 0; > + line[dev_e] =3D 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 =3D=3D 0) { > + /* btrfs reports major number =3D 0 */ > + if (strcmp("btrfs", line+type_s) !=3D 0 || > + dev_major_minor(line+dev_s, &devmajor, &devminor) < 0)= { > + continue; > + } > + } > + > + mount =3D g_malloc0(sizeof(FsMount)); > + mount->dirname =3D g_strdup(line+dir_s); > + mount->devtype =3D 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 =3D 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? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --JohGNciXh9FHjq96pIMrmxVOIVceshd1H Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTo1TVAAoJEKeha0olJ0Nq4DcIAKX3doz1M0bDDRB1ue1v5vjQ Kc/yozl748FHVCH8QqAVEAw1SOka4CGz9PKE7/JLS56QSaXCaUMNh/NHNkHDEn3w 6ImPi5cdDauk3lKgTmAz6fHSrBh9LTXTvl+xVPTVakCX+qHXblg34cRHWfw3lI4F XY3+WPOSO9M3R7wD7xdYrV/y+hJWYc5lHGE5X1UGpobgquTI6Xf2kqw6tyBW1jiJ Fap0ebqz3iqczN3OwKOO0C5q/5j42Q/rjes1iiaSLy+LSV5KsiIi2puw5vyC5Hq/ 1yEzCHZf8fBUoJz/KQ9/3uyCk8Q4iq39Yw7wVrPOqMHTBFKvZzSjlkQ4kpu9QOk= =bUQr -----END PGP SIGNATURE----- --JohGNciXh9FHjq96pIMrmxVOIVceshd1H--