Hi Eric,
Thank you for the review.
On 6/19/14 17:23 , "Eric Blake" <ebl...@redhat.com> 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.
>>
>> 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.
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 = 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 '+'
OK, I will fix these, and also other binary operators.
>> + 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?
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++] = 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?
Again, this is inside '#if defined(__linux__)', so it won't be compiled
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.
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;
>> + }
<snip>
>>
>> +
>> +/* 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
