From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55697) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wy6Dx-0006FE-Jw for qemu-devel@nongnu.org; Fri, 20 Jun 2014 17:19:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wy6Dr-00068R-SL for qemu-devel@nongnu.org; Fri, 20 Jun 2014 17:19:53 -0400 Received: from usindpps05.hds.com ([207.126.252.18]:39668) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wy6Dr-00068D-MF for qemu-devel@nongnu.org; Fri, 20 Jun 2014 17:19:47 -0400 From: Tomoki Sekiyama Date: Fri, 20 Jun 2014 21:19:41 +0000 Message-ID: References: <20140605145714.10787.97053.stgit@hds.com> <20140605145734.10787.28959.stgit@hds.com> <53A354D5.1030506@redhat.com> <53A45171.1080406@redhat.com> In-Reply-To: <53A45171.1080406@redhat.com> Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-ID: <9DDBAA950E621F4788AAD3591BFCF63A@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" On 6/20/14 11:21 , "Eric Blake" wrote: >On 06/19/2014 06:34 PM, Tomoki Sekiyama wrote: > >>>> + } >>>> + 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. >>=20 >> This function is guarded by '#if defined(__linux__)' (and also >> '#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)' ), >> so I believe it is ok here. > >I take it the function gracefully fails on non-Linux. But that's not >very nice to management functions - they learn that the function exists, >but have to call it to see if it actually works. It might be nicer to >conditionally expose the command only if it will work, so that querying >the list of commands makes it obvious whether the agent supports subset >freezing. I think hiding unsupported commands from 'guest-info' is reasonable. Currently the list is auto-generated from qapi-schema.json and all commands are listed even though some of them just returns QERR_ UNSUPPORTED on some platforms(like guest-fstrim and guest-network- get-interfaces etc. on non-Linux platform). We may need a new mechanism to unregister them from the list, or to mark them "unsupported", or to extend the generator so that we can omit them from the list on unsupported platforms. (And that should be done in another patch series.) >>>> + 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. >>=20 >> 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? > >As I said, the cost of properly parsing a row of integers explodes into >so much more code that I'm just fine looking the other way if you want >to use sscanf for compactness - after all, this is a kernel file we're >supposed to be reading, and if that interface has been corrupted to give >us bogus data that doesn't parse correctly, then we're probably already >hurting in other ways. Right... then I'm going to keep sscanf here then. Thanks, Tomoki Sekiyama