From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41844) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V5NXJ-0002y2-7u for qemu-devel@nongnu.org; Fri, 02 Aug 2013 18:09:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V5NXE-000735-6a for qemu-devel@nongnu.org; Fri, 02 Aug 2013 18:09:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37880) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V5NXD-00072q-Uo for qemu-devel@nongnu.org; Fri, 02 Aug 2013 18:09:20 -0400 Message-ID: <51FC2E0A.102@redhat.com> Date: Fri, 02 Aug 2013 16:09:14 -0600 From: Eric Blake MIME-Version: 1.0 References: <1375434137-4452-1-git-send-email-gesaint@linux.vnet.ibm.com> <1375434137-4452-2-git-send-email-gesaint@linux.vnet.ibm.com> In-Reply-To: <1375434137-4452-2-git-send-email-gesaint@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="OJ04fCulewAPmXBjtPU3HMaVxMv5HNWmM" Subject: Re: [Qemu-devel] [PATCH V5 1/6] block/qemu-img: Refine and export infinite loop checking in collect_image_info_list() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xu Wang Cc: kwolf@redhat.com, famz@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, wdongxu@linux.vnet.ibm.com, Xu Wang , xiawenc@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --OJ04fCulewAPmXBjtPU3HMaVxMv5HNWmM Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 08/02/2013 03:02 AM, Xu Wang wrote: > From: Xu Wang >=20 > If there is a loop exists in the backing file chain, many problems > could be caused by it, such as no response and segment fault during > system boot. Hence stopping backing file loop appear is very necessary.= > This patch refine and export loop checking function from collect_image_= > info_list() to block.c and build a independent function named bdrv_ > backing_file_loop_check(). Are we trying to get this series into 1.6 on the grounds that it fixes bugs? The grammar was a bit awkward; may I suggest: If there is a loop in the backing file chain, it could cause problems such as no response or a segfault during system boot. Hence detecting a backing file loop is necessary. This patch extracts the loop check from collect_image_info_list() in block.c into an independent function bdrv_backing_file_loop_check(). >=20 > Signed-off-by: Xu Wang > --- > block.c | 96 +++++++++++++++++++++++++++++++++++++++++++= ++++++++ > include/block/block.h | 4 +++ > qemu-img.c | 44 ++++++++++------------- > 3 files changed, 118 insertions(+), 26 deletions(-) >=20 > =20 > +static gboolean str_equal_func(gconstpointer a, gconstpointer b) > +{ > + return strcmp(a, b) =3D=3D 0; > +} > + > +/** > + * Check backing file chain if there is a loop in it. > + * > + * @filename: topmost image filename, absolute or relative path is OK.= > + * @fmt: topmost image format (may be NULL to autodetect) > + * @backing_file: if this value is set, @filename inode (-1 if it does= n't > + * exist) will insert into hash table directly. Loop ch= eck > + * starts from it. Absolute or relative path is OK for = it. This is confusing - are we passing in a filename or an inode? At the parameter level, it doesn't matter what we hash or even that we have a hash table - just what we pass in. The choice of hashing an inode value is an internal detail. > + * @backing_format: format of backing file Why do we need to pass in two filenames? I'm guessing it's because you want to detect TWO types of loops: Loop 1 - we are creating a NEW file, but requesting a backing file name - if anything in that backing file chain points to the file we are about to create, we would be creating a loop, and want to fail. That is, if we have an existing file "B" that points to a missing backing file "A", and we are now creating file "A" with a backing file of "B", we want the creation to fail because it would create a loop. But in this case, it is sufficient to note that "B" is a broken image (since "A" doesn't exist yet), so we don't have to do a loop check here, rather we can just merely test if "B" and its entire backing chain is reachable. Loop 2 - we are testing if an EXISTING file has a loop. But the loop may not necessarily point back to our own file. That is, a file "A" with metadata that claims a backing file of "A" is a loop. Also, a file "A" with metadata that claims "B" as backing, and existing "B" with metadata that claims "A" as backing, is a loop. But we ALSO want to detect and reject a file "A" that claims "B" as backing, where an existing file "B" claims itself as backing; or even "A" claims "B", "B" claims "C", and "C" claims "B". Thus, we need to detect the loop no matter where in the chain it occurs, and realize that it does not necessarily point back to "A". So, why not just declare that we are starting with "A", without regards to it's backing file? That is, I think your function signature is over-engineered - you really only need to pass in a single file name - that of an existing file, and have the file report success if all backing files can be resolved without hitting any loops in the resolution, and failure if any backing file is missing or if any backing file refers back to a point earlier in the chain. > + * > + * Returns: true for backing file loop or error happened, false for no= loop. > + */ > +bool bdrv_backing_file_loop_check(const char *filename, const char *fm= t, Those return semantics are a bit unusual. A function named _check() that returns a bool usually returns true when the function passed. How will this function usually be called? Let's name it something that will avoid double-negatives; maybe: bool bdrv_backing_chain_okay(const char *filename, const char *fmt) which returns true if filename is safe to use as a backing chain. > + const char *backing_file, > + const char *backing_format) { > + GHashTable *inodes; > + BlockDriverState *bs; > + BlockDriver *drv; > + struct stat sbuf; > + long inode =3D 0; You are not guaranteed that st_ino fits in a long; use ino_t. (IIRC, on 32-bit Cygwin, ino_t is 64-bit, but long is 32-bit) > + int ret; > + char fbuf[1024]; > + > + inodes =3D g_hash_table_new_full(g_str_hash, str_equal_func, NULL,= NULL); So this says you are comparing by name (you are doing strcmp on the hashed data)... > + > + if (backing_file) { > + /* Check if file exists. */ > + if (access(filename, F_OK)) { > + inode =3D -1; > + } else { > + if (stat(filename, &sbuf) =3D=3D -1) { > + error_report("Get file %s stat failed.", filename); > + goto err; > + } > + inode =3D (long)sbuf.st_ino; > + } > + > + filename =3D backing_file; > + fmt =3D backing_format; > + g_hash_table_insert(inodes, (gpointer)&inode, NULL); =2E..but here, you are shoving in integers cast as a name. strcmp() on integers will not work. Furthermore, inode alone is not guaranteed to be unique; you are only guaranteed that the combination of dev_t and ino_t form a unique identifier for any file (that is, you are prone to false collisions if two files on different devices happen to have the same inode). I want to see loop detection working, but this patch still needs an overhaul before it can be considered working. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --OJ04fCulewAPmXBjtPU3HMaVxMv5HNWmM 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.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJR/C4KAAoJEKeha0olJ0Nq/EcH+wfNWV1VeGbycH7mmV0cEE+T A/NiBQGZ8OkVMcw7m9qlMh8LoUf1Uku+7lR2f62yplTfuta/hVBYT8rypYp47IU+ Og5h3YHOpO3TaIrMkuwiJIzplOPoixzUs1gTXSYTFxj9NRa6g2+gfki34kjtBDvU Rqjpdn/MdfaC9oYD4lv07eQI00JczyA5Xi2whOhrNnmrdS//atf6hD4/ORIeG0Hm +mS0xV4+nczKTnQ2ylSWhg48sSrQ77ZPYkYmG/8RrQLYnrCV6Zlvacso/+RVf45j ARviR+H8GAqAkF6Sx81RnTU2Esp7VvlSnpH64vQs6pnI9CoSoBRPUHGcT7h3EhU= =Fnpo -----END PGP SIGNATURE----- --OJ04fCulewAPmXBjtPU3HMaVxMv5HNWmM--