From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59541) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bEFgl-0005yD-IK for qemu-devel@nongnu.org; Sat, 18 Jun 2016 08:49:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bEFgf-0002Dk-Ji for qemu-devel@nongnu.org; Sat, 18 Jun 2016 08:49:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41978) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bEFgf-0002Dg-Am for qemu-devel@nongnu.org; Sat, 18 Jun 2016 08:49:21 -0400 References: <1465745921-22733-1-git-send-email-lma@suse.com> <9212f90f-7960-5eeb-235d-4ada8a251aa1@redhat.com> <576422DB020000620007AFA0@prv-mh.provo.novell.com> From: Max Reitz Message-ID: Date: Sat, 18 Jun 2016 14:49:16 +0200 MIME-Version: 1.0 In-Reply-To: <576422DB020000620007AFA0@prv-mh.provo.novell.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="bC149AgiRON3c9XnTvsMgh9emBBVAsvaR" Subject: Re: [Qemu-devel] =?utf-8?b?562U5aSN77yaIFJlOiAgW1BBVENIXSBTaG93IGFs?= =?utf-8?q?l_of_snapshot_info_on_every_block_device_in_output_of_=27info_s?= =?utf-8?q?napshots=27?= List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Lin Ma Cc: qemu-devel@nongnu.org, kwolf@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --bC149AgiRON3c9XnTvsMgh9emBBVAsvaR From: Max Reitz To: Lin Ma Cc: qemu-devel@nongnu.org, kwolf@redhat.com Message-ID: Subject: =?UTF-8?B?UmU6IOetlOWkje+8miBSZTogW1FlbXUtZGV2ZWxdIFtQQVRDSF0gU2hv?= =?UTF-8?Q?w_all_of_snapshot_info_on_every_block_device_in_output_of_'info_s?= =?UTF-8?Q?napshots'?= References: <1465745921-22733-1-git-send-email-lma@suse.com> <9212f90f-7960-5eeb-235d-4ada8a251aa1@redhat.com> <576422DB020000620007AFA0@prv-mh.provo.novell.com> In-Reply-To: <576422DB020000620007AFA0@prv-mh.provo.novell.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 17.06.2016 10:18, Lin Ma wrote: >=20 >=20 >>>> Max Reitz mreitz@redhat.com> 2016/6/15 =E6=98=9F=E6=9C=9F=E4=B8=89 =E4= =B8=8A=E5=8D=88 1:43 >> > 2016/6/15 =E6=98=9F=E6=9C=9F=E4=B8=89 =E4=B8= =8A=E5=8D=88 1:43 >>> > ...... >>I have many comments, but don't worry, it's nothing that can't be fixed= =2E >>The overall design looks good to me. > Thank you so much for reviewing the patch very carefully and gave me so= many > comments. I would take most of your comments but except some of below: >=20 > ...... >>Nit pick: The following code will always leave an empty line after >>everything. I think that's superfluous, and it can be amended as follow= s >>(if you want to amend it, that is; if you really like that empty line, >>then feel free to disregard my suggestion): >> >>> + monitor_printf(mon, "\n"); >> >>Drop this. >> >>> + QTAILQ_FOREACH(image_entry, &image_list, next) { >>> + if (QTAILQ_EMPTY(&image_entry->snapshots)) { >>> + continue; >>> + } >> >>Put monitor_printf(mon, "\n"); here. > OK. >=20 >>> + monitor_printf(mon, "List of partial (non-loadable) > snapshots on '%s':", >>> + image_entry->imagename); >>> + monitor_printf(mon, "\n"); >> >>(Why did you not concatenate these two strings in a single >>monitor_printf() call?) > OK. >=20 >>> + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NU= LL); >>> + monitor_printf(mon, "\n"); >> >>Drop this. >> >>> + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next= ) { >> >>Put monitor_printf(mon, "\n"); here. > If so, It causes the output looks like this: > -FROM: > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLOC= K > 3 snapb 0 2016-06-16 17:37:25 00:00:00.00= 0 > 4 snapc 0 2016-06-16 17:37:30 00:00:00.00= 0 > 5 snap2 0 2016-06-16 17:37:34 00:00:00.00= 0 > (qemu) > -TO: > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLOC= K > 3 snapb 0 2016-06-16 17:37:25 00:00:00.00= 0 > =20 > 4 snapc 0 2016-06-16 17:37:30 00:00:00.00= 0 > =20 > 5 snap2 0 2016-06-16 17:37:34 00:00:00.00= 0 > (qemu) > =20 > So I'll keep the code. > =20 >>> + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon= , >>> + snapshot_entry->sn); >>> + monitor_printf(mon, "\n"); >> >>And drop this. Again, the suggestions on moving the >>monitor_printf(mon, "\n"); calls around are just suggestions, and it's >>up to you whether you want to follow them or not. > If so, It causes the output looks like this: > -FROM: > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLOC= K > 3 snapb 0 2016-06-16 17:37:25 00:00:00.00= 0 > 4 snapc 0 2016-06-16 17:37:30 00:00:00.00= 0 > 5 snap2 0 2016-06-16 17:37:34 00:00:00.00= 0 > (qemu) > -TO: > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLOC= K > 3 snapb 0 2016-06-16 17:37:25 00:00:00.000= 4 snapc 0 2016-06-16 17:37:30 00:00:00.0005 = snap2 0 2016-06-16 17:37:34 =20 > 00:00:00.000(qemu) > =20 > So I'll keep the code. Well, the idea was to do all of the suggestions, and then these two would counteract each other. However, I just noticed that I was completely wrong about my nit pick anyway. The code won't leave an empty line after printing everything, I made a mistake there. My suggestion instead leads to not having an end-of-line after everything, which is definitely wrong (sorry!). So you should probably leave all the monitor_printf(mon, "\n") statements as they are, except the one where I asked about concatenating it with the previous one. Max --bC149AgiRON3c9XnTvsMgh9emBBVAsvaR Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXZUNNAAoJEDuxQgLoOKytL30IAIRGt2Gb35rlOqYzvcxqTDRq dr25Va8PVXlZ6boWzlLTybSVAZW2KA2uusIwKhNmU+8UTO962VtZ836aoiG27SrF 7Rk5hfJ6xC0c+Szpq0fWNVA+IOfnbXU1iTyjCzXV60GBGaRiEcTeYYqOLiYKaV7e d9WqodJzSLDppBzLqG+EyxM5uHIdzF1vzAouPc4NDGu0BVcs6WHvYkKW4Bqo0fuu jcu+j3eVligmauur5luUplyCwYRYOVCLztByPm/wKuYpj5a0pkI/rWOT4hn2+en+ rpqZS4u9KB1cr4D6YWdMNA507PxbBqU7XTfh99R0s8z9LnN3YCV/26BJK9OV+NA= =Ggsi -----END PGP SIGNATURE----- --bC149AgiRON3c9XnTvsMgh9emBBVAsvaR--