| Message ID | 2ACCDD16-AFEC-49BF-8BD3-3A3EC55DB2E5@gmail.com |
|---|---|
| State | New |
| Headers | show |
Programmingkid <programmingkidx@gmail.com> writes: > Subject was: > Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() > on Mac OS X so that it reports the correct length of a real CD Patch history information goes... > > This patch allows Mac OS X to use a real CDROM disc in QEMU. > Testing this patch will require using QEMU v2.2.0 because the > current git version has a bug in it that prevents /dev/cdrom from > being used. "make check" did pass and my Debian boot disc did work. > > Signed-off-by: John Arbuckle <programmingkidx@gmail.com> > > --- ... below the --- divider. > Fixed code indentation to be inline with removed > size = LLONG_MAX. > > block/raw-posix.c | 15 ++++++++++++++- > 1 files changed, 14 insertions(+), 1 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index e51293a..fa431b2 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1312,7 +1312,20 @@ again: > if (size == 0) > #endif > #if defined(__APPLE__) && defined(__MACH__) > - size = LLONG_MAX; > + { > + uint64_t sectors = 0; > + uint32_t sector_size = 0; Ignorant question: why do you need to initialize these? Patch looks plausible otherwise, but know nothing about Macs :) > + > + if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0 > + && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) { > + size = sectors * sector_size; > + } else { > + size = lseek(fd, 0LL, SEEK_END); > + if (size < 0) { > + return -errno; > + } > + } > + } > #else > size = lseek(fd, 0LL, SEEK_END); > if (size < 0) {
On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote: > Programmingkid <programmingkidx@gmail.com> writes: > >> Subject was: >> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() >> on Mac OS X so that it reports the correct length of a real CD > > Patch history information goes... > >> >> This patch allows Mac OS X to use a real CDROM disc in QEMU. >> Testing this patch will require using QEMU v2.2.0 because the >> current git version has a bug in it that prevents /dev/cdrom from >> being used. "make check" did pass and my Debian boot disc did work. >> >> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> >> >> --- > > ... below the --- divider. I thought I did this. The information above is the description of the patch. Not its history. > >> Fixed code indentation to be inline with removed >> size = LLONG_MAX. >> >> block/raw-posix.c | 15 ++++++++++++++- >> 1 files changed, 14 insertions(+), 1 deletions(-) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index e51293a..fa431b2 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -1312,7 +1312,20 @@ again: >> if (size == 0) >> #endif >> #if defined(__APPLE__) && defined(__MACH__) >> - size = LLONG_MAX; >> + { >> + uint64_t sectors = 0; >> + uint32_t sector_size = 0; > > Ignorant question: why do you need to initialize these? We don't. It's just a habit. > > Patch looks plausible otherwise, but know nothing about Macs :) It does do the job. > >> + >> + if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0 >> + && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) { >> + size = sectors * sector_size; >> + } else { >> + size = lseek(fd, 0LL, SEEK_END); >> + if (size < 0) { >> + return -errno; >> + } >> + } >> + } >> #else >> size = lseek(fd, 0LL, SEEK_END); >> if (size < 0) {
On Mon, Jan 19, 2015 at 10:12 PM, Programmingkid <programmingkidx@gmail.com> wrote: > Subject was: > Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() > on Mac OS X so that it reports the correct length of a real CD > > This patch allows Mac OS X to use a real CDROM disc in QEMU. > Testing this patch will require using QEMU v2.2.0 because the > current git version has a bug in it that prevents /dev/cdrom from > being used. "make check" did pass and my Debian boot disc did work. > > Signed-off-by: John Arbuckle <programmingkidx@gmail.com> > > --- > Fixed code indentation to be inline with removed > size = LLONG_MAX. > > block/raw-posix.c | 15 ++++++++++++++- > 1 files changed, 14 insertions(+), 1 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index e51293a..fa431b2 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1312,7 +1312,20 @@ again: > if (size == 0) > #endif > #if defined(__APPLE__) && defined(__MACH__) > - size = LLONG_MAX; > + { > + uint64_t sectors = 0; > + uint32_t sector_size = 0; > + > + if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0 > + && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) { > + size = sectors * sector_size; > + } else { > + size = lseek(fd, 0LL, SEEK_END); > + if (size < 0) { > + return -errno; > + } > + } > + } > #else > size = lseek(fd, 0LL, SEEK_END); > if (size < 0) { Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 20 January 2015 at 14:46, Stefan Hajnoczi <stefanha@gmail.com> wrote: > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Tested-by: Peter Maydell <peter.maydell@linaro.org> (at least as far as OSX compile and make check goes; my Mac doesn't have a cdrom drive). -- PMM
On 01/20/2015 07:29 AM, Programmingkid wrote: > > On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote: > >> Programmingkid <programmingkidx@gmail.com> writes: >> >>> Subject was: >>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() >>> on Mac OS X so that it reports the correct length of a real CD >> >> Patch history information goes... >> >> ... below the --- divider. > > I thought I did this. The information above is the description of the patch. > Not its history. Anything that mentions 'v7' is history. When you read 'git log', you will not see mentions of 'v7', because no one cares how many tries it took to get a patch into git. Knowing about v7 only matters to the reviewers of v8, hence it is patch history that belongs after the divider. >>> + >>> + if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0 >>> + && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) { Indentation looks off here.
On Jan 20, 2015, at 10:22 AM, Eric Blake wrote: > On 01/20/2015 07:29 AM, Programmingkid wrote: >> >> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote: >> >>> Programmingkid <programmingkidx@gmail.com> writes: >>> >>>> Subject was: >>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() >>>> on Mac OS X so that it reports the correct length of a real CD >>> >>> Patch history information goes... > >>> >>> ... below the --- divider. >> >> I thought I did this. The information above is the description of the patch. >> Not its history. > > Anything that mentions 'v7' is history. When you read 'git log', you > will not see mentions of 'v7', because no one cares how many tries it > took to get a patch into git. Knowing about v7 only matters to the > reviewers of v8, hence it is patch history that belongs after the divider. Ok. > > >>>> + >>>> + if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0 >>>> + && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) { > > Indentation looks off here. It does look a little odd, but it also communicates that this is one statement (IMHO).
Programmingkid <programmingkidx@gmail.com> writes: > On Jan 20, 2015, at 10:22 AM, Eric Blake wrote: > >> On 01/20/2015 07:29 AM, Programmingkid wrote: >>> >>> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote: >>> >>>> Programmingkid <programmingkidx@gmail.com> writes: >>>> >>>>> Subject was: >>>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() >>>>> on Mac OS X so that it reports the correct length of a real CD >>>> >>>> Patch history information goes... >> >>>> >>>> ... below the --- divider. >>> >>> I thought I did this. The information above is the description of the patch. >>> Not its history. >> >> Anything that mentions 'v7' is history. When you read 'git log', you >> will not see mentions of 'v7', because no one cares how many tries it >> took to get a patch into git. Knowing about v7 only matters to the >> reviewers of v8, hence it is patch history that belongs after the divider. > > Ok. > >> >> >>>>> + >>>>> + if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0 >>>>> + && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) { >> >> Indentation looks off here. > > It does look a little odd, but it also communicates that this is one > statement (IMHO). It's not how the rest of QEMU is indented. Please try to blend in :) I feel bad about notpicking v8 of an obviously useful and patch that is basically just fine except for these little things. Thanks for persevering!
On Jan 20, 2015, at 3:28 PM, Markus Armbruster wrote: > Programmingkid <programmingkidx@gmail.com> writes: > >> On Jan 20, 2015, at 10:22 AM, Eric Blake wrote: >> >>> On 01/20/2015 07:29 AM, Programmingkid wrote: >>>> >>>> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote: >>>> >>>>> Programmingkid <programmingkidx@gmail.com> writes: >>>>> >>>>>> Subject was: >>>>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() >>>>>> on Mac OS X so that it reports the correct length of a real CD >>>>> >>>>> Patch history information goes... >>> >>>>> >>>>> ... below the --- divider. >>>> >>>> I thought I did this. The information above is the description of the patch. >>>> Not its history. >>> >>> Anything that mentions 'v7' is history. When you read 'git log', you >>> will not see mentions of 'v7', because no one cares how many tries it >>> took to get a patch into git. Knowing about v7 only matters to the >>> reviewers of v8, hence it is patch history that belongs after the divider. >> >> Ok. >> >>> >>> >>>>>> + >>>>>> + if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0 >>>>>> + && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) { >>> >>> Indentation looks off here. >> >> It does look a little odd, but it also communicates that this is one >> statement (IMHO). > > It's not how the rest of QEMU is indented. Please try to blend in :) > > I feel bad about notpicking v8 of an obviously useful and patch that is > basically just fine except for these little things. Thanks for > persevering! Thanks for your encouragement. I personally would have had that whole if condition on one line, but others want an 80 line maximum.
Programmingkid <programmingkidx@gmail.com> writes: > On Jan 20, 2015, at 3:28 PM, Markus Armbruster wrote: > >> Programmingkid <programmingkidx@gmail.com> writes: >> >>> On Jan 20, 2015, at 10:22 AM, Eric Blake wrote: >>> >>>> On 01/20/2015 07:29 AM, Programmingkid wrote: >>>>> >>>>> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote: >>>>> >>>>>> Programmingkid <programmingkidx@gmail.com> writes: >>>>>> >>>>>>> Subject was: >>>>>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() >>>>>>> on Mac OS X so that it reports the correct length of a real CD >>>>>> >>>>>> Patch history information goes... >>>> >>>>>> >>>>>> ... below the --- divider. >>>>> >>>>> I thought I did this. The information above is the description of >>>>> the patch. >>>>> Not its history. >>>> >>>> Anything that mentions 'v7' is history. When you read 'git log', you >>>> will not see mentions of 'v7', because no one cares how many tries it >>>> took to get a patch into git. Knowing about v7 only matters to the >>>> reviewers of v8, hence it is patch history that belongs after the divider. >>> >>> Ok. >>> >>>> >>>> >>>>>>> + >>>>>>> + if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0 >>>>>>> + && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) { >>>> >>>> Indentation looks off here. >>> >>> It does look a little odd, but it also communicates that this is one >>> statement (IMHO). >> >> It's not how the rest of QEMU is indented. Please try to blend in :) >> >> I feel bad about notpicking v8 of an obviously useful and patch that is >> basically just fine except for these little things. Thanks for >> persevering! > > Thanks for your encouragement. I personally would have had that whole if > condition on one line, but others want an 80 line maximum. Humans tend to have trouble following long lines with their eyes (I sure do). Typographic manuals suggest to limit columns to roughly 60 characters for exactly that reason[*]. Code can be a bit wider since it's commonly indented[**]. For commit messages, 70 characters is already a compromise between legibility and "writability". [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style [**] Deep nesting is no excuse for exceeding the width limit, because deep nesting is no excuse for *anything* :)
On Jan 21, 2015, at 2:54 AM, Markus Armbruster wrote: > Programmingkid <programmingkidx@gmail.com> writes: > >> On Jan 20, 2015, at 3:28 PM, Markus Armbruster wrote: >> >>> Programmingkid <programmingkidx@gmail.com> writes: >>> >>>> On Jan 20, 2015, at 10:22 AM, Eric Blake wrote: >>>> >>>>> On 01/20/2015 07:29 AM, Programmingkid wrote: >>>>>> >>>>>> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote: >>>>>> >>>>>>> Programmingkid <programmingkidx@gmail.com> writes: >>>>>>> >>>>>>>> Subject was: >>>>>>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() >>>>>>>> on Mac OS X so that it reports the correct length of a real CD >>>>>>> >>>>>>> Patch history information goes... >>>>> >>>>>>> >>>>>>> ... below the --- divider. >>>>>> >>>>>> I thought I did this. The information above is the description of >>>>>> the patch. >>>>>> Not its history. >>>>> >>>>> Anything that mentions 'v7' is history. When you read 'git log', you >>>>> will not see mentions of 'v7', because no one cares how many tries it >>>>> took to get a patch into git. Knowing about v7 only matters to the >>>>> reviewers of v8, hence it is patch history that belongs after the divider. >>>> >>>> Ok. >>>> >>>>> >>>>> >>>>>>>> + >>>>>>>> + if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0 >>>>>>>> + && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) { >>>>> >>>>> Indentation looks off here. >>>> >>>> It does look a little odd, but it also communicates that this is one >>>> statement (IMHO). >>> >>> It's not how the rest of QEMU is indented. Please try to blend in :) >>> >>> I feel bad about notpicking v8 of an obviously useful and patch that is >>> basically just fine except for these little things. Thanks for >>> persevering! >> >> Thanks for your encouragement. I personally would have had that whole if >> condition on one line, but others want an 80 line maximum. > > Humans tend to have trouble following long lines with their eyes (I sure > do). Typographic manuals suggest to limit columns to roughly 60 > characters for exactly that reason[*]. Code can be a bit wider since > it's commonly indented[**]. > > For commit messages, 70 characters is already a compromise between > legibility and "writability". > > > [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style > > [**] Deep nesting is no excuse for exceeding the width limit, because > deep nesting is no excuse for *anything* :) <Puts up white flag> I will indent my code at around the 60 to 70 character size.
Am 19.01.2015 um 23:12 hat Programmingkid geschrieben: > Subject was: > Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() > on Mac OS X so that it reports the correct length of a real CD > > This patch allows Mac OS X to use a real CDROM disc in QEMU. > Testing this patch will require using QEMU v2.2.0 because the > current git version has a bug in it that prevents /dev/cdrom from > being used. "make check" did pass and my Debian boot disc did work. > > Signed-off-by: John Arbuckle <programmingkidx@gmail.com> Thanks, applied to the block branch. Kevin
diff --git a/block/raw-posix.c b/block/raw-posix.c index e51293a..fa431b2 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1312,7 +1312,20 @@ again: if (size == 0) #endif #if defined(__APPLE__) && defined(__MACH__) - size = LLONG_MAX; + { + uint64_t sectors = 0; + uint32_t sector_size = 0; + + if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0 + && ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) { + size = sectors * sector_size; + } else { + size = lseek(fd, 0LL, SEEK_END); + if (size < 0) { + return -errno; + } + } + } #else size = lseek(fd, 0LL, SEEK_END); if (size < 0) {
Subject was: Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD This patch allows Mac OS X to use a real CDROM disc in QEMU. Testing this patch will require using QEMU v2.2.0 because the current git version has a bug in it that prevents /dev/cdrom from being used. "make check" did pass and my Debian boot disc did work. Signed-off-by: John Arbuckle <programmingkidx@gmail.com> --- Fixed code indentation to be inline with removed size = LLONG_MAX. block/raw-posix.c | 15 ++++++++++++++- 1 files changed, 14 insertions(+), 1 deletions(-)