Message ID | 1331409387-18234-1-git-send-email-bos@je-eigen-domein.nl |
---|---|
State | New |
Headers | show |
Am 10.03.2012 20:56, schrieb Floris Bos: > Some Linux distributions use the /dev/disk/by-id/scsi-SATA_name-of-disk-model_serial addressing scheme > when refering to partitions in /etc/fstab and elsewhere. > This causes problems when starting a disk image taken from an existing physical server under qemu, > because when running under qemu name-of-disk-model is always "QEMU HARDDISK" > This patch introduces a model=s option which in combination with the existing serial=s option can be used to > fake the disk the operating system was previously on, allowing the OS to boot properly. > > Cc: kwolf@redhat.com > Signed-off-by: Floris Bos <dev@noc-ps.com> Patch looks good to me, except for some formal issues scripts/checkpatch.pl should warn about: > diff --git a/blockdev.c b/blockdev.c > index d78aa51..66fcc14 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -534,6 +536,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > dinfo->refcount = 1; > if (serial) > strncpy(dinfo->serial, serial, sizeof(dinfo->serial) - 1); > + if (model) > + strncpy(dinfo->model, model, sizeof(dinfo->model) - 1); Please use braces for new ifs. > QTAILQ_INSERT_TAIL(&drives, dinfo, next); > > bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error); > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 4d568ac..2a38030 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -1977,7 +1993,8 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0, > if (dinfo) { > if (ide_init_drive(&bus->ifs[i], dinfo->bdrv, > dinfo->media_cd ? IDE_CD : IDE_HD, NULL, > - *dinfo->serial ? dinfo->serial : NULL) < 0) { > + *dinfo->serial ? dinfo->serial : NULL, > + *dinfo->model ? dinfo->model : NULL) < 0) { Indentation uses tabs here; please use spaces. > error_report("Can't set up IDE drive %s", dinfo->id); > exit(1); > } Andreas
On Sat, Mar 10, 2012 at 7:56 PM, Floris Bos <bos@je-eigen-domein.nl> wrote: > @@ -1885,6 +1885,22 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, > snprintf(s->drive_serial_str, sizeof(s->drive_serial_str), > "QM%05d", s->drive_serial); > } > + if (model) { > + strncpy(s->drive_model_str, model, sizeof(s->drive_model_str)); strncpy(3) does not NUL-terminate if the max length is reached. Either you need to use pstrcpy() or specify sizeof(s->drive_model_str) - 1 and make sure s->drive_model_str[40] = '\0'. > @@ -146,6 +155,9 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) > if (!dev->serial) { > dev->serial = g_strdup(s->drive_serial_str); > } > + if (!dev->model) { > + dev->model = g_strdup(s->drive_model_str); > + } Seems this will never be freed but dev->serial has the same issue, so this isn't new. Stefan
On 03/12/2012 11:26 AM, Stefan Hajnoczi wrote: > On Sat, Mar 10, 2012 at 7:56 PM, Floris Bos<bos@je-eigen-domein.nl> wrote: >> @@ -1885,6 +1885,22 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, >> snprintf(s->drive_serial_str, sizeof(s->drive_serial_str), >> "QM%05d", s->drive_serial); >> } >> + if (model) { >> + strncpy(s->drive_model_str, model, sizeof(s->drive_model_str)); > strncpy(3) does not NUL-terminate if the max length is reached. > Either you need to use pstrcpy() or specify sizeof(s->drive_model_str) > - 1 and make sure s->drive_model_str[40] = '\0'. Thanks for the feedback. Will change that line (and serial that used strncpy() as well) to pstrcpy(), correct the cosmetic issues mentioned by Andreas and submit a v2 patch.
Am 12.03.2012 12:30, schrieb Floris Bos / Maxnet: > On 03/12/2012 11:26 AM, Stefan Hajnoczi wrote: >> On Sat, Mar 10, 2012 at 7:56 PM, Floris Bos<bos@je-eigen-domein.nl> wrote: >>> @@ -1885,6 +1885,22 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, >>> snprintf(s->drive_serial_str, sizeof(s->drive_serial_str), >>> "QM%05d", s->drive_serial); >>> } >>> + if (model) { >>> + strncpy(s->drive_model_str, model, sizeof(s->drive_model_str)); >> strncpy(3) does not NUL-terminate if the max length is reached. >> Either you need to use pstrcpy() or specify sizeof(s->drive_model_str) >> - 1 and make sure s->drive_model_str[40] = '\0'. > > Thanks for the feedback. > > Will change that line (and serial that used strncpy() as well) to > pstrcpy(), correct the cosmetic issues mentioned by Andreas and submit a > v2 patch. Fixing serial as well is a good idea. Please submit a separate patch for that, though, as it is an independent fix. Kevin
Am 10.03.2012 20:56, schrieb Floris Bos: > Some Linux distributions use the /dev/disk/by-id/scsi-SATA_name-of-disk-model_serial addressing scheme > when refering to partitions in /etc/fstab and elsewhere. > This causes problems when starting a disk image taken from an existing physical server under qemu, > because when running under qemu name-of-disk-model is always "QEMU HARDDISK" > This patch introduces a model=s option which in combination with the existing serial=s option can be used to > fake the disk the operating system was previously on, allowing the OS to boot properly. > > Cc: kwolf@redhat.com > Signed-off-by: Floris Bos <dev@noc-ps.com> Oh, and now that I look at the actual patch, do we really want to add the model=... option to -drive? I think just the qdev property may be enough. When you need to set the option, you would need to use -device, but it's not that hard. Kevin
On 03/12/2012 12:57 PM, Kevin Wolf wrote: > Am 10.03.2012 20:56, schrieb Floris Bos: >> Some Linux distributions use the /dev/disk/by-id/scsi-SATA_name-of-disk-model_serial addressing scheme >> when refering to partitions in /etc/fstab and elsewhere. >> This causes problems when starting a disk image taken from an existing physical server under qemu, >> because when running under qemu name-of-disk-model is always "QEMU HARDDISK" >> This patch introduces a model=s option which in combination with the existing serial=s option can be used to >> fake the disk the operating system was previously on, allowing the OS to boot properly. >> >> Cc: kwolf@redhat.com >> Signed-off-by: Floris Bos<dev@noc-ps.com> > Oh, and now that I look at the actual patch, do we really want to add > the model=... option to -drive? I think just the qdev property may be > enough. When you need to set the option, you would need to use -device, > but it's not that hard. Well, to me it makes sense to put it at the same places "serial" is, as both options have quite similar functions: faking drive attributes.
Am 12.03.2012 13:09, schrieb Floris Bos / Maxnet: > On 03/12/2012 12:57 PM, Kevin Wolf wrote: >> Am 10.03.2012 20:56, schrieb Floris Bos: >>> Some Linux distributions use the /dev/disk/by-id/scsi-SATA_name-of-disk-model_serial addressing scheme >>> when refering to partitions in /etc/fstab and elsewhere. >>> This causes problems when starting a disk image taken from an existing physical server under qemu, >>> because when running under qemu name-of-disk-model is always "QEMU HARDDISK" >>> This patch introduces a model=s option which in combination with the existing serial=s option can be used to >>> fake the disk the operating system was previously on, allowing the OS to boot properly. >>> >>> Cc: kwolf@redhat.com >>> Signed-off-by: Floris Bos<dev@noc-ps.com> >> Oh, and now that I look at the actual patch, do we really want to add >> the model=... option to -drive? I think just the qdev property may be >> enough. When you need to set the option, you would need to use -device, >> but it's not that hard. > > Well, to me it makes sense to put it at the same places "serial" is, as > both options have quite similar functions: faking drive attributes. -drive serial=... is older than -device, this is why it was okay back then and why we still have to maintain it for compatibility. If serial=... was a new patch today, I would probably be asking the same question there. Kevin
diff --git a/blockdev.c b/blockdev.c index d78aa51..66fcc14 100644 --- a/blockdev.c +++ b/blockdev.c @@ -277,6 +277,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) const char *file = NULL; char devname[128]; const char *serial; + const char *model; const char *mediastr = ""; BlockInterfaceType type; enum { MEDIA_DISK, MEDIA_CDROM } media; @@ -313,6 +314,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) file = qemu_opt_get(opts, "file"); serial = qemu_opt_get(opts, "serial"); + model = qemu_opt_get(opts, "model"); if ((buf = qemu_opt_get(opts, "if")) != NULL) { pstrcpy(devname, sizeof(devname), buf); @@ -534,6 +536,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) dinfo->refcount = 1; if (serial) strncpy(dinfo->serial, serial, sizeof(dinfo->serial) - 1); + if (model) + strncpy(dinfo->model, model, sizeof(dinfo->model) - 1); QTAILQ_INSERT_TAIL(&drives, dinfo, next); bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error); diff --git a/blockdev.h b/blockdev.h index 260e16b..21eb4b5 100644 --- a/blockdev.h +++ b/blockdev.h @@ -18,6 +18,7 @@ void blockdev_mark_auto_del(BlockDriverState *bs); void blockdev_auto_del(BlockDriverState *bs); #define BLOCK_SERIAL_STRLEN 20 +#define BLOCK_MODEL_STRLEN 40 typedef enum { IF_DEFAULT = -1, /* for use with drive_add() only */ @@ -37,6 +38,7 @@ struct DriveInfo { int media_cd; QemuOpts *opts; char serial[BLOCK_SERIAL_STRLEN + 1]; + char model[BLOCK_MODEL_STRLEN + 1]; QTAILQ_ENTRY(DriveInfo) next; int refcount; }; diff --git a/hw/ide/core.c b/hw/ide/core.c index 4d568ac..2a38030 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -101,7 +101,7 @@ static void ide_identify(IDEState *s) put_le16(p + 21, 512); /* cache size in sectors */ put_le16(p + 22, 4); /* ecc bytes */ padstr((char *)(p + 23), s->version, 8); /* firmware version */ - padstr((char *)(p + 27), "QEMU HARDDISK", 40); /* model */ + padstr((char *)(p + 27), s->drive_model_str, 40); /* model */ #if MAX_MULT_SECTORS > 1 put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS); #endif @@ -189,7 +189,7 @@ static void ide_atapi_identify(IDEState *s) put_le16(p + 21, 512); /* cache size in sectors */ put_le16(p + 22, 4); /* ecc bytes */ padstr((char *)(p + 23), s->version, 8); /* firmware version */ - padstr((char *)(p + 27), "QEMU DVD-ROM", 40); /* model */ + padstr((char *)(p + 27), s->drive_model_str, 40); /* model */ put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */ #ifdef USE_DMA_CDROM put_le16(p + 49, 1 << 9 | 1 << 8); /* DMA and LBA supported */ @@ -246,7 +246,7 @@ static void ide_cfata_identify(IDEState *s) padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */ put_le16(p + 22, 0x0004); /* ECC bytes */ padstr((char *) (p + 23), s->version, 8); /* Firmware Revision */ - padstr((char *) (p + 27), "QEMU MICRODRIVE", 40);/* Model number */ + padstr((char *) (p + 27), s->drive_model_str, 40);/* Model number */ #if MAX_MULT_SECTORS > 1 put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS); #else @@ -1834,7 +1834,7 @@ static const BlockDevOps ide_cd_block_ops = { }; int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, - const char *version, const char *serial) + const char *version, const char *serial, const char *model) { int cylinders, heads, secs; uint64_t nb_sectors; @@ -1885,6 +1885,22 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, snprintf(s->drive_serial_str, sizeof(s->drive_serial_str), "QM%05d", s->drive_serial); } + if (model) { + strncpy(s->drive_model_str, model, sizeof(s->drive_model_str)); + } else { + switch (kind) { + case IDE_CD: + strcpy(s->drive_model_str, "QEMU DVD-ROM"); + break; + case IDE_CFATA: + strcpy(s->drive_model_str, "QEMU MICRODRIVE"); + break; + default: + strcpy(s->drive_model_str, "QEMU HARDDISK"); + break; + } + } + if (version) { pstrcpy(s->version, sizeof(s->version), version); } else { @@ -1977,7 +1993,8 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0, if (dinfo) { if (ide_init_drive(&bus->ifs[i], dinfo->bdrv, dinfo->media_cd ? IDE_CD : IDE_HD, NULL, - *dinfo->serial ? dinfo->serial : NULL) < 0) { + *dinfo->serial ? dinfo->serial : NULL, + *dinfo->model ? dinfo->model : NULL) < 0) { error_report("Can't set up IDE drive %s", dinfo->id); exit(1); } diff --git a/hw/ide/internal.h b/hw/ide/internal.h index c808a0d..b1319dc 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -348,6 +348,7 @@ struct IDEState { uint8_t identify_data[512]; int drive_serial; char drive_serial_str[21]; + char drive_model_str[41]; /* ide regs */ uint8_t feature; uint8_t error; @@ -468,6 +469,7 @@ struct IDEDevice { BlockConf conf; char *version; char *serial; + char *model; }; #define BM_STATUS_DMAING 0x01 @@ -534,7 +536,7 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val); uint32_t ide_data_readl(void *opaque, uint32_t addr); int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, - const char *version, const char *serial); + const char *version, const char *serial, const char *model); void ide_init2(IDEBus *bus, qemu_irq irq); void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1, qemu_irq irq); diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index f6a4896..40b1038 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -120,6 +120,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus); IDEState *s = bus->ifs + dev->unit; const char *serial; + const char *model; DriveInfo *dinfo; if (dev->conf.discard_granularity && dev->conf.discard_granularity != 512) { @@ -135,8 +136,16 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) serial = dinfo->serial; } } + model = dev->model; + if (!model) { + /* try to fall back to value set with legacy -drive model=... */ + dinfo = drive_get_by_blockdev(dev->conf.bs); + if (*dinfo->model) { + model = dinfo->model; + } + } - if (ide_init_drive(s, dev->conf.bs, kind, dev->version, serial) < 0) { + if (ide_init_drive(s, dev->conf.bs, kind, dev->version, serial, model) < 0) { return -1; } @@ -146,6 +155,9 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) if (!dev->serial) { dev->serial = g_strdup(s->drive_serial_str); } + if (!dev->model) { + dev->model = g_strdup(s->drive_model_str); + } add_boot_device_path(dev->conf.bootindex, &dev->qdev, dev->unit ? "/disk@1" : "/disk@0"); @@ -173,7 +185,8 @@ static int ide_drive_initfn(IDEDevice *dev) #define DEFINE_IDE_DEV_PROPERTIES() \ DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf), \ DEFINE_PROP_STRING("ver", IDEDrive, dev.version), \ - DEFINE_PROP_STRING("serial", IDEDrive, dev.serial) + DEFINE_PROP_STRING("serial", IDEDrive, dev.serial),\ + DEFINE_PROP_STRING("model", IDEDrive, dev.model) static Property ide_hd_properties[] = { DEFINE_IDE_DEV_PROPERTIES(), diff --git a/qemu-config.c b/qemu-config.c index be84a03..9e7459f 100644 --- a/qemu-config.c +++ b/qemu-config.c @@ -70,6 +70,10 @@ static QemuOptsList qemu_drive_opts = { .type = QEMU_OPT_STRING, .help = "disk serial number", },{ + .name = "model", + .type = QEMU_OPT_STRING, + .help = "disk model name", + },{ .name = "rerror", .type = QEMU_OPT_STRING, .help = "read error action", diff --git a/qemu-options.hx b/qemu-options.hx index daefce3..70b5cf6 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -140,7 +140,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive, "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n" " [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n" " [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n" - " [,serial=s][,addr=A][,id=name][,aio=threads|native]\n" + " [,serial=s][,model=s][,addr=A][,id=name][,aio=threads|native]\n" " [,readonly=on|off][,copy-on-read=on|off]\n" " [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]][[,iops=i]|[[,iops_rd=r][,iops_wr=w]]\n" " use 'file' as a drive image\n", QEMU_ARCH_ALL) @@ -183,6 +183,8 @@ the format. Can be used to specifiy format=raw to avoid interpreting an untrusted format header. @item serial=@var{serial} This option specifies the serial number to assign to the device. +@item model=@var{model} +This option specifies the model name to assign to the device. Default for IDE disks is: QEMU HARDDISK @item addr=@var{addr} Specify the controller's PCI address (if=virtio only). @item werror=@var{action},rerror=@var{action}
Some Linux distributions use the /dev/disk/by-id/scsi-SATA_name-of-disk-model_serial addressing scheme when refering to partitions in /etc/fstab and elsewhere. This causes problems when starting a disk image taken from an existing physical server under qemu, because when running under qemu name-of-disk-model is always "QEMU HARDDISK" This patch introduces a model=s option which in combination with the existing serial=s option can be used to fake the disk the operating system was previously on, allowing the OS to boot properly. Cc: kwolf@redhat.com Signed-off-by: Floris Bos <dev@noc-ps.com> --- blockdev.c | 4 ++++ blockdev.h | 2 ++ hw/ide/core.c | 27 ++++++++++++++++++++++----- hw/ide/internal.h | 4 +++- hw/ide/qdev.c | 17 +++++++++++++++-- qemu-config.c | 4 ++++ qemu-options.hx | 4 +++- 7 files changed, 53 insertions(+), 9 deletions(-)