From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:60215) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RTssL-0000T3-JC for qemu-devel@nongnu.org; Fri, 25 Nov 2011 05:19:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RTssJ-0001tG-95 for qemu-devel@nongnu.org; Fri, 25 Nov 2011 05:19:21 -0500 Received: from mail-vx0-f173.google.com ([209.85.220.173]:57297) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RTssJ-0001sw-37 for qemu-devel@nongnu.org; Fri, 25 Nov 2011 05:19:19 -0500 Received: by vcbfl11 with SMTP id fl11so3093121vcb.4 for ; Fri, 25 Nov 2011 02:19:18 -0800 (PST) MIME-Version: 1.0 Sender: kgrace.liu@gmail.com In-Reply-To: References: <1322043290-20349-1-git-send-email-cyliu@suse.com> <1322043290-20349-2-git-send-email-cyliu@suse.com> Date: Fri, 25 Nov 2011 18:19:17 +0800 Message-ID: From: Chunyan Liu Content-Type: multipart/mixed; boundary=bcaec51b9c7156bd9504b28c7c17 Subject: Re: [Qemu-devel] [PATCH V3 2/2] Add -f option to qemu-nbd List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org --bcaec51b9c7156bd9504b28c7c17 Content-Type: multipart/alternative; boundary=bcaec51b9c7156bd9104b28c7c15 --bcaec51b9c7156bd9104b28c7c15 Content-Type: text/plain; charset=ISO-8859-1 2011/11/24 Stefan Hajnoczi > On Thu, Nov 24, 2011 at 3:38 AM, Chunyan Liu wrote: > > > > > > 2011/11/23 Stefan Hajnoczi > >> > >> On Wed, Nov 23, 2011 at 10:14 AM, Chunyan Liu wrote: > >> > V3: > >> > Remove file lock in main(). > >> > Try to find new free nbd device and connect to it if connecting to the > >> > first > >> > first found free nbd device failed. > >> > > >> > Signed-off-by: Chunyan Liu > >> > --- > >> > qemu-nbd.c | 80 > >> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > >> > 1 files changed, 79 insertions(+), 1 deletions(-) > >> > >> I not seeing the part where you adjusted the ioctl order. > >> > >> The /proc/partitions scanning is unnecessary since we can just loop > >> over /dev/ndb%d and try to initialize. If a device is in use then > >> init will fail and we need to try the next one. If a device is free > >> we can continue with normal operation. I guess I'm saying that once > >> you fix the ioctl order then there's no need for another mechanism to > >> test whether or not a device is in use. > > > > The way of scanning /proc/partitions to find an unused nbd device first > can > > borrow the code for "qemu-nbd -c" to do the left things. . > > The way of loop over /dev/nbd%d and try to initialize, from the first > > thought, needs do all things in the loop, including handling -v, > nbd_init, > > nbd_client, etc. That part of code is quite similar to "qemu-nbd -c". I > > don't know if that is better? > > This might be a chance to refactor the code slightly, that would also > avoid you having to introduce a goto retry. > > About detail implementation of the loop over /dev/nbd%d way, have done some coding and testing. I'm afraid the reorganization work is not very slight, not sure how do you think. First, through nbd_init() success or fail to check if the device is in use, at least nbd_init() and operations before nbd_init() but device relative should be in the loop. Usually, there are two ways to be considered: 1. Try to divide current "qemu-nbd -c" processing code into two parts ( nbd_init() and before, and operations after nbd_init()), 1st part put into the loop, 2nd part outside loop. This way is hard to achieve. nbd_init() should be in client thread, but the loop is outside that thread, it's not proper to drag out nbd_init() from client thread and put it into the loop. 2. Put all "qemu-nbd -c" processing code (device relative) in the loop, directly check the "qemu-nbd -c /dev/nbd%d disk.img" result, if fail, try next nbd device. In this way, will adjust current code order and extract device related codes to a new function so that both "qemu-nbd -c" and "qemu-nbd -f" can use.(some initialization work and cleanup work to be cared) A draft is in attachment. qemu-nbd -f is working, since I'm not sure such change is acceptable or not, not extract code in the loop into a function yet. > Stefan > > --bcaec51b9c7156bd9104b28c7c15 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

2011/11/24 Stefan Hajnoczi <stefanha@g= mail.com>
On Thu, Nov 24, 2011 at 3:38 AM, Chunyan Liu <cyliu@suse.com> wrote:
>
>
> 2011/11/23 Stefan Hajnoczi <stefanha@gmail.com>
>>
>> On Wed, Nov 23, 2011 at 10:14 AM, Chunyan Liu <cyliu@suse.com> wrote:
>> > V3:
>> > Remove file lock in main().
>> > Try to find new free nbd device and connect to it if connecti= ng to the
>> > first
>> > first found free nbd device failed.
>> >
>> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
>> > ---
>> > =A0qemu-nbd.c | =A0 80
>> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-<= br> >> > =A01 files changed, 79 insertions(+), 1 deletions(-)
>>
>> I not seeing the part where you adjusted the ioctl order.
>>
>> The /proc/partitions scanning is unnecessary since we can just loo= p
>> over /dev/ndb%d and try to initialize. =A0If a device is in use th= en
>> init will fail and we need to try the next one. =A0If a device is = free
>> we can continue with normal operation. =A0I guess I'm saying t= hat once
>> you fix the ioctl order then there's no need for another mecha= nism to
>> test whether or not a device is in use.
>
> The way of scanning /proc/partitions to find an unused nbd device firs= t can
> borrow the code for "qemu-nbd -c" to do the left things. . > The way of loop over /dev/nbd%d and try to initialize, from the first<= br> > thought, needs do all things in the loop, including handling -v, nbd_i= nit,
> nbd_client, etc. That part of code is quite similar to "qemu-nbd = -c". I
> don't know if that is better?

This might be a chance to refactor the code slightly, that woul= d also
avoid you having to introduce a goto retry.

About det= ail implementation of the loop over /dev/nbd%d way, have done some coding a= nd testing. I'm afraid the reorganization work is not very slight, not = sure how do you think.
First, through nbd_init() success or fail to check if the device is in use,= at least nbd_init() and operations before nbd_init() but device relative s= hould be in the loop. Usually, there are two ways to be considered:
1. Try to divide current "qemu-nbd -c" processing code into two p= arts ( nbd_init() and before, and operations after nbd_init()), 1st part pu= t into the loop, 2nd part outside loop. This way is hard to achieve. nbd_in= it() should be in client thread, but the loop is outside that thread, it= 9;s not proper to drag out nbd_init() from client thread and put it into th= e loop.
2. Put all "qemu-nbd -c" processing code (device relative) in the= loop, directly check the "qemu-nbd -c /dev/nbd%d disk.img" resul= t, if fail, try next nbd device. In this way, will adjust current code orde= r and extract device related codes to a new function so that both "qem= u-nbd -c" and "qemu-nbd -f" can use.(some initialization wor= k and cleanup work to be cared)

A draft is in attachment. qemu-nbd -f is working, since I= 9;m not sure such change is acceptable or not, not extract code in the loop= into a function yet.=A0
=A0
Stefan


--bcaec51b9c7156bd9104b28c7c15-- --bcaec51b9c7156bd9504b28c7c17 Content-Type: text/x-patch; charset=US-ASCII; name="test_find.patch" Content-Disposition: attachment; filename="test_find.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gvf1ccae0 ZGlmZiAtLWdpdCBhL3FlbXUtbmJkLmMgYi9xZW11LW5iZC5jCmluZGV4IDI5MWNiYTIuLjM4YzNi YzMgMTAwNjQ0Ci0tLSBhL3FlbXUtbmJkLmMKKysrIGIvcWVtdS1uYmQuYwpAQCAtMTcxLDkgKzE3 MSw5IEBAIHN0YXRpYyBpbnQgZmluZF9wYXJ0aXRpb24oQmxvY2tEcml2ZXJTdGF0ZSAqYnMsIGlu dCBwYXJ0aXRpb24sCiBzdGF0aWMgdm9pZCB0ZXJtc2lnX2hhbmRsZXIoaW50IHNpZ251bSkKIHsK ICAgICBzdGF0aWMgaW50IHNpZ3Rlcm1fcmVwb3J0ZWQ7Ci0gICAgaWYgKCFzaWd0ZXJtX3JlcG9y dGVkKSB7CisvLyAgICBpZiAoIXNpZ3Rlcm1fcmVwb3J0ZWQpIHsKICAgICAgICAgc2lndGVybV9y ZXBvcnRlZCA9ICh3cml0ZShzaWd0ZXJtX3dmZCwgIiIsIDEpID09IDEpOwotICAgIH0KKy8vICAg IH0KIH0KIAogc3RhdGljIHZvaWQgKnNob3dfcGFydHModm9pZCAqYXJnKQpAQCAtMjU2LDcgKzI1 Niw3IEBAIGludCBtYWluKGludCBhcmdjLCBjaGFyICoqYXJndikKICAgICBzdHJ1Y3Qgc29ja2Fk ZHJfaW4gYWRkcjsKICAgICBzb2NrbGVuX3QgYWRkcl9sZW4gPSBzaXplb2YoYWRkcik7CiAgICAg b2ZmX3QgZmRfc2l6ZTsKLSAgICBjb25zdCBjaGFyICpzb3B0ID0gImhWYjpvOnA6cnNuUDpjOmR2 azplOnQiOworICAgIGNvbnN0IGNoYXIgKnNvcHQgPSAiaFZiOm86cDpyc25QOmM6ZHZrOmU6dGYi OwogICAgIHN0cnVjdCBvcHRpb24gbG9wdFtdID0gewogICAgICAgICB7ICJoZWxwIiwgMCwgTlVM TCwgJ2gnIH0sCiAgICAgICAgIHsgInZlcnNpb24iLCAwLCBOVUxMLCAnVicgfSwKQEAgLTI3Myw2 ICsyNzMsNyBAQCBpbnQgbWFpbihpbnQgYXJnYywgY2hhciAqKmFyZ3YpCiAgICAgICAgIHsgInNo YXJlZCIsIDEsIE5VTEwsICdlJyB9LAogICAgICAgICB7ICJwZXJzaXN0ZW50IiwgMCwgTlVMTCwg J3QnIH0sCiAgICAgICAgIHsgInZlcmJvc2UiLCAwLCBOVUxMLCAndicgfSwKKyAgICAgICAgeyAi ZmluZCIsIDAsIE5VTEwsICdmJyB9LAogICAgICAgICB7IE5VTEwsIDAsIE5VTEwsIDAgfQogICAg IH07CiAgICAgaW50IGNoOwpAQCAtMjkyLDcgKzI5Myw4IEBAIGludCBtYWluKGludCBhcmdjLCBj aGFyICoqYXJndikKICAgICBpbnQgbWF4X2ZkOwogICAgIGludCBwZXJzaXN0ZW50ID0gMDsKICAg ICBwdGhyZWFkX3QgY2xpZW50X3RocmVhZDsKLQorICAgIGludCBmaW5kID0gMDsKKyAgICBpbnQg ZmluZF9taW5vcjsKICAgICAvKiBUaGUgY2xpZW50IHRocmVhZCB1c2VzIFNJR1RFUk0gdG8gaW50 ZXJydXB0IHRoZSBzZXJ2ZXIuICBBIHNpZ25hbAogICAgICAqIGhhbmRsZXIgZW5zdXJlcyB0aGF0 ICJxZW11LW5iZCAtdiAtYyIgZXhpdHMgd2l0aCBhIG5pY2Ugc3RhdHVzIGNvZGUuCiAgICAgICov CkBAIC0zNzQsNiArMzc2LDEwIEBAIGludCBtYWluKGludCBhcmdjLCBjaGFyICoqYXJndikKICAg ICAgICAgY2FzZSAndic6CiAgICAgICAgICAgICB2ZXJib3NlID0gMTsKICAgICAgICAgICAgIGJy ZWFrOworICAgICAgICBjYXNlICdmJzoKKyAgICAgICAgICAgIGZpbmQgPSAxOworICAgICAgICAg ICAgZGV2aWNlID0gIi9kZXYvbmJkMCI7CisgICAgICAgICAgICBicmVhazsKICAgICAgICAgY2Fz ZSAnVic6CiAgICAgICAgICAgICB2ZXJzaW9uKGFyZ3ZbMF0pOwogICAgICAgICAgICAgZXhpdCgw KTsKQEAgLTQ1OSwxMSArNDY1LDM0IEBAIGludCBtYWluKGludCBhcmdjLCBjaGFyICoqYXJndikK ICAgICAgICAgICAgIGV4aXQoZXJyb3JzKTsKICAgICAgICAgfQogICAgIH0KKyAgICBiZHJ2X2lu aXQoKTsKKyAgICBhdGV4aXQoYmRydl9jbG9zZV9hbGwpOworCisgICAgYnMgPSBiZHJ2X25ldygi aGRiIik7CisgICAgc3JjcGF0aCA9IGFyZ3Zbb3B0aW5kXTsKKyAgICBpZiAoKHJldCA9IGJkcnZf b3Blbihicywgc3JjcGF0aCwgZmxhZ3MsIE5VTEwpKSA8IDApIHsKKyAgICAgICAgZXJybm8gPSAt cmV0OworICAgICAgICBlcnIoRVhJVF9GQUlMVVJFLCAiRmFpbGVkIHRvIGJkcnZfb3BlbiAnJXMn IiwgYXJndltvcHRpbmRdKTsKKyAgICB9CiAKKyAgICBmZF9zaXplID0gYnMtPnRvdGFsX3NlY3Rv cnMgKiA1MTI7CisgICAgaWYgKHBhcnRpdGlvbiAhPSAtMSAmJgorICAgICAgICBmaW5kX3BhcnRp dGlvbihicywgcGFydGl0aW9uLCAmZGV2X29mZnNldCwgJmZkX3NpemUpKSB7CisgICAgICAgIGVy cihFWElUX0ZBSUxVUkUsICJDb3VsZCBub3QgZmluZCBwYXJ0aXRpb24gJWQiLCBwYXJ0aXRpb24p OworICAgIH0KKyAKK2lmIChmaW5kKSB7CisgICAgZm9yIChmaW5kX21pbm9yPTA7IGZpbmRfbWlu b3I8MTY7IGZpbmRfbWlub3IrKyl7CisgICAgYXNwcmludGYoJmRldmljZSwgIi9kZXYvbmJkJWQi LCBmaW5kX21pbm9yKTsKKyAgICBpZiAoIWRldmljZSkgeworICAgICAgICBjb250aW51ZTsKKyAg ICB9CisgCiAgICAgaWYgKGRldmljZSkgewogICAgICAgICAvKiBPcGVuIGJlZm9yZSBzcGF3bmlu ZyBuZXcgdGhyZWFkcy4gIEluIHRoZSBmdXR1cmUsIHdlIG1heQogICAgICAgICAgKiBkcm9wIHBy aXZpbGVnZXMgYWZ0ZXIgb3BlbmluZy4KICAgICAgICAgICovCisKICAgICAgICAgZmQgPSBvcGVu KGRldmljZSwgT19SRFdSKTsKICAgICAgICAgaWYgKGZkID09IC0xKSB7CiAgICAgICAgICAgICBl cnIoRVhJVF9GQUlMVVJFLCAiRmFpbGVkIHRvIG9wZW4gJXMiLCBkZXZpY2UpOwpAQCAtNDc1LDIz ICs1MDQsNiBAQCBpbnQgbWFpbihpbnQgYXJnYywgY2hhciAqKmFyZ3YpCiAgICAgICAgIH0KICAg ICB9CiAKLSAgICBiZHJ2X2luaXQoKTsKLSAgICBhdGV4aXQoYmRydl9jbG9zZV9hbGwpOwotCi0g ICAgYnMgPSBiZHJ2X25ldygiaGRhIik7Ci0gICAgc3JjcGF0aCA9IGFyZ3Zbb3B0aW5kXTsKLSAg ICBpZiAoKHJldCA9IGJkcnZfb3Blbihicywgc3JjcGF0aCwgZmxhZ3MsIE5VTEwpKSA8IDApIHsK LSAgICAgICAgZXJybm8gPSAtcmV0OwotICAgICAgICBlcnIoRVhJVF9GQUlMVVJFLCAiRmFpbGVk IHRvIGJkcnZfb3BlbiAnJXMnIiwgYXJndltvcHRpbmRdKTsKLSAgICB9Ci0KLSAgICBmZF9zaXpl ID0gYnMtPnRvdGFsX3NlY3RvcnMgKiA1MTI7Ci0KLSAgICBpZiAocGFydGl0aW9uICE9IC0xICYm Ci0gICAgICAgIGZpbmRfcGFydGl0aW9uKGJzLCBwYXJ0aXRpb24sICZkZXZfb2Zmc2V0LCAmZmRf c2l6ZSkpIHsKLSAgICAgICAgZXJyKEVYSVRfRkFJTFVSRSwgIkNvdWxkIG5vdCBmaW5kIHBhcnRp dGlvbiAlZCIsIHBhcnRpdGlvbik7Ci0gICAgfQotCiAgICAgc2hhcmluZ19mZHMgPSBnX21hbGxv Yygoc2hhcmVkICsgMSkgKiBzaXplb2YoaW50KSk7CiAKICAgICBpZiAoc29ja3BhdGgpIHsKQEAg LTUzNCwxMSArNTQ2LDE0IEBAIGludCBtYWluKGludCBhcmdjLCBjaGFyICoqYXJndikKICAgICAg ICAgICAgIHJldCA9IHNlbGVjdChtYXhfZmQgKyAxLCAmZmRzLCBOVUxMLCBOVUxMLCBOVUxMKTsK ICAgICAgICAgfSB3aGlsZSAocmV0ID09IC0xICYmIGVycm5vID09IEVJTlRSKTsKICAgICAgICAg aWYgKHJldCA9PSAtMSB8fCBGRF9JU1NFVChzaWd0ZXJtX2ZkWzBdLCAmZmRzKSkgeworICAgICAg ICAgICAgaW50IHNpZ2J1ZlsxXTsKKyAgICAgICAgICAgIHJlYWQoc2lndGVybV9mZFswXSwgc2ln YnVmLCAxKTsKICAgICAgICAgICAgIGJyZWFrOwogICAgICAgICB9CiAKLSAgICAgICAgaWYgKEZE X0lTU0VUKHNoYXJpbmdfZmRzWzBdLCAmZmRzKSkKKyAgICAgICAgaWYgKEZEX0lTU0VUKHNoYXJp bmdfZmRzWzBdLCAmZmRzKSkgCiAgICAgICAgICAgICByZXQtLTsKKyAgICAgICAgCiAgICAgICAg IGZvciAoaSA9IDE7IGkgPCBuYl9mZHMgJiYgcmV0OyBpKyspIHsKICAgICAgICAgICAgIGlmIChG RF9JU1NFVChzaGFyaW5nX2Zkc1tpXSwgJmZkcykpIHsKICAgICAgICAgICAgICAgICBpZiAobmJk X3RyaXAoYnMsIHNoYXJpbmdfZmRzW2ldLCBmZF9zaXplLCBkZXZfb2Zmc2V0LApAQCAtNTc3LDgg KzU5MiwxOCBAQCBpbnQgbWFpbihpbnQgYXJnYywgY2hhciAqKmFyZ3YpCiAgICAgaWYgKGRldmlj ZSkgewogICAgICAgICB2b2lkICpyZXQ7CiAgICAgICAgIHB0aHJlYWRfam9pbihjbGllbnRfdGhy ZWFkLCAmcmV0KTsKLSAgICAgICAgZXhpdChyZXQgIT0gTlVMTCk7CisgICAgICAgIGlmIChyZXQg IT0gRVhJVF9TVUNDRVNTICYmIGZpbmQpIHsKKyAgICAgICAgICAgIHNvY2twYXRoID0gTlVMTDsK KyAgICAgICAgICAgIG5iX2ZkcyA9IDA7CisgICAgICAgICAgICBmcmVlKGRldmljZSk7CisgICAg ICAgICAgICBjb250aW51ZTsKKyAgICAgICAgfQorICAgICAgICBlbHNlIHsKKyAgICAgICAgICAg IGV4aXQocmV0ICE9IE5VTEwpOworICAgICAgICB9CiAgICAgfSBlbHNlIHsKICAgICAgICAgZXhp dChFWElUX1NVQ0NFU1MpOwogICAgIH0KKyAgICB9Cit9CiB9Cg== --bcaec51b9c7156bd9504b28c7c17--