From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51351) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wxmn0-0002PF-T5 for qemu-devel@nongnu.org; Thu, 19 Jun 2014 20:34:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wxmmv-0003gU-36 for qemu-devel@nongnu.org; Thu, 19 Jun 2014 20:34:46 -0400 Received: from usindpps04.hds.com ([207.126.252.17]:57030) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wxmmu-0003gQ-S3 for qemu-devel@nongnu.org; Thu, 19 Jun 2014 20:34:41 -0400 From: Tomoki Sekiyama Date: Fri, 20 Jun 2014 00:34:30 +0000 Message-ID: References: <20140605145714.10787.97053.stgit@hds.com> <20140605145734.10787.28959.stgit@hds.com> <53A354D5.1030506@redhat.com> In-Reply-To: <53A354D5.1030506@redhat.com> Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-ID: <0E213413B71ACE4FA60041E556D34016@hds.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Eric Blake , "qemu-devel@nongnu.org" Cc: Mitsuhiro Tanino , "mdroth@linux.vnet.ibm.com" Hi Eric, Thank you for the review. On 6/19/14 17:23 , "Eric Blake" wrote: >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 >>*devminor) >> +{ >> + 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. This function is guarded by '#if defined(__linux__)' (and also '#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)' ), so I believe it is ok here. >> + >> +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' && name= [i+3] >>=3D=3D '0') { > >Spaces around binary '+' OK, I will fix these, and also other binary operators. >> + name[j++] =3D ' '; >> + i +=3D 3; >> + } else if (name[i+1] =3D=3D '0' && name[i+2] =3D=3D '1' && name= [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' && name= [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' && name= [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? I will replace this with more generic that covers every three octal digits (\000 - \377). Mount names only escape above characters, but anyway it is harmless to make it more generic. >> + 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? Again, this is inside '#if defined(__linux__)', so it won't be compiled 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, >>&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. Hmm, '%*u' can be simply replaced by '%*s' then. For '%u:%u', maybe we can catch this part with '%s' or '%n%*s%n' and convert them into integers later by strtoul(). Does it sound good to you? >> + &dev_s, &dev_e, &check); >> + if (ret < 3) { >> + continue; >> + } >> >> + >> +/* 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. >> +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? I'll rename this series of functions to: __build_guest_fsinfo -> build_guest_fsinfo_for_real_device __build_guest_fsinfo_virtual -> build_guest_fsinfo_for_virtual_device _build_guest_fsinfo -> build_guest_fsinfo_for_device build_guest_fsinfo -> build_guest_fsinfo (unchanged) Thanks, Tomoki Sekiyama